diff --git a/hermes_cli/kanban_survivor.py b/hermes_cli/kanban_survivor.py index 904fc794f62db..a357dedbf1659 100644 --- a/hermes_cli/kanban_survivor.py +++ b/hermes_cli/kanban_survivor.py @@ -346,9 +346,34 @@ def preserve(conn, task_id, metadata=None, *, cleanup=False, workspace=None, # A patch cannot add a gitlink and files below the same path. raise SurvivorUnavailable("survivor_unavailable: nested repository requires separate recovery") keys = {str(r.relative_to(workspace)) for r in repos} + recovered = None if set(bases) - keys: - raise SurvivorUnavailable("survivor_unavailable: recorded repository missing") - patches, refs, bundles, repositories = [], [], [], [] + # The repository recorded at dispatch is gone while the directory + # survived (a reaped clone leaving evidence behind). Without an + # operator survivor this must stay fail-closed: it is what protects + # unpushed implementation work. But the workspace-MISSING branch + # above already treats a remote-verified `--survivor-pr`/`ref` as + # authority, and an operator-named, remote-verified survivor is + # strictly better evidence than the checkout we lost -- so consult + # it here too, or this state has no reachable remedy at all. + if not explicit: + raise SurvivorUnavailable( + f"survivor_unavailable: recorded repository missing; {_ext.HINT}" + ) + # A vanished recorded repository is an unmet claim. Force the + # external path below so the verified survivor is actually recorded, + # and so failing to produce one still HOLDS rather than completing + # with no survivor at all. `recovered` additionally carries it onto + # the in-tree paths: when only SOME recorded repos vanished, the + # survivors of the rest would otherwise satisfy the completion on + # their own and silently drop the operator's ref for the lost one. + claimed = True + if repos: + # Only a PARTIAL loss needs this. With no repo left at all the + # external path below records `explicit` on its own; seeding it + # here too would duplicate the ref. + recovered = dict(explicit, repository=sorted(set(bases) - keys)[0]) + patches, refs, bundles, repositories = [], [recovered] if recovered else [], [], [] for repo in repos: key = str(repo.relative_to(workspace)) published = list(_published_refs(repo, workspace)) @@ -371,7 +396,7 @@ def preserve(conn, task_id, metadata=None, *, cleanup=False, workspace=None, header = f"# kanban repository={json.dumps(key)} base={base}\n".encode() patches.append(header + data) repositories.append({"repository": key, "base_sha": base}) - if repos and len(refs) == len(repos): + if repos and len(refs) == len(repos) + (1 if recovered else 0): survivor = {"kind": "ref", "refs": refs} elif patches or bundles: data = b"".join(patches) diff --git a/tests/hermes_cli/test_kanban_survivor_stale_bases.py b/tests/hermes_cli/test_kanban_survivor_stale_bases.py new file mode 100644 index 0000000000000..dc7a14e5dfd67 --- /dev/null +++ b/tests/hermes_cli/test_kanban_survivor_stale_bases.py @@ -0,0 +1,211 @@ +"""A vanished recorded repository must still have a reachable remedy. + +`preserve()` records `bases` at dispatch, from the git checkout that was in the +workspace then. When that checkout is later gone but the workspace DIRECTORY +survives (evidence, logs, qa-output), the recorded-repository guard fired +unconditionally -- on the branch where `explicit` is never consulted. The +operator escape hatch documented in the error (`--survivor-pr` / `--survivor-ref`) +was therefore structurally unreachable for that state. + +These tests pin the remedy AND the guard: an operator-named, remote-VERIFIED +survivor satisfies the card; anything less still fails closed. +""" +import json +import subprocess +from pathlib import Path + +import pytest + +from hermes_cli import kanban_db as kb + +HEAD = "a1" * 20 +PR = "example/project#68" +STALE = "af0d85e37470550d554abb89a5cd51039dbbe358" + + +@pytest.fixture +def board(tmp_path, monkeypatch): + monkeypatch.setenv("HERMES_HOME", str(tmp_path / "home")) + monkeypatch.setattr(Path, "home", lambda: tmp_path) + with kb.connect_closing() as conn: + yield conn + + +@pytest.fixture +def remote(monkeypatch): + """Answer remote lookups affirmatively; the gate must not rely on a network failure.""" + state = {"state": "OPEN", "headRefOid": HEAD, "mergeCommit": None} + real = subprocess.run + + def run(args, **kwargs): + if args[0] == "gh": + return subprocess.CompletedProcess(args, 0, json.dumps(state).encode(), b"") + return real(args, **kwargs) + + monkeypatch.setattr(subprocess, "run", run) + return state + + +def stale_card(conn, *, title="review lane"): + """A card whose recorded repo is gone but whose workspace dir survives.""" + tid = kb.create_task(conn, title=title) + ws = kb.resolve_workspace(kb.get_task(conn, tid)) + (ws / "qa-output").mkdir(parents=True, exist_ok=True) + (ws / "qa-output" / "verdict.md").write_text("APPROVED\n") + kb.set_workspace_path(conn, tid, ws) + with kb.write_txn(conn): + conn.execute( + "INSERT INTO task_workspace_survivors(task_id, bases) VALUES (?, ?) " + "ON CONFLICT(task_id) DO UPDATE SET bases = excluded.bases", + (tid, json.dumps({".": STALE})), + ) + return tid, ws + + +def test_stale_bases_with_a_verified_survivor_pr_completes(board, remote): + """The case that was impossible: dir exists, repo gone, operator names a PR.""" + tid, ws = stale_card(board) + + assert kb.complete_task(board, tid, summary="approved", survivor_pr=PR) + + saved = kb.latest_run(board, tid).metadata["survivor"] + assert saved["kind"] == "ref" + ref = saved["refs"][0] + # The recorded survivor is the remote-verified external ref, not the dead base. + assert ref["sha"] == HEAD and ref["pr"] == PR and ref["external"] is True + assert ref["sha"] != STALE + assert kb.get_task(board, tid).status == "done" + + +def test_stale_bases_without_an_explicit_survivor_still_refuses(board, remote): + """Guard preserved: no operator survivor means no completion, workspace held.""" + tid, ws = stale_card(board) + + with pytest.raises(ValueError) as excinfo: + kb.complete_task(board, tid, summary="approved") + + assert "recorded repository missing" in str(excinfo.value) + # The error must now name the remedy it previously withheld. + assert "--survivor-pr" in str(excinfo.value) + assert kb.get_task(board, tid).status != "done" + assert (ws / "qa-output" / "verdict.md").is_file(), "held workspace must survive" + + +def test_stale_bases_with_an_unverifiable_survivor_pr_still_refuses(board, remote): + """An operator CLAIM is not authority: a closed PR buys nothing.""" + remote["state"] = "CLOSED" + tid, ws = stale_card(board) + + with pytest.raises(ValueError) as excinfo: + kb.complete_task(board, tid, summary="approved", survivor_pr=PR) + + assert "could not verify --survivor-pr" in str(excinfo.value) + assert kb.get_task(board, tid).status != "done" + assert (ws / "qa-output" / "verdict.md").is_file() + + +def test_stale_bases_does_not_fall_through_to_text_mining(board, remote): + """Teeth: the relaxation is for OPERATOR flags only, never a mined hint. + + Without this, forcing the external path could let `discover()` mine a PR out + of the handoff and convert a fail-closed HOLD into a delete -- the exact + regression test_kanban_survivor_authority.py exists to prevent. + """ + tid, ws = stale_card(board) + kb.add_comment(board, tid, "reviewer", f"context: unrelated {PR} landed earlier") + + with pytest.raises(ValueError): + kb.complete_task(board, tid, result=f"see {PR} at {HEAD}", summary="approved") + + assert kb.get_task(board, tid).status != "done" + assert (ws / "qa-output" / "verdict.md").is_file() + + +# --- CLI surface: a refused terminal transition must be LOUD ---------------- + +def _cli(board, monkeypatch, argv): + import argparse + import contextlib + + from hermes_cli import kanban as cli + + parser = argparse.ArgumentParser(prog="hermes", add_help=False) + cli.build_parser(parser.add_subparsers(dest="command")) + monkeypatch.setattr(kb, "connect_closing", + lambda *a, **k: contextlib.nullcontext(board)) + return cli.kanban_command(parser.parse_args(argv)) + + +def test_cli_refusal_exits_non_zero_with_the_reason_and_hint(board, remote, monkeypatch, capsys): + """A caller must be able to tell 'completed' from 'refused' by exit code.""" + tid, _ = stale_card(board) + + rc = _cli(board, monkeypatch, ["kanban", "complete", tid, "--summary", "ok"]) + err = capsys.readouterr().err + + assert rc != 0, "a refused terminal transition must not report success" + assert "recorded repository missing" in err + assert "--survivor-pr" in err and "--survivor-ref" in err + assert kb.get_task(board, tid).status != "done" + + +def test_cli_successful_completion_exits_zero_and_says_so(board, remote, monkeypatch, capsys): + """Teeth for the test above: the happy path must stay quiet and green.""" + tid, _ = stale_card(board) + + rc = _cli(board, monkeypatch, + ["kanban", "complete", tid, "--summary", "ok", "--survivor-pr", PR]) + out = capsys.readouterr() + + assert rc == 0 + assert f"Completed {tid}" in out.out + assert kb.get_task(board, tid).status == "done" + + +def test_partial_loss_keeps_both_the_surviving_repo_and_the_operator_ref(board, remote, tmp_path, monkeypatch): + """Only SOME recorded repos vanished: neither survivor may be dropped. + + The surviving repo resolves its own remote ref, which on its own would + satisfy the completion and silently discard the operator's ref for the repo + that is gone -- leaving that work pointed at nothing. + """ + import hermes_cli.kanban_survivor as survivor + monkeypatch.setattr(survivor, "_temporary_roots", lambda: [tmp_path / "temporary"]) + + def git(repo, *args): + return subprocess.run([ + "git", "-C", str(repo), *args + ], capture_output=True, check=True).stdout.decode().strip() + + tid = kb.create_task(board, title="partial loss") + ws = kb.resolve_workspace(kb.get_task(board, tid)) + kept = ws / "kept" + kept.mkdir(parents=True) + git(kept, "init", "-b", "main") + git(kept, "config", "user.name", "Test") + git(kept, "config", "user.email", "test@example.invalid") + (kept / "a.py").write_text("value = 1\n") + git(kept, "add", ".") + git(kept, "commit", "-m", "base") + git(kept, "init", "--bare", str(tmp_path / "kept.git")) + git(kept, "remote", "add", "origin", str(tmp_path / "kept.git")) + git(kept, "push", "origin", "HEAD:main") + kb.set_workspace_path(board, tid, ws) + with kb.write_txn(board): + board.execute( + "INSERT INTO task_workspace_survivors(task_id, bases) VALUES (?, ?) " + "ON CONFLICT(task_id) DO UPDATE SET bases = excluded.bases", + (tid, json.dumps({"kept": git(kept, "rev-parse", "HEAD"), "gone": STALE})), + ) + + assert kb.complete_task(board, tid, summary="approved", survivor_pr=PR) + + saved = kb.latest_run(board, tid).metadata["survivor"] + by_repo = {ref["repository"]: ref for ref in saved["refs"]} + assert by_repo["gone"]["pr"] == PR, "the lost repo must carry the operator's verified ref" + assert by_repo["kept"]["remote"] == "origin", "the surviving repo keeps its own ref" + # Exactly one ref per recorded repository. Falling through to the external + # path instead of resolving here appends a SECOND copy of the operator ref + # under repository ".", which does not correspond to anything on disk. + assert len(saved["refs"]) == 2, saved["refs"] + assert set(by_repo) == {"gone", "kept"}, saved["refs"] diff --git a/tests/tools/test_kanban_tool_survivor.py b/tests/tools/test_kanban_tool_survivor.py new file mode 100644 index 0000000000000..350293e4ad1f4 --- /dev/null +++ b/tests/tools/test_kanban_tool_survivor.py @@ -0,0 +1,186 @@ +"""The ``kanban_complete`` TOOL must reach the survivor escape hatch it names. + +`preserve()` refuses a completion whose implementation it cannot find and points +at ``--survivor-pr`` / ``--survivor-ref``. That remedy was reachable from the CLI +and the library but NOT from the tool — the agent-facing surface where the +refusal is actually read, so a worker read a hint it had no way to act on. + +These tests pin the remedy AND the guard: a remote-VERIFIED operator claim +satisfies the card through the tool, anything less still fails closed, and the +tool is never a softer path than the flag. +""" +from __future__ import annotations + +import json +import subprocess +from pathlib import Path + +import pytest + +HEAD = "a1" * 20 +MERGE = "b2" * 20 +PR = "example/project#68" +URL = "https://github.com/example/project.git" +STALE = "af0d85e37470550d554abb89a5cd51039dbbe358" + + +@pytest.fixture +def worker_env(monkeypatch, tmp_path): + """Isolated HERMES_HOME with a claimed task this process OWNS.""" + import os + + home = tmp_path / ".hermes" + home.mkdir() + monkeypatch.setenv("HERMES_HOME", str(home)) + monkeypatch.setenv("HERMES_PROFILE", "test-worker") + monkeypatch.setenv("HERMES_KANBAN_SANDBOX", "1") + for pin in ("HERMES_KANBAN_DB", "HERMES_KANBAN_BOARD", + "HERMES_KANBAN_WORKSPACES_ROOT", "HERMES_SESSION_ID"): + monkeypatch.delenv(pin, raising=False) + monkeypatch.setattr(Path, "home", lambda: tmp_path) + + from hermes_cli import kanban_db as kb + kb._INITIALIZED_PATHS.clear() + kb.init_db() + conn = kb.connect() + try: + tid = kb.create_task(conn, title="worker-test", assignee="test-worker") + kb.claim_task(conn, tid) + run = kb.latest_run(conn, tid) + finally: + conn.close() + monkeypatch.setenv("HERMES_KANBAN_TASK", tid) + monkeypatch.setenv("HERMES_KANBAN_RUN_ID", str(run.id)) + # This process owns the run, so the expected_run_id guard applies — the + # property a shell-out to the CLI silently loses. + monkeypatch.setenv("HERMES_KANBAN_OWNER_PID", str(os.getpid())) + return tid + + +@pytest.fixture +def remote(monkeypatch): + """Answer remote lookups affirmatively; no gate may rest on a network failure.""" + state = {"state": "MERGED", "headRefOid": HEAD, "mergeCommit": {"oid": MERGE}} + calls = [] + real = subprocess.run + + def run(args, **kwargs): + if args and args[0] == "gh": + calls.append(list(args)) + return subprocess.CompletedProcess(args, 0, json.dumps(state).encode(), b"") + if "ls-remote" in args: + calls.append(list(args)) + return subprocess.CompletedProcess( + args, 0, f"{HEAD}\trefs/heads/feature\n".encode(), b"") + return real(args, **kwargs) + + monkeypatch.setattr(subprocess, "run", run) + return state, calls + + +def stale_bases(tid): + """Make the card's recorded repository vanish while its workspace survives. + + This is the exact state whose refusal names ``--survivor-pr``. + """ + from hermes_cli import kanban_db as kb + with kb.connect_closing() as conn: + ws = kb.resolve_workspace(kb.get_task(conn, tid)) + (ws / "qa-output").mkdir(parents=True, exist_ok=True) + (ws / "qa-output" / "verdict.md").write_text("APPROVED\n") + kb.set_workspace_path(conn, tid, ws) + with kb.write_txn(conn): + conn.execute( + "INSERT INTO task_workspace_survivors(task_id, bases) VALUES (?, ?) " + "ON CONFLICT(task_id) DO UPDATE SET bases = excluded.bases", + (tid, json.dumps({".": STALE})), + ) + return ws + + +def complete(**args): + from tools import kanban_tools as kt + return json.loads(kt._handle_complete(args)) + + +def task_state(tid): + from hermes_cli import kanban_db as kb + with kb.connect_closing() as conn: + return kb.get_task(conn, tid), kb.latest_run(conn, tid) + + +def test_tool_survivor_pr_completes_a_stale_bases_card(worker_env, remote): + """The case that was unreachable from the tool: dir exists, repo gone.""" + stale_bases(worker_env) + + out = complete(summary="approved", survivor_pr=PR) + + assert "error" not in out, out + task, run = task_state(worker_env) + assert task.status == "done" + ref = run.metadata["survivor"]["refs"][0] + assert ref["pr"] == PR and ref["sha"] == MERGE + assert remote[1], "must consult the remote, not accept the tool argument" + + +def test_tool_survivor_pr_that_does_not_verify_still_refuses(worker_env, remote): + """A tool argument is a claim, not authority.""" + remote[0]["state"] = "CLOSED" + stale_bases(worker_env) + + out = complete(summary="approved", survivor_pr=PR) + + assert "survivor" in out.get("error", "").lower(), out + assert task_state(worker_env)[0].status != "done" + + +def test_tool_cannot_satisfy_the_guard_from_prose_alone(worker_env, remote): + """Without the argument, naming the PR in the summary must not complete it. + + Text is a hint; the relaxed stale-bases branch is reachable only by an + explicit, verified claim. If prose sufficed, the new argument would be + decoration and the guard would already be gone. + """ + stale_bases(worker_env) + + out = complete(summary=f"approved, shipped {PR} at {HEAD}", + metadata={"changed_files": ["code.py"]}) + + assert "survivor" in out.get("error", "").lower(), out + assert task_state(worker_env)[0].status != "done" + + +def test_tool_survivor_ref_rejection_is_redacted(worker_env, remote): + """An unverifiable ref may carry a token: the tool's error must not echo it.""" + leaky = f"https://oauth2:ghp_SECRET123@github.com/example/project.git#{'c3' * 20}" + + out = complete(summary="approved", survivor_ref=leaky) + + error = out.get("error", "") + assert "could not verify" in error, out + assert "ghp_SECRET123" not in error + assert "oauth2" not in error + + from hermes_cli import kanban_db as kb + with kb.connect_closing() as conn: + rows = conn.execute( + "SELECT held_reason FROM task_workspace_survivors WHERE task_id = ?", + (worker_env,)).fetchall() + assert all("ghp_SECRET123" not in (r[0] or "") for r in rows) + assert not any("ghp_SECRET123" in json.dumps(e.payload) + for e in kb.list_events(conn, worker_env)) + + +def test_tool_schema_exposes_both_survivor_arguments(): + """The refusal names a remedy; the schema is what makes it callable.""" + from tools import kanban_tools as kt + props = kt.KANBAN_COMPLETE_SCHEMA["parameters"]["properties"] + assert {"survivor_pr", "survivor_ref"} <= set(props) + assert all(props[k]["type"] == "string" for k in ("survivor_pr", "survivor_ref")) + + +def test_non_string_survivor_argument_is_rejected_not_coerced(worker_env, remote): + """A malformed claim must fail loudly, never reach the verifier as junk.""" + out = complete(summary="approved", survivor_pr={"repo": "example/project"}) + assert "survivor_pr must be a string" in out.get("error", ""), out + assert remote[1] == [], "must not consult the remote on a malformed claim" diff --git a/tools/kanban_tools.py b/tools/kanban_tools.py index a6b75e5460543..bba95917c85ab 100644 --- a/tools/kanban_tools.py +++ b/tools/kanban_tools.py @@ -822,6 +822,14 @@ def _handle_complete(args: dict, **kw) -> str: f"metadata must be an object/dict, got {type(metadata).__name__}" ) metadata = _stamp_worker_session_metadata(tid, metadata) + survivor_pr, survivor_ref = args.get("survivor_pr"), args.get("survivor_ref") + for value, name in ((survivor_pr, "survivor_pr"), (survivor_ref, "survivor_ref")): + if value is not None and not isinstance(value, str): + return tool_error( + f"{name} must be a string, got {type(value).__name__}" + ) + survivor_pr = (survivor_pr or "").strip() or None + survivor_ref = (survivor_ref or "").strip() or None board = args.get("board") try: kb, conn = _connect(board=board) @@ -851,6 +859,7 @@ def _handle_complete(args: dict, **kw) -> str: result=result, summary=summary, metadata=metadata, created_cards=created_cards, expected_run_id=_worker_run_id(tid), + survivor_pr=survivor_pr, survivor_ref=survivor_ref, ) except kb.ArtifactPreservationError as artifact_err: return tool_error( @@ -2019,6 +2028,33 @@ def _board_schema_prop() -> dict[str, str]: "task in-flight so you can fix the path and retry." ), }, + "survivor_pr": { + "type": "string", + "description": ( + "Only when completion already REFUSED with " + "``survivor_unavailable``: name the pull request that " + "holds this task's implementation, as " + "``owner/repo#123`` or its github.com URL. The kernel " + "verifies it against the remote (it must exist and be " + "OPEN or MERGED) and records it as the durable " + "survivor; an unverifiable claim still refuses. This " + "is the ``--survivor-pr`` escape hatch that error " + "names. Never pass it speculatively — it authorises " + "deleting a workspace whose work is not pushed." + ), + }, + "survivor_ref": { + "type": "string", + "description": ( + "Alternative to ``survivor_pr`` when the work landed " + "on a branch or tag rather than a PR: " + "``#``. The SHA must be a current " + "branch/tag tip on that remote or the completion " + "still refuses. Use a clean clone URL — a URL " + "carrying credentials is rejected, and is redacted " + "before the rejection is echoed or logged." + ), + }, "board": _board_schema_prop(), }, "required": [],