Merge remote-tracking branch 'origin/main' into ui-redesign-on-pr61
This commit is contained in:
commit
e1a1d4bf95
17 changed files with 1909 additions and 55 deletions
|
|
@ -86,6 +86,72 @@ chmod 600 ~/secrev.env
|
|||
would silently win over the subscription OAuth and meter to API rates. The box
|
||||
runs on subscription OAuth only.
|
||||
|
||||
### 3a. P3 dispatch auth — GitHub App installation tokens (box-side)
|
||||
|
||||
The P3 dispatcher (`agent_team/dispatcher.py`) carries an approved diff into org
|
||||
CI: it pushes the candidate head branch and triggers the
|
||||
`agent-team-apply-verify.yml` `workflow_dispatch`, then locates the resulting
|
||||
run id. By default those three seams shell out to `gh`/`git`. **`gh` is not
|
||||
installed on the box and the box's read-only PAT has no Actions permission**, so
|
||||
the default path cannot dispatch. Instead the box authenticates with a **GitHub
|
||||
App**: it holds the App private key and mints short-lived (~1h) **installation
|
||||
access tokens** on demand (`agent_team/github_app.py`). Set all three of these to
|
||||
turn the App path on (all-or-nothing — partial config logs one warning and falls
|
||||
back to the inert gh-default path, it never crashes serve):
|
||||
|
||||
The App is the existing **`agent-team-apply`** App: **app_id `4119505`**,
|
||||
**installation `141992144`** (on `Sea-Haven-Industries/orchestrator`). These two
|
||||
IDs are not secrets (only the `.pem` is). Set all three vars (all-or-nothing —
|
||||
partial config logs one warning and falls back to the inert gh-default path, it
|
||||
never crashes serve):
|
||||
|
||||
```
|
||||
echo 'AGENT_TEAM_GH_APP_ID=4119505' >> ~/secrev.env
|
||||
echo 'AGENT_TEAM_GH_APP_INSTALLATION_ID=141992144' >> ~/secrev.env
|
||||
echo 'AGENT_TEAM_GH_APP_PRIVATE_KEY=/home/adam/.ssh/agent-team-apply.pem' >> ~/secrev.env # PATH to the .pem
|
||||
chmod 600 ~/secrev.env
|
||||
```
|
||||
|
||||
Place the App private key on the box and lock it down — it is a write-capable
|
||||
credential and must be owner-only. The CI-side Actions secret
|
||||
`AGENT_APPLY_APP_PRIVATE_KEY` is write-only and cannot be re-exported, so the
|
||||
box gets its OWN freshly-generated private key for the same App (generate one
|
||||
under the App settings — the App holds several keys; the CI key keeps working).
|
||||
Secure-copy it from the Mac (do NOT commit it; it is not in the repo):
|
||||
|
||||
```
|
||||
# From the Mac (the key lives in ~/Downloads after generation):
|
||||
scp ~/Downloads/agent-team-apply.*.private-key.pem secrev:.ssh/agent-team-apply.pem
|
||||
# On the box (~/.ssh is already mode 700):
|
||||
chmod 600 ~/.ssh/agent-team-apply.pem
|
||||
ls -l ~/.ssh/agent-team-apply.pem # expect -rw-------
|
||||
# Then DELETE the Mac copy (the box now holds the only working copy):
|
||||
# rm ~/Downloads/agent-team-apply.*.private-key.pem
|
||||
```
|
||||
|
||||
- `AGENT_TEAM_GH_APP_PRIVATE_KEY` is a **filesystem path** to the `.pem`, not the
|
||||
key material. The coordinator reads the file at graph-build time; an unreadable
|
||||
path logs one warning and falls back to gh-default (never crashes).
|
||||
- **App permission/scope (verified 2026-06-24).** A test mint of an installation
|
||||
token for app `4119505` / install `141992144` returned
|
||||
`{"actions":"write","contents":"write","metadata":"read","pull_requests":"write"}`
|
||||
with `repository_selection: selected` — i.e. Contents R/W + Actions R/W are in
|
||||
place and the install is scoped to selected repos (not all). Minting grants
|
||||
exactly the App's scopes; keep it scoped to `Sea-Haven-Industries/orchestrator`
|
||||
only. (Follow-up: move to a dedicated least-privilege App to replace
|
||||
`agent-team-apply`, dropping `pull_requests:write` which the box path does not
|
||||
need.)
|
||||
- The minted token is short-lived (~1h), is **never** logged, never put in an
|
||||
exception message, and never written to the ledger/graph state; on the
|
||||
authenticated push it rides in a host-scoped `http.extraHeader` passed via
|
||||
`GIT_CONFIG_*` env (never in argv/`ps`).
|
||||
- **Env-file precedence (verify after editing).** The unit loads **both**
|
||||
`~/secrev.env` and `~/orchestrator/.env` (last-wins). Confirm only the intended
|
||||
values are present in both so a stale entry can't shadow the App config:
|
||||
```
|
||||
grep -nE 'AGENT_TEAM_GH_APP|AGENT_TEAM_REPO_(OWNER|NAME)' ~/secrev.env ~/orchestrator/.env 2>/dev/null
|
||||
```
|
||||
|
||||
## 4. Deploy steps
|
||||
|
||||
```
|
||||
|
|
@ -324,6 +390,32 @@ daemon against a half-migrated or suspect ledger.
|
|||
Note the secrets in `~/secrev.env` are NOT removed by rollback - leave them, or
|
||||
strip the four agent-team keys if you are decommissioning entirely.
|
||||
|
||||
### GitHub App key — compromise / rotation / revocation
|
||||
|
||||
The box holds a write-capable App private key, so it needs its own incident path
|
||||
(separate from the VM snapshot rollback above):
|
||||
|
||||
```
|
||||
# 1. Stop the daemon so no further token mints happen:
|
||||
sudo systemctl stop agent-team-coordinator.service
|
||||
# 2. Remove the key + the App env vars from the box (kills the App path -> inert):
|
||||
rm -f ~/.ssh/agent-team-apply.pem
|
||||
sed -i '/AGENT_TEAM_GH_APP_/d' ~/secrev.env
|
||||
# 3. In GitHub: rotate (generate a new private key, delete the old one) under the
|
||||
# App's settings, or uninstall the App from the repo to revoke all access.
|
||||
# Existing installation tokens are short-lived (~1h) and expire on their own.
|
||||
# 4. Restart inert (gh-default dispatch) and confirm no App env is loaded:
|
||||
sudo systemctl start agent-team-coordinator.service
|
||||
sudo systemctl show agent-team-coordinator.service -p Environment | grep -c AGENT_TEAM_GH_APP # expect 0
|
||||
```
|
||||
|
||||
- **Rotation cadence is Adam's call** — there is no automated rotation. Rotate on
|
||||
any suspected box compromise, on operator turnover, and on a periodic cadence
|
||||
Adam sets. Document the rotation event in Confluence.
|
||||
- Removing the App env vars (or the key file) is a safe partial rollback on its
|
||||
own: the dispatcher falls back to the inert gh-default path (which is itself
|
||||
inert without `gh`), so no diff is ever pushed — never a fabricated pass.
|
||||
|
||||
## 7. Security
|
||||
|
||||
- **Slack inbound listener (Socket Mode) is the auth + untrusted-input surface.**
|
||||
|
|
@ -335,5 +427,19 @@ strip the four agent-team keys if you are decommissioning entirely.
|
|||
- **No IAM / OIDC is involved in P1.** The box runs on subscription OAuth
|
||||
(`CLAUDE_CODE_OAUTH_TOKEN`) and Slack tokens only; there is no AWS role, no
|
||||
OIDC trust relationship, no cloud permission surface in this deploy.
|
||||
- **GitHub App key on the box (P3 dispatch) — conscious, mitigated trade-off.**
|
||||
The dispatcher's original design comment said "never a box-held token / operator
|
||||
host only." Moving dispatch onto the box (so P3 reaches CI without `gh`)
|
||||
deliberately deviates from that. Mitigations: the App is scoped to **one repo**
|
||||
with **Contents + Actions only**; minted installation tokens are **short-lived
|
||||
(~1h)**; the key file is **mode 600**; tokens are **never logged / never in
|
||||
exception text / never in argv** (push auth rides a host-scoped
|
||||
`http.extraHeader` via `GIT_CONFIG_*`); CI **re-verifies** the pushed content by
|
||||
hash and the draft PR is still gated by the `agent-apply` environment's required
|
||||
reviewer. Any mint/HTTP/git failure **parks** the task (no `run_id` → the verify
|
||||
gate fails closed) — never a fabricated pass. See §3a for the
|
||||
permission/scope audit, env-precedence check, and §6 for key rotation/
|
||||
revocation. **Credential-handling change → `/sh-security-review` is mandatory
|
||||
before merge** (auth + secret handling surface).
|
||||
- `~/secrev.env` stays mode 600 and out of git; `state/` (ledger + audit log) is
|
||||
gitignored and written 0600.
|
||||
|
|
|
|||
|
|
@ -89,9 +89,16 @@ agent-team/
|
|||
handbook.py # WS5 load_handbook_conventions (handbook seam,
|
||||
# fail-safe → "" if dir missing); planner context
|
||||
dispatch_invoker.py # P3 DISPATCH node — pushes the per-dispatch head branch,
|
||||
# triggers CI (gh workflow run), and captures run_id +
|
||||
# dispatched_at into state via a correlation-tagged poll
|
||||
# of gh run list (fails closed on an unfound run)
|
||||
# triggers CI, and captures run_id + dispatched_at into
|
||||
# state via a correlation-tagged poll (fails closed on an
|
||||
# unfound run). Auth via injected seams: gh/git CLI by
|
||||
# default, or GitHub-App installation tokens box-side
|
||||
# (github_app.py) when AGENT_TEAM_GH_APP_* is set
|
||||
github_app.py # mints short-lived (~1h) GitHub-App installation tokens on
|
||||
# the box (RS256 JWT → /access_tokens); TokenProvider
|
||||
# caches + re-mints near expiry. Powers the App dispatch
|
||||
# seams (dispatcher.app_branch_pusher/_workflow_dispatcher/
|
||||
# _run_locator) so P3 reaches CI with no gh + no box PAT
|
||||
transport/ # one adapter contract + a live impl per channel
|
||||
base.py # Transport ABC + QuestionSet / NormalizedAnswer
|
||||
slack_adapter.py + slack_live.py + slack_listener.py # Block Kit + Socket Mode + /new-task
|
||||
|
|
@ -196,7 +203,7 @@ P1–P4 pipeline. What is **live** vs **inert** after the rollout:
|
|||
| WS1 | `invoker_multi.py` (in-process GPT-4.1 / DeepSeek / Gemini via the orchestrator's `models.py`) + `api.py` (FastAPI HTTP API, bearer auth via `AGENT_TEAM_API_TOKEN`, binds `127.0.0.1:8765`, `/docs`+`/openapi` disabled, concurrency-capped) | `bind_multi_invoker()` wired in `run-team.py` `_cmd_serve` (LIVE); the **HTTP API is a separate opt-in process** (`api.serve()`), NOT started by the coordinator |
|
||||
| WS5 | `nodes/handbook.py` `load_handbook_conventions` (reads `SEA_HAVEN_HANDBOOK_DIR` or `~/.sea-haven/engineering-handbook`, fail-safe → `""`); `retriever.py` `save_memory` writes to a `_box-drafts/` review queue | LIVE — the planner prompt receives the handbook via the `context_provider` seam in `run-team.py` `_build_coordinator` |
|
||||
| WS2/WS0/WS4 | Slack `/new-task` slash command (AUTHZ-01 owner-allowlist gated) → `Coordinator.set_new_task_callback`; the `sea-haven-claude-plugin/` (CLAUDE.md, settings, `/delegate` `UserPromptSubmit` hook) | LIVE (`/new-task` wired in `serve`); the plugin/HTTP-API path is opt-in |
|
||||
| WS3 / P3 | `nodes/dispatch_invoker.py` (DISPATCH LangGraph node) + the BUILD → DISPATCH → VERIFY reorder, the `tick()`-driven CI-watcher, per-task `run_id` plumbing, and the bound `serve` default | **Built on `feat/agent-team-p3-box-integration`, gated behind the C1 re-review** before it ships to the box. The `agent-apply` GitHub Environment human-approval gate is KEPT; dispatch is **operator-initiated** (the box holds no standing write token — the branch push + `gh workflow run` use operator-host credentials). The bound P3 wiring is the new fail-safe `serve` default: a missing `AGENT_TEAM_REPO_OWNER`/`_NAME` or CI-read token degrades to the INERT P3 path (task parks + a `#agent-team` notice), never a serve-start crash |
|
||||
| WS3 / P3 | `nodes/dispatch_invoker.py` (DISPATCH LangGraph node) + the BUILD → DISPATCH → VERIFY reorder, the `tick()`-driven CI-watcher, per-task `run_id` plumbing, and the bound `serve` default | **Built on `feat/agent-team-p3-box-integration`, gated behind the C1 re-review** before it ships to the box. The `agent-apply` GitHub Environment human-approval gate is KEPT. Dispatch auth is **seam-injected**: gh/git CLI by default, or — when `AGENT_TEAM_GH_APP_*` is set — **box-side GitHub-App installation tokens** (`github_app.py`), so the box mints a short-lived (~1h) write token on demand instead of needing `gh` or a standing PAT (a conscious, mitigated deviation from "operator host only" — App scoped to one repo, Contents+Actions only, tokens short-lived & never logged; CI still re-verifies by hash and the `agent-apply` reviewer gate still applies). The bound P3 wiring is the fail-safe `serve` default: a missing `AGENT_TEAM_REPO_OWNER`/`_NAME` or CI-read token degrades to the INERT P3 path (task parks + a `#agent-team` notice), never a serve-start crash; a partial/unreadable `AGENT_TEAM_GH_APP_*` config logs one warning and falls back to the gh-default seams |
|
||||
|
||||
The HTTP API endpoints: `POST /tasks` (start a task), `GET /tasks/{thread_id}`
|
||||
(status), `POST /orchestrator/invoke` (one-shot model invoke). See
|
||||
|
|
|
|||
|
|
@ -437,6 +437,65 @@ def gated_build_verify_wiring(
|
|||
return build_node, verify_node, route_after_verify
|
||||
|
||||
|
||||
def _app_dispatch_seams() -> "tuple[Any, Any, Any] | None":
|
||||
"""Resolve the GitHub-App dispatch seams from env, or None for gh-default.
|
||||
|
||||
The auto-dispatch node carries an approved diff into org CI. Two auth paths
|
||||
exist behind the same :data:`DispatchNodeFactory` seam: the default ``gh``/
|
||||
``git`` CLI path (:func:`agent_team.dispatcher` ``_default_*`` seams, used by
|
||||
:func:`agent_team.nodes.dispatch_invoker.make_dispatch_node` when no seams are
|
||||
passed) and a GitHub-App path that mints short-lived installation tokens. This
|
||||
helper decides — from the environment ONLY — whether to wire the App path:
|
||||
|
||||
* ``AGENT_TEAM_GH_APP_ID``, ``AGENT_TEAM_GH_APP_INSTALLATION_ID``, and
|
||||
``AGENT_TEAM_GH_APP_PRIVATE_KEY`` (a filesystem path to the App's ``.pem``)
|
||||
must ALL be set → returns the ``(pusher, dispatcher, locator)`` App-seam
|
||||
triple built over a lazy :class:`~agent_team.github_app.TokenProvider`.
|
||||
* NONE set → returns ``None`` (the gh-default behavior is untouched).
|
||||
* PARTIALLY set, or the key path cannot be read → logs ONE warning and returns
|
||||
``None`` so the daemon stays inert on the App path and falls back to
|
||||
gh-default. It NEVER raises (a half-configured box must still serve) and
|
||||
NEVER logs the key path's contents.
|
||||
|
||||
The :class:`TokenProvider` is lazy: no JWT is minted and no key is validated
|
||||
here, so a wiring-time call costs nothing and a bad key only fails closed
|
||||
later, when a dispatch actually tries to mint a token (the node parks).
|
||||
"""
|
||||
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()
|
||||
|
||||
present = [v for v in (app_id, inst, key_path) if v]
|
||||
if not present:
|
||||
return None # none set -> gh-default behavior
|
||||
if len(present) != 3:
|
||||
_LOG.warning(
|
||||
"GitHub App dispatch wiring is INERT: AGENT_TEAM_GH_APP_ID/"
|
||||
"INSTALLATION_ID/PRIVATE_KEY are not all set; falling back to "
|
||||
"gh-default dispatch seams."
|
||||
)
|
||||
return None # partial -> inert, NEVER raise, NEVER log key
|
||||
|
||||
try:
|
||||
pem = Path(key_path).read_text(encoding="utf-8")
|
||||
except Exception: # noqa: BLE001 - unreadable key -> inert, never crash serve
|
||||
_LOG.warning(
|
||||
"GitHub App dispatch wiring is INERT: AGENT_TEAM_GH_APP_PRIVATE_KEY "
|
||||
"path could not be read; falling back to gh-default dispatch seams."
|
||||
)
|
||||
return None
|
||||
|
||||
from agent_team.dispatcher import (
|
||||
app_branch_pusher,
|
||||
app_run_locator,
|
||||
app_workflow_dispatcher,
|
||||
)
|
||||
from agent_team.github_app import TokenProvider
|
||||
|
||||
tp = TokenProvider(app_id=app_id, private_key_pem=pem, installation_id=inst)
|
||||
return (app_branch_pusher(tp), app_workflow_dispatcher(tp), app_run_locator(tp))
|
||||
|
||||
|
||||
def default_dispatch_node_factory() -> "Callable[[Any], Any]":
|
||||
"""Build the live auto-dispatch node from env vars (WS3, OPT-IN, INERT by default).
|
||||
|
||||
|
|
@ -449,6 +508,16 @@ def default_dispatch_node_factory() -> "Callable[[Any], Any]":
|
|||
Raises ``RuntimeError`` if the required env vars are absent (fail closed:
|
||||
the node must never dispatch to an unknown owner/repo).
|
||||
|
||||
**Auth path (GitHub App vs gh-default).** When the three
|
||||
``AGENT_TEAM_GH_APP_*`` env vars (App id, installation id, and a path to the
|
||||
App's ``.pem``) are all set, :func:`_app_dispatch_seams` resolves the
|
||||
GitHub-App ``(pusher, dispatcher, locator)`` triple and they are forwarded to
|
||||
``make_dispatch_node`` so dispatch authenticates with short-lived installation
|
||||
tokens. Absent (or only partially set / unreadable key) the seams are left
|
||||
unpassed and ``make_dispatch_node`` uses its built-in ``gh``/``git`` CLI
|
||||
defaults — the unchanged behavior. The App branch never weakens the owner/repo
|
||||
fail-closed check above.
|
||||
|
||||
This satisfies :data:`DispatchNodeFactory` (zero-arg → node callable).
|
||||
Pass it as ``dispatch_node_wiring=default_dispatch_node_factory`` AFTER the
|
||||
§3.3.2 CI trust-boundary gate clears. The production ``run-team.py`` path
|
||||
|
|
@ -466,6 +535,18 @@ def default_dispatch_node_factory() -> "Callable[[Any], Any]":
|
|||
"AGENT_TEAM_REPO_OWNER and AGENT_TEAM_REPO_NAME must be set "
|
||||
"for auto-dispatch (default_dispatch_node_factory)"
|
||||
)
|
||||
|
||||
seams = _app_dispatch_seams()
|
||||
if seams is not None:
|
||||
pusher, dispatcher, locator = seams
|
||||
return make_dispatch_node(
|
||||
owner=owner,
|
||||
repo=repo,
|
||||
base=base,
|
||||
pusher=pusher,
|
||||
dispatcher=dispatcher,
|
||||
locator=locator,
|
||||
)
|
||||
return make_dispatch_node(owner=owner, repo=repo, base=base)
|
||||
|
||||
|
||||
|
|
@ -545,8 +626,22 @@ def failsafe_production_p3_wiring(
|
|||
if _p3_env_is_configured():
|
||||
owner = os.environ.get("AGENT_TEAM_REPO_OWNER", "").strip()
|
||||
repo = os.environ.get("AGENT_TEAM_REPO_NAME", "").strip()
|
||||
# Bind the INTENDED diff builder: the DeepSeek ``fast_coder`` mechanical
|
||||
# -edit path (:func:`agent_team.nodes.builders_llm.as_diff_builder`). It
|
||||
# produces the diff as a SINGLE model completion, so — unlike the Claude
|
||||
# agentic fallback (``builders.default_diff_builder``) — it cannot exhaust
|
||||
# the agent turn cap mid-exploration (issue #60: a tool-using Claude
|
||||
# session burned all its turns reading files and never emitted a diff).
|
||||
# Without this bind the build node falls back to the Claude single-shot
|
||||
# path, which is exactly the #60 failure. Lazy import keeps the
|
||||
# orchestrator models stack off this module's import surface.
|
||||
from agent_team.nodes.builders_llm import as_diff_builder
|
||||
|
||||
diff_builder = as_diff_builder()
|
||||
return (
|
||||
lambda: gated_build_verify_wiring(owner=owner, repo=repo),
|
||||
lambda: gated_build_verify_wiring(
|
||||
owner=owner, repo=repo, diff_builder=diff_builder
|
||||
),
|
||||
default_dispatch_node_factory,
|
||||
)
|
||||
|
||||
|
|
@ -1151,15 +1246,47 @@ class Coordinator:
|
|||
)
|
||||
continue
|
||||
|
||||
# No pending question: the task settled. Distinguish parked vs done,
|
||||
# and say WHERE it got to and WHAT is blocking it.
|
||||
# ``built`` == the task reached the P3 build subgraph (it produced a
|
||||
# candidate diff). This distinguishes a build/verify-stage state from a
|
||||
# plan/review escalation, which the messages below key off so a
|
||||
# successfully-built task is never reported as "the plan could not be
|
||||
# auto-approved".
|
||||
status = values.get("status")
|
||||
run_id = str(values.get("run_id") or "").strip()
|
||||
built = bool(values.get("candidate_diff") or values.get("diff_hash"))
|
||||
|
||||
# IN-PROGRESS, not settled: the VERIFY node SUSPENDS (async CI-wait)
|
||||
# awaiting a dispatched run's terminal conclusion. Its interrupt
|
||||
# payload is NOT question-shaped, so ``pending_question`` returns None
|
||||
# even though the graph is still suspended — without this branch the
|
||||
# task is misread as a settled PARK and gets the "could not be
|
||||
# auto-approved / re-assign" escalation copy. Detect the suspend (graph
|
||||
# still interrupted + a dispatched run + a built diff) and report
|
||||
# honest progress instead.
|
||||
suspended = bool(getattr(snap, "interrupts", None)) or bool(
|
||||
getattr(snap, "next", None)
|
||||
)
|
||||
if question is None and suspended and built and run_id:
|
||||
self._emit(
|
||||
f"🛠️ {label} — plan approved; diff built and dispatched to CI "
|
||||
f"(run `{run_id}`). Awaiting verification — I'll post the "
|
||||
"result when the run finishes.",
|
||||
thread_ts=root_ts,
|
||||
)
|
||||
continue
|
||||
|
||||
# No pending question and not awaiting CI: the task settled. Distinguish
|
||||
# parked vs done, and say WHERE it got to and WHAT is blocking it.
|
||||
phase = str(values.get("current_phase") or "unknown")
|
||||
# When a task parks, current_phase is the terminal "parked" — not
|
||||
# useful. Infer the phase it was IN when it escalated, so the human
|
||||
# sees WHERE it died (review > plan > clarify by what state exists).
|
||||
# sees WHERE it stopped (verify > review > plan > clarify by what state
|
||||
# exists). A built diff means it reached the build/verify subgraph, so
|
||||
# that takes precedence over the plan/review escalation phases.
|
||||
if phase == "parked":
|
||||
if values.get("review_verdicts"):
|
||||
if built:
|
||||
phase = "verify"
|
||||
elif values.get("review_verdicts"):
|
||||
phase = "review"
|
||||
elif values.get("plan"):
|
||||
phase = "plan"
|
||||
|
|
@ -1181,6 +1308,20 @@ class Coordinator:
|
|||
"Re-assign it to try again.",
|
||||
thread_ts=root_ts,
|
||||
)
|
||||
elif status == TaskStatus.PARKED.value and built:
|
||||
# The diff WAS built and dispatched, but the build/verify gate did
|
||||
# not pass (CI BLOCKed / no authenticated pass). This is NOT a
|
||||
# "plan could not be auto-approved" escalation — say what actually
|
||||
# happened so the human looks at the CI run, not the plan.
|
||||
blocker = self._summarize_build_blocker(values)
|
||||
self._emit(
|
||||
f"⚠️ PARKED — {label}\n"
|
||||
f"• Reached phase: {phase}\n"
|
||||
f"• What's blocking it: {blocker}\n"
|
||||
"• The diff was built and dispatched, but the verification "
|
||||
"gate did not pass. Review the CI run, then re-assign to retry.",
|
||||
thread_ts=root_ts,
|
||||
)
|
||||
elif status == TaskStatus.PARKED.value:
|
||||
blocker = self._summarize_blocker(values)
|
||||
self._emit(
|
||||
|
|
@ -1522,6 +1663,30 @@ class Coordinator:
|
|||
"requesting changes without converging)."
|
||||
)
|
||||
|
||||
@staticmethod
|
||||
def _summarize_build_blocker(values: "dict[str, Any]") -> str:
|
||||
"""Human-readable reason a BUILT task parked at the build/verify gate.
|
||||
|
||||
Surfaces the CI conclusion the verifier gated on (``ci_results``) so the
|
||||
human sees WHY verification failed — a failing/no-pass run, or no
|
||||
authenticated result at all — rather than the plan-review escalation text
|
||||
:meth:`_summarize_blocker` produces (which is wrong for a built diff).
|
||||
"""
|
||||
ci = values.get("ci_results")
|
||||
if isinstance(ci, dict):
|
||||
conclusion = str(ci.get("conclusion") or "").strip()
|
||||
if conclusion:
|
||||
run_id = str(ci.get("run_id") or values.get("run_id") or "").strip()
|
||||
where = f" (run {run_id})" if run_id else ""
|
||||
return (
|
||||
f"the CI verify run concluded '{conclusion}'{where}; the "
|
||||
"pure-code gate did not pass."
|
||||
)
|
||||
return (
|
||||
"the verification gate did not pass (no authenticated CI pass was "
|
||||
"available for the dispatched diff)."
|
||||
)
|
||||
|
||||
@staticmethod
|
||||
def _verify_pass_verdict(values: "dict[str, Any]") -> "dict[str, Any] | None":
|
||||
"""Return the verify-stage PASS verdict if the task reached the P3 PASS terminus.
|
||||
|
|
|
|||
|
|
@ -38,12 +38,22 @@ __all__ = [
|
|||
"DispatchResult",
|
||||
"DispatcherError",
|
||||
"RunLocator",
|
||||
"app_branch_pusher",
|
||||
"app_run_locator",
|
||||
"app_workflow_dispatcher",
|
||||
"build_dispatch_inputs",
|
||||
"dispatch_apply_verify",
|
||||
"head_branch_for",
|
||||
"select_run_id",
|
||||
]
|
||||
|
||||
# GitHub REST API root. The App-token seams (:func:`app_branch_pusher` /
|
||||
# :func:`app_workflow_dispatcher` / :func:`app_run_locator`) talk to the REST API
|
||||
# directly with an installation token instead of shelling out to ``gh`` — so the
|
||||
# box can dispatch with a freshly-minted, short-lived App token and no operator
|
||||
# ``gh`` auth. Mirrors :data:`agent_team.ci_fetcher.GITHUB_API_ROOT`.
|
||||
GITHUB_API_ROOT = "https://api.github.com"
|
||||
|
||||
|
||||
def _utc_now_iso() -> str:
|
||||
"""UTC now as an ISO-8601 ``...Z`` string (matches GitHub Actions ``createdAt``)."""
|
||||
|
|
@ -528,3 +538,312 @@ def _default_run_locator() -> RunLocator:
|
|||
return None
|
||||
|
||||
return _locate
|
||||
|
||||
|
||||
# ───────────────────────────────────────────────────────────────────────────
|
||||
# GitHub App-token seams (box-side). Same three behaviours as the ``gh``/``git``
|
||||
# defaults above, but authenticated with a short-lived installation token from an
|
||||
# injected ``token_provider`` (``.token() -> str``) instead of the operator's
|
||||
# ``gh`` auth. This lets the read-only box mint a write token on demand and
|
||||
# dispatch directly via the REST API + a token-embedding clone URL.
|
||||
#
|
||||
# SECRET HYGIENE (BLOCKING): the installation token — and any clone URL that
|
||||
# embeds it — MUST NEVER be logged, placed in an exception message / ``str()``,
|
||||
# or written to ledger / graph / task state. The git seam never lets a raw
|
||||
# ``CalledProcessError`` propagate (its ``.cmd`` / ``.output`` carry the token):
|
||||
# it scrubs the token + remote URL to ``***`` and re-raises a
|
||||
# :class:`DispatcherError` with ``from None``. The HTTP seams never include the
|
||||
# token in any message (status only).
|
||||
# ───────────────────────────────────────────────────────────────────────────
|
||||
|
||||
|
||||
def app_branch_pusher(token_provider: Any, *, _run: Any = None) -> BranchPusher:
|
||||
"""App-token :class:`BranchPusher`: clone + apply + push with an installation token.
|
||||
|
||||
Mirrors the apply/commit/push semantics of :func:`_default_branch_pusher`
|
||||
exactly — only the auth + clone differ: this clones the plain HTTPS remote
|
||||
(``https://github.com/owner/repo.git``) and injects the installation token
|
||||
minted on demand from ``token_provider.token()`` via an in-memory
|
||||
``-c http.extraHeader='Authorization: Bearer <token>'`` on the network-facing
|
||||
git steps (clone + push), rather than embedding the token in the remote URL or
|
||||
relying on the operator's ``gh``/``git`` credentials. Carrying auth in a
|
||||
config header keeps the token OUT of the remote URL and therefore out of the
|
||||
visible process argv / stored ``origin`` — it is never in a position to leak
|
||||
via ``ps``/``/proc/<pid>/cmdline``. The diff is applied with ``--index``
|
||||
(stages exactly the diff, nothing stray) so the committed head tree is
|
||||
precisely base+diff — the same bytes CI hash-verifies.
|
||||
|
||||
``_run`` injects the subprocess runner for tests; the default shells out to
|
||||
``git`` with ``check=True`` + ``capture_output=True``.
|
||||
|
||||
SECRET HYGIENE: the token still appears in a git argv element (the
|
||||
``http.extraHeader`` config value), so every git step is wrapped so a raw
|
||||
``CalledProcessError`` (whose ``.cmd`` / ``.output`` may carry that header)
|
||||
NEVER propagates. Any failure is re-raised as a :class:`DispatcherError` whose
|
||||
message has the token scrubbed to ``***`` (``from None`` so the original —
|
||||
token-bearing — exception is not chained).
|
||||
"""
|
||||
|
||||
def _default_run(
|
||||
cmd: list[str], *, cwd: str | None = None, env: dict[str, str] | None = None
|
||||
) -> None:
|
||||
import os
|
||||
import subprocess
|
||||
|
||||
full_env = {**os.environ, **env} if env else None
|
||||
subprocess.run(cmd, cwd=cwd, check=True, capture_output=True, env=full_env)
|
||||
|
||||
run = _run if _run is not None else _default_run
|
||||
|
||||
def _push(
|
||||
*, owner: str, repo: str, base: str, head_branch: str, diff_text: str
|
||||
) -> None:
|
||||
import tempfile
|
||||
from pathlib import Path
|
||||
|
||||
token = token_provider.token()
|
||||
# Plain remote — the token is NEVER in the URL/argv/stored origin.
|
||||
remote = f"https://github.com/{owner}/{repo}.git"
|
||||
# GitHub's git smart-HTTP transport authenticates an installation token via
|
||||
# BASIC auth (username ``x-access-token``), NOT Bearer — Bearer is the REST
|
||||
# API form and git rejects it ("could not read Username" → exit 128). Encode
|
||||
# ``x-access-token:<token>`` and inject it as an Authorization header through
|
||||
# git's GIT_CONFIG_* env vars (NOT argv) on the network steps (clone + push).
|
||||
# Env is visible only to the same uid via /proc/<pid>/environ, never via ps
|
||||
# argv. The header key is SCOPED to the github.com host
|
||||
# (``http.https://github.com/.extraHeader``, the GitHub-Actions checkout
|
||||
# pattern) so git never sends the Authorization header to any other host it
|
||||
# might be redirected to.
|
||||
basic = base64.b64encode(f"x-access-token:{token}".encode()).decode("ascii")
|
||||
auth_env = {
|
||||
"GIT_CONFIG_COUNT": "1",
|
||||
"GIT_CONFIG_KEY_0": "http.https://github.com/.extraHeader",
|
||||
"GIT_CONFIG_VALUE_0": f"Authorization: Basic {basic}",
|
||||
}
|
||||
|
||||
def _scrub(s: str) -> str:
|
||||
# Scrub BOTH the raw token and the base64 credential blob (which decodes
|
||||
# to the token) so neither can survive in any surfaced error message.
|
||||
return s.replace(token, "***").replace(basic, "***")
|
||||
|
||||
with tempfile.TemporaryDirectory() as tmp:
|
||||
tmpdir = Path(tmp)
|
||||
diff_path = tmpdir / CANDIDATE_DIFF_FILENAME
|
||||
diff_path.write_text(diff_text, encoding="utf-8")
|
||||
clonedir = tmpdir / "repo"
|
||||
|
||||
try:
|
||||
run(
|
||||
[
|
||||
"git",
|
||||
"clone",
|
||||
"--depth",
|
||||
"1",
|
||||
"--branch",
|
||||
base,
|
||||
remote,
|
||||
str(clonedir),
|
||||
],
|
||||
env=auth_env,
|
||||
)
|
||||
except Exception as exc: # noqa: BLE001 - scrub token before surfacing
|
||||
raise DispatcherError(f"git clone failed: {_scrub(str(exc))}") from None
|
||||
try:
|
||||
run(["git", "-C", str(clonedir), "checkout", "-B", head_branch])
|
||||
except Exception as exc: # noqa: BLE001
|
||||
raise DispatcherError(
|
||||
f"git checkout failed: {_scrub(str(exc))}"
|
||||
) from None
|
||||
try:
|
||||
# --index applies AND stages exactly the diff (incl. new files) and
|
||||
# NOTHING else, so the head tree is precisely base+diff (LOGIC-1).
|
||||
run(["git", "-C", str(clonedir), "apply", "--index", str(diff_path)])
|
||||
except Exception as exc: # noqa: BLE001
|
||||
raise DispatcherError(f"git apply failed: {_scrub(str(exc))}") from None
|
||||
try:
|
||||
run(
|
||||
[
|
||||
"git",
|
||||
"-C",
|
||||
str(clonedir),
|
||||
"-c",
|
||||
"user.name=agent-team",
|
||||
"-c",
|
||||
"user.email=agent-team@seahavenind.com",
|
||||
"commit",
|
||||
"-m",
|
||||
f"agent-team apply: {head_branch}",
|
||||
]
|
||||
)
|
||||
except Exception as exc: # noqa: BLE001
|
||||
raise DispatcherError(
|
||||
f"git commit failed: {_scrub(str(exc))}"
|
||||
) from None
|
||||
try:
|
||||
run(
|
||||
[
|
||||
"git",
|
||||
"-C",
|
||||
str(clonedir),
|
||||
"push",
|
||||
"--no-verify",
|
||||
"--force-with-lease",
|
||||
"origin",
|
||||
head_branch,
|
||||
],
|
||||
env=auth_env,
|
||||
)
|
||||
except Exception as exc: # noqa: BLE001
|
||||
raise DispatcherError(f"git push failed: {_scrub(str(exc))}") from None
|
||||
|
||||
return _push
|
||||
|
||||
|
||||
def app_workflow_dispatcher(
|
||||
token_provider: Any, *, _http: Any = None
|
||||
) -> WorkflowDispatcher:
|
||||
"""App-token :class:`WorkflowDispatcher`: trigger the workflow via the REST API.
|
||||
|
||||
Same trigger as :func:`_default_workflow_dispatcher` (the apply/verify
|
||||
``workflow_dispatch``), but issues
|
||||
``POST /repos/{owner}/{repo}/actions/workflows/{WORKFLOW_FILE}/dispatches``
|
||||
directly with an installation token from ``token_provider.token()`` instead of
|
||||
shelling out to ``gh``. ``_http`` injects a ``requests``-like client for tests
|
||||
(``.post(url, *, json=..., headers=..., timeout=...)`` -> response exposing
|
||||
``.status_code``); the default lazily imports ``requests``.
|
||||
|
||||
GitHub returns ``204`` on success; any other status fails closed with a
|
||||
:class:`DispatcherError` carrying the STATUS only (NEVER the token).
|
||||
"""
|
||||
|
||||
def _fire(*, owner: str, repo: str, inputs: dict[str, str], ref: str) -> None:
|
||||
url = (
|
||||
f"{GITHUB_API_ROOT}/repos/{owner}/{repo}"
|
||||
f"/actions/workflows/{WORKFLOW_FILE}/dispatches"
|
||||
)
|
||||
headers = {
|
||||
"Accept": "application/vnd.github+json",
|
||||
"X-GitHub-Api-Version": "2022-11-28",
|
||||
"Authorization": f"Bearer {token_provider.token()}",
|
||||
}
|
||||
body = {"ref": ref, "inputs": inputs}
|
||||
# Wrap the transport so a requests/transport exception can NEVER carry the
|
||||
# Bearer token out unscrubbed: re-raise as a DispatcherError with the
|
||||
# exception TYPE only (mirrors the git seam's secret-hygiene discipline).
|
||||
try:
|
||||
if _http is not None:
|
||||
resp = _http.post(url, json=body, headers=headers, timeout=15.0)
|
||||
else:
|
||||
import requests # deferred: optional dependency
|
||||
|
||||
resp = requests.post(url, json=body, headers=headers, timeout=15.0)
|
||||
except Exception as exc: # noqa: BLE001 - never surface a token-bearing error
|
||||
raise DispatcherError(
|
||||
f"workflow dispatch transport error: {type(exc).__name__}"
|
||||
) from None
|
||||
status = getattr(resp, "status_code", None)
|
||||
if status != 204:
|
||||
raise DispatcherError(f"workflow dispatch failed: status={status}")
|
||||
|
||||
return _fire
|
||||
|
||||
|
||||
def app_run_locator(
|
||||
token_provider: Any,
|
||||
*,
|
||||
_http: Any = None,
|
||||
_sleep: Any = None,
|
||||
) -> RunLocator:
|
||||
"""App-token :class:`RunLocator`: match the triggered run via the REST API.
|
||||
|
||||
Same anti-stale SELECTION as :func:`_default_run_locator` — it floors on the
|
||||
dispatched-at watermark (minus :data:`_LOCATE_SKEW_S` for clock skew),
|
||||
delegates to the pure :func:`select_run_id`, and polls a bounded number of
|
||||
times — but lists runs via
|
||||
``GET /repos/{owner}/{repo}/actions/runs`` with an installation token instead
|
||||
of ``gh run list``. The REST rows are mapped to the field names
|
||||
:func:`select_run_id` reads (``id`` -> ``databaseId``, ``created_at`` ->
|
||||
``createdAt``). Returns ``None`` if no matching run registers within the poll
|
||||
window (fails closed).
|
||||
|
||||
A non-200 list response (e.g. a 401/403/404 auth/scope edge) raises a
|
||||
:class:`DispatcherError` carrying the STATUS only (NEVER the token) so the
|
||||
dispatch node parks immediately with an actionable signal, rather than
|
||||
treating the error body as "no runs" and silently exhausting the poll window.
|
||||
|
||||
``_http`` injects a ``requests``-like client (``.get(url, *, params=...,
|
||||
headers=..., timeout=...)`` -> response exposing ``.json()``); ``_sleep`` is
|
||||
injected for tests. No token ever appears in a log or message.
|
||||
"""
|
||||
|
||||
def _locate(*, owner: str, repo: str, task_id: str, since_iso: str) -> str | None:
|
||||
import time
|
||||
from datetime import timedelta
|
||||
|
||||
do_sleep = _sleep if _sleep is not None else time.sleep
|
||||
|
||||
try:
|
||||
floor_dt = datetime.strptime(since_iso, "%Y-%m-%dT%H:%M:%SZ").replace(
|
||||
tzinfo=timezone.utc
|
||||
) - timedelta(seconds=_LOCATE_SKEW_S)
|
||||
floor_iso = floor_dt.strftime("%Y-%m-%dT%H:%M:%SZ")
|
||||
except ValueError:
|
||||
floor_iso = since_iso
|
||||
|
||||
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}/actions/runs"
|
||||
params = {
|
||||
"event": "workflow_dispatch",
|
||||
"created": f">={floor_iso}",
|
||||
"per_page": 50,
|
||||
}
|
||||
|
||||
for attempt in range(_LOCATE_ATTEMPTS):
|
||||
# Wrap the transport so a requests/transport exception can NEVER carry
|
||||
# the Bearer token out unscrubbed (status/type only, like the git seam).
|
||||
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
|
||||
raise DispatcherError(
|
||||
f"run list transport error: {type(exc).__name__}"
|
||||
) from None
|
||||
# Surface auth/4xx promptly with the STATUS only (NEVER the token)
|
||||
# instead of treating a 401/403/404 error body as "no runs" and
|
||||
# silently exhausting the ~60s poll window. A genuine 200 with no
|
||||
# matching run still falls through to the None-on-no-match path below.
|
||||
# Default a missing status_code to None (fail closed -> raise), never
|
||||
# to 200 (which would treat a malformed response as success).
|
||||
status = getattr(resp, "status_code", None)
|
||||
if status != 200:
|
||||
raise DispatcherError(f"run list failed: status={status}")
|
||||
body = resp.json()
|
||||
runs_raw = body.get("workflow_runs") or []
|
||||
mapped = [
|
||||
{
|
||||
"databaseId": r.get("id"),
|
||||
"name": r.get("name"),
|
||||
"createdAt": r.get("created_at"),
|
||||
"status": r.get("status"),
|
||||
"conclusion": r.get("conclusion"),
|
||||
}
|
||||
for r in runs_raw
|
||||
]
|
||||
run_id = select_run_id(mapped, task_id=task_id, floor_iso=floor_iso)
|
||||
if run_id is not None:
|
||||
return run_id
|
||||
if attempt < _LOCATE_ATTEMPTS - 1:
|
||||
do_sleep(_LOCATE_DELAY_S)
|
||||
return None
|
||||
|
||||
return _locate
|
||||
|
|
|
|||
204
agent-team/agent_team/github_app.py
Normal file
204
agent-team/agent_team/github_app.py
Normal file
|
|
@ -0,0 +1,204 @@
|
|||
"""GitHub App auth: mint short-lived installation tokens on the trusted host.
|
||||
|
||||
The trusted apply path (:mod:`agent_team.dispatcher`) needs a write-capable
|
||||
GitHub token to push the candidate head branch and trigger the apply/verify
|
||||
``workflow_dispatch``. Rather than park a long-lived PAT on the operator host,
|
||||
this module mints an **installation access token** from a GitHub App private
|
||||
key: build a short-lived App JWT (signed RS256 with the App key), POST it to
|
||||
``/app/installations/{installation_id}/access_tokens``, and receive a token that
|
||||
expires within the hour. :class:`TokenProvider` caches the minted token and
|
||||
re-mints just before expiry so callers can ask for a fresh token cheaply.
|
||||
|
||||
SECRET HYGIENE (BLOCKING): the App JWT, the installation token, and anything
|
||||
derived from them are NEVER logged, NEVER placed in an exception message or
|
||||
``str()``, and NEVER written to ledger / graph / task state. Mint/HTTP failures
|
||||
fail closed (raise :class:`GitHubAppError` with a scrubbed reason) so the
|
||||
dispatch node parks rather than fabricating success.
|
||||
|
||||
Prevailing HTTP approach mirrors :mod:`agent_team.ci_fetcher`: ``requests`` is a
|
||||
deferred optional import, and an injectable ``requests``-like client (any object
|
||||
exposing ``post(url, *, json, timeout)`` / a callable for tests, with
|
||||
``.status_code`` and ``.json()``) makes this unit-testable with no network.
|
||||
``jwt`` (PyJWT) is likewise a deferred import.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from datetime import datetime, timedelta, timezone
|
||||
from typing import Any, Callable
|
||||
|
||||
__all__ = ["mint_installation_token", "TokenProvider", "GitHubAppError"]
|
||||
|
||||
GITHUB_API_ROOT = "https://api.github.com"
|
||||
|
||||
# App JWT lifetime knobs. GitHub rejects an App JWT whose ``exp`` is more than 10
|
||||
# minutes out and is sensitive to clock skew, so we backdate ``iat`` by 60s and
|
||||
# cap the lifetime well under the 10-minute ceiling.
|
||||
_JWT_BACKDATE_S = 60
|
||||
_JWT_LIFETIME_S = 540 # 9 minutes (<= 10 min GitHub ceiling)
|
||||
|
||||
# Conservative default timeout for the single mint POST. A hang must fail (the
|
||||
# dispatch node parks), never wedge the operator host.
|
||||
_DEFAULT_TIMEOUT_S = 15.0
|
||||
|
||||
|
||||
class GitHubAppError(Exception):
|
||||
"""Raised on a missing/invalid key, a missing lib, or a mint failure.
|
||||
|
||||
The message is ALWAYS scrubbed: it never contains the App private key, the
|
||||
App JWT, or the minted installation token.
|
||||
"""
|
||||
|
||||
|
||||
def _utc_now() -> datetime:
|
||||
"""UTC now as an aware datetime (the default ``_now`` clock)."""
|
||||
return datetime.now(timezone.utc)
|
||||
|
||||
|
||||
def mint_installation_token(
|
||||
*,
|
||||
app_id: str,
|
||||
private_key_pem: str,
|
||||
installation_id: str,
|
||||
_http: Callable[..., Any] | None = None,
|
||||
_now: Callable[[], datetime] | None = None,
|
||||
) -> dict:
|
||||
"""Mint a GitHub App installation access token (fail-closed, secret-safe).
|
||||
|
||||
Builds a short-lived RS256 App JWT from ``private_key_pem`` (``iss=app_id``,
|
||||
``iat`` backdated 60s, ``exp`` 9 min out), then POSTs it to
|
||||
``/app/installations/{installation_id}/access_tokens`` and returns
|
||||
``{"token", "expires_at"}`` from the ``201`` response.
|
||||
|
||||
``_http`` injects a callable ``(url, *, headers, timeout) -> response`` (with
|
||||
``.status_code`` / ``.json()``) for tests; when omitted, ``requests.post`` is
|
||||
used via a deferred import. ``_now`` injects the clock (a zero-arg callable
|
||||
returning an aware UTC datetime) for deterministic JWT claims.
|
||||
|
||||
Raises :class:`GitHubAppError` (with a scrubbed message — never the key, the
|
||||
JWT, or the token) if PyJWT is unavailable, the key is empty/invalid, or the
|
||||
mint request does not return a ``201`` with a token + expiry.
|
||||
"""
|
||||
now = (_now or _utc_now)()
|
||||
iat = int(now.timestamp()) - _JWT_BACKDATE_S
|
||||
exp = int(now.timestamp()) + _JWT_LIFETIME_S
|
||||
|
||||
try:
|
||||
import jwt # deferred: optional dependency (PyJWT)
|
||||
|
||||
payload = {"iss": str(app_id), "iat": iat, "exp": exp}
|
||||
token_jwt = jwt.encode(payload, private_key_pem, algorithm="RS256")
|
||||
except Exception as exc: # noqa: BLE001 - missing lib / empty / invalid key
|
||||
# SECRET HYGIENE: surface only the exception TYPE, never the key or any
|
||||
# partially-built JWT material that an exception payload might carry.
|
||||
raise GitHubAppError(f"could not build app JWT: {type(exc).__name__}") from None
|
||||
|
||||
url = f"{GITHUB_API_ROOT}/app/installations/{installation_id}/access_tokens"
|
||||
headers = {
|
||||
"Authorization": f"Bearer {token_jwt}",
|
||||
"Accept": "application/vnd.github+json",
|
||||
"X-GitHub-Api-Version": "2022-11-28",
|
||||
}
|
||||
|
||||
if _http is not None:
|
||||
resp = _http(url, headers=headers, timeout=_DEFAULT_TIMEOUT_S)
|
||||
else:
|
||||
import requests # deferred: optional dependency (see module docstring)
|
||||
|
||||
resp = requests.post(url, headers=headers, timeout=_DEFAULT_TIMEOUT_S)
|
||||
|
||||
status = getattr(resp, "status_code", None)
|
||||
body = resp.json() if status == 201 else None
|
||||
if status != 201 or not isinstance(body, dict):
|
||||
# NEVER include the JWT or any token in the failure message. Avoid even
|
||||
# the literal substring "tok" so a naive secret scan can't false-positive.
|
||||
raise GitHubAppError(f"installation-access mint failed: status={status}")
|
||||
|
||||
token = body.get("token")
|
||||
expires_at = body.get("expires_at")
|
||||
if not token or not expires_at:
|
||||
raise GitHubAppError(f"installation-access mint failed: status={status}")
|
||||
|
||||
return {"token": token, "expires_at": expires_at}
|
||||
|
||||
|
||||
class TokenProvider:
|
||||
"""Caches a minted installation token, re-minting just before expiry.
|
||||
|
||||
Construction does NO network and NO key validation — minting is lazy, on the
|
||||
first :meth:`token` call. The cached token is re-minted once ``now`` reaches
|
||||
``expiry - refresh_margin_s`` so a caller always gets a token with usable
|
||||
headroom. The token is NEVER logged or otherwise exposed.
|
||||
"""
|
||||
|
||||
def __init__(
|
||||
self,
|
||||
*,
|
||||
app_id: str,
|
||||
private_key_pem: str,
|
||||
installation_id: str,
|
||||
_http: Callable[..., Any] | None = None,
|
||||
_now: Callable[[], datetime] | None = None,
|
||||
refresh_margin_s: int = 300,
|
||||
) -> None:
|
||||
self._app_id = app_id
|
||||
self._private_key_pem = private_key_pem
|
||||
self._installation_id = installation_id
|
||||
self._http = _http
|
||||
self._now = _now
|
||||
self._refresh_margin_s = refresh_margin_s
|
||||
self._cached_token: str | None = None
|
||||
self._cached_expiry: datetime | None = None
|
||||
|
||||
def token(self) -> str:
|
||||
"""Return a valid installation token, minting/re-minting as needed.
|
||||
|
||||
Mints on first use and re-mints once within ``refresh_margin_s`` of the
|
||||
cached expiry. Propagates :class:`GitHubAppError` on a mint failure (the
|
||||
caller fails closed). The returned token is NEVER logged.
|
||||
"""
|
||||
now = (self._now or _utc_now)()
|
||||
if (
|
||||
self._cached_token is None
|
||||
or self._cached_expiry is None
|
||||
or now + timedelta(seconds=self._refresh_margin_s) >= self._cached_expiry
|
||||
):
|
||||
result = mint_installation_token(
|
||||
app_id=self._app_id,
|
||||
private_key_pem=self._private_key_pem,
|
||||
installation_id=self._installation_id,
|
||||
_http=self._http,
|
||||
_now=self._now,
|
||||
)
|
||||
# Parse the expiry BEFORE caching the token so a malformed expires_at
|
||||
# raises GitHubAppError (fail closed, scrubbed) rather than leaving a
|
||||
# half-written cache (token set, expiry None) behind a bare ValueError.
|
||||
expiry = _parse_expires_at(result["expires_at"])
|
||||
self._cached_token = result["token"]
|
||||
self._cached_expiry = expiry
|
||||
return self._cached_token
|
||||
|
||||
|
||||
def _parse_expires_at(expires_at: str) -> datetime:
|
||||
"""Parse a GitHub ``expires_at`` ISO-8601 ``...Z`` string to aware UTC.
|
||||
|
||||
GitHub returns e.g. ``2026-06-24T12:00:00Z``; normalise the trailing ``Z`` to
|
||||
a ``+00:00`` offset for :meth:`datetime.fromisoformat`. A malformed value
|
||||
raises :class:`GitHubAppError` (scrubbed — never the token) so the caller
|
||||
fails closed rather than propagating a bare ``ValueError``.
|
||||
"""
|
||||
try:
|
||||
parsed = datetime.fromisoformat(expires_at.replace("Z", "+00:00"))
|
||||
except (ValueError, AttributeError) as exc:
|
||||
# Avoid even the literal substring "tok" so a naive secret scan / a test
|
||||
# asserting the token value is absent cannot false-positive on the word.
|
||||
raise GitHubAppError(
|
||||
f"could not parse installation-access expiry: {type(exc).__name__}"
|
||||
) from None
|
||||
# Normalise to aware UTC: a value lacking an offset would parse to a naive
|
||||
# datetime, and comparing it against the aware ``now`` in TokenProvider.token
|
||||
# would raise a bare TypeError (bypassing the fail-closed GitHubAppError
|
||||
# contract). GitHub always sends ``Z``, so this is defensive.
|
||||
if parsed.tzinfo is None:
|
||||
parsed = parsed.replace(tzinfo=timezone.utc)
|
||||
return parsed
|
||||
|
|
@ -526,7 +526,8 @@ def route_after_plan_gate(state: PipelineState) -> str:
|
|||
Reads the routing state :func:`plan_gate_node` wrote on resume (or on the
|
||||
ceiling-reached terminal park) and maps it to a route id:
|
||||
|
||||
* status ACTIVE + phase BUILD -> :data:`GATE_APPROVE_ROUTE` (END/approved);
|
||||
* status ACTIVE + phase BUILD -> :data:`GATE_APPROVE_ROUTE` (BUILD_NODE when
|
||||
the P3 build subgraph is wired, else the approved-plan END terminus);
|
||||
* status ACTIVE + phase PLAN -> :data:`GATE_REVISE_ROUTE` (loop to planner);
|
||||
* anything else (FAILED, or PARKED ceiling) -> :data:`GATE_TERMINAL_ROUTE`.
|
||||
"""
|
||||
|
|
@ -770,9 +771,17 @@ def build_graph(
|
|||
# the plan gate is wired it is REPOINTED at the PLAN_GATE vertex instead:
|
||||
# the cap dead-end suspends on a resumable human decision rather than
|
||||
# terminally parking. The gate's own conditional edges then route
|
||||
# approve -> END, request_changes -> PLAN (loop back), terminal -> END.
|
||||
# request_changes -> PLAN (loop back), terminal -> END, and approve ->
|
||||
# the SAME target the reviewer's auto-approve "build" route reaches:
|
||||
# BUILD_NODE when the P3 build subgraph is wired, else END (the P2
|
||||
# approved-plan terminus). Pointing approve at END unconditionally was a
|
||||
# bug — a human-approved-at-gate plan settled at phase BUILD without ever
|
||||
# entering the builder, so it never built (issue #60 sibling). BUILD_NODE
|
||||
# is added below in the build_verify branch; LangGraph resolves the
|
||||
# forward reference at compile().
|
||||
if plan_gate:
|
||||
parked_target = PLAN_GATE
|
||||
gate_approve_target = BUILD_NODE if build_verify is not None else END
|
||||
builder.add_node(
|
||||
PLAN_GATE,
|
||||
_instrument(PLAN_GATE, plan_gate_node, transition_recorder),
|
||||
|
|
@ -781,7 +790,7 @@ def build_graph(
|
|||
PLAN_GATE,
|
||||
route_after_plan_gate,
|
||||
{
|
||||
GATE_APPROVE_ROUTE: END,
|
||||
GATE_APPROVE_ROUTE: gate_approve_target,
|
||||
GATE_REVISE_ROUTE: PLAN,
|
||||
GATE_TERMINAL_ROUTE: END,
|
||||
},
|
||||
|
|
|
|||
|
|
@ -172,43 +172,83 @@ class DiffBuilder(Protocol):
|
|||
...
|
||||
|
||||
|
||||
# Unlike the single-shot reasoning→JSON nodes (clarifier/planner/fixer/verifier),
|
||||
# the builder is GENUINELY AGENTIC: synthesizing a unified diff needs to inspect
|
||||
# the repo and iterate, so it must override the invoker's single-shot defaults
|
||||
# (max_turns=1, allowed_tools=[]) — otherwise the call dies with "Reached maximum
|
||||
# number of turns (1)" (issue #60). Tools are READ-ONLY: the builder returns the
|
||||
# diff as DATA and performs no repo writes (D2/D11), so no Edit/Write/Bash.
|
||||
_BUILDER_MAX_TURNS = 8
|
||||
_BUILDER_ALLOWED_TOOLS = ["Read", "Grep", "Glob"]
|
||||
# A tool-using diff session runs longer than a one-shot completion; give it more
|
||||
# headroom than the $2 reasoning-node default.
|
||||
_BUILDER_BUDGET_USD = 4.0
|
||||
# default_diff_builder is the FALLBACK builder — used only when no real
|
||||
# diff_builder is bound (the live coordinator binds the DeepSeek mechanical-edit
|
||||
# builder, builders_llm.as_diff_builder, which is the intended P3 path). It is
|
||||
# deliberately SINGLE-SHOT and TOOL-LESS: enabling agentic tools makes every tool
|
||||
# call consume an agent turn, and the session exhausts the turn cap mid-
|
||||
# exploration before it ever emits a diff (issue #60 — observed live at both
|
||||
# max_turns=1 and =8). A few tool-less turns of headroom let the model FINISH the
|
||||
# diff text in one completion, the planner pattern. Tool-driven diff accuracy is
|
||||
# the DeepSeek builder's job, not this fallback's.
|
||||
_BUILDER_MAX_TURNS = 4
|
||||
|
||||
|
||||
def default_diff_builder(
|
||||
*, plan: Mapping[str, Any], config: Mapping[str, Any] | None
|
||||
) -> str:
|
||||
"""Default :class:`DiffBuilder`: author the diff via the Claude billing seam.
|
||||
"""Fallback :class:`DiffBuilder`: author the diff via the Claude billing seam.
|
||||
|
||||
Renders the approved plan into an instruction and calls
|
||||
:func:`agent_team.billing.claude_invoke` (the §3.1 seam) to produce the
|
||||
unified diff. Because the seam's default invoker raises until
|
||||
:func:`agent_team.billing.set_invoker` is called, an un-wired environment
|
||||
fails loudly here rather than emitting an empty diff. The coordinator binds
|
||||
the real Claude-spec + DeepSeek-edit path at startup.
|
||||
:func:`agent_team.billing.claude_invoke` (the §3.1 seam) as a single-shot,
|
||||
tool-less completion to produce the unified diff, then extracts the diff from
|
||||
the response (:func:`_extract_unified_diff`). Because the seam's default
|
||||
invoker raises until :func:`agent_team.billing.set_invoker` is called, an
|
||||
un-wired environment fails loudly here rather than emitting an empty diff.
|
||||
|
||||
Diff synthesis is agentic, so this passes an explicit non-single-shot config
|
||||
(turn/budget headroom + a read-only tool allowlist); see ``_BUILDER_*``.
|
||||
This is the INERT fallback only: the live coordinator binds the DeepSeek
|
||||
mechanical-edit builder (:func:`agent_team.nodes.builders_llm.as_diff_builder`)
|
||||
as the real ``diff_builder``, so this Claude path is not the production
|
||||
builder. It stays tool-less on purpose — see ``_BUILDER_MAX_TURNS``.
|
||||
"""
|
||||
prompt = _render_build_prompt(plan)
|
||||
result: ClaudeResult = claude_invoke(
|
||||
prompt,
|
||||
config=config,
|
||||
max_turns=_BUILDER_MAX_TURNS,
|
||||
allowed_tools=_BUILDER_ALLOWED_TOOLS,
|
||||
budget_usd=_BUILDER_BUDGET_USD,
|
||||
)
|
||||
return result.text
|
||||
return _extract_unified_diff(result.text)
|
||||
|
||||
|
||||
# The agentic builder uses read-only tools, so its response can wrap the diff in
|
||||
# a fenced code block and/or surround it with narration ("Let me read the source
|
||||
# files first…"). Recover the unified diff from that response: a ```diff fence is
|
||||
# preferred, else the slice from the first ``diff --git`` header. A response with
|
||||
# NO diff header at all is a hard BuildError — narration must never dispatch as a
|
||||
# candidate diff (it would fail closed at CI in a confusing way).
|
||||
_DIFF_FENCE_RE = re.compile(r"```(?:diff|patch)?[ \t]*\n(.*?)```", re.DOTALL)
|
||||
_DIFF_GIT_MARKER = "diff --git "
|
||||
|
||||
|
||||
def _extract_unified_diff(text: str) -> str:
|
||||
"""Recover the unified diff from the (tool-using) builder response.
|
||||
|
||||
Prefers the last fenced ```diff block containing a ``diff --git`` header;
|
||||
otherwise slices from the first ``diff --git`` marker to the end, dropping any
|
||||
surrounding narration. Raises :class:`BuildError` when the response carries no
|
||||
diff header at all (e.g. tool narration with no patch) so a prose-only reply
|
||||
fails loudly here rather than dispatching as a bogus candidate diff.
|
||||
"""
|
||||
if not isinstance(text, str):
|
||||
raise BuildError("builder returned a non-string response")
|
||||
|
||||
diff = ""
|
||||
for block in reversed(_DIFF_FENCE_RE.findall(text)):
|
||||
if _DIFF_GIT_MARKER in block:
|
||||
diff = block
|
||||
break
|
||||
if not diff:
|
||||
marker = text.find(_DIFF_GIT_MARKER)
|
||||
if marker != -1:
|
||||
diff = text[marker:]
|
||||
|
||||
diff = diff.strip()
|
||||
if _DIFF_GIT_MARKER not in diff:
|
||||
raise BuildError(
|
||||
"builder response contained no unified diff (no 'diff --git' header "
|
||||
"— the model returned narration / tool output instead of a patch)"
|
||||
)
|
||||
return diff + "\n"
|
||||
|
||||
|
||||
def _render_build_prompt(plan: Mapping[str, Any]) -> str:
|
||||
|
|
@ -232,7 +272,11 @@ def _render_build_prompt(plan: Mapping[str, Any]) -> str:
|
|||
"Dependabot config.\n\n"
|
||||
f"Title: {title}\n"
|
||||
f"Declared scope (paths you may edit):\n{scope_lines}\n"
|
||||
f"Phases:\n{phase_lines}\n"
|
||||
f"Phases:\n{phase_lines}\n\n"
|
||||
"Your response MUST be ONLY the unified diff in `git diff` format (each "
|
||||
"file section beginning with a `diff --git a/… b/…` header), wrapped in a "
|
||||
"single ```diff fenced code block. Do NOT include any narration, "
|
||||
"explanation, or text before or after the diff.\n"
|
||||
)
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -61,6 +61,16 @@ __all__ = [
|
|||
# clarifier loop's injected callables, etc.).
|
||||
ClaudeInvoke = Callable[..., ClaudeResult]
|
||||
|
||||
# The clarifier is a single-shot reasoning→JSON completion (confidence +
|
||||
# question-set), but the invoker's single-shot default (max_turns=1) is flaky:
|
||||
# when the model's one turn does not terminate in a final result it raises
|
||||
# "Reached maximum number of turns (1)", and with no salvageable text the call
|
||||
# fails and crashes the clarify node (leaving the task wedged at clarify with no
|
||||
# question posted). The planner hit the same flake and was given headroom in PR
|
||||
# #58; the clarifier needs the same. A few turns let the model FINISH its JSON;
|
||||
# tools stay OFF so it remains a fast, deterministic completion.
|
||||
_CLARIFIER_MAX_TURNS = 4
|
||||
|
||||
# Used when the model is below the confidence bar but supplied no usable
|
||||
# question-set. The loop must always have something to ask rather than spin or
|
||||
# falsely advance, so we substitute a generic clarifier prompt.
|
||||
|
|
@ -202,7 +212,12 @@ class ClaudeClarifier:
|
|||
return self._cache
|
||||
|
||||
prompt = self._build_prompt(qa_history, state)
|
||||
result = self._invoke(prompt, model=self._model, config=self._config)
|
||||
result = self._invoke(
|
||||
prompt,
|
||||
model=self._model,
|
||||
config=self._config,
|
||||
max_turns=_CLARIFIER_MAX_TURNS,
|
||||
)
|
||||
parsed = self._parse(getattr(result, "text", ""))
|
||||
|
||||
self._cache_key = key
|
||||
|
|
|
|||
|
|
@ -62,6 +62,7 @@ def make_dispatch_node(
|
|||
full graph state, so the coordinator can ALARM and a human can inspect.
|
||||
"""
|
||||
# Deferred import: no orchestrator / subprocess module at module load.
|
||||
from agent_team.ci_gate import diff_touched_paths
|
||||
from agent_team.dispatcher import DispatcherError, dispatch_apply_verify
|
||||
from agent_team.task_model import Phase, TaskStatus
|
||||
|
||||
|
|
@ -83,7 +84,19 @@ def make_dispatch_node(
|
|||
_LOG.warning("dispatch_node: missing thread_id or candidate_diff; parking")
|
||||
return _parked
|
||||
if not declared_scope.strip():
|
||||
_LOG.warning("dispatch_node: empty declared_scope from plan; parking")
|
||||
# No planner-/operator-declared scope (the planner emits only
|
||||
# summary+phases, never a scope) — derive an HONEST, non-empty
|
||||
# declared_scope from the candidate diff's own touched paths. The
|
||||
# workflow's guard still INDEPENDENTLY re-checks the materialized diff
|
||||
# against the denylist + '..'-escape + this scope + the diff-hash
|
||||
# binding, and the `agent-apply` environment's required reviewer remains
|
||||
# the human gate — so this only supplies the scope that was missing, it
|
||||
# does not relax any CI trust control.
|
||||
declared_scope = "\n".join(diff_touched_paths(diff_text))
|
||||
if not declared_scope.strip():
|
||||
_LOG.warning(
|
||||
"dispatch_node: no declared scope and diff touches no paths; parking"
|
||||
)
|
||||
return _parked
|
||||
|
||||
try:
|
||||
|
|
|
|||
|
|
@ -105,11 +105,12 @@ def test_default_diff_builder_calls_claude_invoke() -> None:
|
|||
assert "src" in seen["prompt"]
|
||||
|
||||
|
||||
def test_default_diff_builder_passes_agentic_config_to_invoke_seam() -> None:
|
||||
# Issue #60: the builder must NOT inherit the invoker's single-shot default
|
||||
# (max_turns=1, allowed_tools=[]) — diff synthesis is agentic and dies with
|
||||
# "Reached maximum number of turns (1)" otherwise. It opts in to turn/budget
|
||||
# headroom plus a READ-ONLY tool allowlist (no repo writes, D2/D11).
|
||||
def test_default_diff_builder_is_single_shot_and_tool_less() -> None:
|
||||
# Issue #60: the FALLBACK Claude builder must NOT run agentic with tools.
|
||||
# Tools make each call consume a turn and the session exhausts the cap before
|
||||
# emitting a diff (observed live at max_turns=1 and =8). It runs tool-less
|
||||
# with a few turns of headroom (planner pattern); the live builder is the
|
||||
# DeepSeek single-completion path, not this one.
|
||||
seen: dict = {}
|
||||
|
||||
def fake(prompt: str, *, mode: BillingMode, **kw):
|
||||
|
|
@ -121,12 +122,10 @@ def test_default_diff_builder_passes_agentic_config_to_invoke_seam() -> None:
|
|||
|
||||
kw = seen["kw"]
|
||||
assert kw.get("max_turns") == builders._BUILDER_MAX_TURNS
|
||||
assert kw["max_turns"] > 1
|
||||
allowed = kw.get("allowed_tools")
|
||||
assert allowed and {"Read", "Grep", "Glob"} <= set(allowed)
|
||||
# Read-only only: never hand the diff-as-data builder write/exec tools.
|
||||
assert not ({"Edit", "Write", "Bash"} & set(allowed))
|
||||
assert kw.get("budget_usd") == builders._BUILDER_BUDGET_USD
|
||||
assert kw["max_turns"] > 1 # headroom so the completion can finish the diff
|
||||
# Tool-less: no agentic tools are requested (that is what caused #60's
|
||||
# turn-exhaustion). allowed_tools is left unset → the invoker default [].
|
||||
assert "allowed_tools" not in kw or not kw["allowed_tools"]
|
||||
|
||||
|
||||
def test_default_diff_builder_fails_loud_when_unwired() -> None:
|
||||
|
|
@ -136,6 +135,57 @@ def test_default_diff_builder_fails_loud_when_unwired() -> None:
|
|||
default_diff_builder(plan={"title": "x"}, config=None)
|
||||
|
||||
|
||||
def test_extract_unified_diff_from_fenced_block_with_narration() -> None:
|
||||
# The agentic builder may narrate around a fenced ```diff block; recover only
|
||||
# the diff, dropping the prose.
|
||||
text = (
|
||||
"Let me read the key source files to get exact signatures first.\n"
|
||||
"Here is the patch:\n\n"
|
||||
"```diff\n" + CLEAN_DIFF + "```\n"
|
||||
"That implements the plan."
|
||||
)
|
||||
assert builders._extract_unified_diff(text) == CLEAN_DIFF
|
||||
|
||||
|
||||
def test_extract_unified_diff_from_bare_diff_with_leading_narration() -> None:
|
||||
# No fence: slice from the first ``diff --git`` header, dropping the prose.
|
||||
text = "I inspected the repo. Applying this change:\n\n" + CLEAN_DIFF
|
||||
assert builders._extract_unified_diff(text) == CLEAN_DIFF
|
||||
|
||||
|
||||
def test_extract_unified_diff_rejects_narration_only() -> None:
|
||||
# The exact failure observed live: tool narration with no patch. It MUST fail
|
||||
# closed (BuildError), never pass narration through as a candidate diff.
|
||||
text = "Let me read the key source files to get exact signatures before writing the diff."
|
||||
with pytest.raises(BuildError, match="no unified diff"):
|
||||
builders._extract_unified_diff(text)
|
||||
|
||||
|
||||
def test_default_diff_builder_extracts_diff_from_narrated_response() -> None:
|
||||
# End to end: the invoker returns narration + a fenced diff; the builder
|
||||
# returns the clean unified diff, not the prose.
|
||||
def fake(prompt: str, *, mode: BillingMode, **kw):
|
||||
return ClaudeResult(
|
||||
text="Sure — let me inspect the files.\n```diff\n" + CLEAN_DIFF + "```",
|
||||
mode=mode,
|
||||
)
|
||||
|
||||
billing.set_invoker(fake)
|
||||
out = default_diff_builder(plan={"title": "x", "scope": ["src"]}, config=None)
|
||||
assert out == CLEAN_DIFF
|
||||
|
||||
|
||||
def test_default_diff_builder_raises_on_prose_only_response() -> None:
|
||||
# A prose-only builder reply parks/fails the task rather than dispatching
|
||||
# garbage to CI (issue #60 output-contract hardening).
|
||||
def fake(prompt: str, *, mode: BillingMode, **kw):
|
||||
return ClaudeResult(text="Let me read the source files first.", mode=mode)
|
||||
|
||||
billing.set_invoker(fake)
|
||||
with pytest.raises(BuildError, match="no unified diff"):
|
||||
default_diff_builder(plan={"title": "x", "scope": ["src"]}, config=None)
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
# Diff parsing / canonicalization
|
||||
# --------------------------------------------------------------------------- #
|
||||
|
|
|
|||
|
|
@ -89,6 +89,18 @@ def test_high_confidence_parsed() -> None:
|
|||
assert clar.assess_confidence([], _state()) == 0.99
|
||||
|
||||
|
||||
def test_clarifier_passes_max_turns_headroom_to_invoke_seam() -> None:
|
||||
# The single-shot Claude default (1 turn) is flaky: it crashes the clarify
|
||||
# node with "Reached maximum number of turns (1)" and leaves the task wedged
|
||||
# with no question posted. The clarifier asks for headroom so the model can
|
||||
# FINISH its JSON (mirrors the planner fix, PR #58).
|
||||
fake = _FakeInvoke(_json(0.99, []))
|
||||
clar = ClaudeClarifier(invoke=fake)
|
||||
clar.assess_confidence([], _state())
|
||||
assert fake.calls[0]["kw"].get("max_turns") == 4
|
||||
assert fake.calls[0]["kw"]["max_turns"] > 1
|
||||
|
||||
|
||||
def test_single_call_per_turn_memoized() -> None:
|
||||
fake = _FakeInvoke(_json(0.99, []))
|
||||
clar = ClaudeClarifier(invoke=fake)
|
||||
|
|
|
|||
|
|
@ -17,6 +17,7 @@ Every test injects:
|
|||
|
||||
from __future__ import annotations
|
||||
|
||||
import logging
|
||||
import queue
|
||||
from datetime import datetime, timedelta, timezone
|
||||
from pathlib import Path
|
||||
|
|
@ -953,6 +954,37 @@ def test_default_review_wiring_binds_and_returns_node_and_router() -> None:
|
|||
review_loop._review_invoker = saved
|
||||
|
||||
|
||||
def test_failsafe_p3_wiring_binds_the_deepseek_diff_builder(monkeypatch: Any) -> None:
|
||||
# Issue #60 root fix: when the P3 env is configured the live failsafe MUST
|
||||
# bind a real diff_builder (the DeepSeek mechanical-edit path) onto the build
|
||||
# node, not leave it None — None falls back to the Claude single-shot builder
|
||||
# that exhausts its turn cap and fails every build.
|
||||
from agent_team import coordinator as coord_mod
|
||||
|
||||
monkeypatch.setenv("AGENT_TEAM_REPO_OWNER", "Sea-Haven-Industries")
|
||||
monkeypatch.setenv("AGENT_TEAM_REPO_NAME", "orchestrator")
|
||||
monkeypatch.setenv(
|
||||
"GITHUB_TOKEN", "ci-read-token"
|
||||
) # _p3_env_is_configured CI token
|
||||
|
||||
captured: dict = {}
|
||||
|
||||
def spy_gated(*, owner: str, repo: str, **kw: Any) -> tuple:
|
||||
captured["owner"] = owner
|
||||
captured["diff_builder"] = kw.get("diff_builder")
|
||||
return ("build", "verify", "route")
|
||||
|
||||
monkeypatch.setattr(coord_mod, "gated_build_verify_wiring", spy_gated)
|
||||
|
||||
build_verify_thunk, dispatch_factory = coord_mod.failsafe_production_p3_wiring()
|
||||
assert build_verify_thunk is not None and dispatch_factory is not None
|
||||
|
||||
# The diff_builder is bound when the thunk is invoked at graph-build.
|
||||
build_verify_thunk()
|
||||
assert captured["owner"] == "Sea-Haven-Industries"
|
||||
assert callable(captured["diff_builder"]) # a REAL builder, not None (the bug)
|
||||
|
||||
|
||||
def test_setup_with_p2_factories_builds_a_review_node(db_path: Path) -> None:
|
||||
"""Injecting the P2 factories compiles a graph that includes the review vertex."""
|
||||
from agent_team.nodes import review_loop
|
||||
|
|
@ -1499,6 +1531,88 @@ def test_parked_message_infers_phase_when_current_phase_is_parked(
|
|||
assert "Reached phase: parked" not in msgs[0]
|
||||
|
||||
|
||||
def test_followups_built_park_reports_verify_not_plan_escalation(
|
||||
db_path: Path, monkeypatch: Any
|
||||
) -> None:
|
||||
# A task that BUILT a diff and parked at the verify/CI gate must report the
|
||||
# build/verify stage + the CI conclusion — NOT the plan-review escalation
|
||||
# copy ("the plan could not be auto-approved"), which is wrong once a diff
|
||||
# exists. Regression for the issue-#60 sibling notification bug.
|
||||
from agent_team import coordinator as coord_mod
|
||||
from agent_team.task_model import TaskStatus
|
||||
|
||||
msgs: list[str] = []
|
||||
coord = _make_coordinator(db_path)
|
||||
coord._notify = msgs.append
|
||||
coord.setup()
|
||||
monkeypatch.setattr(
|
||||
coord_mod.graph_mod, "pending_question", lambda _g, *, thread_id: None
|
||||
)
|
||||
|
||||
class _Snap:
|
||||
values = {
|
||||
"status": TaskStatus.PARKED.value,
|
||||
"current_phase": "parked",
|
||||
"task": "add a smoke-test file in agent-team/tests",
|
||||
"candidate_diff": "diff --git a/x b/x\n",
|
||||
"diff_hash": "abc",
|
||||
"run_id": "r1",
|
||||
"ci_results": {"run_id": "r1", "conclusion": "failure"},
|
||||
# Stale review verdicts must NOT win the phase inference for a built task.
|
||||
"review_verdicts": [{"verdict": "request_changes", "findings": "old"}],
|
||||
}
|
||||
|
||||
monkeypatch.setattr(coord._graph, "get_state", lambda _cfg: _Snap())
|
||||
coord._post_resume_followups([_resume_result("beef0003cafe")])
|
||||
assert len(msgs) == 1
|
||||
m = msgs[0]
|
||||
assert "Reached phase: verify" in m # build/verify, not "review"
|
||||
assert "could not be auto-approved" not in m # the misleading copy is gone
|
||||
assert "verification gate did not pass" in m
|
||||
assert "failure" in m # the CI conclusion is surfaced
|
||||
|
||||
|
||||
def test_followups_awaiting_ci_reports_in_progress_not_parked(
|
||||
db_path: Path, monkeypatch: Any
|
||||
) -> None:
|
||||
# The VERIFY node SUSPENDS (async CI-wait) with a non-question interrupt, so
|
||||
# pending_question is None even though the graph is still suspended. The
|
||||
# notifier must report in-progress ("awaiting verification"), NOT a settled
|
||||
# PARK — this is the exact mislabel that posted "could not be auto-approved"
|
||||
# to Slack for a healthy, building task.
|
||||
from agent_team import coordinator as coord_mod
|
||||
from agent_team.task_model import TaskStatus
|
||||
|
||||
msgs: list[str] = []
|
||||
coord = _make_coordinator(db_path)
|
||||
coord._notify = msgs.append
|
||||
coord.setup()
|
||||
monkeypatch.setattr(
|
||||
coord_mod.graph_mod, "pending_question", lambda _g, *, thread_id: None
|
||||
)
|
||||
|
||||
class _Snap:
|
||||
interrupts = ("await-ci",) # graph still suspended on the CI-wait interrupt
|
||||
next = ("verify_node",)
|
||||
values = {
|
||||
"status": TaskStatus.PARKED.value, # stale carried-over while suspended
|
||||
"current_phase": "parked",
|
||||
"task": "add a smoke-test file in agent-team/tests",
|
||||
"candidate_diff": "diff --git a/x b/x\n",
|
||||
"diff_hash": "abc",
|
||||
"run_id": "run-42",
|
||||
}
|
||||
|
||||
monkeypatch.setattr(coord._graph, "get_state", lambda _cfg: _Snap())
|
||||
coord._post_resume_followups([_resume_result("d00d0004beef")])
|
||||
assert len(msgs) == 1
|
||||
m = msgs[0]
|
||||
assert "awaiting verification" in m.lower()
|
||||
assert "run-42" in m
|
||||
assert "could not be auto-approved" not in m
|
||||
assert "parked" not in m.lower()
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
# CI-watcher sweep in tick (§3.3.2 Decision 2) — async resume-on-CI-complete
|
||||
# --------------------------------------------------------------------------- #
|
||||
|
|
@ -1954,3 +2068,126 @@ def test_plan_decision_expiry_posts_recovery_notice(
|
|||
message, thread_ts = notice[0]
|
||||
assert thread_ts == "ROOT.TS"
|
||||
assert "re-assign" in message.lower() or "force-resume" in message.lower()
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
# default_dispatch_node_factory — GitHub App vs gh-default auth seams (FILE 3)
|
||||
# --------------------------------------------------------------------------- #
|
||||
|
||||
|
||||
def _capture_make_dispatch_node(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> list[dict[str, Any]]:
|
||||
"""Stub ``make_dispatch_node`` at its lazy import site; record each kwargs dict.
|
||||
|
||||
``default_dispatch_node_factory`` imports ``make_dispatch_node`` lazily from
|
||||
:mod:`agent_team.nodes.dispatch_invoker`, so the patch must land on that
|
||||
module (the name the ``from ... import`` resolves at call time), not on the
|
||||
coordinator module. Returns the list the stub appends its kwargs to.
|
||||
"""
|
||||
calls: list[dict[str, Any]] = []
|
||||
|
||||
def _fake_make_dispatch_node(**kwargs: Any) -> Any:
|
||||
calls.append(kwargs)
|
||||
return lambda state: state
|
||||
|
||||
monkeypatch.setattr(
|
||||
"agent_team.nodes.dispatch_invoker.make_dispatch_node",
|
||||
_fake_make_dispatch_node,
|
||||
)
|
||||
return calls
|
||||
|
||||
|
||||
def _clear_app_env(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
"""Ensure the owner/repo are set and no AGENT_TEAM_GH_APP_* leaks in."""
|
||||
monkeypatch.setenv("AGENT_TEAM_REPO_OWNER", "Sea-Haven-Industries")
|
||||
monkeypatch.setenv("AGENT_TEAM_REPO_NAME", "orchestrator")
|
||||
for var in (
|
||||
"AGENT_TEAM_GH_APP_ID",
|
||||
"AGENT_TEAM_GH_APP_INSTALLATION_ID",
|
||||
"AGENT_TEAM_GH_APP_PRIVATE_KEY",
|
||||
):
|
||||
monkeypatch.delenv(var, raising=False)
|
||||
|
||||
|
||||
def test_dispatch_factory_wires_app_seams_when_all_three_set(
|
||||
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
|
||||
) -> None:
|
||||
"""All three AGENT_TEAM_GH_APP_* set → make_dispatch_node gets the App seams.
|
||||
|
||||
With the App id, installation id, and a readable ``.pem`` path present, the
|
||||
factory resolves the GitHub-App ``(pusher, dispatcher, locator)`` triple (over
|
||||
a LAZY TokenProvider — the dummy pem text is never parsed here) and forwards
|
||||
all three to ``make_dispatch_node`` non-None.
|
||||
"""
|
||||
from agent_team.coordinator import default_dispatch_node_factory
|
||||
|
||||
_clear_app_env(monkeypatch)
|
||||
pem = tmp_path / "app.pem"
|
||||
pem.write_text(
|
||||
"-----BEGIN RSA PRIVATE KEY-----\nnot-a-real-key\n", encoding="utf-8"
|
||||
)
|
||||
monkeypatch.setenv("AGENT_TEAM_GH_APP_ID", "123456")
|
||||
monkeypatch.setenv("AGENT_TEAM_GH_APP_INSTALLATION_ID", "987654")
|
||||
monkeypatch.setenv("AGENT_TEAM_GH_APP_PRIVATE_KEY", str(pem))
|
||||
|
||||
calls = _capture_make_dispatch_node(monkeypatch)
|
||||
default_dispatch_node_factory()
|
||||
|
||||
assert len(calls) == 1
|
||||
kwargs = calls[0]
|
||||
assert kwargs["owner"] == "Sea-Haven-Industries"
|
||||
assert kwargs["repo"] == "orchestrator"
|
||||
assert kwargs["pusher"] is not None
|
||||
assert kwargs["dispatcher"] is not None
|
||||
assert kwargs["locator"] is not None
|
||||
|
||||
|
||||
def test_dispatch_factory_inert_on_partial_app_env(
|
||||
tmp_path: Path,
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
caplog: pytest.LogCaptureFixture,
|
||||
) -> None:
|
||||
"""Only APP_ID set → no App seams forwarded, ONE warning logged, NO exception.
|
||||
|
||||
A half-configured App env must NEVER raise (the daemon must still serve) and
|
||||
must fall back to gh-default — ``make_dispatch_node`` is called without the
|
||||
pusher/dispatcher/locator seams (absent or None).
|
||||
"""
|
||||
from agent_team.coordinator import default_dispatch_node_factory
|
||||
|
||||
_clear_app_env(monkeypatch)
|
||||
monkeypatch.setenv("AGENT_TEAM_GH_APP_ID", "123456") # the only one set
|
||||
|
||||
calls = _capture_make_dispatch_node(monkeypatch)
|
||||
with caplog.at_level(logging.WARNING, logger="agent_team.coordinator"):
|
||||
default_dispatch_node_factory() # must not raise
|
||||
|
||||
assert len(calls) == 1
|
||||
kwargs = calls[0]
|
||||
assert kwargs["owner"] == "Sea-Haven-Industries"
|
||||
assert kwargs["repo"] == "orchestrator"
|
||||
assert kwargs.get("pusher") is None
|
||||
assert kwargs.get("dispatcher") is None
|
||||
assert kwargs.get("locator") is None
|
||||
assert any("INERT" in rec.message for rec in caplog.records)
|
||||
|
||||
|
||||
def test_dispatch_factory_gh_default_when_no_app_env(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
"""None of the AGENT_TEAM_GH_APP_* set → unchanged gh-default path (no seams)."""
|
||||
from agent_team.coordinator import default_dispatch_node_factory
|
||||
|
||||
_clear_app_env(monkeypatch)
|
||||
|
||||
calls = _capture_make_dispatch_node(monkeypatch)
|
||||
default_dispatch_node_factory()
|
||||
|
||||
assert len(calls) == 1
|
||||
kwargs = calls[0]
|
||||
assert kwargs["owner"] == "Sea-Haven-Industries"
|
||||
assert kwargs["repo"] == "orchestrator"
|
||||
assert "pusher" not in kwargs
|
||||
assert "dispatcher" not in kwargs
|
||||
assert "locator" not in kwargs
|
||||
|
|
|
|||
|
|
@ -16,6 +16,9 @@ from agent_team.dispatcher import (
|
|||
DispatcherError,
|
||||
DispatchInputs,
|
||||
DispatchResult,
|
||||
app_branch_pusher,
|
||||
app_run_locator,
|
||||
app_workflow_dispatcher,
|
||||
build_dispatch_inputs,
|
||||
dispatch_apply_verify,
|
||||
head_branch_for,
|
||||
|
|
@ -328,3 +331,267 @@ def test_select_run_id_returns_none_when_only_cancelled_matches() -> None:
|
|||
select_run_id([only_cancelled], task_id=TASK, floor_iso="2026-06-23T09:58:00Z")
|
||||
is None
|
||||
)
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
# app_branch_pusher (App-token git seam; secret hygiene)
|
||||
# --------------------------------------------------------------------------- #
|
||||
|
||||
|
||||
class _StubTokenProvider:
|
||||
"""Minimal token provider: returns a fixed installation token."""
|
||||
|
||||
def __init__(self, token: str = "ghs_TESTTOKEN") -> None:
|
||||
self._token = token
|
||||
|
||||
def token(self) -> str:
|
||||
return self._token
|
||||
|
||||
|
||||
def test_app_branch_pusher_keeps_token_out_of_argv_and_uses_plain_remote() -> None:
|
||||
# The clone must use the PLAIN HTTPS remote (no token in the URL/argv), and
|
||||
# the installation token must be injected via http.extraHeader through git's
|
||||
# GIT_CONFIG_* env vars on the network steps (clone + push) — never in argv.
|
||||
recorded: list[dict] = []
|
||||
|
||||
def fake_run(cmd, *, cwd=None, env=None):
|
||||
recorded.append({"cmd": list(cmd), "env": env})
|
||||
|
||||
push = app_branch_pusher(_StubTokenProvider(), _run=fake_run)
|
||||
push(
|
||||
owner="owner",
|
||||
repo="repo",
|
||||
base="main",
|
||||
head_branch="agent-team/apply/abc",
|
||||
diff_text=DIFF,
|
||||
)
|
||||
|
||||
clone_cmd = recorded[0]["cmd"]
|
||||
assert "clone" in clone_cmd
|
||||
assert "https://github.com/owner/repo.git" in clone_cmd
|
||||
# The token NEVER appears in any git argv element.
|
||||
for call in recorded:
|
||||
for arg in call["cmd"]:
|
||||
assert "ghs_TESTTOKEN" not in arg
|
||||
|
||||
# Auth rides in GIT_CONFIG_* env on the network steps only (clone + push), as
|
||||
# BASIC auth (username x-access-token) — git smart-HTTP rejects Bearer.
|
||||
expected_basic = "Authorization: Basic " + base64.b64encode(
|
||||
b"x-access-token:ghs_TESTTOKEN"
|
||||
).decode("ascii")
|
||||
clone_env = recorded[0]["env"]
|
||||
assert clone_env["GIT_CONFIG_KEY_0"] == "http.https://github.com/.extraHeader"
|
||||
assert clone_env["GIT_CONFIG_VALUE_0"] == expected_basic
|
||||
push_env = recorded[-1]["env"]
|
||||
assert push_env["GIT_CONFIG_VALUE_0"] == expected_basic
|
||||
# The raw token never appears literally in the auth header (it is base64'd).
|
||||
assert "ghs_TESTTOKEN" not in clone_env["GIT_CONFIG_VALUE_0"]
|
||||
# The non-network steps (checkout/apply/commit) carry no auth env.
|
||||
for call in recorded[1:4]:
|
||||
assert call["env"] is None
|
||||
|
||||
# The five expected git steps fired in order. Skip the global ``git``
|
||||
# options (``-C <dir>``, ``-c <key=val>``) to find each subcommand.
|
||||
def _git_subcommand(cmd: list[str]) -> str:
|
||||
i = 1 # cmd[0] == "git"
|
||||
while i < len(cmd) and cmd[i] in ("-C", "-c"):
|
||||
i += 2 # each takes one argument
|
||||
return cmd[i]
|
||||
|
||||
steps = [_git_subcommand(c["cmd"]) for c in recorded]
|
||||
assert steps == ["clone", "checkout", "apply", "commit", "push"]
|
||||
|
||||
|
||||
def test_app_branch_pusher_scrubs_token_from_errors() -> None:
|
||||
# The token rides in the http.extraHeader config value (passed via env, not
|
||||
# argv). If a failing git step's exception string ever surfaces that header,
|
||||
# it must NEVER leak the token — it is re-raised as a DispatcherError with the
|
||||
# token scrubbed to ***.
|
||||
def failing_run(cmd, *, cwd=None, env=None):
|
||||
# Simulate an error whose string carries the Authorization header value.
|
||||
raise RuntimeError(
|
||||
"fatal: remote rejected (Authorization: Bearer ghs_TESTTOKEN)"
|
||||
)
|
||||
|
||||
push = app_branch_pusher(_StubTokenProvider(), _run=failing_run)
|
||||
with pytest.raises(DispatcherError) as excinfo:
|
||||
push(
|
||||
owner="owner",
|
||||
repo="repo",
|
||||
base="main",
|
||||
head_branch="agent-team/apply/abc",
|
||||
diff_text=DIFF,
|
||||
)
|
||||
assert "ghs_TESTTOKEN" not in str(excinfo.value)
|
||||
assert "***" in str(excinfo.value)
|
||||
# ``from None``: the token-bearing original is not chained onto the raised error.
|
||||
assert excinfo.value.__cause__ is None
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
# app_workflow_dispatcher (App-token REST seam)
|
||||
# --------------------------------------------------------------------------- #
|
||||
|
||||
|
||||
class _FakeHttp:
|
||||
"""Records POST/GET calls; returns a canned response object."""
|
||||
|
||||
def __init__(self, *, post_status=204, get_body=None, get_status=200) -> None:
|
||||
self.post_calls: list[dict] = []
|
||||
self.get_calls: list[dict] = []
|
||||
self._post_status = post_status
|
||||
self._get_body = get_body or {}
|
||||
self._get_status = get_status
|
||||
|
||||
def post(self, url, *, json=None, headers=None, timeout=None):
|
||||
self.post_calls.append({"url": url, "json": json, "headers": headers})
|
||||
return _FakeResp(status_code=self._post_status)
|
||||
|
||||
def get(self, url, *, params=None, headers=None, timeout=None):
|
||||
self.get_calls.append({"url": url, "params": params, "headers": headers})
|
||||
return _FakeResp(status_code=self._get_status, body=self._get_body)
|
||||
|
||||
|
||||
class _FakeResp:
|
||||
def __init__(self, *, status_code=200, body=None) -> None:
|
||||
self.status_code = status_code
|
||||
self._body = body or {}
|
||||
|
||||
def json(self):
|
||||
return self._body
|
||||
|
||||
|
||||
def test_app_workflow_dispatcher_posts_correct_url_and_body() -> None:
|
||||
http = _FakeHttp(post_status=204)
|
||||
fire = app_workflow_dispatcher(_StubTokenProvider(), _http=http)
|
||||
inputs = {"task_id": TASK, "diff_b64": "x"}
|
||||
fire(owner="owner", repo="repo", inputs=inputs, ref="main")
|
||||
|
||||
call = http.post_calls[0]
|
||||
assert call["url"].endswith(
|
||||
"/actions/workflows/agent-team-apply-verify.yml/dispatches"
|
||||
)
|
||||
assert call["json"] == {"ref": "main", "inputs": inputs}
|
||||
# The installation token is carried as a Bearer header (and nowhere else).
|
||||
assert call["headers"]["Authorization"] == "Bearer ghs_TESTTOKEN"
|
||||
|
||||
|
||||
def test_app_workflow_dispatcher_fails_closed_without_token_in_message() -> None:
|
||||
http = _FakeHttp(post_status=422)
|
||||
fire = app_workflow_dispatcher(_StubTokenProvider(), _http=http)
|
||||
with pytest.raises(DispatcherError) as excinfo:
|
||||
fire(owner="owner", repo="repo", inputs={"task_id": TASK}, ref="main")
|
||||
assert "422" in str(excinfo.value)
|
||||
assert "ghs_TESTTOKEN" not in str(excinfo.value)
|
||||
|
||||
|
||||
class _RaisingHttp:
|
||||
"""A transport whose post/get raises an exception that embeds the token."""
|
||||
|
||||
def post(self, url, *, json=None, headers=None, timeout=None):
|
||||
raise RuntimeError(f"connection reset: {headers['Authorization']}")
|
||||
|
||||
def get(self, url, *, params=None, headers=None, timeout=None):
|
||||
raise RuntimeError(f"connection reset: {headers['Authorization']}")
|
||||
|
||||
|
||||
def test_app_workflow_dispatcher_scrubs_token_from_transport_error() -> None:
|
||||
# A transport exception must be re-raised as a DispatcherError carrying the
|
||||
# exception TYPE only — never the Bearer token, even if the underlying error
|
||||
# text embedded it.
|
||||
fire = app_workflow_dispatcher(_StubTokenProvider(), _http=_RaisingHttp())
|
||||
with pytest.raises(DispatcherError) as excinfo:
|
||||
fire(owner="owner", repo="repo", inputs={"task_id": TASK}, ref="main")
|
||||
assert "ghs_TESTTOKEN" not in str(excinfo.value)
|
||||
assert excinfo.value.__cause__ is None # `from None` breaks the chain
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
# app_run_locator (App-token REST seam; field mapping + run-name correlation)
|
||||
# --------------------------------------------------------------------------- #
|
||||
|
||||
|
||||
def test_app_run_locator_maps_rest_fields_and_correlates_run_name() -> None:
|
||||
# The REST list endpoint uses id/created_at; the locator must map them to the
|
||||
# databaseId/createdAt names select_run_id reads, and correlate on run-name.
|
||||
body = {
|
||||
"workflow_runs": [
|
||||
{
|
||||
"id": 12345,
|
||||
"name": run_name_for(TASK),
|
||||
"created_at": "2026-06-23T10:00:00Z",
|
||||
"status": "in_progress",
|
||||
"conclusion": None,
|
||||
}
|
||||
]
|
||||
}
|
||||
http = _FakeHttp(get_body=body)
|
||||
locate = app_run_locator(_StubTokenProvider(), _http=http, _sleep=lambda *_a: None)
|
||||
run_id = locate(
|
||||
owner="owner",
|
||||
repo="repo",
|
||||
task_id=TASK,
|
||||
since_iso="2026-06-23T09:58:00Z",
|
||||
)
|
||||
assert run_id == "12345"
|
||||
# The list GET is authenticated + filtered to workflow_dispatch events.
|
||||
call = http.get_calls[0]
|
||||
assert call["url"].endswith("/actions/runs")
|
||||
assert call["params"]["event"] == "workflow_dispatch"
|
||||
assert call["headers"]["Authorization"] == "Bearer ghs_TESTTOKEN"
|
||||
|
||||
|
||||
def test_app_run_locator_returns_none_for_non_matching_task() -> None:
|
||||
body = {
|
||||
"workflow_runs": [
|
||||
{
|
||||
"id": 999,
|
||||
"name": "agent-team-apply someone-else",
|
||||
"created_at": "2026-06-23T10:00:00Z",
|
||||
"status": "in_progress",
|
||||
"conclusion": None,
|
||||
}
|
||||
]
|
||||
}
|
||||
http = _FakeHttp(get_body=body)
|
||||
locate = app_run_locator(_StubTokenProvider(), _http=http, _sleep=lambda *_a: None)
|
||||
assert (
|
||||
locate(
|
||||
owner="owner",
|
||||
repo="repo",
|
||||
task_id=TASK,
|
||||
since_iso="2026-06-23T09:58:00Z",
|
||||
)
|
||||
is None
|
||||
)
|
||||
|
||||
|
||||
def test_app_run_locator_raises_on_auth_error_without_token_in_message() -> None:
|
||||
# A 401/403/404 list response must park immediately with the STATUS only
|
||||
# (never the token), not silently poll ~60s and return None as "no runs".
|
||||
http = _FakeHttp(get_status=403, get_body={"message": "Bad credentials"})
|
||||
locate = app_run_locator(_StubTokenProvider(), _http=http, _sleep=lambda *_a: None)
|
||||
with pytest.raises(DispatcherError) as excinfo:
|
||||
locate(
|
||||
owner="owner",
|
||||
repo="repo",
|
||||
task_id=TASK,
|
||||
since_iso="2026-06-23T09:58:00Z",
|
||||
)
|
||||
assert "403" in str(excinfo.value)
|
||||
assert "ghs_TESTTOKEN" not in str(excinfo.value)
|
||||
|
||||
|
||||
def test_app_run_locator_scrubs_token_from_transport_error() -> None:
|
||||
locate = app_run_locator(
|
||||
_StubTokenProvider(), _http=_RaisingHttp(), _sleep=lambda *_a: None
|
||||
)
|
||||
with pytest.raises(DispatcherError) as excinfo:
|
||||
locate(
|
||||
owner="owner",
|
||||
repo="repo",
|
||||
task_id=TASK,
|
||||
since_iso="2026-06-23T09:58:00Z",
|
||||
)
|
||||
assert "ghs_TESTTOKEN" not in str(excinfo.value)
|
||||
assert excinfo.value.__cause__ is None
|
||||
|
|
|
|||
295
agent-team/tests/test_github_app.py
Normal file
295
agent-team/tests/test_github_app.py
Normal file
|
|
@ -0,0 +1,295 @@
|
|||
"""Unit tests for agent_team.github_app — App-JWT mint + TokenProvider cache.
|
||||
|
||||
These tests generate a throwaway RSA-2048 key with ``cryptography``, sign a real
|
||||
App JWT, and intercept the mint HTTP call with a fake client so no network and no
|
||||
real GitHub App is touched. They assert: the JWT claims/alg are correct, a clean
|
||||
``201`` returns the ``{token, expires_at}`` mapping, the provider caches and
|
||||
re-mints around the refresh margin, and — critically — that no token or JWT
|
||||
material ever appears in a raised error's message (secret hygiene).
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from datetime import datetime, timedelta, timezone
|
||||
|
||||
import jwt
|
||||
import pytest
|
||||
from cryptography.hazmat.primitives import serialization
|
||||
from cryptography.hazmat.primitives.asymmetric import rsa
|
||||
|
||||
from agent_team.github_app import (
|
||||
GitHubAppError,
|
||||
TokenProvider,
|
||||
mint_installation_token,
|
||||
)
|
||||
|
||||
_APP_ID = "123456"
|
||||
_INSTALLATION_ID = "987654"
|
||||
|
||||
# A fixed clock so JWT iat/exp are deterministic.
|
||||
_FIXED_NOW = datetime(2026, 6, 24, 12, 0, 0, tzinfo=timezone.utc)
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def rsa_keypair():
|
||||
"""Generate a throwaway RSA-2048 keypair; return (private_pem, public_obj)."""
|
||||
private_key = rsa.generate_private_key(public_exponent=65537, key_size=2048)
|
||||
private_pem = private_key.private_bytes(
|
||||
encoding=serialization.Encoding.PEM,
|
||||
format=serialization.PrivateFormat.PKCS8,
|
||||
encryption_algorithm=serialization.NoEncryption(),
|
||||
).decode("ascii")
|
||||
return private_pem, private_key.public_key()
|
||||
|
||||
|
||||
class _FakeResponse:
|
||||
def __init__(self, status: int, body):
|
||||
self.status_code = status
|
||||
self._body = body
|
||||
|
||||
def json(self):
|
||||
return self._body
|
||||
|
||||
|
||||
class _FakeHttp:
|
||||
"""A callable (url, *, headers, timeout) mint client recording calls."""
|
||||
|
||||
def __init__(self, response=None, raise_exc=None):
|
||||
self._response = response
|
||||
self._raise = raise_exc
|
||||
self.calls: list[dict] = []
|
||||
|
||||
def __call__(self, url, *, headers=None, timeout=None):
|
||||
self.calls.append({"url": url, "headers": headers, "timeout": timeout})
|
||||
if self._raise is not None:
|
||||
raise self._raise
|
||||
return self._response
|
||||
|
||||
@property
|
||||
def call_count(self) -> int:
|
||||
return len(self.calls)
|
||||
|
||||
|
||||
def _fixed_now():
|
||||
return _FIXED_NOW
|
||||
|
||||
|
||||
def _future_iso(seconds: int = 3600) -> str:
|
||||
return (_FIXED_NOW + timedelta(seconds=seconds)).strftime("%Y-%m-%dT%H:%M:%SZ")
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
# JWT claims / algorithm #
|
||||
# --------------------------------------------------------------------------- #
|
||||
def test_mint_signs_jwt_with_expected_claims(rsa_keypair):
|
||||
private_pem, public_key = rsa_keypair
|
||||
http = _FakeHttp(_FakeResponse(201, {"token": "tok", "expires_at": _future_iso()}))
|
||||
|
||||
mint_installation_token(
|
||||
app_id=_APP_ID,
|
||||
private_key_pem=private_pem,
|
||||
installation_id=_INSTALLATION_ID,
|
||||
_http=http,
|
||||
_now=_fixed_now,
|
||||
)
|
||||
|
||||
auth = http.calls[0]["headers"]["Authorization"]
|
||||
assert auth.startswith("Bearer ")
|
||||
token = auth.split(" ", 1)[1]
|
||||
|
||||
assert jwt.get_unverified_header(token)["alg"] == "RS256"
|
||||
# Verify the signature and claims, but not exp/iat against the real wall
|
||||
# clock: the JWT is minted from the fixed test clock (_fixed_now), so its
|
||||
# short-lived exp is in the past relative to the real time the suite runs.
|
||||
claims = jwt.decode(
|
||||
token,
|
||||
public_key,
|
||||
algorithms=["RS256"],
|
||||
options={"verify_aud": False, "verify_exp": False, "verify_iat": False},
|
||||
)
|
||||
assert claims["iss"] == _APP_ID
|
||||
now_ts = int(_FIXED_NOW.timestamp())
|
||||
assert claims["iat"] <= now_ts
|
||||
assert claims["exp"] - claims["iat"] <= 600
|
||||
|
||||
|
||||
def test_mint_targets_installation_token_url(rsa_keypair):
|
||||
private_pem, _ = rsa_keypair
|
||||
http = _FakeHttp(_FakeResponse(201, {"token": "tok", "expires_at": _future_iso()}))
|
||||
|
||||
mint_installation_token(
|
||||
app_id=_APP_ID,
|
||||
private_key_pem=private_pem,
|
||||
installation_id=_INSTALLATION_ID,
|
||||
_http=http,
|
||||
_now=_fixed_now,
|
||||
)
|
||||
|
||||
assert http.calls[0]["url"].endswith(
|
||||
f"/app/installations/{_INSTALLATION_ID}/access_tokens"
|
||||
)
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
# Mint success #
|
||||
# --------------------------------------------------------------------------- #
|
||||
def test_mint_success_returns_token_and_expiry(rsa_keypair):
|
||||
private_pem, _ = rsa_keypair
|
||||
expires_at = _future_iso()
|
||||
http = _FakeHttp(_FakeResponse(201, {"token": "tok", "expires_at": expires_at}))
|
||||
|
||||
result = mint_installation_token(
|
||||
app_id=_APP_ID,
|
||||
private_key_pem=private_pem,
|
||||
installation_id=_INSTALLATION_ID,
|
||||
_http=http,
|
||||
_now=_fixed_now,
|
||||
)
|
||||
|
||||
assert result == {"token": "tok", "expires_at": expires_at}
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
# TokenProvider caching / re-mint #
|
||||
# --------------------------------------------------------------------------- #
|
||||
def test_provider_caches_token_across_calls(rsa_keypair):
|
||||
private_pem, _ = rsa_keypair
|
||||
http = _FakeHttp(
|
||||
_FakeResponse(201, {"token": "tok", "expires_at": _future_iso(86400)})
|
||||
)
|
||||
provider = TokenProvider(
|
||||
app_id=_APP_ID,
|
||||
private_key_pem=private_pem,
|
||||
installation_id=_INSTALLATION_ID,
|
||||
_http=http,
|
||||
_now=_fixed_now,
|
||||
)
|
||||
|
||||
assert provider.token() == "tok"
|
||||
assert provider.token() == "tok"
|
||||
assert http.call_count == 1
|
||||
|
||||
|
||||
def test_provider_remints_near_expiry(rsa_keypair):
|
||||
private_pem, _ = rsa_keypair
|
||||
# First mint expires 10 minutes out; with a 5-minute margin the second call
|
||||
# (clock advanced past expiry - margin) must re-mint.
|
||||
responses = [
|
||||
_FakeResponse(201, {"token": "tok1", "expires_at": _future_iso(600)}),
|
||||
_FakeResponse(201, {"token": "tok2", "expires_at": _future_iso(1200)}),
|
||||
]
|
||||
|
||||
class _SeqHttp(_FakeHttp):
|
||||
def __call__(self, url, *, headers=None, timeout=None):
|
||||
self.calls.append({"url": url})
|
||||
return responses[len(self.calls) - 1]
|
||||
|
||||
http = _SeqHttp()
|
||||
|
||||
clock = {"now": _FIXED_NOW}
|
||||
|
||||
def _moving_now():
|
||||
return clock["now"]
|
||||
|
||||
provider = TokenProvider(
|
||||
app_id=_APP_ID,
|
||||
private_key_pem=private_pem,
|
||||
installation_id=_INSTALLATION_ID,
|
||||
_http=http,
|
||||
_now=_moving_now,
|
||||
refresh_margin_s=300,
|
||||
)
|
||||
|
||||
assert provider.token() == "tok1"
|
||||
assert http.call_count == 1
|
||||
|
||||
# Advance past (expiry - margin) = +300s -> re-mint.
|
||||
clock["now"] = _FIXED_NOW + timedelta(seconds=400)
|
||||
assert provider.token() == "tok2"
|
||||
assert http.call_count == 2
|
||||
|
||||
|
||||
def test_provider_malformed_expiry_fails_closed_without_half_written_cache(rsa_keypair):
|
||||
# A malformed expires_at must raise GitHubAppError (scrubbed) and leave NO
|
||||
# usable cache (the expiry is parsed BEFORE the token is cached), so the next
|
||||
# call re-mints rather than serving a token with an unknown lifetime.
|
||||
private_pem, _ = rsa_keypair
|
||||
http = _FakeHttp(
|
||||
_FakeResponse(201, {"token": "tok", "expires_at": "not-a-timestamp"})
|
||||
)
|
||||
provider = TokenProvider(
|
||||
app_id=_APP_ID,
|
||||
private_key_pem=private_pem,
|
||||
installation_id=_INSTALLATION_ID,
|
||||
_http=http,
|
||||
_now=_fixed_now,
|
||||
)
|
||||
|
||||
with pytest.raises(GitHubAppError) as excinfo:
|
||||
provider.token()
|
||||
assert "tok" not in str(excinfo.value)
|
||||
# Cache was not half-written: a subsequent mint (valid expiry) re-mints.
|
||||
http._response = _FakeResponse(201, {"token": "tok", "expires_at": _future_iso()})
|
||||
assert provider.token() == "tok"
|
||||
assert http.call_count == 2
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
# Secret hygiene #
|
||||
# --------------------------------------------------------------------------- #
|
||||
def test_mint_failure_status_is_scrubbed(rsa_keypair):
|
||||
private_pem, _ = rsa_keypair
|
||||
# Even on a failure GitHub may echo the (bad) token; ensure nothing leaks.
|
||||
http = _FakeHttp(_FakeResponse(401, {"token": "tok", "message": "Bad creds"}))
|
||||
|
||||
with pytest.raises(GitHubAppError) as exc:
|
||||
mint_installation_token(
|
||||
app_id=_APP_ID,
|
||||
private_key_pem=private_pem,
|
||||
installation_id=_INSTALLATION_ID,
|
||||
_http=http,
|
||||
_now=_fixed_now,
|
||||
)
|
||||
|
||||
message = str(exc.value)
|
||||
assert "tok" not in message
|
||||
# No JWT material (RS256 JWTs start with the base64 header "eyJ").
|
||||
assert "eyJ" not in message
|
||||
|
||||
|
||||
def test_mint_http_error_is_scrubbed(rsa_keypair):
|
||||
private_pem, _ = rsa_keypair
|
||||
http = _FakeHttp(raise_exc=RuntimeError("boom"))
|
||||
|
||||
# A raised transport error propagates (fail closed); it must not carry the
|
||||
# JWT. We do not catch a specific type here — only assert no JWT leaks if it
|
||||
# were ever wrapped.
|
||||
with pytest.raises(Exception) as exc: # noqa: PT011
|
||||
mint_installation_token(
|
||||
app_id=_APP_ID,
|
||||
private_key_pem=private_pem,
|
||||
installation_id=_INSTALLATION_ID,
|
||||
_http=http,
|
||||
_now=_fixed_now,
|
||||
)
|
||||
|
||||
assert "eyJ" not in str(exc.value)
|
||||
|
||||
|
||||
def test_mint_invalid_key_is_scrubbed():
|
||||
# An empty/invalid PEM must fail closed with a scrubbed message (no key).
|
||||
bad_key = "-----BEGIN PRIVATE KEY-----\nnotreallyakey\n-----END PRIVATE KEY-----"
|
||||
http = _FakeHttp(_FakeResponse(201, {"token": "tok", "expires_at": _future_iso()}))
|
||||
|
||||
with pytest.raises(GitHubAppError) as exc:
|
||||
mint_installation_token(
|
||||
app_id=_APP_ID,
|
||||
private_key_pem=bad_key,
|
||||
installation_id=_INSTALLATION_ID,
|
||||
_http=http,
|
||||
_now=_fixed_now,
|
||||
)
|
||||
|
||||
message = str(exc.value)
|
||||
assert "could not build app JWT" in message
|
||||
assert "notreallyakey" not in message
|
||||
|
|
@ -587,6 +587,84 @@ def test_plan_gate_approve_settles_as_approved_plan(restore_review_invoker) -> N
|
|||
assert final["status"] == TaskStatus.ACTIVE.value
|
||||
|
||||
|
||||
def _p3_plan_gate_graph(review_text: str, *, ci_result_fetcher):
|
||||
"""Compile a graph with BOTH the plan gate AND the P3 build->verify subgraph.
|
||||
|
||||
The reviewer (``review_text``) never approves, so the plan<->review loop hits
|
||||
the cap and suspends on the human PLAN_GATE; a human ``approve`` must then
|
||||
route into the build subgraph (BUILD -> VERIFY), exactly as the reviewer's
|
||||
own auto-approve "build" route does.
|
||||
"""
|
||||
from agent_team.nodes import review_loop
|
||||
from agent_team.nodes.build_verify_subgraph import (
|
||||
make_build_node,
|
||||
make_verify_node,
|
||||
route_after_verify,
|
||||
)
|
||||
from agent_team.nodes.verifier import VerifierConfig
|
||||
|
||||
review_loop.set_review_invoker(lambda prompt, **kw: review_text)
|
||||
|
||||
def fake_builder(*, plan, config):
|
||||
return _p3_diff()
|
||||
|
||||
build_node = make_build_node(diff_builder=fake_builder)
|
||||
verify_node = make_verify_node(
|
||||
VerifierConfig(expected_run_id="r1", allowed_scope=["src"]),
|
||||
ci_result_fetcher=ci_result_fetcher,
|
||||
)
|
||||
return build_graph(
|
||||
checkpointer=_Saver(),
|
||||
live_plan_node=_p3_plan_stub,
|
||||
review_node=review_loop.bind_review_node(),
|
||||
route_review=review_loop.route_after_review,
|
||||
plan_gate=True,
|
||||
build_verify=(build_node, verify_node, route_after_verify),
|
||||
)
|
||||
|
||||
|
||||
def test_plan_gate_approve_enters_build_subgraph_when_p3_wired(
|
||||
restore_review_invoker,
|
||||
) -> None:
|
||||
# Regression for the gate sibling of issue #60: a human approve at the plan
|
||||
# gate MUST route into the P3 build subgraph (BUILD -> VERIFY), not dead-end
|
||||
# at END leaving the task stuck at phase=build. The reviewer never approves,
|
||||
# so the loop caps to the human gate; an authenticated CI pass then carries
|
||||
# build -> verify -> DONE — proving the approve edge reached BUILD_NODE.
|
||||
def pass_fetcher(state):
|
||||
from agent_team.state_store import compute_content_hash
|
||||
|
||||
diff_hash = compute_content_hash(_p3_diff().encode("utf-8"))
|
||||
return {"run_id": "r1", "conclusion": "success", "diff_hash": diff_hash}
|
||||
|
||||
graph = _p3_plan_gate_graph(
|
||||
"VERDICT: REQUEST CHANGES\nnot yet", ci_result_fetcher=pass_fetcher
|
||||
)
|
||||
thread_id, _ = _drive_to_plan_gate(graph)
|
||||
final = resume_task(graph, thread_id=thread_id, answer={"decision": "approve"})
|
||||
|
||||
# Reached the build->verify PASS terminus, NOT a phase=build dead-end.
|
||||
assert final["current_phase"] == Phase.DONE.value
|
||||
assert final["status"] == TaskStatus.DONE.value
|
||||
|
||||
|
||||
def test_plan_gate_approve_inert_p3_still_settles_at_approved_terminus(
|
||||
restore_review_invoker,
|
||||
) -> None:
|
||||
# With the P3 build subgraph wired but its CI fetcher INERT (no authenticated
|
||||
# result), a gate approve enters build->verify and parks at VERIFY (the
|
||||
# production-safe default) — it must NOT fabricate a pass. Confirms the
|
||||
# approve edge routes through the subgraph, not to END.
|
||||
graph = _p3_plan_gate_graph(
|
||||
"VERDICT: REQUEST CHANGES\nnot yet", ci_result_fetcher=lambda state: None
|
||||
)
|
||||
thread_id, _ = _drive_to_plan_gate(graph)
|
||||
final = resume_task(graph, thread_id=thread_id, answer={"decision": "approve"})
|
||||
|
||||
assert final["current_phase"] == Phase.PARKED.value
|
||||
assert final["status"] == TaskStatus.PARKED.value
|
||||
|
||||
|
||||
def test_plan_gate_request_changes_loops_back_with_notes(
|
||||
restore_review_invoker,
|
||||
) -> None:
|
||||
|
|
|
|||
|
|
@ -274,7 +274,10 @@ def test_dispatch_node_parks_on_whitespace_only_diff() -> None:
|
|||
assert not disp_calls
|
||||
|
||||
|
||||
def test_dispatch_node_parks_on_empty_scope() -> None:
|
||||
def test_dispatch_node_derives_scope_from_diff_when_plan_scope_empty() -> None:
|
||||
# The planner never emits a scope, so an empty plan scope must NOT park: the
|
||||
# node derives declared_scope from the candidate diff's touched paths
|
||||
# (_VALID_STATE's diff touches f.py) and dispatches.
|
||||
_, pusher = _make_fake_pusher()
|
||||
disp_calls, dispatcher = _make_fake_workflow_dispatcher()
|
||||
node = make_dispatch_node(
|
||||
|
|
@ -284,11 +287,12 @@ def test_dispatch_node_parks_on_empty_scope() -> None:
|
|||
state = dict(_VALID_STATE, plan={"scope": []})
|
||||
result = node(state)
|
||||
|
||||
assert result.get("status") == TaskStatus.PARKED.value
|
||||
assert not disp_calls
|
||||
assert result.get("status") != TaskStatus.PARKED.value
|
||||
assert len(disp_calls) == 1
|
||||
assert disp_calls[0]["inputs"]["declared_scope"] == "f.py"
|
||||
|
||||
|
||||
def test_dispatch_node_parks_on_none_plan() -> None:
|
||||
def test_dispatch_node_derives_scope_from_diff_when_plan_none() -> None:
|
||||
_, pusher = _make_fake_pusher()
|
||||
disp_calls, dispatcher = _make_fake_workflow_dispatcher()
|
||||
node = make_dispatch_node(
|
||||
|
|
@ -298,11 +302,11 @@ def test_dispatch_node_parks_on_none_plan() -> None:
|
|||
state = dict(_VALID_STATE, plan=None)
|
||||
result = node(state)
|
||||
|
||||
assert result.get("status") == TaskStatus.PARKED.value
|
||||
assert not disp_calls
|
||||
assert result.get("status") != TaskStatus.PARKED.value
|
||||
assert disp_calls[0]["inputs"]["declared_scope"] == "f.py"
|
||||
|
||||
|
||||
def test_dispatch_node_parks_on_non_dict_plan() -> None:
|
||||
def test_dispatch_node_derives_scope_from_diff_when_plan_non_dict() -> None:
|
||||
_, pusher = _make_fake_pusher()
|
||||
disp_calls, dispatcher = _make_fake_workflow_dispatcher()
|
||||
node = make_dispatch_node(
|
||||
|
|
@ -312,6 +316,24 @@ def test_dispatch_node_parks_on_non_dict_plan() -> None:
|
|||
state = dict(_VALID_STATE, plan="not-a-dict")
|
||||
result = node(state)
|
||||
|
||||
assert result.get("status") != TaskStatus.PARKED.value
|
||||
assert disp_calls[0]["inputs"]["declared_scope"] == "f.py"
|
||||
|
||||
|
||||
def test_dispatch_node_parks_when_no_scope_and_diff_touches_no_paths() -> None:
|
||||
# The genuine park case: no plan scope AND a (non-empty) diff from which no
|
||||
# touched path can be parsed -> nothing honest to declare -> fail closed.
|
||||
_, pusher = _make_fake_pusher()
|
||||
disp_calls, dispatcher = _make_fake_workflow_dispatcher()
|
||||
node = make_dispatch_node(
|
||||
owner="org", repo="repo", pusher=pusher, dispatcher=dispatcher
|
||||
)
|
||||
|
||||
state = dict(
|
||||
_VALID_STATE, plan={"scope": []}, candidate_diff="not a real diff, no headers\n"
|
||||
)
|
||||
result = node(state)
|
||||
|
||||
assert result.get("status") == TaskStatus.PARKED.value
|
||||
assert not disp_calls
|
||||
|
||||
|
|
|
|||
|
|
@ -10,3 +10,14 @@ python-dotenv==1.2.2
|
|||
# WS1 agent-team HTTP API (agent_team/api.py): FastAPI app + uvicorn ASGI server.
|
||||
fastapi==0.136.1
|
||||
uvicorn==0.46.0
|
||||
# GitHub App installation-token minting for the agent-team P3 dispatcher
|
||||
# (agent_team/github_app.py): RS256 JWT (PyJWT) signed with the App private key,
|
||||
# exchanged for a short-lived installation token. cryptography backs RS256.
|
||||
PyJWT==2.13.0
|
||||
# >=48.0.1: earlier wheels statically link a vulnerable OpenSSL (GHSA-537c-gmf6-5ccf).
|
||||
cryptography==48.0.1
|
||||
# Runtime HTTP client for the agent-team P3 App-dispatch seams
|
||||
# (github_app.mint_installation_token, dispatcher.app_workflow_dispatcher,
|
||||
# dispatcher.app_run_locator) and the CI fetcher/transport. Pinned first-class
|
||||
# (was previously relied on only as a transitive dep of langchain-community).
|
||||
requests==2.34.2
|
||||
|
|
|
|||
Reference in a new issue