From f03b03ab5cad23316df0f60a673c965032780be2 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 23 Jul 2026 04:21:13 +0000 Subject: [PATCH 1/2] Fetch origin and cut agent branches from origin/HEAD at spawn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Agents spawned by handler were getting branches several commits behind main. Two compounding causes: sync_project ran 'git pull --ff-only' in the project root, which only moves whichever branch the root checkout happens to be on — an agent parked on a feature branch left origin/* stale (and the pull's 'no tracking information' failure degraded to an easy-to-miss sync_note). Then worktree spawns cut new branches from the root's HEAD, inheriting that stale state. sync_project now fetches origin (refreshing origin/* regardless of the checkout), re-pins origin/HEAD, and fast-forwards the checkout only when it sits on the default branch — a diverged default branch still fails loudly. New worktree branches are cut from origin/HEAD with --no-track so they start at the remote default branch's tip and don't adopt it as upstream. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01V7mF6qeryi9nJthaxYkfPm --- src/handler/control/gitops.py | 34 +++++++++++-- src/handler/control/reposync.py | 31 ++++++++--- src/handler/control/spawn.py | 10 ++-- src/handler/control/worktree.py | 33 ++++++++++-- tests/test_project_from_server.py | 18 +++++-- tests/test_reposync_git.py | 85 +++++++++++++++++++++++++++++++ tests/test_worktree.py | 54 ++++++++++++++++++++ 7 files changed, 243 insertions(+), 22 deletions(-) create mode 100644 tests/test_reposync_git.py diff --git a/src/handler/control/gitops.py b/src/handler/control/gitops.py index 6e6c12b..71ec0c1 100644 --- a/src/handler/control/gitops.py +++ b/src/handler/control/gitops.py @@ -114,6 +114,34 @@ def clone( return _run(args, cwd=None, env=env, timeout=_NETWORK_TIMEOUT) -def pull_ff(cwd: str, env: dict[str, str] | None = None) -> tuple[bool, str]: - """Fast-forward-only pull — never merges, so a diverged clone fails loudly.""" - return _run(["pull", "--ff-only"], cwd, env=env, timeout=_NETWORK_TIMEOUT) +def fetch(cwd: str, env: dict[str, str] | None = None) -> tuple[bool, str]: + """``git fetch origin`` — refreshes every ``origin/*`` ref. + + Unlike a plain pull, this works no matter which branch the working tree has + checked out (or left parked), so the remote's state is always current locally. + """ + return _run(["fetch", "origin"], cwd, env=env, timeout=_NETWORK_TIMEOUT) + + +def set_default_head(cwd: str, env: dict[str, str] | None = None) -> tuple[bool, str]: + """``git remote set-head origin --auto`` — (re)pin ``origin/HEAD``. + + ``origin/HEAD`` is how the remote's default branch is known locally; clones made + by older tooling can lack it, and the remote's default can change. + """ + return _run(["remote", "set-head", "origin", "--auto"], cwd, env=env, + timeout=_NETWORK_TIMEOUT) + + +def default_branch_ref(cwd: str) -> str | None: + """The remote default branch as a local ref name (``origin/main``), or ``None``.""" + ok, out = _run(["symbolic-ref", "--quiet", "refs/remotes/origin/HEAD"], cwd) + if not ok or not out: + return None + return out.removeprefix("refs/remotes/") or None + + +def merge_ff(cwd: str, ref: str) -> tuple[bool, str]: + """Fast-forward-only merge of ``ref`` — never merges, so a diverged checkout + fails loudly instead of silently growing a merge commit.""" + return _run(["merge", "--ff-only", ref], cwd) diff --git a/src/handler/control/reposync.py b/src/handler/control/reposync.py index 7643512..e76741b 100644 --- a/src/handler/control/reposync.py +++ b/src/handler/control/reposync.py @@ -1,4 +1,4 @@ -"""Clone-or-pull a project's repository — the stateless "always pull" step. +"""Clone-or-fetch a project's repository — the stateless "always pull" step. Where a clone lands on disk is Handler's concern, not the operator's: the API computes ``root_dir`` under ``PROJECTS_ROOT`` at registration, and this module makes the checkout @@ -70,11 +70,16 @@ def ssh_env(git_remote: str | None, conn: Connection) -> dict[str, str]: def sync_project(project: dict, conn: Connection | None = None) -> dict: - """Clone the project's remote into ``root_dir``, or fast-forward an existing clone. + """Clone the project's remote into ``root_dir``, or refresh an existing clone. Idempotent by design — scheduled/stateless runs call this before every spawn so the - working tree always starts from the remote's latest state. Raises :class:`SyncError` - when the project has no remote or git fails. + working tree always starts from the remote's latest state. An existing clone is + ``git fetch``\\ ed rather than pulled: a pull only moves the branch the root happens + to have checked out, and an agent parked on a feature branch used to leave + ``origin/*`` (and therefore every branch cut for a new agent) hours stale. After + the fetch, ``origin/HEAD`` is re-pinned and the checkout is fast-forwarded only + when it is sitting on the default branch. Raises :class:`SyncError` when the + project has no remote or git fails. """ remote = project.get("git_remote") root = project["root_dir"] @@ -88,10 +93,22 @@ def sync_project(project: dict, conn: Connection | None = None) -> dict: env, config = _auth_context(project, conn) if gitops.is_repo(root): - ok, out = gitops.pull_ff(root, env=env) + ok, out = gitops.fetch(root, env=env) if not ok: - raise SyncError(f"pull failed in {root}: {out}") - return {"action": "pulled", "root_dir": root, "detail": out} + raise SyncError(f"fetch failed in {root}: {out}") + # Best-effort: worktree spawns cut new branches from origin/HEAD, so keep it + # pinned even for clones that predate it (or whose remote default changed). + gitops.set_default_head(root, env=env) + detail = out + default_ref = gitops.default_branch_ref(root) + if default_ref and gitops.current_branch(root) == default_ref.split("/", 1)[1]: + ok, ff_out = gitops.merge_ff(root, default_ref) + if not ok: + raise SyncError( + f"fast-forward of {default_ref} failed in {root}: {ff_out}" + ) + detail = ff_out or detail + return {"action": "pulled", "root_dir": root, "detail": detail} os.makedirs(os.path.dirname(root) or ".", exist_ok=True) ok, out = gitops.clone(remote, root, env=env, config=config) diff --git a/src/handler/control/spawn.py b/src/handler/control/spawn.py index 7e53ee0..a0b71cf 100644 --- a/src/handler/control/spawn.py +++ b/src/handler/control/spawn.py @@ -95,10 +95,12 @@ def spawn( raise SpawnError(f"agent '{name}' already exists in project '{project_id}'") # Stateless workflows: start every run from the remote's latest state. An - # existing clone is fast-forwarded (failure degrades to a note — a stale tree is - # usable, an offline forge shouldn't brick spawning); a missing/empty root is - # cloned, and that failing is fatal (there is nothing to run against). A - # non-empty root that isn't a git repo is left alone — it's manually managed. + # existing clone is fetched — refreshing origin/* even when the root checkout is + # parked on an agent's branch — and fast-forwarded when it sits on the default + # branch (failure degrades to a note — a stale tree is usable, an offline forge + # shouldn't brick spawning); a missing/empty root is cloned, and that failing is + # fatal (there is nothing to run against). A non-empty root that isn't a git + # repo is left alone — it's manually managed. root = project["root_dir"] if project.get("git_remote"): if gitops.is_repo(root): diff --git a/src/handler/control/worktree.py b/src/handler/control/worktree.py index c490d5b..2541e1c 100644 --- a/src/handler/control/worktree.py +++ b/src/handler/control/worktree.py @@ -35,6 +35,25 @@ def _slug(name: str) -> str: return slug or "agent" +def _default_branch_start(project_root: str) -> str | None: + """The remote default branch ref (``origin/main``) when the clone knows it. + + New agent branches are cut from here rather than the root's ``HEAD``: the root + checkout can legitimately be parked on some earlier agent's branch, and cutting + from it handed fresh agents history that was hours behind the remote. + """ + result = subprocess.run( + ["git", "-C", project_root, "symbolic-ref", "--quiet", + "refs/remotes/origin/HEAD"], + capture_output=True, + text=True, + ) + ref = result.stdout.strip() + if result.returncode != 0 or not ref.startswith("refs/remotes/"): + return None + return ref.removeprefix("refs/remotes/") + + def _branch_exists(project_root: str, branch: str) -> bool: return ( subprocess.run( @@ -69,13 +88,21 @@ def resolve_working_dir( raise IsolationError(f"{target} escapes project root {project_root}") # `git worktree add ` only checks out an *existing* ref; a fresh # feature branch won't exist yet (spawn starts from the remote's latest state), so - # create it with -b. An existing local branch is checked out as-is; if it exists - # only on a remote, git DWIMs a tracking branch from the bare `add`. + # create it with -b — from origin/HEAD when the clone has a remote, so the new + # branch starts at the remote default branch's tip regardless of where the root + # checkout is parked (--no-track: the branch must not adopt the default branch as + # its upstream, or the agent's plain `git push` would aim at it). An existing + # local branch is checked out as-is; if it exists only on a remote, git DWIMs a + # tracking branch from the bare `add`. cmd = ["git", "-C", project_root, "worktree", "add"] if _branch_exists(project_root, worktree_branch): cmd += [target, worktree_branch] else: - cmd += ["-b", worktree_branch, target] + start = _default_branch_start(project_root) + if start: + cmd += ["--no-track", "-b", worktree_branch, target, start] + else: + cmd += ["-b", worktree_branch, target] result = subprocess.run(cmd, capture_output=True, text=True) if result.returncode != 0: raise WorktreeError( diff --git a/tests/test_project_from_server.py b/tests/test_project_from_server.py index 312d11b..feb9263 100644 --- a/tests/test_project_from_server.py +++ b/tests/test_project_from_server.py @@ -150,7 +150,7 @@ def test_sync_endpoint_400_without_remote(client, auth, env, tmp_path): @pytest.fixture def fake_sync_gitops(monkeypatch): """Fake the clone/pull side of the gitops seam.""" - state = {"clone": [], "pull": [], "config": [], "repos": set(), "ok": True, "out": ""} + state = {"clone": [], "fetch": [], "config": [], "repos": set(), "ok": True, "out": ""} def is_repo(path): return path in state["repos"] @@ -162,17 +162,25 @@ def fake_sync_gitops(monkeypatch): state["repos"].add(dest) return state["ok"], state["out"] - def pull_ff(cwd, env=None): - state["pull"].append({"cwd": cwd, "env": env or {}}) + def fetch(cwd, env=None): + state["fetch"].append({"cwd": cwd, "env": env or {}}) return state["ok"], state["out"] + def set_default_head(cwd, env=None): + return True, "" + + def default_branch_ref(cwd): + return None + def config_local(cwd, key, value): state["config"].append({"cwd": cwd, "key": key, "value": value}) return True, "" monkeypatch.setattr(gitops, "is_repo", is_repo) monkeypatch.setattr(gitops, "clone", clone) - monkeypatch.setattr(gitops, "pull_ff", pull_ff) + monkeypatch.setattr(gitops, "fetch", fetch) + monkeypatch.setattr(gitops, "set_default_head", set_default_head) + monkeypatch.setattr(gitops, "default_branch_ref", default_branch_ref) monkeypatch.setattr(gitops, "config_local", config_local) return state @@ -192,7 +200,7 @@ def test_cmd_sync_clones_then_pulls(env, fake_sync_gitops): result = worker.execute_command(command) assert result["action"] == "pulled" - assert fake_sync_gitops["pull"][0]["cwd"] == "/tmp/r" + assert fake_sync_gitops["fetch"][0]["cwd"] == "/tmp/r" def test_cmd_sync_failure_is_command_error(env, fake_sync_gitops): diff --git a/tests/test_reposync_git.py b/tests/test_reposync_git.py new file mode 100644 index 0000000..9965333 --- /dev/null +++ b/tests/test_reposync_git.py @@ -0,0 +1,85 @@ +"""``sync_project`` against real git repos — the fetch-not-pull regression. + +A pull only moves whichever branch the root has checked out; with the root parked on +an agent's feature branch, ``origin/*`` went stale and every branch cut for a new +agent started several commits behind the remote's default branch. These tests use a +filesystem remote (no host, no credentials) so the git behaviour itself is exercised. +""" + +from __future__ import annotations + +import subprocess + +import pytest + +from handler.control import reposync + + +def _git(cwd, *args): + subprocess.run(["git", "-C", str(cwd), *args], check=True, capture_output=True) + + +def _sha(cwd, ref="HEAD") -> str: + return subprocess.run( + ["git", "-C", str(cwd), "rev-parse", ref], + check=True, capture_output=True, text=True, + ).stdout.strip() + + +@pytest.fixture +def remote_and_root(tmp_path): + """An origin repo, a clone of it as the project root, then origin moves ahead.""" + origin = tmp_path / "origin" + origin.mkdir() + _git(origin, "init", "-q", "-b", "main") + for repo_dir in (origin,): + _git(repo_dir, "config", "user.email", "t@t.co") + _git(repo_dir, "config", "user.name", "t") + _git(repo_dir, "config", "commit.gpgsign", "false") + _git(origin, "commit", "-q", "--allow-empty", "-m", "one") + root = tmp_path / "proj" + subprocess.run( + ["git", "clone", "-q", str(origin), str(root)], check=True, capture_output=True + ) + _git(root, "config", "user.email", "t@t.co") + _git(root, "config", "user.name", "t") + _git(root, "config", "commit.gpgsign", "false") + _git(origin, "commit", "-q", "--allow-empty", "-m", "two") + return origin, root + + +def _project(origin, root) -> dict: + return {"id": "p", "root_dir": str(root), "git_remote": str(origin)} + + +def test_sync_refreshes_origin_even_when_root_parked_on_agent_branch( + env, remote_and_root +): + origin, root = remote_and_root + _git(root, "checkout", "-q", "-b", "agent/old-work") + # Simulate a clone that never had origin/HEAD — sync must re-pin it too. + _git(root, "remote", "set-head", "origin", "-d") + + result = reposync.sync_project(_project(origin, root)) + + assert result["action"] == "pulled" + assert _sha(root, "origin/main") == _sha(origin, "main") + assert _sha(root, "refs/remotes/origin/HEAD") == _sha(origin, "main") + # The parked checkout itself is left alone — only origin/* moves. + assert _sha(root, "HEAD") != _sha(origin, "main") + + +def test_sync_fast_forwards_a_checkout_sitting_on_the_default_branch( + env, remote_and_root +): + origin, root = remote_and_root + result = reposync.sync_project(_project(origin, root)) + assert result["action"] == "pulled" + assert _sha(root, "HEAD") == _sha(origin, "main") + + +def test_sync_fails_loudly_when_the_default_branch_diverged(env, remote_and_root): + origin, root = remote_and_root + _git(root, "commit", "-q", "--allow-empty", "-m", "local divergence") + with pytest.raises(reposync.SyncError, match="fast-forward"): + reposync.sync_project(_project(origin, root)) diff --git a/tests/test_worktree.py b/tests/test_worktree.py index 0b4df88..4e1ec2e 100644 --- a/tests/test_worktree.py +++ b/tests/test_worktree.py @@ -31,6 +31,36 @@ def repo(tmp_path): return root +@pytest.fixture +def cloned(tmp_path): + """A root cloned from an origin whose main has since moved on. + + Mirrors the spawn-time state after ``reposync.sync_project``: origin/* is fresh + (fetched), but the root's checkout still sits on the older commit. + """ + origin = tmp_path / "origin" + origin.mkdir() + _git(origin, "init", "-q", "-b", "main") + _git(origin, "config", "user.email", "t@t.co") + _git(origin, "config", "user.name", "t") + _git(origin, "config", "commit.gpgsign", "false") + _git(origin, "commit", "-q", "--allow-empty", "-m", "one") + root = tmp_path / "proj" + subprocess.run( + ["git", "clone", "-q", str(origin), str(root)], check=True, capture_output=True + ) + _git(origin, "commit", "-q", "--allow-empty", "-m", "two") + _git(root, "fetch", "-q", "origin") + return root + + +def _sha(cwd, ref="HEAD") -> str: + return subprocess.run( + ["git", "-C", str(cwd), "rev-parse", ref], + check=True, capture_output=True, text=True, + ).stdout.strip() + + def _is_worktree(root, path) -> bool: out = subprocess.run( ["git", "-C", str(root), "worktree", "list", "--porcelain"], @@ -54,6 +84,30 @@ def test_creates_branch_when_it_does_not_exist(repo): assert "feat/new-thing" in branches +def test_new_branch_is_cut_from_remote_default_not_root_head(cloned): + # The regression: new agent branches were cut from the root's HEAD, so a checkout + # sitting behind (or parked on an earlier agent's branch) produced branches several + # commits behind origin's default branch. + target = worktree.resolve_working_dir( + str(cloned), "agent", worktree_branch="feat/fresh" + ) + assert _sha(target) == _sha(cloned, "origin/main") + assert _sha(target) != _sha(cloned, "HEAD") + + +def test_new_branch_from_remote_default_gets_no_upstream(cloned): + # --no-track: without it the new branch adopts origin/main as upstream, and the + # agent's plain `git push` would aim at the default branch. + target = worktree.resolve_working_dir( + str(cloned), "agent", worktree_branch="feat/fresh" + ) + upstream = subprocess.run( + ["git", "-C", target, "rev-parse", "--abbrev-ref", "@{upstream}"], + capture_output=True, text=True, + ) + assert upstream.returncode != 0 + + def test_checks_out_existing_branch(repo): _git(repo, "branch", "feat/existing") target = worktree.resolve_working_dir( From 9ad13b24887721eb27473499dff9b6ce03f1bec0 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 23 Jul 2026 06:49:39 +0000 Subject: [PATCH 2/2] Gate agent completion on commits, pushes, and tests; capture real checkpoints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Agents could be marked done while leaving work uncommitted or unpushed: the Stop gate only ran the test suite, and the headless supervisor's fallback marked any still-working agent done on a clean process exit even when the Stop gate never recorded a verdict. Checkmarks also only ever carried hook-written boilerplate, so the webui had no real checkpoint to show. The Stop gate now blocks the turn on any of: failing tests, uncommitted changes, or commits no origin/* ref contains (rev-list --not --remotes=origin, so it works for the --no-track worktree branches). All blockers are reported at once; status 'done' only ever accompanies a fully passing gate. Working dirs that aren't git checkouts, and repos without an origin remote, skip the git half so local-only projects can't deadlock. The supervisor's clean-exit fallback now reconciles a still-working agent to blocked instead of done — done is a gate verdict, not an exit code (operator cancels still settle as done). The agent's final message is captured deterministically from the session transcript onto the checkmark's where_it_stopped, so the dashboard always shows a real checkpoint regardless of whether the agent thought to leave one; a blocked checkmark shows the blockers. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01V7mF6qeryi9nJthaxYkfPm --- README.md | 11 ++- src/handler/control/gitops.py | 23 ++++++ src/handler/control/headless.py | 8 +- src/handler/hooks/checkpoint.py | 114 +++++++++++++++++++++++----- src/handler/hooks/context.py | 4 + tests/test_headless_run.py | 4 +- tests/test_hook_checkpoint.py | 114 +++++++++++++++++++++++++++- tests/test_integration_web_spawn.py | 8 +- 8 files changed, 256 insertions(+), 30 deletions(-) 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"]