From ad986aa884c7b80f83b3c07678a809a6286f1039 Mon Sep 17 00:00:00 2001 From: sprooty Date: Wed, 5 Aug 2026 04:11:25 +0000 Subject: [PATCH] feat: refuse to start against a base branch that has fallen behind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A stale base is the one wrong setting every later stage reports as a success. The agent works, the checks pass, the reviewer approves, the commit lands — onto a lineage nobody develops on any more. There is no failure to notice, so nothing noticed, and the cost is not one item but every item in the run. Measured on rdpapp: work was based on a branch cut from a local working tree that turned out to be 121 commits behind the authoritative remote and 27 ahead, carrying an alternate implementation never promoted into the released lineage. Six items were delivered onto it before a human who knew the lineage said so. All six are discarded. `preflight` fetches once and compares. Behind by a little is reported and allowed, because every branch is a few commits behind and blocking on that would make the check noise nobody reads. Behind by more than `STALE_BASE_LIMIT` is refused, with `--allow-stale-base` to override, mirroring `--allow-dirty`. Two things this gets right only because a test caught them getting it wrong: **A branch that tracks nothing is still measured.** The first version returned "no single obvious line of work to compare against" when a base had no upstream and the repository had several remotes — which is the rdpapp shape exactly, so it passed the case it was written for. It now compares against every remote's default head, because "which remote is authoritative" is not a question a preflight check can answer and not one it should guess at. **An unrelated remote cannot vouch for a stale base.** Measuring against every remote then let a two-commit unrelated repository, sitting in the same checkout as a second remote, mark a base thirty behind the real one as current. Only remotes sharing history with the base count as candidate lineages. Unreachable remotes, absent upstreams and repositories with no remote at all are reported and pass. None of them are evidence that a base is stale, and reporting them as failures trains people to pass the override by default. Runs in **both** modes, unlike the dirty-tree check: session mode gives each item a pristine worktree, and a pristine worktree of the wrong lineage is still the wrong lineage. Closes #180. Co-Authored-By: Claude Opus 5 (1M context) --- src/agent_harness/__main__.py | 25 ++++++ src/agent_harness/preflight.py | 138 +++++++++++++++++++++++++++++++++ tests/test_preflight.py | 121 +++++++++++++++++++++++++++++ 3 files changed, 284 insertions(+) diff --git a/src/agent_harness/__main__.py b/src/agent_harness/__main__.py index d94c539..72b24dd 100644 --- a/src/agent_harness/__main__.py +++ b/src/agent_harness/__main__.py @@ -681,6 +681,22 @@ def live_routes() -> dict[str, Chain]: if not clean: log.warning("--allow-dirty: %s", why) + # Applies to BOTH modes, unlike the check above. A worktree cut from a base + # that has fallen behind is the rdpapp failure exactly: session mode gives + # each item a pristine worktree, and a pristine worktree of the wrong + # lineage is still the wrong lineage (#180). + if args.work and args.base and (Path(args.work) / ".git").exists(): + from .preflight import _base_is_current + + current, where = _base_is_current(str(args.work), args.base) + if not current and not args.allow_stale_base: + print(f"refusing to start: {where}", file=sys.stderr) + return 2 + if not current: + log.warning("--allow-stale-base: %s", where) + else: + log.info("base: %s", where) + if demo_mode: from .demo import demo_transport @@ -1264,6 +1280,15 @@ def main(argv: list[str] | None = None) -> int: "neither recoverably — so a dirty checkout is refused by default. Pass " "this only when the tree is genuinely disposable.", ) + p_run.add_argument( + "--allow-stale-base", + action="store_true", + help="run against a base branch that has fallen well behind its upstream. " + "A stale base is the one wrong setting every later stage reports as a " + "success — the agent works, the checks pass, the reviewer approves, and " + "the commit lands on a lineage nobody develops on any more — so it is " + "refused by default. Pass this when the base really is the line of work.", + ) p_run.add_argument( "--context-budget", type=int, diff --git a/src/agent_harness/preflight.py b/src/agent_harness/preflight.py index 4af0e94..07312fe 100644 --- a/src/agent_harness/preflight.py +++ b/src/agent_harness/preflight.py @@ -136,6 +136,129 @@ def _is_clean_tree(path: str) -> tuple[bool, str]: ) +#: How far behind its upstream a base may be before the run is refused. A few +#: commits behind is the ordinary state of any branch and blocking on it would +#: make the check noise nobody reads. Two dozen is a different repository. +STALE_BASE_LIMIT = 25 + + +def _git(path: str, *args: str, timeout: float = 60.0) -> tuple[int, str, str]: + try: + result = subprocess.run( # noqa: S603 + ["git", "-C", path, *args], # noqa: S607 + capture_output=True, + text=True, + timeout=timeout, + ) + except (OSError, subprocess.SubprocessError) as exc: + return (1, "", str(exc)) + return (result.returncode, result.stdout.strip(), result.stderr.strip()) + + +def _base_is_current(path: str, base: str) -> tuple[bool, str]: + """Whether the base branch still resembles the line of work it came from. + + **This is the one wrong configuration every downstream stage reports as a + success.** The agent works, the checks pass, the reviewer approves and the + commit lands — onto a branch nobody is developing on any more. There is no + failure to notice, so nothing notices, and the cost is not one item but + every item in the run. + + Measured on `rdpapp`: a base cut from a local working tree turned out to be + 121 commits behind `origin/master` and 27 ahead, carrying an alternate + implementation that was never promoted. Six items were delivered onto it + before a human who knew the lineage said so (#180). + + Deliberately quiet about what it cannot answer. A branch with no upstream, + a repository with no remote, an unreachable remote — none of those are + evidence that the base is stale, and reporting them as failures would train + people to pass the override flag by default. + """ + upstreams = _upstreams_for(path, base) + if not upstreams: + return (True, f"nothing to compare {base} against: no upstream and no remote default") + + # Behind *every* candidate is the finding. A base cut locally for one run + # legitimately tracks nothing, and a repository can have several remotes + # only one of which is authoritative — so being current with any one of + # them is enough to believe the base is on a live line of work. + # + # The first draft of this returned early when a branch tracked nothing and + # the repository had more than one remote, which is exactly the shape of + # the case it was written for: `harness/base` tracked nothing, `rdpapp` has + # a Forgejo `origin` and a GitHub secondary, and the check said nothing at + # all while the base sat 121 commits behind. + results: list[tuple[int | None, str]] = [] + for upstream in upstreams: + remote = upstream.split("/", 1)[0] + code, _, err = _git(path, "fetch", "--quiet", remote, timeout=120.0) + if code == 0 and _git(path, "merge-base", base, upstream)[0] != 0: + # No common ancestor: a different project that happens to be a + # remote of this checkout, not a line of work this base could have + # come from. Counting it would let an unrelated remote with two + # commits in it vouch for a base that is a hundred behind the one + # that matters. + continue + if code != 0: + # Unreachable is a fact about the network, not about the base. + results.append((None, f"could not reach {remote}: {err[:100]}")) + continue + code, counts, _ = _git(path, "rev-list", "--left-right", "--count", f"{base}...{upstream}") + if code != 0 or "\t" not in counts: + results.append((None, f"could not compare {base} against {upstream}")) + continue + ahead_s, _, behind_s = counts.partition("\t") + try: + ahead, behind = int(ahead_s), int(behind_s.strip()) + except ValueError: # pragma: no cover - git's output is two integers + results.append((None, f"could not read {base}...{upstream}")) + continue + results.append( + ( + behind, + f"{behind} commit(s) behind {upstream}" + (f" and {ahead} ahead" if ahead else ""), + ) + ) + + measured = [(behind, text) for behind, text in results if behind is not None] + if not measured: + return (True, f"could not compare {base}: " + "; ".join(text for _, text in results)) + + closest, detail = min(measured, key=lambda pair: pair[0]) + if closest == 0: + return (True, f"{base} is current with {detail.split(' behind ', 1)[-1]}") + where = f"{base} is " + "; ".join(text for _, text in measured) + if closest <= STALE_BASE_LIMIT: + return (True, where) + return ( + False, + f"{where}. Work based here lands on a lineage that has moved on, and every " + "stage after this one — the agent, the checks, the reviewer, the commit — " + "will report success while it happens. Rebase or cut a new base from the " + "current head, or pass --allow-stale-base if this really is the line of work.", + ) + + +def _upstreams_for(path: str, base: str) -> list[str]: + """Every remote ref this base could reasonably be measured against. + + Its own tracking branch when it has one, since that is the answer the + person who made the branch already gave. Otherwise every remote's default + head — plural on purpose, because "which remote is authoritative" is not a + question a preflight check can answer and not one it should guess at. + """ + code, upstream, _ = _git(path, "rev-parse", "--abbrev-ref", f"{base}@{{upstream}}") + if code == 0 and upstream: + return [upstream] + code, remotes, _ = _git(path, "remote") + heads = [] + for remote in (name.strip() for name in remotes.splitlines() if name.strip()): + code, head, _ = _git(path, "symbolic-ref", "--short", f"refs/remotes/{remote}/HEAD") + if code == 0 and head: + heads.append(head) + return heads + + def _gh_can_write(repo: str) -> tuple[bool, str]: """Whether `gh` can actually write to the repo. @@ -504,6 +627,8 @@ def preflight_project( disk_probe: Callable[[str, float], tuple[bool, str]] = disk_space_probe, clean_probe: Callable[[str], tuple[bool, str]] = _is_clean_tree, allow_dirty: bool = False, + base_probe: Callable[[str, str], tuple[bool, str]] = _base_is_current, + allow_stale_base: bool = False, ) -> Preflight: """Everything that must hold before this project can produce a pull request.""" checks: list[Check] = [] @@ -555,6 +680,19 @@ def preflight_project( else f"{clean_detail}{' Allowed by --allow-dirty.' if allow_dirty else ''}", ) ) + base = str(getattr(project, "base_branch", "") or "") + if base: + base_ok, base_detail = base_probe(work_dir, base) + checks.append( + Check( + "base branch", + base_ok or allow_stale_base, + base_detail + if base_ok + else f"{base_detail}" + + (" Allowed by --allow-stale-base." if allow_stale_base else ""), + ) + ) disk_ok, disk_detail = disk_probe( work_dir, float(getattr(project, "min_free_disk_gb", 0.0) or 0.0) ) diff --git a/tests/test_preflight.py b/tests/test_preflight.py index fd1b706..505d2cf 100644 --- a/tests/test_preflight.py +++ b/tests/test_preflight.py @@ -16,6 +16,7 @@ import pytest from agent_harness.preflight import ( + STALE_BASE_LIMIT, Answer, _is_clean_tree, clean_checks_probe, @@ -406,3 +407,123 @@ def test_allow_dirty_lets_it_start_and_still_says_what_is_at_risk(tmp_path: Path assert check["ok"] assert "3 uncommitted change(s)" in check["detail"] assert "Allowed by --allow-dirty" in check["detail"] + + +# --------------------------------------------- a base that has fallen behind + + +def _repo_with_remote(tmp_path: Path, *, behind: int, remote: str = "origin") -> Path: + """A checkout whose `work` branch is `behind` commits behind its remote.""" + upstream = tmp_path / f"{remote}-upstream" + upstream.mkdir() + _git_init(upstream) + (upstream / "f.txt").write_text("0\n") + _run(upstream, "add", "-A") + _run(upstream, "commit", "-qm", "base") + + local = tmp_path / f"local-{remote}-{behind}" + _run(tmp_path, "clone", "-q", str(upstream), str(local)) + _run(local, "config", "user.email", "t@t") + _run(local, "config", "user.name", "t") + _run(local, "checkout", "-qb", "work") + + for n in range(behind): + (upstream / "f.txt").write_text(f"{n + 1}\n") + _run(upstream, "add", "-A") + _run(upstream, "commit", "-qm", f"upstream {n}") + return local + + +def _git_init(path: Path) -> None: + _run(path, "init", "-q", "-b", "main") + _run(path, "config", "user.email", "t@t") + _run(path, "config", "user.name", "t") + + +def _run(path: Path, *args: str) -> str: + import subprocess + + return subprocess.run( + ["git", "-C", str(path), *args], capture_output=True, text=True, check=True + ).stdout + + +def test_a_base_far_behind_its_upstream_is_refused(tmp_path: Path) -> None: + """The rdpapp failure: 121 commits behind, and every later stage said ok.""" + from agent_harness.preflight import _base_is_current + + repo = _repo_with_remote(tmp_path, behind=STALE_BASE_LIMIT + 5) + + ok, why = _base_is_current(str(repo), "work") + + assert ok is False + assert f"{STALE_BASE_LIMIT + 5} commit(s) behind" in why + assert "--allow-stale-base" in why + + +def test_a_base_slightly_behind_is_reported_but_not_refused(tmp_path: Path) -> None: + """Every branch is a few commits behind. Blocking on that is noise nobody reads.""" + from agent_harness.preflight import _base_is_current + + repo = _repo_with_remote(tmp_path, behind=2) + + ok, why = _base_is_current(str(repo), "work") + + assert ok is True + assert "2 commit(s) behind" in why + + +def test_a_current_base_says_so(tmp_path: Path) -> None: + from agent_harness.preflight import _base_is_current + + repo = _repo_with_remote(tmp_path, behind=0) + + ok, why = _base_is_current(str(repo), "work") + + assert ok is True + assert "current with" in why + + +def test_a_branch_tracking_nothing_is_still_measured_against_every_remote( + tmp_path: Path, +) -> None: + """The bug in the first draft of this check, and the exact rdpapp shape. + + `harness/base` tracked nothing and the repository had two remotes, so the + check returned "no single obvious line of work to compare against" and said + nothing while the base sat 121 commits behind the authoritative one. + """ + from agent_harness.preflight import _base_is_current + + repo = _repo_with_remote(tmp_path, behind=STALE_BASE_LIMIT + 5) + # A second remote, exactly as rdpapp has a Forgejo origin and a GitHub one. + other = tmp_path / "second" + other.mkdir() + _git_init(other) + (other / "g.txt").write_text("x\n") + _run(other, "add", "-A") + _run(other, "commit", "-qm", "unrelated") + _run(repo, "remote", "add", "github", str(other)) + _run(repo, "fetch", "-q", "github") + # `work` has no upstream by construction — it was cut locally with + # `checkout -b`, which is the ordinary case and the one that went unnoticed. + + ok, why = _base_is_current(str(repo), "work") + + assert ok is False, why + assert "behind" in why + + +def test_an_unreachable_remote_is_not_evidence_that_the_base_is_stale( + tmp_path: Path, +) -> None: + """A network fact must not be reported as a fact about the lineage.""" + from agent_harness.preflight import _base_is_current + + repo = _repo_with_remote(tmp_path, behind=0) + _run(repo, "remote", "set-url", "origin", str(tmp_path / "does-not-exist")) + + ok, why = _base_is_current(str(repo), "work") + + assert ok is True + assert "could not" in why