diff --git a/agent-team/agent_team/dispatcher.py b/agent-team/agent_team/dispatcher.py index dc84dcd..a241c2c 100644 --- a/agent-team/agent_team/dispatcher.py +++ b/agent-team/agent_team/dispatcher.py @@ -26,19 +26,24 @@ production. from __future__ import annotations import base64 +import logging import re +from collections.abc import Callable from dataclasses import dataclass from datetime import datetime, timezone from typing import Any, Protocol from agent_team.state_store import compute_content_hash +_LOG = logging.getLogger(__name__) + __all__ = [ "DispatchInputs", "DispatchResult", "DispatcherError", "RunLocator", "app_branch_pusher", + "app_draft_pr_lister", "app_run_locator", "app_workflow_dispatcher", "build_dispatch_inputs", @@ -748,6 +753,92 @@ def app_workflow_dispatcher( return _fire +def app_draft_pr_lister( + token_provider: Any, *, _http: Any = None +) -> "Callable[..., list[dict[str, Any]]]": + """App-token draft-PR lister for the A4 monitor — REST, never ``gh``. + + Enumerates the currently-open *agent-team* draft PRs via + ``GET /repos/{owner}/{repo}/pulls?state=open`` with an installation token from + ``token_provider.token()``, replacing the ``gh pr list`` shell-out (the box + has no ``gh`` installed, so that path raised ``FileNotFoundError`` every tick + and the monitor was permanently blind). The rows are filtered client-side to + ``draft is True`` and a head ref under ``head_prefix`` (the dispatcher's own + ``agent-team/apply/`` namespace) so an unrelated human draft PR is never + counted, and mapped to the three fields the monitor keys off + (``number`` / ``createdAt`` / ``updatedAt``). + + Fail-SOFT, mirroring the ``gh`` provider it replaces and the sibling sweeps: + any transport, auth (non-200), or parse error yields an EMPTY list (the + monitor no-ops this pass) rather than raising — a maintenance sweep must never + crash the tick loop. The Bearer token is NEVER logged or surfaced in an error. + + ``_http`` injects a ``requests``-like client for tests (``.get(url, *, + params=..., headers=..., timeout=...)`` -> response exposing ``.status_code`` + and ``.json()``); the default lazily imports ``requests``. + """ + + def _list(*, owner: str, repo: str, head_prefix: str) -> list[dict[str, Any]]: + headers = { + "Accept": "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + "Authorization": f"Bearer {token_provider.token()}", + } + url = f"{GITHUB_API_ROOT}/repos/{owner}/{repo}/pulls" + params = {"state": "open", "per_page": 100} + try: + if _http is not None: + resp = _http.get(url, params=params, headers=headers, timeout=15.0) + else: + import requests # deferred: optional dependency + + resp = requests.get(url, params=params, headers=headers, timeout=15.0) + except Exception as exc: # noqa: BLE001 - never surface a token-bearing error + _LOG.warning( + "draft-pr-monitor: REST pulls enumeration transport error (%s); " + "treating as no open draft PRs this pass", + type(exc).__name__, + ) + return [] + status = getattr(resp, "status_code", None) + if status != 200: + _LOG.warning( + "draft-pr-monitor: REST pulls enumeration failed (status=%s); " + "treating as no open draft PRs this pass", + status, + ) + return [] + try: + rows = resp.json() + except Exception: # noqa: BLE001 - malformed body -> fail soft + _LOG.warning( + "draft-pr-monitor: REST pulls returned unparseable JSON; " + "treating as no open draft PRs this pass" + ) + return [] + + out: list[dict[str, Any]] = [] + for row in rows or []: + if not isinstance(row, dict) or not row.get("draft"): + continue + head_ref = ((row.get("head") or {}).get("ref")) or "" + if not head_ref.startswith(head_prefix): + continue + number = row.get("number") + if number is None: + continue + out.append( + { + "number": number, + "createdAt": row.get("created_at"), + "updatedAt": row.get("updated_at"), + } + ) + return out + + return _list + + def app_run_locator( token_provider: Any, *, diff --git a/agent-team/agent_team/nodes/builders.py b/agent-team/agent_team/nodes/builders.py index d69cfdc..cc35021 100644 --- a/agent-team/agent_team/nodes/builders.py +++ b/agent-team/agent_team/nodes/builders.py @@ -55,6 +55,7 @@ __all__ = [ "BuildError", "DiffBuilder", "TrustBoundaryViolation", + "assert_applicable_diff_shape", "build_candidate_diff", "builders_node", "default_diff_builder", @@ -583,6 +584,55 @@ class _BuildOutcome: return not self.violations +# A canonical unified-diff hunk header: "@@ -[,] +[,] @@". +# A builder that emits PLACEHOLDER line numbers (observed live: "@@ -X,Y +A,B @@") +# produces a patch that fails `git apply` with exit 128 at dispatch — the task +# then parks with an opaque "git apply failed" error far from the real cause. We +# reject it HERE, at build, with an actionable reason instead. +_HUNK_HEADER_RE = re.compile(r"^@@ -\d+(?:,\d+)? \+\d+(?:,\d+)? @@") + + +def assert_applicable_diff_shape(diff: str) -> None: + """Reject a candidate diff that cannot apply or is a no-op, BEFORE dispatch. + + Two failure modes seen live both slipped past the empty-string guard in + :func:`build_candidate_diff` and only manifested downstream — one as a cryptic + dispatch crash, the other as a BLANK draft PR: + + * **Malformed hunk header** — a builder that emits placeholder line numbers + (``@@ -X,Y +A,B @@``) yields a patch ``git apply`` rejects (exit 128). The + task parks at dispatch with an opaque error instead of here with the cause. + * **No-op patch** — a diff that authors no content (e.g. a bare empty-file + creation, ``new file mode … index 0000000..e69de29`` with no hunk) applies + cleanly and becomes a blank PR with "no actual changes". + + Pure + deterministic (no network, no checkout): it inspects the diff text + only. Raises :class:`BuildError` (the build-failure contract) naming the + reason, so the coordinator parks with an actionable signal rather than + dispatching a patch that is already known not to apply. + """ + has_content_change = False + for line in diff.splitlines(): + if line.startswith("@@"): + if not _HUNK_HEADER_RE.match(line): + raise BuildError( + "candidate diff has a malformed/placeholder hunk header " + f"(it will not apply): {line!r}" + ) + continue + # Skip the file-marker lines so only true body content counts; an added + # or removed line in the file body starts with a bare '+'/'-'. + if line.startswith(("+++ ", "--- ")): + continue + if line.startswith(("+", "-")): + has_content_change = True + if not has_content_change: + raise BuildError( + "candidate diff makes no content changes (empty/no-op patch — it " + "would open a blank PR)" + ) + + def build_candidate_diff( plan: Mapping[str, Any], *, @@ -611,6 +661,11 @@ def build_candidate_diff( if not isinstance(diff, str) or not diff.strip(): raise BuildError("diff builder produced an empty candidate diff") + # Catch a malformed (placeholder-hunk) or no-op (blank-PR) diff at build, + # where the reason is actionable, instead of at dispatch (opaque git-apply + # crash) or in a blank draft PR. + assert_applicable_diff_shape(diff) + diff_hash = compute_content_hash(diff.encode("utf-8")) violations = scan_trust_control_surface(diff, scope=plan.get("scope")) return _BuildOutcome(diff=diff, diff_hash=diff_hash, violations=violations) diff --git a/agent-team/run-team.py b/agent-team/run-team.py index db4ac55..fa406bc 100644 --- a/agent-team/run-team.py +++ b/agent-team/run-team.py @@ -581,28 +581,88 @@ _DRAFT_PR_HEAD_PREFIX = "agent-team/apply/" _DRAFT_PR_LIST_TIMEOUT_S = 30 +def _app_token_provider_from_env() -> "Any | None": + """Build a :class:`agent_team.github_app.TokenProvider` from env, or ``None``. + + Mirrors :func:`agent_team.coordinator._app_dispatch_seams`' resolution: all + three of ``AGENT_TEAM_GH_APP_ID`` / ``_INSTALLATION_ID`` / ``_PRIVATE_KEY`` + (a path to the App ``.pem``) must be set and the key file readable, else this + returns ``None`` and the caller falls back to the ``gh`` path. Never raises + and NEVER logs the key path's contents — a half-configured box still serves. + """ + import os # noqa: PLC0415 + from pathlib import Path # noqa: PLC0415 + + app_id = os.environ.get("AGENT_TEAM_GH_APP_ID", "").strip() + inst = os.environ.get("AGENT_TEAM_GH_APP_INSTALLATION_ID", "").strip() + key_path = os.environ.get("AGENT_TEAM_GH_APP_PRIVATE_KEY", "").strip() + if not (app_id and inst and key_path): + return None + try: + pem = Path(key_path).read_text(encoding="utf-8") + except Exception: # noqa: BLE001 - unreadable key -> gh fallback, never crash + _LOG.warning( + "draft-pr-monitor: AGENT_TEAM_GH_APP_PRIVATE_KEY path could not be " + "read; falling back to the gh draft-PR provider." + ) + return None + from agent_team.github_app import TokenProvider # noqa: PLC0415 + + return TokenProvider(app_id=app_id, private_key_pem=pem, installation_id=inst) + + def _default_draft_pr_provider(*, owner: str, repo: str) -> "Callable[[], list[Any]]": """Build the production READ-ONLY draft-PR provider for the A4 monitor. Returns a zero-arg callable that enumerates the currently-open *agent-team* - draft PRs via a single read-only ``gh pr list`` (one GET; it NEVER writes, - closes, or dispatches anything) and maps each into a - :class:`agent_team.draft_pr_monitor.DraftPr` snapshot the monitor consumes. + draft PRs and maps each into a :class:`agent_team.draft_pr_monitor.DraftPr` + snapshot the monitor consumes. It NEVER writes, closes, or dispatches + anything. + + **Auth path (GitHub App vs gh-default).** When the three ``AGENT_TEAM_GH_APP_*`` + env vars resolve a token provider, enumeration goes through the REST + ``GET /repos/{owner}/{repo}/pulls`` with a short-lived installation token + (:func:`agent_team.dispatcher.app_draft_pr_lister`) — the SAME App auth the + dispatcher already uses, and the path that does not need ``gh`` on the box. + This is the live R720 path: the box has no ``gh`` installed, so the old + ``gh pr list`` shell-out raised ``FileNotFoundError`` every tick and the + monitor was permanently blind. Absent the App env (or an unreadable key) it + falls back to the ``gh`` CLI for parity with environments that have it. The query is scoped to the ``agent-team/apply/`` head namespace (:data:`_DRAFT_PR_HEAD_PREFIX`) so it only ever sees the dispatcher's own - apply/verify draft PRs — never an unrelated human draft PR. ``--json`` pulls - exactly the three fields the monitor keys off (``number`` / ``createdAt`` -> - ``opened_at`` for the runaway window, ``updatedAt`` -> ``updated_at`` for - staleness). - - Mirrors :func:`agent_team.ci_watcher.default_ci_poller`: a thin closure over - ``owner`` / ``repo`` that fails closed — a non-zero ``gh`` exit, a timeout, or - unparseable JSON yields an empty snapshot (the monitor then no-ops this pass) - rather than raising, so a transient gh hiccup never breaks the tick loop. (The - coordinator's ``_draft_pr_monitor_sweep`` ALSO swallows provider errors, so - this is belt-and-suspenders.) + apply/verify draft PRs — never an unrelated human draft PR. Both paths fail + closed — any transport, auth, or parse error yields an empty snapshot (the + monitor no-ops this pass) rather than raising, so a transient hiccup never + breaks the tick loop. (The coordinator's ``_draft_pr_monitor_sweep`` ALSO + swallows provider errors, so this is belt-and-suspenders.) """ + from agent_team.draft_pr_monitor import DraftPr # noqa: PLC0415 + + token_provider = _app_token_provider_from_env() + if token_provider is not None: + from agent_team.dispatcher import app_draft_pr_lister # noqa: PLC0415 + + lister = app_draft_pr_lister(token_provider) + + def app_provider() -> list[Any]: + rows = lister(owner=owner, repo=repo, head_prefix=_DRAFT_PR_HEAD_PREFIX) + snapshots: list[Any] = [] + for row in rows: + number = row.get("number") + if number is None: + continue + snapshots.append( + DraftPr( + number=int(number), + opened_at=row.get("createdAt"), + updated_at=row.get("updatedAt"), + ) + ) + return snapshots + + return app_provider + repo_slug = f"{owner}/{repo}" def provider() -> list[Any]: diff --git a/agent-team/tests/test_builders.py b/agent-team/tests/test_builders.py index 072ce76..09f096f 100644 --- a/agent-team/tests/test_builders.py +++ b/agent-team/tests/test_builders.py @@ -480,6 +480,45 @@ def test_build_rejects_empty_diff() -> None: build_candidate_diff(plan, builder=_stub_builder(" \n ")) +# Builder failure modes seen live (both slipped past the empty-string guard): +# * a placeholder-hunk diff (correct content, "@@ -X,Y +A,B @@" headers) that +# git apply rejects (exit 128) -> the task parked at dispatch with an opaque +# error and no PR. +# * a no-op empty-file creation that applied cleanly -> a BLANK draft PR. +PLACEHOLDER_HUNK_DIFF = """diff --git a/README.md b/README.md +index abcdef1..1234567 100644 +--- a/README.md ++++ b/README.md +@@ -X,Y +A,B @@ ++## New section ++body line +""" + +EMPTY_NEW_FILE_DIFF = """diff --git a/tests/test_smoke.py b/tests/test_smoke.py +new file mode 100644 +index 0000000..e69de29 +""" + + +def test_build_rejects_placeholder_hunk_header() -> None: + plan = _scope_plan(None) + with pytest.raises(BuildError, match="hunk header"): + build_candidate_diff(plan, builder=_stub_builder(PLACEHOLDER_HUNK_DIFF)) + + +def test_build_rejects_noop_empty_file_diff() -> None: + plan = _scope_plan(None) + with pytest.raises(BuildError, match="no content changes"): + build_candidate_diff(plan, builder=_stub_builder(EMPTY_NEW_FILE_DIFF)) + + +def test_valid_diffs_pass_shape_check() -> None: + # Regression guard: the new shape check must NOT reject the legitimate + # fixtures (a modify-in-place and a new-file-with-content diff). + for diff in (CLEAN_DIFF, NEW_FILE_DIFF): + builders.assert_applicable_diff_shape(diff) # does not raise + + def test_build_uses_default_builder_when_none() -> None: billing.set_invoker( lambda prompt, *, mode, **kw: ClaudeResult(text=CLEAN_DIFF, mode=mode) @@ -516,7 +555,16 @@ def test_node_violation_parks_for_human_review() -> None: def test_node_out_of_scope_parks() -> None: - diff = "+++ b/other/x.py\n" + # A realistic out-of-scope diff (with content, so it passes the shape check + # and reaches the scope scan that parks it). + diff = ( + "diff --git a/other/x.py b/other/x.py\n" + "--- a/other/x.py\n" + "+++ b/other/x.py\n" + "@@ -1 +1 @@\n" + "-a = 1\n" + "+a = 2\n" + ) state = {"plan": _scope_plan(None, scope=("src",))} update = builders_node(state, builder=_stub_builder(diff)) assert update["status"] == TaskStatus.PARKED.value diff --git a/agent-team/tests/test_dispatcher.py b/agent-team/tests/test_dispatcher.py index ebfb8ba..40a92a9 100644 --- a/agent-team/tests/test_dispatcher.py +++ b/agent-team/tests/test_dispatcher.py @@ -17,6 +17,7 @@ from agent_team.dispatcher import ( DispatchInputs, DispatchResult, app_branch_pusher, + app_draft_pr_lister, app_run_locator, app_workflow_dispatcher, build_dispatch_inputs, @@ -595,3 +596,66 @@ def test_app_run_locator_scrubs_token_from_transport_error() -> None: ) assert "ghs_TESTTOKEN" not in str(excinfo.value) assert excinfo.value.__cause__ is None + + +# --------------------------------------------------------------------------- # +# app_draft_pr_lister (App-token REST seam; replaces the `gh pr list` shell-out) +# --------------------------------------------------------------------------- # + +_PULLS_BODY = [ + # In scope + draft -> included. + { + "number": 72, + "draft": True, + "head": {"ref": "agent-team/apply/abc123"}, + "created_at": "2026-06-25T16:00:00Z", + "updated_at": "2026-06-25T16:30:00Z", + }, + # Draft but NOT in the agent-team/apply namespace -> excluded. + { + "number": 50, + "draft": True, + "head": {"ref": "feature/human-thing"}, + "created_at": "2026-06-24T10:00:00Z", + "updated_at": "2026-06-24T11:00:00Z", + }, + # In namespace but NOT a draft (a ready PR) -> excluded. + { + "number": 60, + "draft": False, + "head": {"ref": "agent-team/apply/def456"}, + "created_at": "2026-06-25T12:00:00Z", + "updated_at": "2026-06-25T12:30:00Z", + }, +] + + +def test_app_draft_pr_lister_filters_to_agent_team_drafts() -> None: + http = _FakeHttp(get_body=_PULLS_BODY, get_status=200) + lister = app_draft_pr_lister(_StubTokenProvider(), _http=http) + rows = lister(owner="owner", repo="repo", head_prefix="agent-team/apply/") + + assert [r["number"] for r in rows] == [72] + row = rows[0] + assert row["createdAt"] == "2026-06-25T16:00:00Z" + assert row["updatedAt"] == "2026-06-25T16:30:00Z" + + call = http.get_calls[0] + assert call["url"].endswith("/repos/owner/repo/pulls") + assert call["params"]["state"] == "open" + assert call["headers"]["Authorization"] == "Bearer ghs_TESTTOKEN" + + +def test_app_draft_pr_lister_fails_soft_on_auth_error() -> None: + # A non-200 (e.g. 401/403) yields an EMPTY list (monitor no-ops) — it must + # NOT raise, unlike the dispatch seams, because a sweep cannot crash the tick. + http = _FakeHttp(get_body={}, get_status=403) + lister = app_draft_pr_lister(_StubTokenProvider(), _http=http) + assert lister(owner="o", repo="r", head_prefix="agent-team/apply/") == [] + + +def test_app_draft_pr_lister_fails_soft_and_scrubs_token_on_transport_error() -> None: + lister = app_draft_pr_lister(_StubTokenProvider(), _http=_RaisingHttp()) + # Returns [] (fail-soft); the raising transport embedded the token in its + # message, but the lister swallows it and never re-raises it. + assert lister(owner="o", repo="r", head_prefix="agent-team/apply/") == []