diff --git a/README.md b/README.md index 6ef589d..a3368ce 100644 --- a/README.md +++ b/README.md @@ -342,9 +342,14 @@ All routes require `Authorization: Bearer `. `GET /health` is unauth Wired into each agent as `python -m handler.hooks `: -- **`Stop` / `SessionEnd`** — checkpoint. On `Stop`, run `mise run test`; on failure, - return `decision: "block"` with the output so the turn cannot end on red. Records - `tests_status` / `tested_at`; `status = 'done'` only ever accompanies a pass. +- **`Stop` / `SessionEnd`** — checkpoint + completion gate. On `Stop`, run + `mise run test` and check the tree deterministically: failing tests, uncommitted + changes, or commits that exist only locally each return `decision: "block"` with the + full blocker list, so a turn cannot end on red or walk away from unshipped work. + Records `tests_status` / `tested_at`; `status = 'done'` only ever accompanies a + passing gate (tests green, tree clean, everything pushed). The agent's final message + is captured from the session transcript onto the checkmark, so the dashboard always + shows a real checkpoint whether or not the agent thought to leave one. - **`PreToolUse`** — two jobs. An `AskUserQuestion` is *deferred*: the question is persisted, the checkmark set to `paused_for_input`, and the tool call denied so control hands off to the async answer/resume flow. A `Bash` command running `git push` triggers diff --git a/src/handler/control/gitops.py b/src/handler/control/gitops.py index 71ec0c1..9f9fc9a 100644 --- a/src/handler/control/gitops.py +++ b/src/handler/control/gitops.py @@ -82,6 +82,29 @@ def ahead_count(cwd: str) -> int | None: return None +def has_origin(cwd: str) -> bool: + """Whether the repo has an ``origin`` remote configured.""" + ok, out = _run(["remote"], cwd) + return ok and "origin" in out.split() + + +def unpushed_count(cwd: str) -> int | None: + """Commits reachable from HEAD that no ``origin/*`` ref contains. + + 0 means everything on the current line of history has been pushed *somewhere* on + origin. Works with or without an upstream — worktree branches are created with + ``--no-track``, so an ``@{upstream}``-based count would come up empty for them. + ``None`` when the count cannot be computed. + """ + ok, out = _run(["rev-list", "--count", "HEAD", "--not", "--remotes=origin"], cwd) + if not ok: + return None + try: + return int(out.strip()) + except ValueError: + return None + + def add(cwd: str, paths: list[str]) -> tuple[bool, str]: return _run(["add", *paths], cwd) diff --git a/src/handler/control/headless.py b/src/handler/control/headless.py index a50ba64..7719143 100644 --- a/src/handler/control/headless.py +++ b/src/handler/control/headless.py @@ -337,13 +337,15 @@ class RunSupervisor: ) # Hooks are the authority on agent status — they ran inside the run and # may have set paused_for_input/blocked/done already. Only an agent still - # marked ``working`` needs the process's verdict. + # marked ``working`` needs the process's verdict — and ``done`` is a gate + # outcome, not an exit code: an agent still ``working`` after a clean + # exit means the Stop gate never recorded a verdict (hook misfired, + # identity missing), so nothing has verified its tests/commits/pushes. + # An operator cancel is the one completion that needs no gate. agent = repo.get_agent_by_id(conn, self.agent["id"]) if finished and agent is not None and agent["status"] == "working": if self._canceled: repo.set_agent_status(conn, self.agent["id"], "done") - elif clean: - repo.set_agent_status(conn, self.agent["id"], "done") else: repo.set_agent_status(conn, self.agent["id"], "blocked") if not clean and not self._canceled: diff --git a/src/handler/hooks/checkpoint.py b/src/handler/hooks/checkpoint.py index d36b7d4..9004b9f 100644 --- a/src/handler/hooks/checkpoint.py +++ b/src/handler/hooks/checkpoint.py @@ -1,14 +1,19 @@ """Stop / SessionEnd — the checkpoint + verification gate (README 3.5). -On ``Stop`` the gate runs the project's own ``test`` task and blocks the turn on -failure, so a turn cannot end on a broken suite. The result feeds straight into the -schema: ``status = 'done'`` is only ever recorded alongside a passing test run — not a -claim taken on faith. ``SessionEnd`` cannot be blocked, so it just records a final -checkpoint with the end reason. +On ``Stop`` the gate runs the project's own ``test`` task and checks the working tree +deterministically: failing tests, uncommitted changes, or commits that exist only +locally each block the turn, so an agent cannot end on a broken suite or walk away +from work it never committed or pushed. ``status = 'done'`` is only ever recorded +alongside a passing gate — not a claim taken on faith. The agent's final message is +captured from the session transcript onto the checkmark, so the dashboard always has a +real checkpoint to show regardless of whether the agent thought to leave one. +``SessionEnd`` cannot be blocked, so it just records a final checkpoint with the end +reason. """ from __future__ import annotations +import json from datetime import UTC, datetime from sqlalchemy import Connection @@ -84,16 +89,89 @@ def handle_mise_init_stop(conn: Connection, ident: Identity, hook_input: HookInp return {} +def _completion_blockers(working_dir: str) -> list[str]: + """The commit/push half of the completion gate — ``[]`` when the tree is settled. + + Deterministic checks against git itself, not the agent's account of its work: a + dirty tree means work was never committed; commits no ``origin/*`` ref contains + mean work was never pushed. A working dir that isn't a git checkout (manually + managed roots, empty repos) has nothing to gate. + """ + if gitops.head_sha(working_dir) is None: + return [] + blockers = [] + if not gitops.is_clean(working_dir): + blockers.append("there are uncommitted changes — commit your work") + if gitops.has_origin(working_dir): + unpushed = gitops.unpushed_count(working_dir) + if unpushed: + blockers.append( + f"{unpushed} commit(s) exist only locally — push them " + "(`git push -u origin `)" + ) + return blockers + + +def _final_assistant_text(transcript_path: str | None) -> str | None: + """The agent's last assistant message from the session transcript. + + Captured deterministically so the dashboard's checkpoint never depends on the + agent remembering to file one. Returns ``None`` when there is no transcript or no + text in it; never raises. + """ + if not transcript_path: + return None + try: + fh = open(transcript_path, encoding="utf-8") + except OSError: + return None + last = None + with fh: + for line in fh: + try: + entry = json.loads(line) + except ValueError: + continue + if entry.get("type") != "assistant": + continue + content = (entry.get("message") or {}).get("content") + if isinstance(content, str): + texts = [content] + elif isinstance(content, list): + texts = [ + block.get("text", "") + for block in content + if isinstance(block, dict) and block.get("type") == "text" + ] + else: + continue + text = "\n".join(t for t in texts if t).strip() + if text: + last = text + return last + + def handle_stop(conn: Connection, ident: Identity, hook_input: HookInput) -> dict: if ident.mise_init: return handle_mise_init_stop(conn, ident, hook_input) working_dir = ident.working_dir or hook_input.cwd or "." - ok, output = verify.run_test(working_dir) + tests_ok, output = verify.run_test(working_dir) + blockers = [] if tests_ok else ["the test suite is failing (`mise run test`)"] + blockers += _completion_blockers(working_dir) now = datetime.now(UTC) - status = "done" if ok else "blocked" - tests_status = "pass" if ok else "fail" - summary = "checkpoint: tests passed" if ok else "checkpoint blocked: tests failed" + status = "done" if not blockers else "blocked" + tests_status = "pass" if tests_ok else "fail" + summary = ( + "checkpoint: tests passed, work committed and pushed" + if not blockers + else "checkpoint blocked: " + "; ".join(blockers) + ) + # A done agent's checkmark carries its own closing message — the substance the + # dashboard shows; a blocked one carries the blockers (the next turn will re-capture + # the narrative once the gate clears). + final_text = _final_assistant_text(hook_input.transcript_path) + where_it_stopped = final_text[:4000] if not blockers and final_text else summary log_id = repo.insert_log_entry( conn, @@ -108,25 +186,25 @@ def handle_stop(conn: Connection, ident: Identity, hook_input: HookInput) -> dic agent_id=ident.agent_id, checkpoint_at=now, status=status, - where_it_stopped=summary, + where_it_stopped=where_it_stopped, log_entry_id=log_id, tests_status=tests_status, tested_at=now, ) repo.set_agent_status(conn, ident.agent_id, status) - if not ok: + if blockers: # Guard against an infinite block loop: if we already re-invoked once, record # the failure but let the turn end rather than blocking forever. if hook_input.stop_hook_active: return {} - return { - "decision": "block", - "reason": ( - "The test gate failed; the turn cannot end on a broken suite. " - f"`mise run test` output:\n{output[-4000:]}" - ), - } + reason = ( + "The completion gate failed — an agent is only done when its tests pass " + "and its work is committed and pushed. Blockers:\n- " + "\n- ".join(blockers) + ) + if not tests_ok: + reason += f"\n\n`mise run test` output:\n{output[-4000:]}" + return {"decision": "block", "reason": reason} return {} diff --git a/src/handler/hooks/context.py b/src/handler/hooks/context.py index d0348c6..5b03ad4 100644 --- a/src/handler/hooks/context.py +++ b/src/handler/hooks/context.py @@ -44,6 +44,10 @@ class HookInput: def message(self) -> str | None: return self.raw.get("message") + @property + def transcript_path(self) -> str | None: + return self.raw.get("transcript_path") + @property def stop_hook_active(self) -> bool: return bool(self.raw.get("stop_hook_active")) diff --git a/tests/test_headless_run.py b/tests/test_headless_run.py index e413ccf..76d092a 100644 --- a/tests/test_headless_run.py +++ b/tests/test_headless_run.py @@ -81,7 +81,9 @@ def test_spawn_run_streams_events_and_completes(headless_env, tmp_path): assert [e["seq"] for e in events] == [1, 2, 3] # last_output is now derived from assistant text — the log the UI shows is real. assert updated["last_output"] == "working on: build the thing" - assert updated["status"] == "done" + # done is a gate outcome, not an exit code: the fake claude never ran the Stop + # hook, so nothing verified tests/commits/pushes and the agent must not be done. + assert updated["status"] == "blocked" assert updated["session_id"] == run["session_id"] assert updated["worker_id"] == "w1" # The session archive was uploaded at exit for cross-worker resume. diff --git a/tests/test_hook_checkpoint.py b/tests/test_hook_checkpoint.py index 762147a..41e2ac0 100644 --- a/tests/test_hook_checkpoint.py +++ b/tests/test_hook_checkpoint.py @@ -20,13 +20,22 @@ def _fake_mise_state(monkeypatch, *, has_test, clean, ahead): monkeypatch.setattr(gitops, "ahead_count", lambda cwd: ahead) +def _fake_git_state(monkeypatch, *, is_repo=True, clean=True, unpushed=0, has_origin=True): + monkeypatch.setattr(gitops, "head_sha", lambda cwd: "abc123" if is_repo else None) + monkeypatch.setattr(gitops, "is_clean", lambda cwd: clean) + monkeypatch.setattr(gitops, "has_origin", lambda cwd: has_origin) + monkeypatch.setattr(gitops, "unpushed_count", lambda cwd: unpushed) + + def test_stop_blocks_on_failing_tests(conn, monkeypatch): ident = _seed(conn) monkeypatch.setattr(verify, "run_test", lambda cwd: (False, "1 failed")) + _fake_git_state(monkeypatch) result = checkpoint.handle_stop(conn, ident, HookInput({"session_id": "s1"}, "stop")) assert result["decision"] == "block" - assert "test gate failed" in result["reason"] + assert "test suite is failing" in result["reason"] + assert "1 failed" in result["reason"] cm = repo.get_checkmark(conn, ident.agent_id) assert cm["tests_status"] == "fail" @@ -35,9 +44,59 @@ def test_stop_blocks_on_failing_tests(conn, monkeypatch): assert repo.get_agent_by_name(conn, "p", "a")["status"] == "blocked" -def test_stop_allows_done_on_passing_tests(conn, monkeypatch): +def test_stop_blocks_on_uncommitted_changes_even_with_green_tests(conn, monkeypatch): ident = _seed(conn) monkeypatch.setattr(verify, "run_test", lambda cwd: (True, "ok")) + _fake_git_state(monkeypatch, clean=False) + + result = checkpoint.handle_stop(conn, ident, HookInput({"session_id": "s1"}, "stop")) + assert result["decision"] == "block" + assert "uncommitted" in result["reason"] + + cm = repo.get_checkmark(conn, ident.agent_id) + assert cm["tests_status"] == "pass" # tests still recorded honestly + assert cm["status"] == "blocked" + assert repo.get_agent_by_name(conn, "p", "a")["status"] == "blocked" + + +def test_stop_blocks_on_unpushed_commits(conn, monkeypatch): + ident = _seed(conn) + monkeypatch.setattr(verify, "run_test", lambda cwd: (True, "ok")) + _fake_git_state(monkeypatch, unpushed=3) + + result = checkpoint.handle_stop(conn, ident, HookInput({"session_id": "s1"}, "stop")) + assert result["decision"] == "block" + assert "3 commit(s) exist only locally" in result["reason"] + assert repo.get_agent_by_name(conn, "p", "a")["status"] == "blocked" + + +def test_stop_reports_every_blocker_at_once(conn, monkeypatch): + ident = _seed(conn) + monkeypatch.setattr(verify, "run_test", lambda cwd: (False, "boom")) + _fake_git_state(monkeypatch, clean=False, unpushed=1) + + result = checkpoint.handle_stop(conn, ident, HookInput({"session_id": "s1"}, "stop")) + assert "test suite is failing" in result["reason"] + assert "uncommitted" in result["reason"] + assert "exist only locally" in result["reason"] + + +def test_stop_skips_push_gate_without_an_origin_remote(conn, monkeypatch): + ident = _seed(conn) + monkeypatch.setattr(verify, "run_test", lambda cwd: (True, "ok")) + # Local-only project: commits exist that no remote has, but there is no origin + # to push them to — the push gate must not deadlock the agent. + _fake_git_state(monkeypatch, unpushed=5, has_origin=False) + + result = checkpoint.handle_stop(conn, ident, HookInput({"session_id": "s1"}, "stop")) + assert result == {} + assert repo.get_checkmark(conn, ident.agent_id)["status"] == "done" + + +def test_stop_allows_done_when_tests_pass_and_tree_is_settled(conn, monkeypatch): + ident = _seed(conn) + monkeypatch.setattr(verify, "run_test", lambda cwd: (True, "ok")) + _fake_git_state(monkeypatch) result = checkpoint.handle_stop(conn, ident, HookInput({"session_id": "s1"}, "stop")) assert result == {} # no block @@ -48,9 +107,60 @@ def test_stop_allows_done_on_passing_tests(conn, monkeypatch): assert cm["log_entry_id"] is not None +def test_stop_allows_done_when_working_dir_is_not_a_repo(conn, monkeypatch): + # A manually managed root has nothing to gate beyond its tests. + ident = _seed(conn) + monkeypatch.setattr(verify, "run_test", lambda cwd: (True, "ok")) + _fake_git_state(monkeypatch, is_repo=False, clean=False, unpushed=9) + + result = checkpoint.handle_stop(conn, ident, HookInput({"session_id": "s1"}, "stop")) + assert result == {} + assert repo.get_checkmark(conn, ident.agent_id)["status"] == "done" + + +def test_stop_captures_final_message_as_checkpoint(conn, monkeypatch, tmp_path): + ident = _seed(conn) + monkeypatch.setattr(verify, "run_test", lambda cwd: (True, "ok")) + _fake_git_state(monkeypatch) + transcript = tmp_path / "session.jsonl" + transcript.write_text( + '{"type": "user", "message": {"content": "do the thing"}}\n' + "this line is not json\n" + '{"type": "assistant", "message": {"content": [{"type": "text", ' + '"text": "Working on it."}]}}\n' + '{"type": "assistant", "message": {"content": [{"type": "tool_use", "id": "x"},' + ' {"type": "text", "text": "Shipped the fix in abc123; tests green."}]}}\n' + ) + + hi = HookInput({"session_id": "s1", "transcript_path": str(transcript)}, "stop") + result = checkpoint.handle_stop(conn, ident, hi) + assert result == {} + cm = repo.get_checkmark(conn, ident.agent_id) + # The webui checkpoint carries the agent's own closing message, captured + # deterministically from the transcript — not left to the agent's discretion. + assert cm["where_it_stopped"] == "Shipped the fix in abc123; tests green." + + +def test_blocked_stop_shows_blockers_not_narrative(conn, monkeypatch, tmp_path): + ident = _seed(conn) + monkeypatch.setattr(verify, "run_test", lambda cwd: (True, "ok")) + _fake_git_state(monkeypatch, clean=False) + transcript = tmp_path / "session.jsonl" + transcript.write_text( + '{"type": "assistant", "message": {"content": [{"type": "text", ' + '"text": "All done!"}]}}\n' + ) + + hi = HookInput({"session_id": "s1", "transcript_path": str(transcript)}, "stop") + checkpoint.handle_stop(conn, ident, hi) + cm = repo.get_checkmark(conn, ident.agent_id) + assert "uncommitted" in cm["where_it_stopped"] + + def test_stop_does_not_reblock_when_already_active(conn, monkeypatch): ident = _seed(conn) monkeypatch.setattr(verify, "run_test", lambda cwd: (False, "still failing")) + _fake_git_state(monkeypatch) hi = HookInput({"session_id": "s1", "stop_hook_active": True}, "stop") result = checkpoint.handle_stop(conn, ident, hi) assert result == {} # recorded, but not an infinite block diff --git a/tests/test_integration_web_spawn.py b/tests/test_integration_web_spawn.py index 01b7dae..92c6bb5 100644 --- a/tests/test_integration_web_spawn.py +++ b/tests/test_integration_web_spawn.py @@ -71,13 +71,15 @@ def test_spawn_via_api_then_worker_runs_headless_claude(client, auth, headless_e assert got["result"]["name"] == "api" # ...and the run's whole life shows up via the API: events stream in, the agent - # reconciles to done, last_output is the assistant's text. + # reconciles, last_output is the assistant's text. The fake claude never runs the + # Stop gate, so the clean exit reconciles to blocked, not done — done is only ever + # a gate verdict (tests pass, work committed and pushed). def finished(): agents = client.get("/projects/proj/agents", headers=auth).json() - return agents if agents and agents[0]["status"] == "done" else None + return agents if agents and agents[0]["status"] == "blocked" else None agents = _wait(finished) - assert agents is not None, "run never reconciled to done" + assert agents is not None, "run never reconciled" agent = agents[0] assert agent["name"] == "api" assert agent["session_id"]