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