* feat(agent-team): read-only CI-result fetcher for P3 verify gate (opt-in, inert)
ci_fetcher.py: fail-closed CiResultFetcher reading the GitHub Actions run
conclusion via a read-only PAT (AGENT_TEAM_CI_READ_TOKEN→GITHUB_TOKEN), returns
{run_id,conclusion,diff_hash} or None on any error. Data-fetcher only — ci_gate
owns the verdict; never writes, no OIDC/AWS, never reads patch artifacts.
coordinator gains opt-in gated_build_verify_wiring() composing it via
bind_ci_result_fetcher; NOT wired into the default run-team.py path. 20 tests.
* harden(agent-team): P3 apply/verify workflow — GitHub App token, CWE-94, fail-closed
Decision-1 auth model: gate-and-pr uses a GitHub App installation token
(pull-requests:write) behind the agent-apply environment; ALL OIDC/id-token/AWS
removed. Hardening: task_id env-indirection (CWE-94 — GitHub expands ${{ }} into
the run shell before exec, so %s/quoting is insufficient); run-id pinning on both
download-artifact; post-build denied-path check (build-hook writes into denied
paths fail the job); empty-hash fail-closed in BOTH the embedded gate (fixed a
real ''=='' pass bug) and ci_gate.py. App-token + draft-PR steps stay if:${{ false }}
until provisioning (App + environment + branch protection). +17 tests.
* harden(agent-team): apply P3-live security-gate fixes (GPT-4.1 xreview + sh-security-review)
BLOCK-1/FIX-4: gate-and-pr re-comments pull-requests:write + environment:agent-apply
(provisioning-time uncomment) and gains needs.guard/build-test=='success' job guard —
zero privilege until provisioning. BLOCK-2/3+FIX-5: ci_fetcher validates run_id (^[0-9]{1,20}$),
owner/repo (^[A-Za-z0-9_.-]{1,100}$), and fetched_id (int) — fail closed, no SSRF/path
injection. FIX-1: conclusion allowlist. FIX-3: api_root removed from public builder (no
injectable endpoint). INJ-02: post-build denied-path check uses NUL-delimited git output +
explicit rename parsing, no backslash mangling, non-UTF8=violation. INJ-03: all three trust-
control denylists unified to one 22-entry union + drift-guard test. Q1: documented run_id/
diff_hash trust source (dispatcher/ledger only). 884 tests, ruff clean. Privileged steps stay
if:${{ false }} until provisioning.
* build(security-review): prune .claude worktrees from deterministic scanners
Agent worktrees under .claude/worktrees/ are full repo copies; the cfn-lint
find|xargs template scan overflowed ('command line cannot be assembled') and the
pre-push hook fail-closed to BLOCK whenever a worktree was present. Prune .claude
in the cfn-lint find + semgrep/checkov excludes, and gitignore .claude/ so it is
never scanned or committed. Unblocks main-tree pushes during parallel agent work.
365 lines
16 KiB
Python
365 lines
16 KiB
Python
"""Real, read-only CI-result fetcher — the GATED-LIVE seam for VERIFY (§3.3.2 #4).
|
|
|
|
This module implements the production :data:`~agent_team.nodes.build_verify_subgraph.CiResultFetcher`
|
|
that the build->verify subgraph's VERIFY node injects once the §3.3.2 CI
|
|
trust-boundary clears ``/sh-security-review`` + the GPT-4.1 cross-review. Until
|
|
then the subgraph runs with the INERT default fetcher (``_no_ci_result`` -> the
|
|
gate BLOCKs and the task parks); binding THIS fetcher only gives the pure-code
|
|
gate (:func:`agent_team.ci_gate.evaluate_ci_gate`) an authenticated conclusion
|
|
to read — it never makes the LLM the pass authority.
|
|
|
|
What it is (and, just as importantly, what it is NOT):
|
|
|
|
* It is a **pure DATA fetcher.** Given the task ``state``, it reads
|
|
``state["run_id"]`` (and echoes ``state.get("diff_hash")``), calls the GitHub
|
|
Actions REST API ``GET /repos/{owner}/{repo}/actions/runs/{run_id}`` with a
|
|
**READ-ONLY** token, and returns the authenticated run ``conclusion`` as a
|
|
mapping ``{"run_id", "conclusion", "diff_hash"}``. The gate owns the verdict;
|
|
this module never derives pass/fail itself.
|
|
* It is **fail-closed.** ANY error — missing ``run_id``, missing token, 404,
|
|
auth failure, malformed JSON, a network/timeout error, an unexpected status —
|
|
returns ``None``. A ``None`` result makes the gate BLOCK (never a silent
|
|
pass), so a broken fetch parks the task for a human rather than shipping.
|
|
* It **never writes.** No ``POST``/``PATCH``, no ``git``, no patch apply, no
|
|
filesystem mutation. It performs exactly one read-only GET.
|
|
* It uses **no cloud credentials and no token-federation** — only a GitHub
|
|
read-only token, resolved at CALL time from the environment (matching the
|
|
prevailing transport idiom in :mod:`agent_team.transport.github_live`). It
|
|
NEVER reads a success/failure file the patch could have written (boundary 4).
|
|
|
|
Prevailing HTTP approach: mirrors :mod:`agent_team.transport.github_live` — a
|
|
thin ``requests`` session, deferred import (``requests`` is optional and may be
|
|
absent pre-deploy), call-time token resolution, and an injectable ``client`` for
|
|
testability. Unlike the poster, a missing token / missing ``requests`` here does
|
|
NOT raise: it fails closed to ``None`` so the gate BLOCKs (the fetcher's whole
|
|
contract is "no authenticated result -> None"). The dedicated read-only env var
|
|
``AGENT_TEAM_CI_READ_TOKEN`` is preferred, falling back to ``GITHUB_TOKEN``.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import logging
|
|
import os
|
|
import re
|
|
from collections.abc import Mapping
|
|
from typing import Any
|
|
|
|
from agent_team.nodes.build_verify_subgraph import CiResultFetcher
|
|
|
|
__all__ = [
|
|
"CI_READ_TOKEN_ENV",
|
|
"GITHUB_API_ROOT",
|
|
"build_ci_result_fetcher",
|
|
"fetch_ci_result",
|
|
]
|
|
|
|
_LOG = logging.getLogger("agent_team.ci_fetcher")
|
|
|
|
# ───────────────────────────────────────────────────────────────────────────
|
|
# TRUST SOURCE (design §3.3.2, QUESTION-1). ``state["run_id"]`` and
|
|
# ``state["diff_hash"]`` are TRUSTED inputs to this fetcher and the pure-code
|
|
# gate: they MUST be written ONLY by the trusted dispatcher / ledger at dispatch
|
|
# time — NEVER by an LLM/builder/verifier node or by anything a candidate diff
|
|
# can influence. The dispatcher records the run id it dispatched the apply/verify
|
|
# workflow under (and the ledger-recorded diff hash) into task state; the LLM
|
|
# build/verify nodes only READ them. If that ever changes, the run_id format
|
|
# validation below (fail-closed) plus the gate's run-id EQUALITY check
|
|
# (:func:`agent_team.ci_gate.evaluate_ci_gate`, which compares the fetched run id
|
|
# against the dispatcher's ``expected_run_id``) are the defense: an attacker who
|
|
# could rewrite ``state["run_id"]`` to point at a different (passing) run would
|
|
# still have to match the dispatcher's expected_run_id, and a malformed/injected
|
|
# value fails the regex and returns None -> gate BLOCK.
|
|
# ───────────────────────────────────────────────────────────────────────────
|
|
|
|
# A GitHub Actions run id is a positive integer. Validate the state-supplied
|
|
# run_id against this BEFORE building any URL: a non-numeric value is either a
|
|
# bug or an injection attempt (it would be path-spliced into the API URL), so we
|
|
# fail closed (-> None -> gate BLOCK) rather than fetch an attacker-chosen path.
|
|
_RUN_ID_RE = re.compile(r"[0-9]{1,20}")
|
|
|
|
# owner / repo path segments. GitHub restricts these to a conservative charset;
|
|
# we validate both before URL construction so a crafted owner/repo cannot splice
|
|
# extra path segments / traversal into the GitHub API URL. Fail closed on miss.
|
|
_OWNER_REPO_RE = re.compile(r"[A-Za-z0-9_.-]{1,100}")
|
|
|
|
# Authenticated GitHub run conclusions we recognise. An unrecognised/missing
|
|
# conclusion is ambiguous and must fail closed here (the gate also BLOCKs on
|
|
# unknowns; validating at the fetcher makes the trust boundary explicit). Keep
|
|
# this in sync with agent_team.ci_gate's PASS + _FAILURE_CONCLUSIONS plus the
|
|
# benign non-failure values GitHub can return.
|
|
_ALLOWED_CONCLUSIONS: frozenset[str] = frozenset(
|
|
{
|
|
"success",
|
|
"failure",
|
|
"cancelled",
|
|
"skipped",
|
|
"timed_out",
|
|
"action_required",
|
|
"neutral",
|
|
"stale",
|
|
"startup_failure",
|
|
}
|
|
)
|
|
|
|
# The dedicated read-only token env var (preferred), falling back to the generic
|
|
# GITHUB_TOKEN (the idiom github_live uses). The token MUST be read-only — the
|
|
# fetcher only ever GETs; a write-scoped token here would be unnecessary blast
|
|
# radius (provisioning issues a read-only fine-grained PAT / read-only var).
|
|
CI_READ_TOKEN_ENV = "AGENT_TEAM_CI_READ_TOKEN"
|
|
_FALLBACK_TOKEN_ENV = "GITHUB_TOKEN"
|
|
|
|
GITHUB_API_ROOT = "https://api.github.com"
|
|
|
|
# Conservative default timeout for the single read-only GET. A hang must fail
|
|
# closed (-> None -> gate BLOCK), never wedge the verifier.
|
|
_DEFAULT_TIMEOUT_S = 15.0
|
|
|
|
|
|
def _resolve_read_token() -> str | None:
|
|
"""Resolve the read-only GitHub token at call time, or ``None``.
|
|
|
|
Prefers :data:`CI_READ_TOKEN_ENV`, falls back to ``GITHUB_TOKEN``. Returns
|
|
``None`` when neither is set so the fetcher fails closed (the caller maps a
|
|
missing token to a ``None`` result -> gate BLOCK), rather than raising.
|
|
"""
|
|
return os.environ.get(CI_READ_TOKEN_ENV) or os.environ.get(_FALLBACK_TOKEN_ENV)
|
|
|
|
|
|
def _build_session(token: str) -> Any:
|
|
"""Lazily construct a read-only ``requests.Session`` (deferred optional import).
|
|
|
|
Mirrors :func:`agent_team.transport.github_live._build_session` (deferred
|
|
``requests`` import, ``Authorization: Bearer`` + the API-version header). The
|
|
session is used for exactly one GET; no write verbs are ever issued.
|
|
|
|
Raises :class:`RuntimeError` only if ``requests`` is unavailable — the caller
|
|
catches it and fails closed to ``None`` so a pre-deploy environment without
|
|
the optional dependency simply BLOCKs (never a spurious pass).
|
|
"""
|
|
import requests # deferred: optional dependency (see module docstring)
|
|
|
|
session = requests.Session()
|
|
session.headers.update(
|
|
{
|
|
"Authorization": f"Bearer {token}",
|
|
"Accept": "application/vnd.github+json",
|
|
"X-GitHub-Api-Version": "2022-11-28",
|
|
}
|
|
)
|
|
return session
|
|
|
|
|
|
def fetch_ci_result(
|
|
state: Mapping[str, Any],
|
|
*,
|
|
owner: str,
|
|
repo: str,
|
|
client: Any = None,
|
|
api_root: str = GITHUB_API_ROOT,
|
|
timeout: float = _DEFAULT_TIMEOUT_S,
|
|
) -> dict[str, Any] | None:
|
|
"""Fetch the authenticated CI run conclusion as DATA, or ``None`` (fail-closed).
|
|
|
|
Reads ``state["run_id"]`` (required) and echoes ``state.get("diff_hash")``,
|
|
then GETs ``{api_root}/repos/{owner}/{repo}/actions/runs/{run_id}`` with the
|
|
read-only token and returns::
|
|
|
|
{"run_id": <str>, "conclusion": <str>, "diff_hash": <echoed-or-None>}
|
|
|
|
Returns ``None`` on ANY failure — no ``run_id`` in state, no resolvable
|
|
token, ``requests`` unavailable, a non-2xx status (404/401/403/...), a
|
|
malformed/absent JSON body, a missing ``conclusion``, or a network/timeout
|
|
error. ``None`` is the fail-closed signal: the pure-code gate treats it as
|
|
"no authenticated result" and BLOCKs, so a broken fetch parks the task. This
|
|
function NEVER derives the verdict (the gate owns pass/fail) and NEVER
|
|
writes.
|
|
|
|
``client`` injects a pre-built ``requests``-like session for tests (any
|
|
object with ``get(url, *, timeout) -> response`` exposing ``status_code``
|
|
and ``json()``). When omitted, a read-only session is built from the
|
|
resolved token.
|
|
"""
|
|
run_id = state.get("run_id")
|
|
if run_id is None or str(run_id) == "":
|
|
_LOG.warning("ci_fetcher: no run_id in state; failing closed to None")
|
|
return None
|
|
run_id = str(run_id)
|
|
|
|
# BLOCK-2: validate the run id BEFORE it is spliced into the API URL. A GitHub
|
|
# run id is a positive integer; anything else is a bug or a path-injection
|
|
# attempt -> fail closed. (See the TRUST SOURCE note: state["run_id"] is a
|
|
# trusted dispatcher input, but we validate it as defense-in-depth.)
|
|
if not _RUN_ID_RE.fullmatch(run_id):
|
|
_LOG.warning(
|
|
"ci_fetcher: run_id %r is not a valid integer run id; failing closed",
|
|
run_id,
|
|
)
|
|
return None
|
|
|
|
# BLOCK-3: validate owner/repo before URL construction so a crafted value
|
|
# cannot splice extra path segments / traversal into the GitHub API URL.
|
|
for _name, _value in (("owner", owner), ("repo", repo)):
|
|
if not _OWNER_REPO_RE.fullmatch(_value):
|
|
_LOG.warning(
|
|
"ci_fetcher: %s %r is not a valid GitHub path segment; failing closed",
|
|
_name,
|
|
_value,
|
|
)
|
|
return None
|
|
|
|
echoed_hash = state.get("diff_hash")
|
|
|
|
if client is None:
|
|
token = _resolve_read_token()
|
|
if not token:
|
|
_LOG.warning(
|
|
"ci_fetcher: no read-only token (%s/%s) set; failing closed to None",
|
|
CI_READ_TOKEN_ENV,
|
|
_FALLBACK_TOKEN_ENV,
|
|
)
|
|
return None
|
|
try:
|
|
client = _build_session(token)
|
|
except Exception: # noqa: BLE001 - any build failure fails closed
|
|
_LOG.warning("ci_fetcher: could not build HTTP client; failing closed")
|
|
return None
|
|
|
|
url = f"{api_root.rstrip('/')}/repos/{owner}/{repo}/actions/runs/{run_id}"
|
|
|
|
try:
|
|
response = client.get(url, timeout=timeout)
|
|
except Exception: # noqa: BLE001 - timeout / connection / any -> fail closed
|
|
_LOG.warning("ci_fetcher: GET %s failed (network/timeout); failing closed", url)
|
|
return None
|
|
|
|
status = _status_of(response)
|
|
if status is None or not (200 <= status < 300):
|
|
_LOG.warning("ci_fetcher: run GET returned status %r; failing closed", status)
|
|
return None
|
|
|
|
data = _json_of(response)
|
|
if not isinstance(data, dict):
|
|
_LOG.warning("ci_fetcher: run body was not a JSON object; failing closed")
|
|
return None
|
|
|
|
conclusion = data.get("conclusion")
|
|
# An in-progress run has conclusion=None; that is NOT an authenticated
|
|
# verdict, so fail closed (the gate would BLOCK on it anyway, but returning
|
|
# None keeps the "no result" contract clean and avoids echoing a non-verdict).
|
|
if conclusion is None or not isinstance(conclusion, str):
|
|
_LOG.info("ci_fetcher: run %s has no conclusion yet; failing closed", run_id)
|
|
return None
|
|
|
|
# FIX-1: allowlist the conclusion. GitHub returns a fixed set of conclusion
|
|
# strings; an unrecognised value is ambiguous (a new GitHub state, a typo, or
|
|
# an injected value) and must fail closed here rather than be laundered to the
|
|
# gate. (The gate also BLOCKs unknowns; this makes the boundary explicit.)
|
|
normalized_conclusion = conclusion.strip().lower()
|
|
if normalized_conclusion not in _ALLOWED_CONCLUSIONS:
|
|
_LOG.warning(
|
|
"ci_fetcher: run %s has unrecognised conclusion %r; failing closed",
|
|
run_id,
|
|
conclusion,
|
|
)
|
|
return None
|
|
|
|
# Bind the returned run_id to the run actually fetched (the API echoes id);
|
|
# fall back to the requested (already-validated) run_id. The gate
|
|
# independently re-checks this against expected_run_id, so this is provenance,
|
|
# not the trust decision.
|
|
#
|
|
# FIX-5: the API ``id`` must be an integer / integer-string before it is used
|
|
# as the result run id; we do NOT launder an arbitrary string. An int is
|
|
# accepted; a digit-only string is accepted; anything else falls back to the
|
|
# already-validated requested run_id rather than propagating an unvalidated
|
|
# value into the gate's run-id equality check.
|
|
fetched_id = data.get("id")
|
|
if isinstance(fetched_id, bool):
|
|
# bool is an int subclass; a JSON ``true``/``false`` id is not a run id.
|
|
result_run_id = run_id
|
|
elif isinstance(fetched_id, int):
|
|
result_run_id = str(fetched_id)
|
|
elif isinstance(fetched_id, str) and _RUN_ID_RE.fullmatch(fetched_id):
|
|
result_run_id = fetched_id
|
|
else:
|
|
result_run_id = run_id
|
|
|
|
return {
|
|
"run_id": result_run_id,
|
|
"conclusion": normalized_conclusion,
|
|
"diff_hash": echoed_hash,
|
|
}
|
|
|
|
|
|
def build_ci_result_fetcher(
|
|
*,
|
|
owner: str,
|
|
repo: str,
|
|
client: Any = None,
|
|
timeout: float = _DEFAULT_TIMEOUT_S,
|
|
) -> CiResultFetcher:
|
|
"""Build a :data:`CiResultFetcher` bound to ``owner``/``repo`` (GATED-LIVE seam).
|
|
|
|
Returns a single-argument ``state -> mapping | None`` callable shaped exactly
|
|
like the VERIFY node's injected ``ci_result_fetcher`` seam, closing over the
|
|
target ``owner``/``repo`` (and the optional injected ``client`` / ``timeout``).
|
|
Compose it with
|
|
:func:`agent_team.nodes.build_verify_subgraph.bind_ci_result_fetcher` (or the
|
|
opt-in :func:`agent_team.coordinator.gated_build_verify_wiring`) once the
|
|
§3.3.2 gate clears. It is read-only and fails closed (see
|
|
:func:`fetch_ci_result`); binding it does not enable any apply/verify
|
|
behaviour, it only gives the gate an authenticated conclusion to read.
|
|
|
|
FIX-3: the production path does NOT accept a caller-overridable ``api_root``.
|
|
The GitHub API host is hardcoded to :data:`GITHUB_API_ROOT` so a caller (and
|
|
anything that influences the coordinator's wiring) cannot redirect the
|
|
read-only GET at an attacker-controlled host. ``fetch_ci_result`` retains an
|
|
``api_root`` kwarg for unit tests only; it is unreachable from this production
|
|
builder (and therefore from :func:`agent_team.coordinator.gated_build_verify_wiring`).
|
|
"""
|
|
|
|
def fetcher(state: Mapping[str, Any]) -> dict[str, Any] | None:
|
|
return fetch_ci_result(
|
|
state,
|
|
owner=owner,
|
|
repo=repo,
|
|
client=client,
|
|
# api_root deliberately NOT forwarded: hardcoded GITHUB_API_ROOT in
|
|
# the production path (FIX-3). Only the test-only direct caller of
|
|
# fetch_ci_result may override it.
|
|
timeout=timeout,
|
|
)
|
|
|
|
return fetcher
|
|
|
|
|
|
def _status_of(response: Any) -> int | None:
|
|
"""Read the HTTP status from a ``requests``-like response, or ``None``.
|
|
|
|
Accepts ``status_code`` (``requests``) or ``status`` (a minimal fake).
|
|
Returns ``None`` if neither is present so the caller fails closed rather than
|
|
raising on an exotic object.
|
|
"""
|
|
for attr in ("status_code", "status"):
|
|
value = getattr(response, attr, None)
|
|
if value is not None:
|
|
try:
|
|
return int(value)
|
|
except (TypeError, ValueError):
|
|
return None
|
|
return None
|
|
|
|
|
|
def _json_of(response: Any) -> Any:
|
|
"""Parse the JSON body of a ``requests``-like response, or ``None`` on failure.
|
|
|
|
A malformed/absent body must fail closed (-> ``None`` -> the caller returns
|
|
``None`` -> gate BLOCK), never raise into the verifier node.
|
|
"""
|
|
parser = getattr(response, "json", None)
|
|
if not callable(parser):
|
|
return None
|
|
try:
|
|
return parser()
|
|
except Exception: # noqa: BLE001 - any JSON decode error fails closed
|
|
return None
|