diff --git a/hermes_cli/kanban.py b/hermes_cli/kanban.py index 31c4bf68ae85..283c4051c0a8 100644 --- a/hermes_cli/kanban.py +++ b/hermes_cli/kanban.py @@ -361,6 +361,12 @@ def build_parser(parent_subparsers: argparse._SubParsersAction) -> argparse.Argu help="Initial card status. Use 'blocked' for cards " "that require immediate human ops (R3 gate) " "to skip the brief running-to-blocked transition.") + p_create.add_argument("--model", default=None, dest="model_override", + metavar="MODEL", + help="Per-task model override. Pins the worker to " + "this model (passed as `hermes -m MODEL`) " + "instead of the assignee profile's default. " + "Omit to use the profile default.") p_create.add_argument("--json", action="store_true", help="Emit JSON output") # --- swarm --- @@ -535,7 +541,7 @@ def build_parser(parent_subparsers: argparse._SubParsersAction) -> argparse.Argu p_edit.add_argument("task_id") p_edit.add_argument( "--result", - required=True, + default=None, help="Backfilled task result text for a done task", ) p_edit.add_argument( @@ -548,6 +554,22 @@ def build_parser(parent_subparsers: argparse._SubParsersAction) -> argparse.Argu default=None, help="JSON dict of structured facts to store on the latest completed run.", ) + p_edit.add_argument( + "--model", + default=None, + dest="model_override", + metavar="MODEL", + help="Set the per-task model override (passed to the worker as " + "`hermes -m MODEL`). Omitting --model leaves the existing " + "override untouched; use --clear-model to remove it.", + ) + p_edit.add_argument( + "--clear-model", + action="store_true", + dest="clear_model", + help="Clear the per-task model override (revert to the assignee " + "profile default). Mutually exclusive with --model.", + ) p_block = sub.add_parser("block", help="Mark one or more tasks blocked") p_block.add_argument("task_id") @@ -1346,6 +1368,7 @@ def _cmd_create(args: argparse.Namespace) -> int: goal_mode=bool(getattr(args, "goal_mode", False)), goal_max_turns=getattr(args, "goal_max_turns", None), initial_status=getattr(args, "initial_status", "running"), + model_override=getattr(args, "model_override", None), ) task = kb.get_task(conn, task_id) if getattr(args, "json", False): @@ -1919,21 +1942,60 @@ def _cmd_edit(args: argparse.Namespace) -> int: except (ValueError, json.JSONDecodeError) as exc: print(f"kanban: --metadata: {exc}", file=sys.stderr) return 2 + + model_override = getattr(args, "model_override", None) + clear_model = bool(getattr(args, "clear_model", False)) + if model_override is not None and clear_model: + print( + "kanban: --model and --clear-model are mutually exclusive", + file=sys.stderr, + ) + return 2 + + # The result-backfill edit is only attempted when --result is given; + # --model / --clear-model are independent edits that apply to any task. + do_result = getattr(args, "result", None) is not None + do_model = model_override is not None or clear_model + + if not do_result and not do_model: + print( + "kanban: nothing to edit (pass --result, --model, or --clear-model)", + file=sys.stderr, + ) + return 2 + + rc = 0 with kb.connect_closing() as conn: - if not kb.edit_completed_task_result( - conn, - args.task_id, - result=args.result, - summary=getattr(args, "summary", None), - metadata=metadata, - ): - print( - f"cannot edit {args.task_id} (unknown id or task is not done)", - file=sys.stderr, - ) - return 1 - print(f"Edited {args.task_id}") - return 0 + if do_model: + # --clear-model writes NULL; --model X writes X literally. The + # None sentinel ("--model omitted") never reaches here. + new_model = None if clear_model else model_override + affected = kb.set_task_model(conn, args.task_id, new_model) + if affected == 0: + print( + f"cannot set model on {args.task_id} (unknown id)", + file=sys.stderr, + ) + return 1 + if clear_model: + print(f"Cleared model override on {args.task_id}") + else: + print(f"Set model override on {args.task_id}: {new_model}") + if do_result: + if not kb.edit_completed_task_result( + conn, + args.task_id, + result=args.result, + summary=getattr(args, "summary", None), + metadata=metadata, + ): + print( + f"cannot edit {args.task_id} (unknown id or task is not done)", + file=sys.stderr, + ) + return 1 + print(f"Edited {args.task_id}") + return rc def _cmd_block(args: argparse.Namespace) -> int: diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index 8afb93c65784..07e74f113c16 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -2072,6 +2072,7 @@ def create_task( initial_status: str = "running", session_id: Optional[str] = None, board: Optional[str] = None, + model_override: Optional[str] = None, ) -> str: """Create a new task and optionally link it under parent tasks. @@ -2236,8 +2237,9 @@ def create_task( id, title, body, assignee, status, priority, created_by, created_at, workspace_kind, workspace_path, branch_name, tenant, idempotency_key, max_runtime_seconds, - skills, max_retries, goal_mode, goal_max_turns, session_id - ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + skills, max_retries, goal_mode, goal_max_turns, session_id, + model_override + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) """, ( task_id, @@ -2259,6 +2261,7 @@ def create_task( 1 if goal_mode else 0, int(goal_max_turns) if goal_max_turns is not None else None, session_id, + model_override, ), ) for pid in parents: @@ -4708,6 +4711,28 @@ def set_workspace_path( ) +def set_task_model( + conn: sqlite3.Connection, task_id: str, model: Optional[str] +) -> int: + """Set (or clear) a task's per-task model override. + + ``model`` is taken literally: a non-empty string pins that model, and + ``None`` writes SQL NULL (clears the override). The DB layer does NOT + interpret ``""`` — empty-string handling is a CLI concern; whatever is + passed is stored verbatim. + + Returns the number of rows affected: a call against a nonexistent + ``task_id`` returns ``0`` (never a silent success), so callers can tell + a real write from a no-op. + """ + with write_txn(conn): + cur = conn.execute( + "UPDATE tasks SET model_override = ? WHERE id = ?", + (model, task_id), + ) + return int(cur.rowcount or 0) + + # --------------------------------------------------------------------------- def schedule_task( conn: sqlite3.Connection, @@ -6758,6 +6783,16 @@ def _default_spawn( cmd.extend(["--skills", sk]) if task.model_override: cmd.extend(["-m", task.model_override]) + # Structured spawn line so per-task model overrides are auditable + # post-hoc ("why did this task cost Opus money"). Only emitted when + # an override is actually set — a cleared/no-override task logs + # nothing here, so a stale line never misattributes spend. + _log.info( + "kanban spawn task=%s assignee=%s model_override=%s", + task.id, + profile_arg, + task.model_override, + ) cmd.extend([ "chat", "-q", prompt, diff --git a/tests/hermes_cli/test_kanban_model_override.py b/tests/hermes_cli/test_kanban_model_override.py new file mode 100644 index 000000000000..0bd79bd606d3 --- /dev/null +++ b/tests/hermes_cli/test_kanban_model_override.py @@ -0,0 +1,340 @@ +"""Tests for the per-task model-override write path. + +Covers the WRITE side that was the only gap (the column, read, and the +`_default_spawn` `-m` append already existed): + + * ``create_task(model_override=...)`` persists; no-override → NULL. + * ``set_task_model`` clears to NULL and signals rows-affected. + * CLI ``--model`` / ``--clear-model`` / sentinel-untouched. + * argv-injection round-trip (write → SQLite → reload → spawn argv). + * retry/re-read path: a model changed between spawns is re-read. + * the structured spawn log line (emitted only when set). +""" + +from __future__ import annotations + +import logging +import re +from pathlib import Path + +import pytest + +from hermes_cli import kanban as kc +from hermes_cli import kanban_db as kb + + +@pytest.fixture +def kanban_home(tmp_path, monkeypatch): + home = tmp_path / ".hermes" + home.mkdir() + monkeypatch.setenv("HERMES_HOME", str(home)) + monkeypatch.setattr(Path, "home", lambda: tmp_path) + # Legacy crash-grace semantics are irrelevant here; keep instant reclaim + # so the retry path doesn't wait on a grace window. + monkeypatch.setenv("HERMES_KANBAN_CRASH_GRACE_SECONDS", "0") + kb.init_db() + return home + + +# --------------------------------------------------------------------------- +# Phase 1 — DB write path +# --------------------------------------------------------------------------- + +def test_create_task_persists_model_override(kanban_home): + with kb.connect() as conn: + tid = kb.create_task( + conn, title="opus task", assignee="worker", + model_override="claude-opus-4-8", + ) + task = kb.get_task(conn, tid) + assert task.model_override == "claude-opus-4-8" + + +def test_create_task_without_override_is_null(kanban_home): + """No-override create → column is SQL NULL, not '' and not a default.""" + with kb.connect() as conn: + tid = kb.create_task(conn, title="plain", assignee="worker") + task = kb.get_task(conn, tid) + raw = conn.execute( + "SELECT model_override FROM tasks WHERE id = ?", (tid,) + ).fetchone() + assert task.model_override is None + assert raw["model_override"] is None # genuine NULL at the storage layer + + +def test_set_task_model_sets_and_clears(kanban_home): + with kb.connect() as conn: + tid = kb.create_task(conn, title="x", assignee="worker") + # set + affected = kb.set_task_model(conn, tid, "claude-opus-4-8") + assert affected == 1 + assert kb.get_task(conn, tid).model_override == "claude-opus-4-8" + # clear → NULL + affected = kb.set_task_model(conn, tid, None) + assert affected == 1 + assert kb.get_task(conn, tid).model_override is None + raw = conn.execute( + "SELECT model_override FROM tasks WHERE id = ?", (tid,) + ).fetchone() + assert raw["model_override"] is None + + +def test_set_task_model_literal_empty_string(kanban_home): + """The DB layer takes str|None literally — it does NOT interpret ''.""" + with kb.connect() as conn: + tid = kb.create_task(conn, title="x", assignee="worker") + affected = kb.set_task_model(conn, tid, "") + assert affected == 1 + raw = conn.execute( + "SELECT model_override FROM tasks WHERE id = ?", (tid,) + ).fetchone() + # Stored verbatim as empty string, not coerced to NULL. + assert raw["model_override"] == "" + + +def test_set_task_model_nonexistent_id_rows_affected_zero(kanban_home): + with kb.connect() as conn: + affected = kb.set_task_model(conn, "t_doesnotexist", "claude-opus-4-8") + assert affected == 0 + + +# --------------------------------------------------------------------------- +# Phase 2 — CLI flags (via run_slash, the same entry CLI + gateway use) +# --------------------------------------------------------------------------- + +def _created_id(out: str) -> str: + m = re.search(r"(t_[a-f0-9]+)", out) + assert m, f"no task id in output: {out!r}" + return m.group(1) + + +def test_cli_create_model_shows_in_show(kanban_home): + out = kc.run_slash("create 'opus task' --assignee alice --model claude-opus-4-8") + tid = _created_id(out) + show = kc.run_slash(f"show {tid}") + assert "model:" in show + assert "claude-opus-4-8" in show + + +def test_cli_create_no_model_no_model_line(kanban_home): + out = kc.run_slash("create 'plain' --assignee alice") + tid = _created_id(out) + show = kc.run_slash(f"show {tid}") + assert "model:" not in show + + +def test_cli_edit_clear_model_removes_line(kanban_home): + out = kc.run_slash("create 'opus' --assignee alice --model claude-opus-4-8") + tid = _created_id(out) + assert "claude-opus-4-8" in kc.run_slash(f"show {tid}") + edited = kc.run_slash(f"edit {tid} --clear-model") + assert "Cleared model override" in edited + show = kc.run_slash(f"show {tid}") + assert "model:" not in show + + +def test_cli_edit_set_model(kanban_home): + out = kc.run_slash("create 'plain' --assignee alice") + tid = _created_id(out) + edited = kc.run_slash(f"edit {tid} --model claude-opus-4-8") + assert "Set model override" in edited + with kb.connect() as conn: + assert kb.get_task(conn, tid).model_override == "claude-opus-4-8" + + +def test_cli_edit_unrelated_field_leaves_model_untouched(kanban_home): + """Editing without --model must leave an existing override intact + (the None sentinel means 'unchanged', not 'clear').""" + out = kc.run_slash("create 'opus' --assignee alice --model claude-opus-4-8") + tid = _created_id(out) + # An edit that touches nothing model-related: pass only --result. + # (result-backfill needs a done task; we don't assert it succeeds — we + # assert the model override is left alone regardless.) + kc.run_slash(f"edit {tid} --result 'some backfill'") + with kb.connect() as conn: + assert kb.get_task(conn, tid).model_override == "claude-opus-4-8" + + +def test_cli_edit_model_and_clear_model_mutually_exclusive(kanban_home): + out = kc.run_slash("create 'x' --assignee alice") + tid = _created_id(out) + res = kc.run_slash(f"edit {tid} --model claude-opus-4-8 --clear-model") + assert "mutually exclusive" in res + + +def test_cli_edit_nonexistent_id_reports_failure(kanban_home): + res = kc.run_slash("edit t_nope --model claude-opus-4-8") + assert "cannot set model" in res + + +# --------------------------------------------------------------------------- +# Phase 3 — argv-injection round-trip (write → store → reload → spawn argv) +# --------------------------------------------------------------------------- + +def _spawn_argv_for(monkeypatch, task) -> list: + """Drive _default_spawn with Popen stubbed; return the captured argv.""" + monkeypatch.setattr(kb, "_kanban_worker_skill_available", lambda _h: False) + captured = {} + + class FakeProc: + pid = 4242 + + def fake_popen(cmd, **kwargs): + captured["cmd"] = list(cmd) + return FakeProc() + + monkeypatch.setattr("subprocess.Popen", fake_popen) + workspace = kb.resolve_workspace(task) + pid = kb._default_spawn(task, str(workspace)) + assert pid == 4242 + return captured["cmd"] + + +@pytest.mark.parametrize("injected", ["x ; y", "a\nb", "--provider evil", "$(rm -rf /)"]) +def test_argv_injection_single_m_token_round_trip(kanban_home, monkeypatch, injected): + """A metacharacter/newline/flag-like model string set via the CLI must + survive write → SQLite → reload and land as EXACTLY ONE -m value token, + byte-for-byte equal to the stored string.""" + # Write via the CLI write path using set_task_model semantics (the CLI + # edit path). Use the DB setter directly to avoid shell-quoting in the + # test harness masking the round-trip — the point is the store→reload→argv + # leg, and the CLI uses the same setter. + with kb.connect() as conn: + tid = kb.create_task(conn, title="inject", assignee="worker") + affected = kb.set_task_model(conn, tid, injected) + assert affected == 1 + + # Reload from a fresh connection — proves persistence, not in-memory state. + with kb.connect() as conn: + task = kb.get_task(conn, tid) + assert task.model_override == injected # byte-for-byte after reload + + argv = _spawn_argv_for(monkeypatch, task) + # Exactly one -m, and its value is the stored string verbatim. + assert argv.count("-m") == 1 + idx = argv.index("-m") + assert argv[idx + 1] == injected + # Nothing split it into a second argv token. + assert injected in argv + assert argv.count(injected) == 1 + + +def test_argv_injection_via_cli_create(kanban_home, monkeypatch): + """Same round-trip but the write enters through the CLI create flag, + proving CLI arg parsing doesn't split the value either.""" + # shlex in run_slash handles quoting; embed a metachar string. + out = kc.run_slash("create 'inj' --assignee worker --model 'x ; y'") + tid = _created_id(out) + with kb.connect() as conn: + task = kb.get_task(conn, tid) + assert task.model_override == "x ; y" + argv = _spawn_argv_for(monkeypatch, task) + assert argv.count("-m") == 1 + assert argv[argv.index("-m") + 1] == "x ; y" + + +# --------------------------------------------------------------------------- +# Phase 3 — retry / re-read path: model changed between spawns is re-read +# --------------------------------------------------------------------------- + +def test_model_reread_on_retry_spawn(kanban_home, monkeypatch, all_assignees_spawnable): + """dispatch → worker exits → set_task_model to a NEW value between + attempts → the SECOND spawn actually occurs and carries the new model.""" + spawns: list[list] = [] + + def _stub_spawn(task, ws, *, board=None): + # Capture the argv the real _default_spawn WOULD build, so we assert + # the model the dispatcher would pass on this attempt. We rebuild the + # -m portion from the task the dispatcher handed us (which it re-read + # from the DB at claim time). + argv = ["hermes", "-p", task.assignee or ""] + if task.model_override: + argv += ["-m", task.model_override] + spawns.append(argv) + # Return a pid that is already dead so the next dispatch reclaims it. + return 2 # init; effectively never our child → treated as crashed + + conn = kb.connect() + try: + tid = kb.create_task( + conn, title="retry", assignee="worker", + model_override="claude-sonnet-4-5", + ) + # First dispatch → spawn #1 with the original model. + kb.dispatch_once(conn, spawn_fn=_stub_spawn) + assert len(spawns) == 1 + assert spawns[0][-2:] == ["-m", "claude-sonnet-4-5"] + + # Worker "exits": clear the pid + return the task to ready so the + # next tick re-dispatches it. detect_crashed_workers handles the + # dead pid; force the task back to ready directly to keep the test + # deterministic regardless of crash-detection timing. + with kb.write_txn(conn): + conn.execute( + "UPDATE tasks SET status='ready', claim_lock=NULL, " + "claim_expires=NULL, worker_pid=NULL WHERE id=?", + (tid,), + ) + + # Change the model BETWEEN attempts. + assert kb.set_task_model(conn, tid, "claude-opus-4-8") == 1 + + # Second dispatch → spawn #2 must occur AND carry the new model. + kb.dispatch_once(conn, spawn_fn=_stub_spawn) + assert len(spawns) == 2, "retry must re-invoke the spawn fn" + assert spawns[1][-2:] == ["-m", "claude-opus-4-8"] + finally: + conn.close() + + +# --------------------------------------------------------------------------- +# Phase 3 — observability: spawn log line emitted ONLY when set +# --------------------------------------------------------------------------- + +def test_spawn_logs_override_when_set(kanban_home, monkeypatch, caplog): + monkeypatch.setattr(kb, "_kanban_worker_skill_available", lambda _h: False) + + class FakeProc: + pid = 7 + + monkeypatch.setattr("subprocess.Popen", lambda cmd, **kw: FakeProc()) + + with kb.connect() as conn: + tid = kb.create_task( + conn, title="x", assignee="worker", + model_override="claude-opus-4-8", + ) + task = kb.get_task(conn, tid) + workspace = kb.resolve_workspace(task) + + with caplog.at_level(logging.INFO, logger="hermes_cli.kanban_db"): + kb._default_spawn(task, str(workspace)) + + msgs = [r.getMessage() for r in caplog.records] + assert any( + "kanban spawn" in m and f"task={tid}" in m + and "model_override=claude-opus-4-8" in m + for m in msgs + ), f"expected structured spawn line, got: {msgs}" + + +def test_spawn_no_log_when_no_override(kanban_home, monkeypatch, caplog): + monkeypatch.setattr(kb, "_kanban_worker_skill_available", lambda _h: False) + + class FakeProc: + pid = 8 + + monkeypatch.setattr("subprocess.Popen", lambda cmd, **kw: FakeProc()) + + with kb.connect() as conn: + tid = kb.create_task(conn, title="x", assignee="worker") + task = kb.get_task(conn, tid) + workspace = kb.resolve_workspace(task) + + with caplog.at_level(logging.INFO, logger="hermes_cli.kanban_db"): + kb._default_spawn(task, str(workspace)) + + msgs = [r.getMessage() for r in caplog.records] + assert not any("model_override=" in m for m in msgs), ( + f"no override → must emit no model_override spawn line, got: {msgs}" + ) diff --git a/tests/tools/test_kanban_tools.py b/tests/tools/test_kanban_tools.py index 2bf89449905a..9f713cf2ae3d 100644 --- a/tests/tools/test_kanban_tools.py +++ b/tests/tools/test_kanban_tools.py @@ -925,6 +925,45 @@ def test_create_rejects_no_assignee(worker_env): assert json.loads(kt._handle_create({"title": "t"})).get("error") +def test_create_persists_model_override(worker_env): + """kanban_create accepts model_override and persists it (the read side + at 327/372 already surfaces it).""" + from tools import kanban_tools as kt + from hermes_cli import kanban_db as kb + d = json.loads(kt._handle_create({ + "title": "opus child", "assignee": "peer", + "model_override": "claude-opus-4-8", + })) + assert d["ok"] is True + conn = kb.connect() + try: + child = kb.get_task(conn, d["task_id"]) + assert child.model_override == "claude-opus-4-8" + finally: + conn.close() + + +def test_create_no_model_override_is_null(worker_env): + from tools import kanban_tools as kt + from hermes_cli import kanban_db as kb + d = json.loads(kt._handle_create({"title": "plain child", "assignee": "peer"})) + assert d["ok"] is True + conn = kb.connect() + try: + child = kb.get_task(conn, d["task_id"]) + assert child.model_override is None + finally: + conn.close() + + +def test_create_rejects_non_string_model_override(worker_env): + from tools import kanban_tools as kt + out = kt._handle_create({ + "title": "t", "assignee": "a", "model_override": 42, + }) + assert json.loads(out).get("error") + + def test_create_rejects_non_list_parents(worker_env): from tools import kanban_tools as kt out = kt._handle_create({"title": "t", "assignee": "a", "parents": 42}) diff --git a/tools/kanban_tools.py b/tools/kanban_tools.py index 67157dfc1c62..faf4cc547f19 100644 --- a/tools/kanban_tools.py +++ b/tools/kanban_tools.py @@ -773,6 +773,12 @@ def _handle_create(args: dict, **kw) -> str: if goal_bool_error: return tool_error(goal_bool_error) goal_max_turns = args.get("goal_max_turns") + model_override = args.get("model_override") + if model_override is not None and not isinstance(model_override, str): + return tool_error( + f"model_override must be a model name string, got " + f"{type(model_override).__name__}" + ) if isinstance(parents, str): parents = [parents] if not isinstance(parents, (list, tuple)): @@ -816,6 +822,7 @@ def _handle_create(args: dict, **kw) -> str: initial_status=str(initial_status), created_by=os.environ.get("HERMES_PROFILE") or "worker", session_id=session_id, + model_override=model_override, ) new_task = kb.get_task(conn, new_tid) return _ok( @@ -1277,6 +1284,17 @@ def _board_schema_prop() -> dict[str, str]: "assignee's profile." ), }, + "model_override": { + "type": "string", + "description": ( + "Per-task model override. Pins the dispatched worker " + "to this model (passed as `hermes -m MODEL`) instead " + "of the assignee profile's default model. Use this to " + "run one task on a stronger/cheaper model without " + "cloning a whole profile. Omit to use the profile " + "default." + ), + }, "goal_mode": { "type": "boolean", "description": (