mirror of
https://github.com/0xWheatyz/handler.git
synced 2026-08-29 19:21:40 +00:00
Merge pull request #34 from 0xWheatyz/claude/mobile-app-feature-parity-run9dz
Root checkout is a ref store: worktree spawns never touch it
This commit is contained in:
@@ -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/<name>` 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
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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/<name> 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
|
||||
|
||||
@@ -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 <root>/<agent> <branch>``.
|
||||
- neither: the project root itself.
|
||||
|
||||
``reset_existing`` is set for spawn-derived ``agent/<name>`` 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 <path> <branch>` 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:
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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/<name> 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
|
||||
|
||||
Reference in New Issue
Block a user