Harden agent-team apply: reject bad diffs, list draft PRs via App #73

Closed
amoussa1229 wants to merge 2 commits from fix/agent-team-builder-validation-and-app-monitor into main
5 changed files with 333 additions and 15 deletions

View file

@ -26,19 +26,24 @@ production.
from __future__ import annotations from __future__ import annotations
import base64 import base64
import logging
import re import re
from collections.abc import Callable
from dataclasses import dataclass from dataclasses import dataclass
from datetime import datetime, timezone from datetime import datetime, timezone
from typing import Any, Protocol from typing import Any, Protocol
from agent_team.state_store import compute_content_hash from agent_team.state_store import compute_content_hash
_LOG = logging.getLogger(__name__)
__all__ = [ __all__ = [
"DispatchInputs", "DispatchInputs",
"DispatchResult", "DispatchResult",
"DispatcherError", "DispatcherError",
"RunLocator", "RunLocator",
"app_branch_pusher", "app_branch_pusher",
"app_draft_pr_lister",
"app_run_locator", "app_run_locator",
"app_workflow_dispatcher", "app_workflow_dispatcher",
"build_dispatch_inputs", "build_dispatch_inputs",
@ -748,6 +753,92 @@ def app_workflow_dispatcher(
return _fire 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( def app_run_locator(
token_provider: Any, token_provider: Any,
*, *,

View file

@ -55,6 +55,7 @@ __all__ = [
"BuildError", "BuildError",
"DiffBuilder", "DiffBuilder",
"TrustBoundaryViolation", "TrustBoundaryViolation",
"assert_applicable_diff_shape",
"build_candidate_diff", "build_candidate_diff",
"builders_node", "builders_node",
"default_diff_builder", "default_diff_builder",
@ -583,6 +584,55 @@ class _BuildOutcome:
return not self.violations return not self.violations
# A canonical unified-diff hunk header: "@@ -<start>[,<len>] +<start>[,<len>] @@".
# 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( def build_candidate_diff(
plan: Mapping[str, Any], plan: Mapping[str, Any],
*, *,
@ -611,6 +661,11 @@ def build_candidate_diff(
if not isinstance(diff, str) or not diff.strip(): if not isinstance(diff, str) or not diff.strip():
raise BuildError("diff builder produced an empty candidate diff") 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")) diff_hash = compute_content_hash(diff.encode("utf-8"))
violations = scan_trust_control_surface(diff, scope=plan.get("scope")) violations = scan_trust_control_surface(diff, scope=plan.get("scope"))
return _BuildOutcome(diff=diff, diff_hash=diff_hash, violations=violations) return _BuildOutcome(diff=diff, diff_hash=diff_hash, violations=violations)

View file

@ -581,28 +581,88 @@ _DRAFT_PR_HEAD_PREFIX = "agent-team/apply/"
_DRAFT_PR_LIST_TIMEOUT_S = 30 _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]]": def _default_draft_pr_provider(*, owner: str, repo: str) -> "Callable[[], list[Any]]":
"""Build the production READ-ONLY draft-PR provider for the A4 monitor. """Build the production READ-ONLY draft-PR provider for the A4 monitor.
Returns a zero-arg callable that enumerates the currently-open *agent-team* 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, draft PRs and maps each into a :class:`agent_team.draft_pr_monitor.DraftPr`
closes, or dispatches anything) and maps each into a snapshot the monitor consumes. It NEVER writes, closes, or dispatches
:class:`agent_team.draft_pr_monitor.DraftPr` snapshot the monitor consumes. 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 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 (: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 apply/verify draft PRs — never an unrelated human draft PR. Both paths fail
exactly the three fields the monitor keys off (``number`` / ``createdAt`` -> closed — any transport, auth, or parse error yields an empty snapshot (the
``opened_at`` for the runaway window, ``updatedAt`` -> ``updated_at`` for monitor no-ops this pass) rather than raising, so a transient hiccup never
staleness). breaks the tick loop. (The coordinator's ``_draft_pr_monitor_sweep`` ALSO
swallows provider errors, so this is belt-and-suspenders.)
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.)
""" """
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}" repo_slug = f"{owner}/{repo}"
def provider() -> list[Any]: def provider() -> list[Any]:

View file

@ -480,6 +480,45 @@ def test_build_rejects_empty_diff() -> None:
build_candidate_diff(plan, builder=_stub_builder(" \n ")) 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: def test_build_uses_default_builder_when_none() -> None:
billing.set_invoker( billing.set_invoker(
lambda prompt, *, mode, **kw: ClaudeResult(text=CLEAN_DIFF, mode=mode) 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: 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",))} state = {"plan": _scope_plan(None, scope=("src",))}
update = builders_node(state, builder=_stub_builder(diff)) update = builders_node(state, builder=_stub_builder(diff))
assert update["status"] == TaskStatus.PARKED.value assert update["status"] == TaskStatus.PARKED.value

View file

@ -17,6 +17,7 @@ from agent_team.dispatcher import (
DispatchInputs, DispatchInputs,
DispatchResult, DispatchResult,
app_branch_pusher, app_branch_pusher,
app_draft_pr_lister,
app_run_locator, app_run_locator,
app_workflow_dispatcher, app_workflow_dispatcher,
build_dispatch_inputs, 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 "ghs_TESTTOKEN" not in str(excinfo.value)
assert excinfo.value.__cause__ is None 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/") == []