diff --git a/CHANGELOG.md b/CHANGELOG.md index 6a393d9..ddcdf5e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,24 @@ the image workflows publish (plus `latest` from every push to `main`). ### Fixed +- **The project-root checkout is now a ref store, never a working tree Handler moves.** + Worktree spawns fetch only: the agent's branch is cut from `origin/*` and the root + checkout — which may hold the operator's own work — is never fast-forwarded, merged, + or re-parked. A failed fetch on a worktree spawn is now **fatal** instead of a silent + note (the contract is "starts at the remote's latest push"; cutting from stale refs + would break it quietly). Branch starts fall back `origin/HEAD` → `origin/main` → + `origin/master` when the head pin is missing, and a stale `agent/` branch left + by a deleted agent is reset to the remote tip instead of shadowing it. Root/subdir + placements (schedule firings, mise-init) keep the old fast-forward behavior for + their shared tree. 2 end-to-end regression tests (bare remote, parked root, + out-of-band push). +- **UI-serving tests skip when the web export is absent** instead of failing every + fresh clone: the export is a generated, deliberately untracked artifact (built in + the Docker image's node stage), so the three `test_api_ui` checks now guard any + environment that has it and skip with a clear reason where it was never built. + +### Fixed + - **Agents missing freshly pushed commits.** A spawn with no explicit placement ran the agent in the shared project-root checkout, and the root only fast-forwards while parked on the default branch — so as soon as one agent left it on a feature diff --git a/src/handler/control/reposync.py b/src/handler/control/reposync.py index e76741b..d66608f 100644 --- a/src/handler/control/reposync.py +++ b/src/handler/control/reposync.py @@ -69,16 +69,23 @@ def ssh_env(git_remote: str | None, conn: Connection) -> dict[str, str]: return {"GIT_SSH_COMMAND": sshkeys.git_ssh_command(key_path)} -def sync_project(project: dict, conn: Connection | None = None) -> dict: +def sync_project( + project: dict, conn: Connection | None = None, *, ff: bool = True +) -> dict: """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. 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 + Idempotent by design — spawns call this so agents always start 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. + + ``ff`` decides whether the root *checkout* is also fast-forwarded (only ever when + it sits on the default branch). Worktree spawns pass ``ff=False``: they cut their + branch from ``origin/*`` and must never move — let alone risk disturbing — the + root's working tree, which may hold an operator's own checkout. Root/subdir + placements (schedule firings, mise-init) and the explicit sync command keep + ``ff=True`` so their shared tree advances. Raises :class:`SyncError` when the project has no remote or git fails. """ remote = project.get("git_remote") @@ -100,6 +107,8 @@ def sync_project(project: dict, conn: Connection | None = None) -> dict: # pinned even for clones that predate it (or whose remote default changed). gitops.set_default_head(root, env=env) detail = out + if not ff: + return {"action": "fetched", "root_dir": root, "detail": detail} 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) diff --git a/src/handler/control/spawn.py b/src/handler/control/spawn.py index b2bff50..262571b 100644 --- a/src/handler/control/spawn.py +++ b/src/handler/control/spawn.py @@ -110,26 +110,22 @@ def spawn( if repo.get_agent_by_name(conn, project_id, name) is not None: 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 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. + # Start every agent from the remote's latest push. A missing/empty root is + # cloned (that failing is fatal — nothing to run against); a non-empty root + # that isn't a git repo is left alone (manually managed). root = project["root_dir"] - if project.get("git_remote"): - if gitops.is_repo(root): - try: - reposync.sync_project(project, conn) - except reposync.SyncError as exc: - sync_note = str(exc) - elif not os.path.isdir(root) or not os.listdir(root): + if project.get("git_remote") and not gitops.is_repo(root): + if not os.path.isdir(root) or not os.listdir(root): try: reposync.sync_project(project, conn) except reposync.SyncError as exc: raise SpawnError(str(exc)) from exc + # Isolation by default: without this, every no-placement spawn shares the root + # checkout, whose tree goes stale as soon as an agent parks it off the default + # branch. Derived branches are marked so a stale leftover (deleted agent) is + # reset to the remote tip instead of shadowing it. + derived = False if ( auto_worktree and worktree_branch is None @@ -137,16 +133,39 @@ def spawn( and not mise_init # the bootstrap commits .mise.toml to the default branch and gitops.is_repo(root) ): - # Isolation by default: without this, every no-placement spawn shares the - # root checkout, whose tree goes stale as soon as an agent parks it off the - # default branch. (Name reuse after a deleted agent can leave a stale - # agent/ branch behind; it is then checked out as-is, same as any - # explicitly named existing branch.) worktree_branch = f"agent/{name}" + derived = True + + if project.get("git_remote") and gitops.is_repo(root): + if worktree_branch: + # Worktree spawns: fetch-only. The agent's branch is cut from origin/*, + # and the root checkout — which may hold the operator's own work — is + # never fast-forwarded, merged, or otherwise touched. A failed fetch is + # fatal here: the whole contract is "starts at the remote's latest + # push", and silently cutting from stale refs breaks it. + try: + reposync.sync_project(project, conn, ff=False) + except reposync.SyncError as exc: + raise SpawnError( + f"cannot fetch the remote's latest state: {exc}" + ) from exc + else: + # Root/subdir placements (schedule firings, mise-init) share the root + # tree, so it is fast-forwarded when parked on the default branch; + # failure degrades to a note — a stale shared tree is usable, and an + # offline forge shouldn't brick a scheduled run. + try: + reposync.sync_project(project, conn) + except reposync.SyncError as exc: + sync_note = str(exc) try: working_dir = worktree.resolve_working_dir( - project["root_dir"], name, subdir=subdir, worktree_branch=worktree_branch + project["root_dir"], + name, + subdir=subdir, + worktree_branch=worktree_branch, + reset_existing=derived, ) except (worktree.WorktreeError, worktree.IsolationError) as exc: raise SpawnError(str(exc)) from exc diff --git a/src/handler/control/worktree.py b/src/handler/control/worktree.py index 2541e1c..d0f58f5 100644 --- a/src/handler/control/worktree.py +++ b/src/handler/control/worktree.py @@ -40,7 +40,10 @@ def _default_branch_start(project_root: str) -> str | None: 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. + from it handed fresh agents history that was hours behind the remote. When + ``origin/HEAD`` was never pinned (older clones, a failed ``remote set-head``), + fall back to ``origin/main`` / ``origin/master`` rather than silently cutting + from the root's parked HEAD. """ result = subprocess.run( ["git", "-C", project_root, "symbolic-ref", "--quiet", @@ -49,9 +52,18 @@ def _default_branch_start(project_root: str) -> str | None: text=True, ) ref = result.stdout.strip() - if result.returncode != 0 or not ref.startswith("refs/remotes/"): - return None - return ref.removeprefix("refs/remotes/") + if result.returncode == 0 and ref.startswith("refs/remotes/"): + return ref.removeprefix("refs/remotes/") + for candidate in ("origin/main", "origin/master"): + exists = subprocess.run( + ["git", "-C", project_root, "rev-parse", "--verify", "--quiet", + f"refs/remotes/{candidate}"], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + ) + if exists.returncode == 0: + return candidate + return None def _branch_exists(project_root: str, branch: str) -> bool: @@ -72,12 +84,20 @@ def resolve_working_dir( *, subdir: str | None = None, worktree_branch: str | None = None, + reset_existing: bool = False, ) -> str: """Return (and, for worktrees, create) the agent's working directory. - ``subdir``: an existing/created subdirectory under the project root. - ``worktree_branch``: ``git worktree add / ``. - neither: the project root itself. + + ``reset_existing`` is set for spawn-derived ``agent/`` branches: nobody owns + a derived branch whose agent name is free again, so a stale leftover (from a + deleted agent) is reset to the remote default tip with ``-B`` instead of being + checked out as-is — otherwise a dead branch would silently shadow the remote's + latest push. Operator-named branches keep checkout-as-is semantics (an in-flight + branch must never be clobbered). """ if subdir and worktree_branch: raise ValueError("pass at most one of subdir / worktree_branch") @@ -88,17 +108,22 @@ 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 — 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 + # create it with -b — from the remote default branch when the clone has one, so + # the new branch starts at that 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`. + start = _default_branch_start(project_root) cmd = ["git", "-C", project_root, "worktree", "add"] if _branch_exists(project_root, worktree_branch): - cmd += [target, worktree_branch] + if reset_existing and start: + # -B fails when the branch is checked out in another worktree, which is + # exactly the protection an in-flight agent needs. + cmd += ["--no-track", "-B", worktree_branch, target, start] + else: + cmd += [target, worktree_branch] else: - start = _default_branch_start(project_root) if start: cmd += ["--no-track", "-b", worktree_branch, target, start] else: diff --git a/tests/test_api_ui.py b/tests/test_api_ui.py index 5232792..bdd7952 100644 --- a/tests/test_api_ui.py +++ b/tests/test_api_ui.py @@ -9,10 +9,21 @@ from __future__ import annotations import re from pathlib import Path +import pytest from fastapi.testclient import TestClient _STATIC_DIR = Path(__file__).resolve().parents[1] / "src" / "handler" / "api" / "static" +# The export is a generated artifact and deliberately untracked (it caused guaranteed +# merge conflicts — see .gitignore): the Docker image builds it in its own node stage, +# and a source checkout only has it after `npm run export`. The serving tests still +# guard any environment that *has* the export; a fresh clone just skips them. +_needs_export = pytest.mark.skipif( + not (_STATIC_DIR / "index.html").is_file(), + reason="web UI export not built (frontend: npm run export); " + "generated artifact, untracked by design", +) + def _reset_caches() -> None: from handler import config @@ -36,6 +47,7 @@ def _fresh_client(monkeypatch, **overrides) -> TestClient: # --- shell + assets are served, unauthenticated ------------------------------------- +@_needs_export def test_index_served_unauthenticated(client): res = client.get("/") # no Authorization header assert res.status_code == 200 @@ -45,6 +57,7 @@ def test_index_served_unauthenticated(client): assert "Bearer" not in res.text +@_needs_export def test_next_assets_served_unauthenticated(client): # The export references its hashed bundles under /_next/. Discover one from the shell # and confirm it's served same-origin without auth (filenames are content-hashed, so @@ -57,6 +70,7 @@ def test_next_assets_served_unauthenticated(client): assert res.headers["content-type"].startswith(("application/javascript", "text/javascript")) +@_needs_export def test_static_export_is_bundled(): # The built export ships inside the package tree so `pip install .` bundles it. assert (_STATIC_DIR / "index.html").is_file() diff --git a/tests/test_control_spawn.py b/tests/test_control_spawn.py index 9d69583..76f5156 100644 --- a/tests/test_control_spawn.py +++ b/tests/test_control_spawn.py @@ -301,3 +301,91 @@ def test_worker_spawn_scheduled_keeps_root_placement(env, monkeypatch): assert calls[0]["auto_worktree"] is False assert calls[1]["auto_worktree"] is True + + +def _git_out(root, *args): + import subprocess + + return subprocess.run( + ["git", "-c", "user.email=t@t", "-c", "user.name=t", "-C", str(root), *args], + check=True, + capture_output=True, + text=True, + ).stdout.strip() + + +def test_spawn_worktree_sees_latest_push_and_never_touches_root(env, fake_launch): + """The operator's scenario: root clone parked on some agent's old branch, a new + commit pushed to the remote default branch from elsewhere. The spawned agent's + worktree must contain that push, and the root checkout must not be moved.""" + bare = env["tmp"] / "origin.git" + import subprocess + + subprocess.run(["git", "init", "-q", "--bare", "-b", "main", str(bare)], check=True) + + # Seed the remote through a scratch clone: mise config + first commit. + seed = env["tmp"] / "seed" + subprocess.run(["git", "clone", "-q", str(bare), str(seed)], check=True) + _write_mise(seed, with_test=True) + _git(seed, "add", "-A") + _git(seed, "commit", "-q", "-m", "init") + _git(seed, "push", "-q", "origin", "main") + + # The project root clone, parked on a side branch (what a root-placed agent left). + root = env["tmp"] / "proj" + subprocess.run(["git", "clone", "-q", str(bare), str(root)], check=True) + _git(root, "checkout", "-q", "-b", "old-agent-branch") + + # The operator's hand-made commit, pushed to main from a different checkout. + (seed / "HANDOFF.md").write_text("the context the agent needs\n") + _git(seed, "add", "-A") + _git(seed, "commit", "-q", "-m", "operator context") + _git(seed, "push", "-q", "origin", "main") + + with get_engine().begin() as conn: + repo.create_project(conn, "proj", str(root), git_remote=str(bare)) + + agent = spawn.spawn("proj", "api", task="use the handoff") + + # The worktree starts at the remote tip: the operator's push is present. + wt = env["tmp"] / "proj" / "api" + assert agent["working_dir"] == str(wt) + assert (wt / "HANDOFF.md").read_text() == "the context the agent needs\n" + assert _git_out(wt, "branch", "--show-current") == "agent/api" + # The root checkout was not moved, merged, or re-parked. + assert _git_out(root, "branch", "--show-current") == "old-agent-branch" + + +def test_spawn_derived_branch_resets_stale_leftover(env, fake_launch): + """A stale agent/ branch from a deleted agent must not shadow the remote + tip — derived branches are reset to the remote default when recreated.""" + bare = env["tmp"] / "origin.git" + import subprocess + + subprocess.run(["git", "init", "-q", "--bare", "-b", "main", str(bare)], check=True) + seed = env["tmp"] / "seed" + subprocess.run(["git", "clone", "-q", str(bare), str(seed)], check=True) + _write_mise(seed, with_test=True) + _git(seed, "add", "-A") + _git(seed, "commit", "-q", "-m", "init") + _git(seed, "push", "-q", "origin", "main") + + root = env["tmp"] / "proj" + subprocess.run(["git", "clone", "-q", str(bare), str(root)], check=True) + # A stale derived branch parked at the old tip (its worktree long gone). + _git(root, "branch", "agent/api") + + # The remote advances past it. + (seed / "NEW.md").write_text("newer\n") + _git(seed, "add", "-A") + _git(seed, "commit", "-q", "-m", "newer") + _git(seed, "push", "-q", "origin", "main") + + with get_engine().begin() as conn: + repo.create_project(conn, "proj", str(root), git_remote=str(bare)) + + agent = spawn.spawn("proj", "api", task="do it") + + wt = env["tmp"] / "proj" / "api" + assert agent["working_dir"] == str(wt) + assert (wt / "NEW.md").exists() # reset to the remote tip, not the stale branch