From cb84629c2bd483035bf4d4af69ecab4186cf7c16 Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Tue, 23 Jun 2026 19:06:08 -0400 Subject: [PATCH] =?UTF-8?q?feat(agent-team):=20P3=20Phases=20A/B/E=20?= =?UTF-8?q?=E2=80=94=20safety=20tooling,=20wiring,=20docs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase A (safety): - scripts/p3_rollback.sh (+test): restore all privileged P3 surfaces from a recorded baseline; --dry-run default, --apply gated. Correct App-uninstall (App JWT) model; per-task env-reviewer restore by numeric id; real protection post-restore assert (normalize reads argv, fails loud, divergent state exits non-zero — regression-tested). KNOWN-LIMITATIONS header flags the branch-protection GET->PUT transform + live-validation for the C1 gate. - scripts/assert_no_write_token.py (+test): box/CI audit that no write token (incl. ghu_/ghr_ prefixes + App PEM) lives on the box. - draft_pr_monitor.py (+test): runaway (>3/15min) + stale (7d) draft-PR sweep, wired into tick() and bound a read-only provider in serve. Phase B (wiring): systemd EnvironmentFile P3 vars + verification; new-draft-PR lifecycle notice. Phase E (docs): P3-LIVE-FLIP-PLAN/README/ci-README reflect CI-live-since-6/22 + box-integration; runbook consolidated (rollback Incident 7 + box-env wiring); removed a stray duplicate runbook. Suite: 1360 passed, ruff clean. Branch only; not merged/deployed. REMAINING HUMAN GATES: C1 /sh-security-review + GPT-4.1 cross-review on the enabled workflow + rollback script; D box deploy + smoke + merge. --- agent-team/README.md | 45 +- agent-team/agent_team/coordinator.py | 188 +++- agent-team/agent_team/draft_pr_monitor.py | 422 +++++++++ agent-team/ci/README.md | 36 + agent-team/run-team.py | 116 +++ agent-team/scripts/assert_no_write_token.py | 292 ++++++ agent-team/scripts/p3_rollback.sh | 865 ++++++++++++++++++ .../systemd/agent-team-coordinator.service | 21 + agent-team/tests/test_coordinator.py | 180 +++- agent-team/tests/test_no_write_token.py | 327 +++++++ agent-team/tests/test_rollback.py | 802 ++++++++++++++++ agent-team/tests/test_run_team.py | 109 +++ agent-team/tests/test_runaway_monitor.py | 400 ++++++++ docs/provisioning/OPERATOR-RUNBOOK.md | 151 +++ docs/provisioning/P3-LIVE-FLIP-PLAN.md | 205 +++-- 15 files changed, 4069 insertions(+), 90 deletions(-) create mode 100644 agent-team/agent_team/draft_pr_monitor.py create mode 100644 agent-team/scripts/assert_no_write_token.py create mode 100755 agent-team/scripts/p3_rollback.sh create mode 100644 agent-team/tests/test_no_write_token.py create mode 100644 agent-team/tests/test_rollback.py create mode 100644 agent-team/tests/test_runaway_monitor.py diff --git a/agent-team/README.md b/agent-team/README.md index 59fac93..95729af 100644 --- a/agent-team/README.md +++ b/agent-team/README.md @@ -2,7 +2,7 @@ The durable, human-gated agentic SDLC pipeline for the R720 (`sh-secrev` VM), design: `../docs/r720-agent-team-design.md`. A task flows INTAKE → CLARIFY (the -human gate) → PLAN → REVIEW, and (opt-in, deploy-gated) → BUILD → VERIFY → draft +human gate) → PLAN → REVIEW, and (P3) → BUILD → DISPATCH → VERIFY → draft PR. Every stage is durable and resumable (LangGraph + a SQLite checkpointer); the human gate suspends on `interrupt()` and resumes on a real answer. @@ -11,20 +11,25 @@ INTAKE → CLARIFY (Claude, human gate) → PLAN (Claude) → REVIEW (GPT-4.1) ▲ │ └── loop-back ───┤ approve/escalate → END - (P3, opt-in + INERT until the CI gate clears): - approve → BUILD (DeepSeek) → VERIFY (ci_gate) → draft PR + (P3 box path — gated behind the C1 re-review): + approve → BUILD (DeepSeek) → DISPATCH (push branch, trigger CI, + capture run_id, suspend) → [CI-watcher resumes on terminal + conclusion] → VERIFY (ci_gate) → draft PR ``` -> **Status (2026-06-18).** P1 (human gate) + P2 (planner + adversarial review +> **Status (2026-06-23).** P1 (human gate) + P2 (planner + adversarial review > loop) + the live runtime (coordinator, Slack/GitHub/Claude-Code transports, -> intake) + P3-inert (build/verify subgraph, opt-in) + P4 (more transports + -> GitHub-issue intake) are **built, reviewed, and merged to main** (~795 tests). -> **Nothing is provisioned**: not rsync'd to the box, no live tokens, no -> systemd, no live CI. Production default runs **P2** (no builders). -> **Deploy-gated / not yet built:** the P3 *live* CI apply/verify + OIDC role -> (held behind `/sh-security-review` + the mandatory GPT-4.1 cross-review), and -> all provisioning. See the project memory `project_r720_agent_team` and §7 of -> the design for the phased rollout. +> intake) + P3 (build/dispatch/verify subgraph) + P4 (more transports + +> GitHub-issue intake) are **built, reviewed, and merged to main**. +> **The CI apply/verify workflow is LIVE + provisioned** (the `agent-apply` +> environment, the `AGENT_APPLY_APP_*` secrets, and dispatched runs all exist as +> of 2026-06-22). **The box-side integration that drives it** (run_id capture, +> the BUILD → DISPATCH → VERIFY reorder, async CI-watch, and the fail-safe bound +> `serve` default) is built on this branch but **gated behind the C1 re-review** +> — the `/sh-security-review` + mandatory GPT-4.1 cross-review of the new +> CI-trigger boundary — before it ships to the box. See the project memory +> `project_r720_agent_team` and §7 of the design (and `docs/P3-PHASE0-DESIGN.md`) +> for the phased rollout. ## Layout @@ -62,11 +67,17 @@ agent-team/ planner.py # plan (Claude) review_loop.py + review_loop_llm.py # adversarial review (GPT-4.1 via orchestrator) builders.py + builders_llm.py # candidate diff (DeepSeek) — INERT, proposes only - verifier.py + verifier_llm.py # ci_gate sole PASS authority; LLM = fix-proposer - build_verify_subgraph.py # P3 BUILD→VERIFY topology (opt-in) + verifier.py + verifier_llm.py # ci_gate sole PASS authority; LLM = fix-proposer; + # binds expected_run_id per-task from state["run_id"] + build_verify_subgraph.py # P3 BUILD→DISPATCH→VERIFY topology + the tick()-driven + # CI-watcher that resumes a suspended task on the + # dispatched run's terminal conclusion (or parks on timeout) handbook.py # WS5 load_handbook_conventions (handbook seam, # fail-safe → "" if dir missing); planner context - dispatch_invoker.py # WS3 auto-dispatch node — INERT (NOT wired live) + 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) 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 @@ -75,7 +86,7 @@ agent-team/ web/ # React/Vite/TypeScript status-dashboard SPA (React Flow map, # task list, click-through task history); built to web/dist scripts/ # deploy-r720-ws-rollout.sh — attended WS0–WS5 UPDATE of the box - ci/ # §3.3.2 split-job CI apply/verify workflow (DEPLOY-GATED) + ci/ # §3.3.2 split-job CI apply/verify workflow (LIVE since 2026-06-22) systemd/ # agent-team-coordinator.service + agent-team-status.service DEPLOY-R720.md # provisioning runbook (snapshot-first, rsync, tokens, demo) tests/ # pytest, one module per source module + sim harness @@ -149,7 +160,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 | `nodes/dispatch_invoker.py` (auto-dispatch LangGraph node) + graph/coordinator wiring | **INERT — NOT wired live.** The `agent-apply` GitHub Environment human-approval gate is KEPT; the P3 dispatch/build-verify path stays inert pending per-task `run_id` plumbing + a CI-boundary security re-review | +| 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 | The HTTP API endpoints: `POST /tasks` (start a task), `GET /tasks/{thread_id}` (status), `POST /orchestrator/invoke` (one-shot model invoke). See diff --git a/agent-team/agent_team/coordinator.py b/agent-team/agent_team/coordinator.py index 2f8610a..9bea15b 100644 --- a/agent-team/agent_team/coordinator.py +++ b/agent-team/agent_team/coordinator.py @@ -581,6 +581,7 @@ class Coordinator: ci_pending_provider: "Callable[[], list[Any]] | None" = None, ci_poller: "Callable[[Any], Any] | None" = None, ci_timeout: timedelta | None = None, + draft_pr_provider: "Callable[[], list[Any]] | None" = None, ) -> None: self._db_path = Path(db_path) self._transport = transport @@ -629,6 +630,15 @@ class Coordinator: self._ci_pending_provider = ci_pending_provider self._ci_poller = ci_poller self._ci_timeout = ci_timeout + # Draft-PR runaway/stale monitor seam (P3 A4). OPT-IN and default None, so + # the draft-PR sweep in tick() is a NO-OP unless the live P3 path provides + # ``draft_pr_provider`` — an enumerator of the currently-open draft PRs (as + # :class:`agent_team.draft_pr_monitor.DraftPr`). Left None, no draft-PR + # sweep runs (the INERT default + the unit-test path). The flapping-backoff + # memory persists for the daemon's lifetime so a sustained condition is not + # re-ALARMed / re-reminded every tick. + self._draft_pr_provider = draft_pr_provider + self._draft_pr_memory: Any = None # Built by setup(). self._graph: Any = None @@ -1125,6 +1135,21 @@ class Coordinator: "Re-assign with more detail, or adjust the requirement to unblock.", thread_ts=root_ts, ) + elif self._verify_pass_verdict(values) is not None: + # P3 terminal PASS: the task's CI apply/verify run reached a + # terminal PASS (the pure-code gate PASSed) and the APPROVED route + # opened the draft PR (§3.3.2 PASS terminus). Emit the POSITIVE + # lifecycle notice (NOT the park-ALARM path) with the run/PR link + # so the human can go review the draft PR. Distinguished from the + # P2 plan-ready terminus below by the verify-stage PASS verdict — + # both settle at status/phase DONE, so the verdict is the + # discriminator (a plan-ready task carries no verify verdict). + verdict = self._verify_pass_verdict(values) + self._emit( + f"🎉 {label} — CI PASSED, draft PR opened.\n" + f"{self._draft_pr_notice(values, verdict)}", + thread_ts=root_ts, + ) else: # Plan approved + settled at the P2 terminus. PRESENT the plan # (condensed) so the human can actually review it in-thread, not @@ -1197,6 +1222,71 @@ class Coordinator: "requesting changes without converging)." ) + @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. + + The P3 build→dispatch→verify PASS terminus and the P2 plan-ready terminus + BOTH settle at status/phase ``DONE`` (see :func:`agent_team.graph.plan_node` + and :func:`agent_team.nodes.verifier.verifier_node`), so status alone can + not tell them apart. The discriminator is the verifier's verdict: only a + task that went through VERIFY and PASSed the pure-code CI gate appends a + ``review_verdicts`` entry with ``stage == "verify"`` and a PASS decision + (:func:`agent_team.nodes.verifier._verdict`). A plan-ready task carries no + such verdict. Returns the most recent matching verdict (the one that + opened the draft PR) or ``None`` when the task did not terminally PASS CI. + """ + verdicts = values.get("review_verdicts") or [] + if not isinstance(verdicts, list): + return None + for verdict in reversed(verdicts): + if not isinstance(verdict, dict): + continue + if ( + verdict.get("stage") == "verify" + and str(verdict.get("decision") or "").lower() == "pass" + ): + return verdict + return None + + @staticmethod + def _draft_pr_notice( + values: "dict[str, Any]", verdict: "dict[str, Any] | None" + ) -> str: + """Body of the POSITIVE draft-PR LIFECYCLE notice (run/PR link). + + Surfaces the link the human needs to go review the freshly-opened draft + PR. Two links may be present: the GitHub Actions **run** link (always + derivable from the run id the dispatcher captured + the configured + owner/repo) and the **PR** link (only once a draft-PR transport writes a + ``pr_url`` into state — forward-compatible; absent today). Both are + best-effort and fail soft: a missing run id / unset owner/repo simply + drops that line rather than crashing the milestone. The run id falls back + to ``state["run_id"]`` when the verdict carries none. + """ + lines: list[str] = [] + pr_url = str(values.get("pr_url") or "").strip() + if pr_url: + lines.append(f"• Draft PR: {pr_url}") + + run_id = "" + if isinstance(verdict, dict): + run_id = str(verdict.get("run_id") or "").strip() + if not run_id: + run_id = str(values.get("run_id") or "").strip() + if run_id: + owner = os.environ.get("AGENT_TEAM_REPO_OWNER", "").strip() + repo = os.environ.get("AGENT_TEAM_REPO_NAME", "").strip() + if owner and repo: + lines.append( + f"• CI run: https://github.com/{owner}/{repo}/actions/runs/{run_id}" + ) + else: + lines.append(f"• CI run: {run_id}") + + lines.append("• Review the draft PR when you have a moment.") + return "\n".join(lines) + def tick(self) -> list[ResumeResult]: """One maintenance pass: deadline sweep + CI-watch sweep, then drain (§3.3.1, §3.3.2). @@ -1208,9 +1298,11 @@ class Coordinator: CI-watcher sweep (:meth:`_ci_watch`) alongside the deadline sweep — the P3 async resume-on-CI-complete pass that resumes tasks whose CI run terminated and parks tasks whose run timed out / failed to poll (§4 - Decision 2). Both sweeps are fail-soft. Finally drains any resume jobs - that landed (including resumes the CI-watch sweep enqueued). Returns the - drain results. + Decision 2). It then runs the draft-PR runaway/stale sweep + (:meth:`_draft_pr_monitor_sweep`) — the P3 A4 pass that ALARMs on a + draft-PR open-burst and reminds on a stale draft PR. All sweeps are + fail-soft. Finally drains any resume jobs that landed (including resumes + the CI-watch sweep enqueued). Returns the drain results. """ conn = connect(self._db_path) try: @@ -1226,6 +1318,10 @@ class Coordinator: # breaks the maintenance loop. self._ci_watch() + # Draft-PR runaway/stale sweep (P3 A4). NO-OP unless ``draft_pr_provider`` + # is wired; fail-soft so a monitor error never breaks the maintenance loop. + self._draft_pr_monitor_sweep() + results = self.drain_resumes() self._post_resume_followups(results) return results @@ -1339,6 +1435,92 @@ class Coordinator: ) self._alarm_hook(task.thread_id) + def _draft_pr_monitor_sweep(self) -> Any: + """Run one draft-PR runaway/stale sweep (P3 A4). + + NO-OP unless ``draft_pr_provider`` is wired (the INERT default + the + unit-test path skip it entirely). When wired, it: + + 1. enumerates the currently-open draft PRs (via ``draft_pr_provider``, as + :class:`agent_team.draft_pr_monitor.DraftPr`); and + 2. runs :func:`agent_team.draft_pr_monitor.run_draft_pr_monitor` with the + daemon-lifetime flapping-backoff memory and injected ALARM / reminder + side effects: the ALARM posts a runaway notice to ``#agent-team`` via + the lifecycle path (operator remediation = stop auto-dispatch; the + monitor never self-stops), and the reminder posts a stale-PR notice + (never auto-closes). + + Fail-soft: any error in enumeration or the sweep is logged and swallowed + so a monitor failure never breaks the tick loop. Returns the + :class:`~agent_team.draft_pr_monitor.MonitorReport` (or ``None`` when + skipped / on error) for logging/tests. + """ + if self._draft_pr_provider is None: + return None + + from agent_team.draft_pr_monitor import ( # noqa: PLC0415 + MonitorMemory, + run_draft_pr_monitor, + ) + + if self._draft_pr_memory is None: + self._draft_pr_memory = MonitorMemory() + + try: + draft_prs = self._draft_pr_provider() + except Exception: # noqa: BLE001 - enumeration must not break tick + _LOG.warning("draft-pr-monitor: draft-PR enumeration raised", exc_info=True) + return None + + if not draft_prs: + return None + + try: + report = run_draft_pr_monitor( + list(draft_prs), + on_alarm=self._draft_pr_alarm, + on_stale_reminder=self._draft_pr_stale_reminder, + memory=self._draft_pr_memory, + ) + except Exception: # noqa: BLE001 - a sweep failure must not break tick + _LOG.warning("draft-pr-monitor: sweep raised", exc_info=True) + return None + + if report.alarmed: + _LOG.error( + "draft-pr-monitor: RUNAWAY ALARM raised — %d draft PRs opened " + "within the window (operator remediation: stop auto-dispatch)", + report.opened_in_window, + ) + return report + + def _draft_pr_alarm(self, opened_in_window: int) -> None: + """Post the draft-PR runaway ALARM to the operator channel (P3 A4). + + Surfaces the burst + the operator remediation (stop auto-dispatch). The + monitor does NOT self-stop — ``systemctl stop`` is the human action; this + only makes the condition visible. Routed through the lifecycle ``_emit`` + sink (never raises), so a Slack failure cannot break the tick loop. + """ + self._emit( + f"🚨 ALARM: draft-PR runaway — {opened_in_window} draft PRs opened " + "within 15 minutes (threshold > 3). Auto-dispatch may be looping. " + "Remediation: stop auto-dispatch on the box (`systemctl stop`). The " + "monitor surfaces this; it does not self-stop or auto-close." + ) + + def _draft_pr_stale_reminder(self, pr: Any) -> None: + """Post a stale draft-PR reminder to the operator channel (P3 A4). + + A draft PR idle > 7 days is surfaced once (per cooldown) so it is not + silently forgotten. NEVER auto-closes — closing is a human decision. + Routed through the lifecycle ``_emit`` sink (never raises). + """ + self._emit( + f"⏳ Reminder: draft PR #{pr.number} has been idle for over 7 days. " + "Review, update, or close it (the monitor never auto-closes)." + ) + def _enumerate_ci_pending(self) -> list[Any]: """Enumerate the durable threads suspended at VERIFY awaiting CI (§3.3.2). diff --git a/agent-team/agent_team/draft_pr_monitor.py b/agent-team/agent_team/draft_pr_monitor.py new file mode 100644 index 0000000..19a94aa --- /dev/null +++ b/agent-team/agent_team/draft_pr_monitor.py @@ -0,0 +1,422 @@ +"""Draft-PR runaway monitor + stale cleanup sweep (P3 box-integration, A4). + +Once the box-side BUILD → DISPATCH → VERIFY path is live (CI apply/verify is +already provisioned), a passing task ends at a **draft PR** (the §3.3.2 PASS +terminus). Two failure modes then need a maintenance sweep, mirroring the shape +of :mod:`agent_team.deadline_timer` and :mod:`agent_team.ci_watcher` (a pure, +restart-safe ``tick()``-driven pass whose side effects are injected callables, so +it is unit-testable with NO network and NO live graph): + +* **RUNAWAY** — a dispatch loop (a stuck builder, a re-dispatch storm, or a + fixer that keeps re-opening) can open draft PRs far faster than a human can + review them. If **more than 3 draft PRs are opened within any 15-minute + window**, raise an ALARM to ``#agent-team`` (via the existing lifecycle / ALARM + path). The remediation is **operator-driven**: stop auto-dispatch + (``systemctl stop``). The monitor *surfaces* the condition; it never + self-restarts, self-stops, or auto-closes anything — it has no standing write + authority and must not act on infrastructure. + +* **STALE** — a draft PR that has sat **idle for more than 7 days** is surfaced + with a single ``#agent-team`` reminder so it is not silently forgotten. The + monitor **never auto-closes** a stale PR — closing is a human decision; the + reminder is the only action. + +Flapping backoff (the load-bearing anti-spam discipline): a RUNAWAY condition +typically persists across many ticks (the offending PRs stay inside the window +for the whole 15 minutes), and a STALE PR stays stale until a human acts. Without +backoff the sweep would re-ALARM / re-remind on *every* tick. So the monitor +carries a small :class:`MonitorMemory` (injected, durable-across-ticks): it +ALARMs at most once per ``alarm_cooldown`` and reminds about a given PR at most +once per ``stale_cooldown``. The memory is passed in (not module-global) so the +coordinator owns its lifetime and a test can assert the backoff deterministically. + +Fail-soft / fail-closed discipline (mirroring the sibling sweeps): + +* Every side effect (``on_alarm`` / ``on_stale_reminder``) is an injected + callable; the monitor performs no transport I/O of its own. +* A draft PR with an unparseable ``opened_at`` is ignored for the runaway count + (it cannot be placed in the window) but a present ``updated_at`` is still + considered for staleness; a PR with neither parseable timestamp is skipped + rather than crashing the sweep. +* A side effect that raises is isolated per condition (logged, recorded) so one + broken post never aborts the pass — the daemon's ``tick()`` loop keeps running. +* All inputs are read fresh each pass (the injected provider re-enumerates the + open draft PRs); the only retained state is the small backoff memory. +""" + +from __future__ import annotations + +import logging +from collections.abc import Callable +from dataclasses import dataclass, field +from datetime import datetime, timedelta, timezone +from enum import Enum + +__all__ = [ + "AlarmFn", + "DEFAULT_ALARM_COOLDOWN", + "DEFAULT_ALARM_THRESHOLD", + "DEFAULT_ALARM_WINDOW", + "DEFAULT_STALE_AFTER", + "DEFAULT_STALE_COOLDOWN", + "DraftPr", + "MonitorAction", + "MonitorMemory", + "MonitorOutcome", + "MonitorReport", + "StaleReminderFn", + "run_draft_pr_monitor", +] + +logger = logging.getLogger(__name__) + +# Concrete thresholds (A4). RUNAWAY: > 3 draft PRs opened within 15 minutes. +DEFAULT_ALARM_THRESHOLD = 3 +DEFAULT_ALARM_WINDOW = timedelta(minutes=15) +# STALE: a draft PR idle (no update) for more than 7 days. +DEFAULT_STALE_AFTER = timedelta(days=7) + +# Flapping backoff windows. A runaway condition persists across many ticks while +# the offending PRs stay inside the 15-minute window, and a stale PR stays stale +# until a human acts; these cooldowns stop the sweep re-alarming / re-reminding +# every tick. One ALARM per 15 min, one reminder per PR per day. +DEFAULT_ALARM_COOLDOWN = timedelta(minutes=15) +DEFAULT_STALE_COOLDOWN = timedelta(days=1) + + +class MonitorAction(Enum): + """The action the sweep took this pass for one surfaced condition. + + ``ALARMED`` — a runaway burst was detected and the ALARM was posted. + ``ALARM_SUPPRESSED`` — a runaway burst was detected but an ALARM was posted + recently (within ``alarm_cooldown``), so it was suppressed (flapping + backoff). ``STALE_REMINDED`` — a stale PR's reminder was posted. + ``STALE_SUPPRESSED`` — a stale PR was found but it was reminded about + recently (within ``stale_cooldown``), so the reminder was suppressed. + ``ERRORED`` — a side effect raised; the condition was detected but the post + failed (isolated, the sweep continues). + """ + + ALARMED = "alarmed" + ALARM_SUPPRESSED = "alarm_suppressed" + STALE_REMINDED = "stale_reminded" + STALE_SUPPRESSED = "stale_suppressed" + ERRORED = "errored" + + +@dataclass(frozen=True) +class DraftPr: + """A minimal read-snapshot of one open draft PR (A4). + + Only the fields the monitor needs: identity (``number``) and the two + timestamps the runaway-count / staleness checks key off. ``opened_at`` is + when the draft PR was created (runaway window); ``updated_at`` is the last + activity (staleness). Frozen because it is a snapshot — the monitor never + mutates a PR; it acts only through the injected callables. + """ + + number: int + opened_at: str | None = None + updated_at: str | None = None + + +@dataclass(frozen=True) +class MonitorOutcome: + """The result of one surfaced condition this pass (A4).""" + + action: MonitorAction + # For a runaway ALARM: the count of PRs opened inside the window. For a stale + # outcome: the PR number. ``None`` is never expected but keeps the dataclass + # total for the ERRORED path. + detail: str | None = None + error: str | None = None + + +@dataclass +class MonitorReport: + """Aggregate result of one draft-PR monitor pass (mirrors the sibling sweeps). + + ``outcomes`` is one entry per surfaced condition (a runaway ALARM and/or each + stale reminder). The summary counters let the coordinator log/ALARM without + re-walking the list. + """ + + outcomes: list[MonitorOutcome] = field(default_factory=list) + # The number of draft PRs counted as opened inside the runaway window this + # pass, for observability (not every counted PR produces an outcome — only a + # *breach* does). + opened_in_window: int = 0 + + @property + def alarmed(self) -> int: + """Runaway ALARMs actually posted this pass.""" + return sum(1 for o in self.outcomes if o.action is MonitorAction.ALARMED) + + @property + def alarm_suppressed(self) -> int: + """Runaway breaches detected but suppressed by the alarm cooldown.""" + return sum( + 1 for o in self.outcomes if o.action is MonitorAction.ALARM_SUPPRESSED + ) + + @property + def stale_reminded(self) -> int: + """Stale reminders actually posted this pass.""" + return sum(1 for o in self.outcomes if o.action is MonitorAction.STALE_REMINDED) + + @property + def stale_suppressed(self) -> int: + """Stale PRs found but suppressed by the per-PR reminder cooldown.""" + return sum( + 1 for o in self.outcomes if o.action is MonitorAction.STALE_SUPPRESSED + ) + + @property + def errored(self) -> int: + """Conditions whose side effect raised (isolated; sweep continued).""" + return sum(1 for o in self.outcomes if o.action is MonitorAction.ERRORED) + + +@dataclass +class MonitorMemory: + """Durable-across-ticks backoff memory (the flapping-backoff seam). + + The monitor itself is otherwise pure: it reads the open draft PRs fresh each + pass. This small mutable record is the ONE piece of state that must survive + between ticks so a persistent condition does not re-ALARM / re-remind every + pass. The coordinator owns one instance for the daemon's lifetime; a test + constructs its own to assert the backoff deterministically. + + ``last_alarm_at`` — when a runaway ALARM was last posted (None = never). + ``last_reminded_at`` — per-PR-number, when that PR was last reminded about. + Entries for PRs no longer present are pruned each pass so the map cannot grow + without bound across a long-running daemon. + """ + + last_alarm_at: datetime | None = None + last_reminded_at: dict[int, datetime] = field(default_factory=dict) + + +# Injected side-effect seams. Keeping these as callables means the monitor +# performs no transport I/O of its own (testable, faithful to the deadline_timer +# / ci_watcher shape). +AlarmFn = Callable[[int], None] +"""Called once (per cooldown) when a runaway burst is detected. Receives the +count of draft PRs opened inside the window. The handler posts the ALARM to +``#agent-team`` and surfaces the operator remediation (stop auto-dispatch); it +does NOT self-stop.""" + +StaleReminderFn = Callable[[DraftPr], None] +"""Called once (per cooldown) per stale draft PR. The handler posts a +``#agent-team`` reminder. It NEVER auto-closes the PR.""" + + +def _utc_now() -> datetime: + """Return the current UTC time (injectable via ``now`` in the sweep).""" + return datetime.now(timezone.utc) + + +def _parse_iso(value: str | None) -> datetime | None: + """Parse an ISO-8601 timestamp to an aware UTC datetime, or ``None``. + + A missing / unparseable timestamp yields ``None`` so the caller fails soft + (skips that PR for the affected check) rather than crashing the sweep. A + naive stamp is treated as UTC (the ledger / GitHub API write UTC) so the + aware/naive compares below never raise. + """ + if not value: + return None + try: + parsed = datetime.fromisoformat(value) + except (TypeError, ValueError): + return None + if parsed.tzinfo is None: + parsed = parsed.replace(tzinfo=timezone.utc) + return parsed + + +def run_draft_pr_monitor( + draft_prs: list[DraftPr], + *, + on_alarm: AlarmFn, + on_stale_reminder: StaleReminderFn, + memory: MonitorMemory | None = None, + alarm_threshold: int = DEFAULT_ALARM_THRESHOLD, + alarm_window: timedelta = DEFAULT_ALARM_WINDOW, + stale_after: timedelta = DEFAULT_STALE_AFTER, + alarm_cooldown: timedelta = DEFAULT_ALARM_COOLDOWN, + stale_cooldown: timedelta = DEFAULT_STALE_COOLDOWN, + now: datetime | None = None, +) -> MonitorReport: + """Run one draft-PR runaway + stale sweep (A4). + + ``draft_prs`` is the set of currently-open draft PRs (the coordinator + supplies them fresh each pass via an injected provider). The sweep: + + 1. **Runaway.** Counts draft PRs whose ``opened_at`` is within ``now - + alarm_window``. If that count is **strictly greater than** + ``alarm_threshold`` (the A4 rule: > 3 within 15 min), call ``on_alarm`` + once — but only if a prior ALARM is older than ``alarm_cooldown`` (flapping + backoff); otherwise record ``ALARM_SUPPRESSED``. The remediation is the + operator's (``systemctl stop``); the monitor never self-stops. + 2. **Stale.** For each draft PR idle longer than ``stale_after`` (``now - + updated_at > stale_after``), call ``on_stale_reminder`` once — but only if + that PR was last reminded longer ago than ``stale_cooldown`` (per-PR + backoff); otherwise record ``STALE_SUPPRESSED``. The monitor never + auto-closes a stale PR. + + Side effects are isolated per condition: an ``on_alarm`` / ``on_stale_reminder`` + that raises yields an ``ERRORED`` outcome (the failure is recorded and the + backoff watermark is NOT advanced, so the next pass retries) and the sweep + continues with the rest of the batch. + + Restart-safety: inputs are read fresh each pass; the only retained state is + ``memory`` (the backoff watermarks). A fresh ``memory`` (e.g. after a reboot) + simply means the first post-reboot breach/stale PR is surfaced again — a + re-notification, never a missed or duplicated *action* (the monitor takes no + infrastructure action). + + Returns a :class:`MonitorReport` describing what happened. + """ + current = now or _utc_now() + mem = memory if memory is not None else MonitorMemory() + report = MonitorReport() + + present_numbers = {pr.number for pr in draft_prs} + + # --- Runaway check ----------------------------------------------------- # + window_start = current - alarm_window + opened_in_window = 0 + for pr in draft_prs: + opened_at = _parse_iso(pr.opened_at) + if opened_at is not None and opened_at >= window_start: + opened_in_window += 1 + report.opened_in_window = opened_in_window + + if opened_in_window > alarm_threshold: + _handle_runaway( + opened_in_window, + on_alarm=on_alarm, + mem=mem, + alarm_cooldown=alarm_cooldown, + now=current, + report=report, + ) + + # --- Stale check ------------------------------------------------------- # + for pr in draft_prs: + updated_at = _parse_iso(pr.updated_at) + if updated_at is None: + # No parseable last-activity stamp -> cannot assess staleness. Skip + # rather than guess (fail-soft); the runaway count is unaffected. + continue + if current - updated_at <= stale_after: + continue + _handle_stale( + pr, + on_stale_reminder=on_stale_reminder, + mem=mem, + stale_cooldown=stale_cooldown, + now=current, + report=report, + ) + + # Prune backoff watermarks for PRs no longer open so the memory cannot grow + # without bound over a long-running daemon. + for number in list(mem.last_reminded_at): + if number not in present_numbers: + del mem.last_reminded_at[number] + + return report + + +def _handle_runaway( + opened_in_window: int, + *, + on_alarm: AlarmFn, + mem: MonitorMemory, + alarm_cooldown: timedelta, + now: datetime, + report: MonitorReport, +) -> None: + """Post (or suppress) a runaway ALARM under the flapping-backoff cooldown.""" + last = mem.last_alarm_at + if last is not None and now - last < alarm_cooldown: + logger.debug( + "draft-pr-monitor: runaway breach (%d in window) suppressed by cooldown", + opened_in_window, + ) + report.outcomes.append( + MonitorOutcome( + action=MonitorAction.ALARM_SUPPRESSED, + detail=str(opened_in_window), + ) + ) + return + + try: + on_alarm(opened_in_window) + except Exception as exc: # noqa: BLE001 - isolate one side-effect failure + logger.exception( + "draft-pr-monitor: runaway ALARM side effect raised; watermark not " + "advanced (will retry next pass)" + ) + report.outcomes.append( + MonitorOutcome( + action=MonitorAction.ERRORED, + detail=str(opened_in_window), + error=f"{type(exc).__name__}: {exc}", + ) + ) + return + + # Advance the watermark ONLY after a successful post so a failed post retries. + mem.last_alarm_at = now + logger.warning( + "draft-pr-monitor: RUNAWAY — %d draft PRs opened within the window; " + "ALARM raised (operator remediation: stop auto-dispatch)", + opened_in_window, + ) + report.outcomes.append( + MonitorOutcome(action=MonitorAction.ALARMED, detail=str(opened_in_window)) + ) + + +def _handle_stale( + pr: DraftPr, + *, + on_stale_reminder: StaleReminderFn, + mem: MonitorMemory, + stale_cooldown: timedelta, + now: datetime, + report: MonitorReport, +) -> None: + """Post (or suppress) a stale reminder for one PR under per-PR backoff.""" + last = mem.last_reminded_at.get(pr.number) + if last is not None and now - last < stale_cooldown: + report.outcomes.append( + MonitorOutcome(action=MonitorAction.STALE_SUPPRESSED, detail=str(pr.number)) + ) + return + + try: + on_stale_reminder(pr) + except Exception as exc: # noqa: BLE001 - isolate one side-effect failure + logger.exception( + "draft-pr-monitor: stale reminder side effect for PR #%s raised; " + "watermark not advanced (will retry next pass)", + pr.number, + ) + report.outcomes.append( + MonitorOutcome( + action=MonitorAction.ERRORED, + detail=str(pr.number), + error=f"{type(exc).__name__}: {exc}", + ) + ) + return + + mem.last_reminded_at[pr.number] = now + report.outcomes.append( + MonitorOutcome(action=MonitorAction.STALE_REMINDED, detail=str(pr.number)) + ) diff --git a/agent-team/ci/README.md b/agent-team/ci/README.md index 2964a2e..1801fc6 100644 --- a/agent-team/ci/README.md +++ b/agent-team/ci/README.md @@ -121,6 +121,42 @@ workflow_dispatch (task_id, diff_artifact_name, expected_diff_hash, declared_sco its own grant explicitly. The trigger is `workflow_dispatch` only — the patch never runs in a context carrying write or secret scope. +## Run-name correlation (box dispatcher → run_id) + +The box-side dispatcher (`agent_team/dispatcher.py`) triggers this workflow with +`gh workflow run`, which does **not** return the resulting run id. The dispatcher +must still resolve that `run_id` so the verifier's read-only CI-result fetcher can +poll the correct run (a `None`/unfound run_id fails closed → the verifier gate +BLOCKs / the task parks; never a vacuous pass). The correlation key is the +workflow **run name**: + +```yaml +run-name: "agent-team-apply ${{ inputs.task_id }}" +``` + +Why the run name and not the workflow input or the head branch: + +- `gh run list --json name` exposes the run name, but **workflow inputs are not + queryable** via the run list, so the `task_id` cannot be matched on the input. +- A `workflow_dispatch` run reports against the **`main` ref**, not the dispatch + head branch, so the branch is not a usable discriminator either. + +So the dispatcher polls `gh run list` read-only, matches the row whose `name` +equals `agent-team-apply ` (mirrored in code by `dispatcher.run_name_for`), +and bounds the match to runs created after the dispatch watermark. The workflow's +`concurrency` group already guarantees a single in-flight run per `task_id`, so +the run name plus the dispatched-at floor identify the dispatched run +unambiguously even under many simultaneous dispatches; the pure +`dispatcher.select_run_id` then applies the anti-stale tie-break (skip a superseded +cancelled run sharing the name, prefer the later-created `databaseId`). + +This `run-name` is **additive**: it adds no job, permission, secret, or trigger, +and changes no privileged step — it only surfaces the dispatching task's id for +correlation. Because the file is nonetheless a CI trust-boundary workflow +(untrusted-input handling), the edit is **flagged for the C1 re-run of +`/sh-security-review` AND the mandatory GPT-4.1 cross-review** before it ships on +this branch, matching the in-YAML `P3-BOX-INTEGRATION` comment. + ## SHA-pinned actions (handbook Pinning Principle, §3.3.2) Every third-party action is pinned to a full commit SHA with the human-readable diff --git a/agent-team/run-team.py b/agent-team/run-team.py index 51b1d4e..e611052 100644 --- a/agent-team/run-team.py +++ b/agent-team/run-team.py @@ -569,6 +569,110 @@ def _build_notifiers( return notify, alarm_hook +# The branch namespace the dispatcher pushes its apply/verify draft PRs under +# (see :func:`agent_team.dispatcher.apply_branch_name` -> ``agent-team/apply/``). +# The runaway/stale monitor is scoped to THIS namespace so it only ever surfaces +# agent-team's own draft PRs, never an unrelated human draft PR in the repo. +_DRAFT_PR_HEAD_PREFIX = "agent-team/apply/" + +# gh's draft-PR enumeration must never block the daemon's tick() loop. A read-only +# ``gh pr list`` is one GET; bound it so a hung gh invocation parks the sweep +# rather than the whole coordinator. +_DRAFT_PR_LIST_TIMEOUT_S = 30 + + +def _default_draft_pr_provider(*, owner: str, repo: str) -> "Callable[[], list[Any]]": + """Build the production READ-ONLY draft-PR provider for the A4 monitor. + + Returns a zero-arg callable that enumerates the currently-open *agent-team* + draft PRs via a single read-only ``gh pr list`` (one GET; it NEVER writes, + closes, or dispatches anything) and maps each into a + :class:`agent_team.draft_pr_monitor.DraftPr` snapshot the monitor consumes. + + The query is scoped to the ``agent-team/apply/`` head namespace + (:data:`_DRAFT_PR_HEAD_PREFIX`) so it only ever sees the dispatcher's own + apply/verify draft PRs — never an unrelated human draft PR. ``--json`` pulls + exactly the three fields the monitor keys off (``number`` / ``createdAt`` -> + ``opened_at`` for the runaway window, ``updatedAt`` -> ``updated_at`` for + staleness). + + Mirrors :func:`agent_team.ci_watcher.default_ci_poller`: a thin closure over + ``owner`` / ``repo`` that fails closed — a non-zero ``gh`` exit, a timeout, or + unparseable JSON yields an empty snapshot (the monitor then no-ops this pass) + rather than raising, so a transient gh hiccup never breaks the tick loop. (The + coordinator's ``_draft_pr_monitor_sweep`` ALSO swallows provider errors, so + this is belt-and-suspenders.) + """ + repo_slug = f"{owner}/{repo}" + + def provider() -> list[Any]: + import json as _json + import subprocess # noqa: PLC0415 - deferred so import needs no gh + + from agent_team.draft_pr_monitor import DraftPr # noqa: PLC0415 + + try: + proc = subprocess.run( # noqa: S603 - args are a fixed, non-shell list + [ + "gh", + "pr", + "list", + "--repo", + repo_slug, + "--draft", + "--state", + "open", + "--search", + f"head:{_DRAFT_PR_HEAD_PREFIX}", + "--json", + "number,createdAt,updatedAt", + "--limit", + "100", + ], + check=True, + capture_output=True, + text=True, + timeout=_DRAFT_PR_LIST_TIMEOUT_S, + ) + except ( + subprocess.CalledProcessError, + subprocess.TimeoutExpired, + OSError, + ): + _LOG.warning( + "draft-pr-monitor: read-only `gh pr list` enumeration failed; " + "treating as no open draft PRs this pass", + exc_info=True, + ) + return [] + + try: + rows = _json.loads(proc.stdout or "[]") + except ValueError: + _LOG.warning( + "draft-pr-monitor: `gh pr list` returned unparseable JSON; " + "treating as no open draft PRs this pass", + exc_info=True, + ) + return [] + + snapshots: list[Any] = [] + for row in rows: + number = row.get("number") + if number is None: + continue + snapshots.append( + DraftPr( + number=int(number), + opened_at=row.get("createdAt"), + updated_at=row.get("updatedAt"), + ) + ) + return snapshots + + return provider + + def _build_coordinator(args: argparse.Namespace) -> Any: """Construct a :class:`Coordinator` for the ``start`` / ``serve`` commands. @@ -669,6 +773,18 @@ def _build_coordinator(args: argparse.Namespace) -> Any: # coordinator. Left unbound on the inert box, the CI sweep stays a NO-OP. if ci_poller is not None: coordinator._ci_pending_provider = coordinator._enumerate_ci_pending + # Draft-PR runaway/stale monitor seam (P3 A4). Gated on the SAME live-pair + # signal as the CI watcher (``ci_poller`` is set ⇔ ``_p3_env_is_configured`` + # via ``failsafe_production_p3_wiring``), so on the inert/unconfigured box it + # stays None and ``_draft_pr_monitor_sweep`` is a NO-OP (no behaviour change). + # On the live box it binds a read-only ``gh pr list`` enumerator of the open + # ``agent-team/apply/`` draft PRs so the sweep has a real snapshot to ALARM / + # remind on — without this the wired sweep would always see no PRs. Bound + # post-construction to mirror ``_ci_pending_provider``. + if ci_poller is not None: + coordinator._draft_pr_provider = _default_draft_pr_provider( + owner=owner, repo=repo + ) # WS2: an allowlisted Slack /new-task starts a task on THIS coordinator. Set # post-construction (the adapter closes over the just-built coordinator), and # before serve() builds the listener. AUTHZ-01 (owner allowlist) gates this diff --git a/agent-team/scripts/assert_no_write_token.py b/agent-team/scripts/assert_no_write_token.py new file mode 100644 index 0000000..a561772 --- /dev/null +++ b/agent-team/scripts/assert_no_write_token.py @@ -0,0 +1,292 @@ +#!/usr/bin/env python3 +"""assert_no_write_token.py — fail closed if a GitHub *write* token is on the box. + +The agent-team apply/verify path mints its ``pull-requests: write`` GitHub App +installation token **inside the CI runner** (via SHA-pinned +``actions/create-github-app-token``), from the ``AGENT_APPLY_APP_ID`` / +``AGENT_APPLY_APP_PRIVATE_KEY`` **Actions secrets**. By design the always-on +R720 box holds **no standing write credential**: it triggers CI with the +operator's host ``gh`` auth and reads results with a *read-only* token +(``AGENT_TEAM_CI_READ_TOKEN`` / ``GITHUB_TOKEN``). See ``ci/README.md`` and +``docs/P3-PHASE0-DESIGN.md`` ("the box holds no standing write token"). + +This audit asserts that invariant. It is runnable both on the box (as a +provisioning/runtime self-check) and in CI (as a regression guard). It: + + 1. scans the live process environment (``os.environ``) and the coordinator's + environment-derived config for token-shaped variables that would grant + ``pull-requests: write`` / ``contents: write``; + 2. greps the box's local credential files (``~/secrev.env`` and + ``~/orchestrator/.env`` by default; paths are configurable) for the App id + and for any PEM private-key header (RSA / EC / OPENSSH / PKCS#8); + +and **exits non-zero with a clear message** if any are found, or exits ``0`` +with a short summary otherwise. + +Usage:: + + python -m scripts.assert_no_write_token + python scripts/assert_no_write_token.py --env-file ~/secrev.env --env-file ~/x.env + +Exit codes: + 0 no write-shaped token / App secret / private key found + 1 at least one finding (the box is mis-provisioned — remediate before deploy) +""" + +from __future__ import annotations + +import argparse +import os +import re +import sys +from collections.abc import Mapping, Sequence +from pathlib import Path + +# --------------------------------------------------------------------------- # +# What "must never live on the box" looks like. +# --------------------------------------------------------------------------- # + +# The GitHub App that holds ``pull-requests: write`` lives ONLY as Actions +# secrets. Its id and private key must never appear on the box (env or files). +APP_ID_ENV = "AGENT_APPLY_APP_ID" +APP_PRIVATE_KEY_ENV = "AGENT_APPLY_APP_PRIVATE_KEY" + +# Env vars that are explicitly the App's write credentials. +_FORBIDDEN_ENV_VARS: frozenset[str] = frozenset( + { + APP_ID_ENV, + APP_PRIVATE_KEY_ENV, + } +) + +# Env vars that are KNOWN-GOOD read-only / non-write and must NOT be flagged by +# the heuristic name match below (they are tokens, but read-only by contract). +_ALLOWED_TOKEN_ENV_VARS: frozenset[str] = frozenset( + { + "AGENT_TEAM_CI_READ_TOKEN", # read-only CI-result fetcher token + "AGENT_TEAM_API_TOKEN", # read-only dashboard/API bearer + "SLACK_APP_TOKEN", # Slack socket-mode app-level token (not GitHub) + "SLACK_BOT_TOKEN", # Slack bot token (not GitHub) + "CLAUDE_CODE_OAUTH_TOKEN", # Anthropic subscription OAuth (not GitHub) + "ANTHROPIC_API_KEY", # model API key (not GitHub) + } +) + +# A name-shaped heuristic for "this looks like a GitHub *write* token". We +# deliberately scope to GitHub-write shapes so the generic read-only +# ``GITHUB_TOKEN`` fallback (a runtime read token, not a standing write secret) +# is not a false positive while the App's write material always is. +_WRITE_TOKEN_NAME_RE = re.compile( + r"(?:^|_)(?:GH|GITHUB)_(?:APP|PAT|WRITE|APPLY)_?(?:TOKEN|KEY|PRIVATE_KEY)?", + re.IGNORECASE, +) + +# Value shapes that indicate a GitHub write-capable credential regardless of the +# var's name. ``ghp_`` (classic PAT) and ``github_pat_`` (fine-grained PAT) can +# both carry write scopes; an installation token (``ghs_``) is write-capable; a +# user-to-server token (``ghu_``) acts with the user's write access; and a refresh +# token (``ghr_``) mints fresh write-capable user-to-server tokens. All are +# write-risk material that must not live on the box. +_WRITE_TOKEN_VALUE_RE = re.compile( + r"\b(?:ghp_|ghs_|ghu_|ghr_|github_pat_)[A-Za-z0-9_]{20,}\b" +) + +# Any PEM private-key header (RSA / EC / OPENSSH / generic PKCS#8). The App's +# private key is a PEM block; finding ANY private key in a box credential file +# is a finding. +_PRIVATE_KEY_RE = re.compile(r"-----BEGIN (?:[A-Z0-9]+ )*PRIVATE KEY-----") + +# Default box credential files to grep. Configurable via --env-file / the +# ``ASSERT_NO_WRITE_TOKEN_ENV_FILES`` env var so tests use temp files. +_DEFAULT_ENV_FILES: tuple[str, ...] = ("~/secrev.env", "~/orchestrator/.env") + + +# --------------------------------------------------------------------------- # +# Scanners. Each returns a list of human-readable finding strings. +# --------------------------------------------------------------------------- # + + +def scan_environ(environ: Mapping[str, str]) -> list[str]: + """Scan a process environment for write-shaped GitHub token material. + + Flags (a) the explicit App-credential env vars, (b) any var whose *name* + matches the GitHub-write heuristic, and (c) any var whose *value* carries a + write-capable GitHub token prefix. Known read-only tokens are exempt. + """ + findings: list[str] = [] + for name, value in environ.items(): + if name in _ALLOWED_TOKEN_ENV_VARS: + continue + if name in _FORBIDDEN_ENV_VARS: + findings.append( + f"environment variable {name!r} is set — the App write " + "credential must live ONLY as an Actions secret, never on the box" + ) + continue + if _WRITE_TOKEN_NAME_RE.search(name): + findings.append( + f"environment variable {name!r} has a GitHub write-token-shaped " + "name; the box must hold no standing write token" + ) + continue + if value and _WRITE_TOKEN_VALUE_RE.search(value): + findings.append( + f"environment variable {name!r} holds a write-capable GitHub " + "token value (ghp_/ghs_/ghu_/ghr_/github_pat_ prefix)" + ) + return findings + + +def scan_config(config: Mapping[str, object] | None) -> list[str]: + """Scan a coordinator config mapping for write-shaped token material. + + The coordinator is environment-driven, so this is normally a thin pass over + whatever config dict a caller hands in (string values only). It applies the + same name/value heuristics as :func:`scan_environ`. + """ + if not config: + return [] + findings: list[str] = [] + for key, raw in config.items(): + name = str(key) + if name in _ALLOWED_TOKEN_ENV_VARS: + continue + if name in _FORBIDDEN_ENV_VARS or _WRITE_TOKEN_NAME_RE.search(name): + findings.append( + f"coordinator config key {name!r} is a GitHub write-token-shaped " + "key; the box config must hold no standing write token" + ) + continue + if isinstance(raw, str) and _WRITE_TOKEN_VALUE_RE.search(raw): + findings.append( + f"coordinator config key {name!r} holds a write-capable GitHub " + "token value (ghp_/ghs_/ghu_/ghr_/github_pat_ prefix)" + ) + return findings + + +def scan_env_file(path: Path, *, app_id: str | None = None) -> list[str]: + """Grep one credential file for the App id and any PEM private-key header. + + A non-existent file is **not** a finding (the box legitimately may not have + every file). An unreadable-but-present file is reported as a finding so a + permissions mistake can't silently mask leaked material. + + When ``app_id`` is given (the configured ``AGENT_APPLY_APP_ID`` value), the + grep also flags that literal id appearing in the file. The + ``AGENT_APPLY_APP_ID`` / ``AGENT_APPLY_APP_PRIVATE_KEY`` *names* are always + flagged regardless. + """ + findings: list[str] = [] + if not path.exists(): + return findings + try: + text = path.read_text(encoding="utf-8", errors="replace") + except OSError as exc: # present but unreadable — fail loud, not silent + return [ + f"could not read credential file {path} ({exc}); cannot prove it is clean" + ] + + for lineno, line in enumerate(text.splitlines(), start=1): + if APP_ID_ENV in line or APP_PRIVATE_KEY_ENV in line: + findings.append( + f"{path}:{lineno}: references the App credential " + f"({APP_ID_ENV}/{APP_PRIVATE_KEY_ENV}); it must live ONLY as an " + "Actions secret" + ) + if app_id and app_id in line: + findings.append( + f"{path}:{lineno}: contains the configured App id value; the App " + "id must not be present on the box" + ) + + if _PRIVATE_KEY_RE.search(text): + findings.append( + f"{path}: contains a PEM private-key block " + "(-----BEGIN ... PRIVATE KEY-----); no private key may live on the box" + ) + return findings + + +# --------------------------------------------------------------------------- # +# Orchestration. +# --------------------------------------------------------------------------- # + + +def _resolve_env_files( + cli_files: Sequence[str] | None, environ: Mapping[str, str] +) -> list[Path]: + """Resolve the credential files to grep (CLI > env var > defaults).""" + if cli_files: + raw = list(cli_files) + elif environ.get("ASSERT_NO_WRITE_TOKEN_ENV_FILES"): + raw = [ + p.strip() + for p in environ["ASSERT_NO_WRITE_TOKEN_ENV_FILES"].split(os.pathsep) + if p.strip() + ] + else: + raw = list(_DEFAULT_ENV_FILES) + return [Path(p).expanduser() for p in raw] + + +def audit( + *, + environ: Mapping[str, str] | None = None, + config: Mapping[str, object] | None = None, + env_files: Sequence[str] | None = None, +) -> list[str]: + """Run every scanner and return the combined list of findings (empty == clean).""" + environ = os.environ if environ is None else environ + findings: list[str] = [] + findings += scan_environ(environ) + findings += scan_config(config) + app_id = environ.get(APP_ID_ENV) or None + for path in _resolve_env_files(env_files, environ): + findings += scan_env_file(path, app_id=app_id) + return findings + + +def main(argv: Sequence[str] | None = None) -> int: + parser = argparse.ArgumentParser( + description=( + "Assert no pull-requests:write / contents:write GitHub token (or the " + "apply App's id/private key) is present on this box." + ) + ) + parser.add_argument( + "--env-file", + action="append", + dest="env_files", + metavar="PATH", + help=( + "Credential file to grep for the App id + private keys (repeatable). " + "Defaults to ~/secrev.env and ~/orchestrator/.env, or the os.pathsep-" + "separated ASSERT_NO_WRITE_TOKEN_ENV_FILES env var." + ), + ) + args = parser.parse_args(argv) + + findings = audit(env_files=args.env_files) + + if findings: + print( + "FAIL: write-capable GitHub credential material found on the box " + f"({len(findings)} finding(s)). The pull-requests:write App token " + "must only ever live as an Actions secret:", + file=sys.stderr, + ) + for f in findings: + print(f" - {f}", file=sys.stderr) + return 1 + + print( + "OK: no pull-requests:write / contents:write token, App id, or private " + "key found in the environment, coordinator config, or credential files. " + "The box holds no standing write token." + ) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/agent-team/scripts/p3_rollback.sh b/agent-team/scripts/p3_rollback.sh new file mode 100755 index 0000000..c75efc1 --- /dev/null +++ b/agent-team/scripts/p3_rollback.sh @@ -0,0 +1,865 @@ +#!/usr/bin/env bash +# p3_rollback.sh — restore EVERY privileged P3 surface to a recorded baseline. +# +# The P3 apply/verify CI workflow is ALREADY LIVE (flipped + provisioned +# 2026-06-22): the `agent-apply` environment with its required reviewer, the +# GitHub App installation (pull-requests: write), the run-name/permissions edits +# on the workflow, and branch protection on `main`. This script is the inverse: +# it takes a recorded baseline of those surfaces and restores them, then asserts +# post-restore == baseline. It is what you run when the flip must be unwound, or +# when a premature flip RAN and opened a draft PR that should not exist. +# +# DESTRUCTIVE-SAFE BY DEFAULT. Every gh/git mutation is held behind a --dry-run +# default that only PRINTS the plan. --apply performs the mutations. Nothing +# destructive happens without --apply on the command line. +# +# KNOWN LIMITATIONS — VALIDATE LIVE BEFORE RELYING ON --apply (C1 gate). These +# GitHub REST calls are exercised only against a stubbed `gh` in tests; their +# exact live shapes MUST be confirmed with a real `--dry-run` against the target +# repo/App during the mandatory C1 /sh-security-review + GPT-4.1 cross-review +# (this script touches IAM-adjacent surfaces): +# * Branch-protection RESTORE: the recorded baseline is the verbatim GET object, +# but GitHub's branch-protection GET and PUT schemas are NOT symmetric +# (GET enforce_admins is {enabled,url} vs PUT's bare bool; GET carries +# read-only url/checks fields PUT rejects; PUT requires a `restrictions` key +# GET may omit). A real PUT of the raw GET object can 422 — transform GET->PUT +# (enforce_admins->bool, required_status_checks->{strict,contexts}, +# required_pull_request_reviews->writable subkeys, restrictions->null if +# absent) and confirm against the live API before --apply. +# * App neutralise is uninstall (DELETE /app/installations/{id}) which needs an +# App JWT ($AGENT_APPLY_APP_JWT), NOT operator gh; --apply hard-fails without +# it rather than pretending operator gh can do it. +# * environment.deployment_branch_policy is recorded from the live env GET; if a +# custom branch-name policy is in use, confirm the restore preserves it. +# The post-restore protection assert IS real (normalize_protection reads argv, +# fails loud on parse error, and a divergent live state exits non-zero — see +# tests/test_rollback.py::test_apply_protection_DETECTS_divergent_live_restore). +# +# Surfaces restored (design: docs/P3-PHASE0-DESIGN.md; the workflow: +# .github/workflows/agent-team-apply-verify.yml): +# 1. workflow — the apply/verify flip. +# PRE-merge: close the flip PR + delete its branch. +# POST-merge: git revert the flip commit + push + re-run CI. +# LIVE: the run-name/permissions edits are already on +# the branch, so also revert them to the recorded +# baseline SHA (`workflow_baseline_sha`). +# 2. environment — the `agent-apply` environment: required reviewer(s) + +# deployment branch policy. Reviewers are restored BY NUMERIC +# USER ID (logins are resolved to ids when the baseline only +# recorded logins), via a proper JSON reviewers array. +# 3. app — neutralise the GitHub App. There is NO "reduce App +# permissions" REST endpoint (PATCH .../permissions is a 404), +# and an installation token cannot be revoked by operator gh +# (DELETE /installation/token revokes the token you +# authenticate WITH — operator host gh is not that token). +# The only programmatic neutralise is UNINSTALL +# (DELETE /app/installations/{id}), which requires an APP JWT +# (NOT operator gh). The fallback is an OUT-OF-BAND operator +# action (remove the install in the org UI / rotate the App +# private key). This script PLANS those steps and honours +# --apply only for the JWT-authenticated uninstall. +# 4. protection — branch protection on `main`; restored from the FULL recorded +# protection baseline (not just enforce_admins) and asserted. +# Baseline asserts include_administrators == ON. +# 5. incident — "premature flip that RAN": neutralise the App (uninstall via +# App JWT, or out-of-band — see surface 3), revert any draft +# PR/branch the App opened, audit the Checks trail, restore +# environment + protection, and file an incident note. +# +# Usage: +# scripts/p3_rollback.sh [--apply] [--baseline FILE] [options] +# +# ∈ { workflow | environment | app | protection | all | incident } +# +# Options: +# --record-baseline capture the CURRENT live state into the baseline JSON +# (workflow SHA, env reviewer ids, full branch protection, +# App installation id) instead of restoring. Honours +# --apply (default --dry-run just prints what it would +# capture). +# --apply perform the mutations (default is --dry-run: print only) +# --dry-run print the plan only (default; explicit for clarity) +# --baseline FILE path to the recorded baseline JSON (default: +# $P3_ROLLBACK_BASELINE or .security-review/p3-baseline.json) +# --repo OWNER/NAME target repo (default: $AGENT_TEAM_REPO_OWNER/_NAME) +# --env-name NAME environment name for --record-baseline (default agent-apply) +# --app-installation-id N App installation id to record for --record-baseline +# --flip-pr N the flip PR number (pre-merge close path) +# --flip-branch REF the flip PR head branch (pre-merge delete path) +# --merged the flip is already merged (post-merge revert path) +# --flip-commit SHA the merged flip commit to revert (post-merge path) +# -h | --help show this help and exit +# +# Baseline JSON (recorded BEFORE the flip; the restore target). Keys consumed: +# { +# "repo": "Sea-Haven-Industries/orchestrator", +# "default_branch": "main", +# "workflow_path": ".github/workflows/agent-team-apply-verify.yml", +# "workflow_baseline_sha": "", +# "environment": { +# "name": "agent-apply", +# // Prefer NUMERIC ids (restore is exact + assertable). Logins are also +# // accepted and resolved to ids at restore via `gh api users/{login}`. +# "required_reviewer_ids": [1234567], +# "required_reviewers": ["amoussa1229"], +# "deployment_branch_policy": "protected" +# }, +# "app": { +# "slug": "agent-apply", +# "installation_id": 0, +# // The ONLY programmatic neutralise is "uninstall" (App JWT). There is no +# // permission-reduction REST endpoint, so "reduce" is not an apply action — +# // it degrades to an out-of-band operator instruction. +# "action": "uninstall" // "uninstall" | "out-of-band" +# }, +# "protection": { +# "branch": "main", +# "include_administrators": true, +# // The FULL protection payload recorded pre-flip (the exact body to PUT +# // back to repos/{repo}/branches/{branch}/protection). Captured verbatim +# // by --record-baseline. enforce_admins is also asserted on its own. +# "full": { ... }, +# "required_status_checks": ["guard", "build-test"] +# } +# } +# +# Record a baseline FIRST (before the flip) with: +# scripts/p3_rollback.sh --record-baseline [--baseline FILE] [--repo OWNER/NAME] \ +# [--env-name agent-apply] [--app-installation-id N] +# which captures the live workflow SHA, env reviewer ids, full branch protection, +# and App installation id into the baseline JSON (creating .security-review/ if +# absent). The restore surfaces then read from it. +# +# Exit non-zero on any failure (set -euo pipefail). Requires bash + gh + (git for +# the post-merge revert path). No standing box token is used — run from the Mac / +# operator host where `gh` is authenticated. The App UNINSTALL step additionally +# needs an APP JWT (NOT operator gh) — see surface 3. +set -euo pipefail + +# ── defaults ───────────────────────────────────────────────────────────────── +APPLY=0 +RECORD_BASELINE=0 +BASELINE="${P3_ROLLBACK_BASELINE:-.security-review/p3-baseline.json}" +REPO_DEFAULT="" +if [ -n "${AGENT_TEAM_REPO_OWNER:-}" ] && [ -n "${AGENT_TEAM_REPO_NAME:-}" ]; then + REPO_DEFAULT="${AGENT_TEAM_REPO_OWNER}/${AGENT_TEAM_REPO_NAME}" +fi +REPO="${REPO_DEFAULT}" +SURFACE="" +ENV_NAME="agent-apply" +APP_INSTALLATION_ID="" +FLIP_PR="" +FLIP_BRANCH="" +FLIP_COMMIT="" +MERGED=0 + +PROG="$(basename "$0")" + +say() { printf '\n\033[1;36m== %s\033[0m\n' "$*"; } +plan() { printf ' \033[1;33m[PLAN]\033[0m %s\n' "$*"; } +do_() { printf ' \033[1;32m[APPLY]\033[0m %s\n' "$*"; } +err() { printf '\033[1;31m!! %s\033[0m\n' "$*" >&2; } + +usage() { + # Print the leading comment block (everything up to the first blank line after + # `set -euo pipefail`) as help. Kept simple: emit a concise synopsis. + cat < [--apply] [--baseline FILE] [options] + +Surfaces: + workflow restore the apply/verify flip (pre/post-merge + live revert) + environment restore the agent-apply environment (required reviewer ids + policy) + app neutralise the GitHub App (uninstall via App JWT, or out-of-band) + protection restore the FULL branch protection baseline (include_administrators ON) + all run workflow + environment + app + protection in order + incident premature-flip-that-RAN path (neutralise App, revert PR, audit, note) + +Options: + --record-baseline capture current live state into the baseline JSON instead + of restoring (honours --apply) + --apply perform mutations (default: --dry-run, print plan only) + --dry-run print plan only (default) + --baseline FILE recorded baseline JSON (default: \$P3_ROLLBACK_BASELINE + or .security-review/p3-baseline.json) + --repo OWNER/NAME target repo (default: \$AGENT_TEAM_REPO_OWNER/_NAME) + --env-name NAME environment name for --record-baseline (default agent-apply) + --app-installation-id N App installation id to record for --record-baseline + --flip-pr N flip PR number (pre-merge close path) + --flip-branch REF flip PR head branch (pre-merge delete path) + --merged flip already merged (post-merge revert path) + --flip-commit SHA merged flip commit to revert (post-merge path) + -h, --help show this help +USAGE +} + +# ── arg parsing ────────────────────────────────────────────────────────────── +# A no-op-safe parser: a single positional surface, then flags. Unknown flags +# are a hard error (fail closed — never silently ignore an option that would +# change destructive behaviour). +parse_args() { + while [ "$#" -gt 0 ]; do + case "$1" in + workflow|environment|app|protection|all|incident) + if [ -n "${SURFACE}" ]; then + err "surface already set to '${SURFACE}'; got extra '$1'"; return 2 + fi + SURFACE="$1" ;; + --record-baseline) RECORD_BASELINE=1 ;; + --apply) APPLY=1 ;; + --dry-run) APPLY=0 ;; + --env-name) shift; ENV_NAME="${1:-}"; [ -n "${ENV_NAME}" ] || { err "--env-name needs a value"; return 2; } ;; + --env-name=*) ENV_NAME="${1#*=}" ;; + --app-installation-id) shift; APP_INSTALLATION_ID="${1:-}"; [ -n "${APP_INSTALLATION_ID}" ] || { err "--app-installation-id needs a value"; return 2; } ;; + --app-installation-id=*) APP_INSTALLATION_ID="${1#*=}" ;; + --baseline) shift; BASELINE="${1:-}"; [ -n "${BASELINE}" ] || { err "--baseline needs a value"; return 2; } ;; + --baseline=*) BASELINE="${1#*=}" ;; + --repo) shift; REPO="${1:-}"; [ -n "${REPO}" ] || { err "--repo needs a value"; return 2; } ;; + --repo=*) REPO="${1#*=}" ;; + --flip-pr) shift; FLIP_PR="${1:-}"; [ -n "${FLIP_PR}" ] || { err "--flip-pr needs a value"; return 2; } ;; + --flip-pr=*) FLIP_PR="${1#*=}" ;; + --flip-branch) shift; FLIP_BRANCH="${1:-}"; [ -n "${FLIP_BRANCH}" ] || { err "--flip-branch needs a value"; return 2; } ;; + --flip-branch=*) FLIP_BRANCH="${1#*=}" ;; + --flip-commit) shift; FLIP_COMMIT="${1:-}"; [ -n "${FLIP_COMMIT}" ] || { err "--flip-commit needs a value"; return 2; } ;; + --flip-commit=*) FLIP_COMMIT="${1#*=}" ;; + --merged) MERGED=1 ;; + -h|--help) usage; exit 0 ;; + *) err "unknown argument: $1"; usage >&2; return 2 ;; + esac + shift + done + # --record-baseline does not take a surface; restore modes require one. + if [ "${RECORD_BASELINE}" = "1" ]; then + if [ -n "${SURFACE}" ]; then + err "--record-baseline does not take a surface (got '${SURFACE}')"; return 2 + fi + return 0 + fi + if [ -z "${SURFACE}" ]; then + err "a surface is required (workflow|environment|app|protection|all|incident)" + usage >&2 + return 2 + fi +} + +# ── helpers ────────────────────────────────────────────────────────────────── +# jget KEY — read a value from the baseline JSON via gh's bundled jq-less reader. +# We use `gh` only for API; for JSON parsing we prefer python3 (stdlib) so there +# is no jq dependency and behaviour is identical in tests. +jget() { + local expr="$1" + python3 - "$BASELINE" "$expr" <<'PY' +import json, sys +path, expr = sys.argv[1], sys.argv[2] +try: + with open(path, encoding="utf-8") as fh: + data = json.load(fh) +except FileNotFoundError: + print("", end="") + sys.exit(0) +cur = data +for part in expr.split("."): + if part == "": + continue + if isinstance(cur, dict) and part in cur: + cur = cur[part] + else: + cur = None + break +if cur is None: + print("", end="") +elif isinstance(cur, bool): + print("true" if cur else "false", end="") +elif isinstance(cur, (list, dict)): + print(json.dumps(cur), end="") +else: + print(cur, end="") +PY +} + +require_baseline() { + if [ ! -f "${BASELINE}" ]; then + err "baseline file not found: ${BASELINE} (record it BEFORE the flip)" + return 1 + fi + local repo_from_baseline + repo_from_baseline="$(jget repo)" + if [ -z "${REPO}" ]; then + REPO="${repo_from_baseline}" + fi + if [ -z "${REPO}" ]; then + err "no target repo: pass --repo OWNER/NAME or set repo in the baseline" + return 1 + fi + # If both are present they must agree — refuse to restore against a repo the + # baseline was not recorded for (fail closed: a mismatched restore is worse + # than no restore). + if [ -n "${repo_from_baseline}" ] && [ "${repo_from_baseline}" != "${REPO}" ]; then + err "repo mismatch: baseline=${repo_from_baseline} requested=${REPO}" + return 1 + fi +} + +# run_or_plan "" gh ... — print the plan; only execute on --apply. +run_or_plan() { + local desc="$1"; shift + if [ "${APPLY}" = "1" ]; then + do_ "${desc}" + "$@" + else + plan "${desc}: $*" + fi +} + +# assert_equal EXPECTED ACTUAL CONTEXT — fail closed on a post-restore mismatch. +assert_equal() { + local expected="$1" actual="$2" ctx="$3" + if [ "${expected}" != "${actual}" ]; then + err "POST-RESTORE ASSERT FAILED [${ctx}]: expected '${expected}', got '${actual}'" + return 1 + fi + printf ' \033[1;32m[OK]\033[0m %s == %s (%s)\n' "${expected}" "${actual}" "${ctx}" +} + +# ── surface 1: workflow flip ───────────────────────────────────────────────── +restore_workflow() { + say "1. Restore the apply/verify workflow flip" + local wf_path baseline_sha + wf_path="$(jget workflow_path)" + [ -n "${wf_path}" ] || wf_path=".github/workflows/agent-team-apply-verify.yml" + baseline_sha="$(jget workflow_baseline_sha)" + + if [ "${MERGED}" = "1" ]; then + # POST-merge: revert the flip commit, push, re-run CI. + if [ -z "${FLIP_COMMIT}" ]; then + err "post-merge path needs --flip-commit SHA"; return 1 + fi + run_or_plan "git revert the merged flip commit ${FLIP_COMMIT}" \ + git revert --no-edit "${FLIP_COMMIT}" + run_or_plan "push the revert to ${REPO}" \ + git push origin HEAD + run_or_plan "re-run CI on the revert for ${REPO}" \ + gh workflow run --repo "${REPO}" "$(basename "${wf_path}")" + else + # PRE-merge: close the flip PR + delete its branch. + if [ -z "${FLIP_PR}" ]; then + err "pre-merge path needs --flip-pr N"; return 1 + fi + run_or_plan "close the flip PR #${FLIP_PR} on ${REPO}" \ + gh pr close "${FLIP_PR}" --repo "${REPO}" \ + --comment "Reverting the P3 apply/verify flip to baseline." --delete-branch + if [ -n "${FLIP_BRANCH}" ]; then + run_or_plan "delete the flip head branch ${FLIP_BRANCH}" \ + gh api -X DELETE "repos/${REPO}/git/refs/heads/${FLIP_BRANCH}" + fi + fi + + # LIVE: the run-name/permissions edits are already on the branch — revert the + # workflow file to the recorded baseline SHA so the live YAML matches baseline. + if [ -n "${baseline_sha}" ]; then + run_or_plan "revert ${wf_path} to baseline SHA ${baseline_sha}" \ + git checkout "${baseline_sha}" -- "${wf_path}" + else + plan "(no workflow_baseline_sha recorded — skipping live YAML revert)" + fi +} + +# resolve_reviewer_ids — emit a sorted, space-separated list of NUMERIC user ids +# for the environment's required reviewers. Prefers baseline-recorded ids +# (environment.required_reviewer_ids); for any baseline that only recorded logins +# (environment.required_reviewers), resolve login->id via `gh api users/{login}`. +# Sorting makes the post-restore compare order-independent. +resolve_reviewer_ids() { + local ids logins login id + ids="$(jget environment.required_reviewer_ids)" # JSON array of ints, or "" + logins="$(jget environment.required_reviewers)" # JSON array of strings, or "" + + python3 - "$ids" "$logins" <<'PY' +import json, sys +ids_raw, logins_raw = sys.argv[1], sys.argv[2] +out = [] +if ids_raw: + out = [str(int(x)) for x in json.loads(ids_raw)] +elif logins_raw: + # Marker: logins need resolving. Print each login prefixed so the caller + # can resolve via gh (we can't shell out from inside python here). + for login in json.loads(logins_raw): + print("LOGIN:" + str(login)) + sys.exit(0) +print(" ".join(sorted(out, key=int))) +PY +} + +# reviewers_put_args ID... — echo the repeatable -F args for the reviewers array +# in the exact field form gh expects: -F 'reviewers[][type]=User' -F 'reviewers[][id]=N' +reviewers_put_args() { + local id + for id in "$@"; do + printf -- '-F\nreviewers[][type]=User\n-F\nreviewers[][id]=%s\n' "${id}" + done +} + +# ── surface 2: agent-apply environment ─────────────────────────────────────── +restore_environment() { + say "2. Restore the agent-apply environment (required reviewer ids + policy)" + local env_name policy raw line id ids + env_name="$(jget environment.name)" + [ -n "${env_name}" ] || env_name="agent-apply" + policy="$(jget environment.deployment_branch_policy)" + + # Resolve the baseline reviewer set to NUMERIC ids. If the baseline only + # recorded logins, resolve each login->id via `gh api users/{login}`. + raw="$(resolve_reviewer_ids)" + ids="" + if printf '%s' "${raw}" | grep -q '^LOGIN:'; then + while IFS= read -r line; do + [ -n "${line}" ] || continue + local login="${line#LOGIN:}" + if [ "${APPLY}" = "1" ]; then + id="$(gh api "users/${login}" --jq '.id' 2>/dev/null || true)" + if [ -z "${id}" ]; then + err "could not resolve reviewer login '${login}' to a numeric id"; return 1 + fi + else + # Dry-run: we don't hit the network. Show the resolution we WOULD do. + plan "resolve reviewer login '${login}' -> id via 'gh api users/${login} --jq .id'" + id="" + fi + ids="${ids:+${ids} }${id}" + done </dev/null || true)" + assert_equal "${ids}" "${got_ids}" "environment.required_reviewer_ids" + else + plan "(post-restore: assert LIVE env '${env_name}' reviewer ids == baseline [${ids}])" + fi +} + +# ── surface 3: GitHub App installation ─────────────────────────────────────── +# Honest model of what's actually possible against the GitHub REST API: +# +# * You CANNOT "rotate" the App's installation token from operator gh. +# DELETE /installation/token revokes the token you authenticate WITH — i.e. +# the installation token itself, not some other installation's token. The +# operator host `gh` (user OAuth / PAT) is NOT that token, so it returns 403. +# A leaked installation token is short-lived (<=1h) and self-expires; to kill +# it sooner you must either run that DELETE *with that token* (inside the +# runner) or neutralise the source of new tokens (below). +# +# * There is NO permission-reduction REST endpoint. +# PATCH /app/installations/{id}/permissions does NOT exist (404). Installation +# permissions are reduced only by editing the App in the UI/settings API and +# re-accepting, which is an out-of-band operator action. +# +# * The only programmatic NEUTRALISE is UNINSTALL: +# DELETE /app/installations/{id} — which requires an APP JWT (NOT operator gh, +# NOT an installation token, NOT a fine-grained PAT). Provide the JWT via +# $AGENT_APPLY_APP_JWT; this surface only --apply's the uninstall when it is +# set. Without it, the step degrades to an out-of-band instruction. +restore_app() { + say "3. Neutralise the GitHub App installation" + local action installation_id slug + action="$(jget app.action)" + [ -n "${action}" ] || action="uninstall" + installation_id="$(jget app.installation_id)" + slug="$(jget app.slug)" + + # Be explicit that operator gh cannot revoke the installation token. + plan "(installation token: cannot be revoked by operator gh — DELETE /installation/token" + plan " revokes the token you AUTHENTICATE WITH; the short-lived install token self-expires." + plan " To kill it sooner, run that DELETE inside the runner with that token, or uninstall below.)" + + case "${action}" in + uninstall) + if [ -z "${installation_id}" ] || [ "${installation_id}" = "0" ]; then + err "uninstall requested but no app.installation_id in baseline"; return 1 + fi + # DELETE /app/installations/{id} requires an APP JWT — NOT operator gh. + if [ -n "${AGENT_APPLY_APP_JWT:-}" ]; then + run_or_plan "UNINSTALL App '${slug}' installation ${installation_id} (requires APP JWT)" \ + gh api -X DELETE "app/installations/${installation_id}" \ + -H "Authorization: Bearer ${AGENT_APPLY_APP_JWT}" + else + # No JWT available: never pretend operator gh can do this. Plan only. + plan "UNINSTALL App '${slug}' installation ${installation_id} requires an APP JWT" + plan " (set \$AGENT_APPLY_APP_JWT). Without it: do it OUT-OF-BAND —" + plan " remove the installation in the org UI, or run:" + plan " gh api -X DELETE app/installations/${installation_id} -H 'Authorization: Bearer '" + if [ "${APPLY}" = "1" ]; then + err "uninstall needs an APP JWT (\$AGENT_APPLY_APP_JWT) — operator gh cannot do this; do it out-of-band" + return 1 + fi + fi + ;; + out-of-band) + # Honest: no API path. Tell the operator exactly what to do by hand. + plan "OUT-OF-BAND App neutralise for '${slug}' (no REST endpoint reduces App permissions):" + plan " - remove the installation in the org UI (Settings > GitHub Apps > Configure > Uninstall), AND/OR" + plan " - rotate the App private key out-of-band (App settings > Generate a new private key," + plan " update the AGENT_APPLY_APP_PRIVATE_KEY Actions secret), which invalidates new-token minting." + if [ "${APPLY}" = "1" ]; then + err "app.action 'out-of-band' has no API path — perform the steps above by hand, then re-run --apply with a recorded uninstall" + return 1 + fi + ;; + *) + err "unknown app.action '${action}' (expected uninstall|out-of-band)"; return 1 ;; + esac +} + +# ── surface 4: branch protection ───────────────────────────────────────────── +restore_protection() { + say "4. Restore the FULL branch protection baseline (include_administrators ON)" + local branch include_admins full + branch="$(jget protection.branch)" + [ -n "${branch}" ] || branch="$(jget default_branch)" + [ -n "${branch}" ] || branch="main" + include_admins="$(jget protection.include_administrators)" + full="$(jget protection.full)" # the full protection payload, captured verbatim + + # The baseline MUST require admins to be included — a rollback that left admins + # exempt would be a weaker posture than baseline. Fail closed otherwise. + if [ "${include_admins}" != "true" ]; then + err "baseline protection.include_administrators is not true (got '${include_admins}'); refusing" + return 1 + fi + + # Restore the FULL protection object (not only enforce_admins): one PUT to + # repos/{repo}/branches/{branch}/protection with the recorded body. This rebuilds + # required_status_checks, required_pull_request_reviews, restrictions, etc. + if [ -n "${full}" ] && [ "${full}" != "null" ]; then + if [ "${APPLY}" = "1" ]; then + do_ "PUT full branch protection on ${branch} from baseline protection.full" + printf '%s' "${full}" | gh api -X PUT \ + "repos/${REPO}/branches/${branch}/protection" --input - + else + plan "PUT full branch protection on ${branch} from baseline protection.full: ${full}" + fi + else + # No full payload recorded: at minimum re-enable enforce_admins so admins are + # not left exempt. Be explicit that this is a partial restore. + plan "(no protection.full recorded — partial restore: enforce_admins only)" + run_or_plan "enforce_admins ON for ${branch} (partial; record protection.full for a full restore)" \ + gh api -X PUT "repos/${REPO}/branches/${branch}/protection/enforce_admins" + fi + + if [ "${APPLY}" = "1" ]; then + # Assert enforce_admins is ON. + local got + got="$(gh api "repos/${REPO}/branches/${branch}/protection/enforce_admins" --jq '.enabled' 2>/dev/null || true)" + assert_equal "true" "${got}" "protection.include_administrators" + # If a full baseline was recorded, assert the LIVE protection matches it. + if [ -n "${full}" ] && [ "${full}" != "null" ]; then + local live_norm base_norm + live_norm="$(normalize_protection "$(gh api "repos/${REPO}/branches/${branch}/protection" 2>/dev/null)")" + base_norm="$(normalize_protection "${full}")" + assert_equal "${base_norm}" "${live_norm}" "protection.full" + fi + else + plan "(post-restore: assert enforce_admins.enabled == true on ${branch})" + plan "(post-restore: assert LIVE protection == baseline protection.full)" + fi +} + +# normalize_protection — read a branch-protection JSON object on stdin and emit a +# canonical, comparable projection of the fields we restore. GitHub's GET adds +# url/metadata fields that a PUT body never carries, so we project only the +# semantically meaningful settings and sort, making the baseline-vs-live compare +# robust to representational noise. +normalize_protection() { + # The JSON is passed as $1 (NOT piped): a `python3 - <<'PY'` heredoc occupies + # stdin, so json.load(sys.stdin) would read the program text, not the data — + # which silently yielded "" for every input and made the post-restore assert + # vacuous. Read argv[1] instead (mirrors the other heredoc helpers here). + python3 - "${1-}" <<'PY' +import json, sys +raw = sys.argv[1] if len(sys.argv) > 1 else "" +if not raw.strip(): + # Empty input (e.g. the live GET failed) — emit a distinct sentinel so the + # baseline-vs-live compare MISMATCHES and the restore is flagged, never a + # silent pass. + print("__NORMALIZE_EMPTY__") + sys.exit(0) +try: + d = json.loads(raw) +except Exception as exc: + print(f"__NORMALIZE_ERROR__:{exc}") + sys.exit(0) + +def boolish(node, *path, key="enabled"): + cur = node + for p in path: + if not isinstance(cur, dict): + return None + cur = cur.get(p) + if isinstance(cur, dict): + return bool(cur.get(key)) + if isinstance(cur, bool): + return cur + return None + +proj = { + "enforce_admins": boolish(d, "enforce_admins"), + "required_status_checks_strict": ( + (d.get("required_status_checks") or {}).get("strict") + if isinstance(d.get("required_status_checks"), dict) else None + ), + "required_status_checks_contexts": sorted( + (d.get("required_status_checks") or {}).get("contexts", []) or [] + ) if isinstance(d.get("required_status_checks"), dict) else None, + "required_pull_request_reviews": ( + (lambda r: { + "required_approving_review_count": r.get("required_approving_review_count"), + "dismiss_stale_reviews": r.get("dismiss_stale_reviews"), + "require_code_owner_reviews": r.get("require_code_owner_reviews"), + })(d["required_pull_request_reviews"]) + if isinstance(d.get("required_pull_request_reviews"), dict) else None + ), + "required_linear_history": boolish(d, "required_linear_history"), + "allow_force_pushes": boolish(d, "allow_force_pushes"), + "allow_deletions": boolish(d, "allow_deletions"), +} +print(json.dumps(proj, sort_keys=True, separators=(",", ":"))) +PY +} + +# ── incident: premature flip that RAN ──────────────────────────────────────── +incident_premature_flip() { + say "INCIDENT — premature flip that RAN (rotate / revert / audit / restore / note)" + local branch note_path + branch="$(jget protection.branch)" + [ -n "${branch}" ] || branch="$(jget default_branch)" + [ -n "${branch}" ] || branch="main" + + # (a) Neutralise the App FIRST — anything the premature run minted is suspect. + # NB: operator gh CANNOT revoke the installation token (DELETE + # /installation/token revokes the token you authenticate WITH). The minted + # token is short-lived and self-expires; the durable neutralise is to + # uninstall the App (App JWT) or rotate its private key out-of-band. + say "a. Neutralise the GitHub App (uninstall via App JWT, or out-of-band)" + restore_app + + # (b) Revert any draft PR / branch the App opened. The draft PR head is the + # dispatcher's namespaced ref (agent-team/apply/); close + delete. + say "b. Revert the draft PR / branch the App opened" + if [ -n "${FLIP_PR}" ]; then + run_or_plan "close the premature draft PR #${FLIP_PR}" \ + gh pr close "${FLIP_PR}" --repo "${REPO}" \ + --comment "Closing premature agent-apply draft PR (incident rollback)." \ + --delete-branch + else + plan "(no --flip-pr given: list open agent-team/apply/* PRs to triage)" + run_or_plan "list open agent-apply draft PRs for triage" \ + gh pr list --repo "${REPO}" --draft --search "head:agent-team/apply/" --json number,headRefName + fi + if [ -n "${FLIP_BRANCH}" ]; then + run_or_plan "delete the premature head branch ${FLIP_BRANCH}" \ + gh api -X DELETE "repos/${REPO}/git/refs/heads/${FLIP_BRANCH}" + fi + + # (c) Audit the Checks trail of the premature run (read-only — always run). + say "c. Audit the Checks trail (read-only)" + run_or_plan "audit recent apply/verify runs (Checks trail)" \ + gh run list --repo "${REPO}" --workflow agent-team-apply-verify.yml \ + --limit 20 --json databaseId,headBranch,event,conclusion,createdAt + + # (d) Restore environment + branch protection to baseline. + say "d. Restore environment + protection to baseline" + restore_environment + restore_protection + + # (e) File an incident note. + say "e. File an incident note" + note_path=".security-review/incidents/p3-premature-flip-$(date +%Y%m%dT%H%M%SZ).md" + if [ "${APPLY}" = "1" ]; then + do_ "write incident note ${note_path}" + mkdir -p "$(dirname "${note_path}")" + { + printf '# Incident: premature P3 apply/verify flip that RAN\n\n' + printf -- '- Repo: %s\n' "${REPO}" + printf -- '- Baseline: %s\n' "${BASELINE}" + printf -- '- Flip PR: %s\n' "${FLIP_PR:-}" + printf -- '- Flip branch: %s\n' "${FLIP_BRANCH:-}" + printf -- '- Recorded: %s\n' "$(date -u +%Y-%m-%dT%H:%M:%SZ)" + printf '\n## Actions taken\n' + printf -- '1. Neutralised the GitHub App (uninstall via App JWT, or out-of-band).\n' + printf -- ' NB: the minted installation token cannot be revoked by operator gh;\n' + printf -- ' it is short-lived and self-expires.\n' + printf -- '2. Closed the premature draft PR + deleted its branch.\n' + printf -- '3. Audited the apply/verify Checks trail.\n' + printf -- '4. Restored the agent-apply environment + branch protection.\n' + } > "${note_path}" + else + plan "write incident note to ${note_path} (repo/flip/baseline + actions taken)" + fi +} + +# ── --record-baseline ──────────────────────────────────────────────────────── +# Capture the CURRENT live state into the baseline JSON: the live workflow SHA, +# the env required-reviewer NUMERIC ids, the FULL branch protection object, and +# the App installation id. Honours --apply (default --dry-run just prints what it +# would capture). Creates .security-review/ if absent. +record_baseline() { + say "RECORD BASELINE — capture current live state into ${BASELINE}" + # Resolve a repo: --repo, or env default. The baseline file may not exist yet, + # so we do NOT call require_baseline here. + if [ -z "${REPO}" ]; then + err "no target repo: pass --repo OWNER/NAME (baseline does not exist yet to read it from)" + return 1 + fi + + local branch wf_path + branch="main" + wf_path=".github/workflows/agent-team-apply-verify.yml" + + if [ "${APPLY}" != "1" ]; then + plan "capture live workflow SHA for ${wf_path} via 'gh api repos/${REPO}/contents/${wf_path} --jq .sha'" + plan "capture env '${ENV_NAME}' required reviewer NUMERIC ids" + plan "capture FULL branch protection for ${branch}" + plan "capture App installation id (${APP_INSTALLATION_ID:-<--app-installation-id N>})" + plan "write baseline JSON to ${BASELINE} (creating $(dirname "${BASELINE}") if absent)" + return 0 + fi + + do_ "capture live state into ${BASELINE}" + mkdir -p "$(dirname "${BASELINE}")" + + local wf_sha reviewer_ids protection_full + wf_sha="$(gh api "repos/${REPO}/contents/${wf_path}" --jq '.sha' 2>/dev/null || true)" + reviewer_ids="$(gh api "repos/${REPO}/environments/${ENV_NAME}" \ + --jq '[.protection_rules[]? | select(.type=="required_reviewers") + | .reviewers[]? | select(.type=="User") | .reviewer.id] | sort' \ + 2>/dev/null || echo '[]')" + [ -n "${reviewer_ids}" ] || reviewer_ids='[]' + protection_full="$(gh api "repos/${REPO}/branches/${branch}/protection" 2>/dev/null || echo 'null')" + [ -n "${protection_full}" ] || protection_full='null' + + # Assemble the baseline JSON deterministically with python3 (stdlib). + REPO="${REPO}" BRANCH="${branch}" WF_PATH="${wf_path}" WF_SHA="${wf_sha}" \ + ENV_NAME="${ENV_NAME}" REVIEWER_IDS="${reviewer_ids}" \ + APP_INSTALLATION_ID="${APP_INSTALLATION_ID}" PROTECTION_FULL="${protection_full}" \ + python3 - "${BASELINE}" <<'PY' +import json, os, sys + +out_path = sys.argv[1] +try: + reviewer_ids = json.loads(os.environ.get("REVIEWER_IDS") or "[]") +except Exception: + reviewer_ids = [] +try: + protection_full = json.loads(os.environ.get("PROTECTION_FULL") or "null") +except Exception: + protection_full = None + +include_admins = bool( + isinstance(protection_full, dict) + and isinstance(protection_full.get("enforce_admins"), dict) + and protection_full["enforce_admins"].get("enabled") +) + +inst = os.environ.get("APP_INSTALLATION_ID") or "" +baseline = { + "repo": os.environ["REPO"], + "default_branch": os.environ["BRANCH"], + "workflow_path": os.environ["WF_PATH"], + "workflow_baseline_sha": os.environ.get("WF_SHA") or "", + "environment": { + "name": os.environ["ENV_NAME"], + "required_reviewer_ids": reviewer_ids, + "deployment_branch_policy": "protected", + }, + "app": { + "slug": os.environ["ENV_NAME"], + "installation_id": int(inst) if inst.isdigit() else 0, + "action": "uninstall", + }, + "protection": { + "branch": os.environ["BRANCH"], + "include_administrators": include_admins, + "full": protection_full, + }, +} +with open(out_path, "w", encoding="utf-8") as fh: + json.dump(baseline, fh, indent=2, sort_keys=True) + fh.write("\n") +print(f" wrote {out_path}") +PY + printf ' \033[1;32m[OK]\033[0m baseline recorded to %s\n' "${BASELINE}" +} + +# ── main ───────────────────────────────────────────────────────────────────── +main() { + parse_args "$@" + + if [ "${RECORD_BASELINE}" = "1" ]; then + if [ "${APPLY}" = "1" ]; then + say "MODE: --record-baseline --apply (will CAPTURE live state) on repo ${REPO}" + else + say "MODE: --record-baseline --dry-run (plan only) on repo ${REPO}" + fi + record_baseline + say "Done (record-baseline, $([ "${APPLY}" = "1" ] && echo apply || echo dry-run))." + return 0 + fi + + require_baseline + + if [ "${APPLY}" = "1" ]; then + say "MODE: --apply (mutations WILL be performed) on repo ${REPO}" + else + say "MODE: --dry-run (plan only; no mutations) on repo ${REPO}" + fi + + case "${SURFACE}" in + workflow) restore_workflow ;; + environment) restore_environment ;; + app) restore_app ;; + protection) restore_protection ;; + all) + restore_workflow + restore_environment + restore_app + restore_protection + ;; + incident) incident_premature_flip ;; + *) err "unhandled surface: ${SURFACE}"; return 2 ;; + esac + + say "Done (${SURFACE}, $([ "${APPLY}" = "1" ] && echo apply || echo dry-run))." +} + +main "$@" diff --git a/agent-team/systemd/agent-team-coordinator.service b/agent-team/systemd/agent-team-coordinator.service index 3261b3d..2d8b593 100644 --- a/agent-team/systemd/agent-team-coordinator.service +++ b/agent-team/systemd/agent-team-coordinator.service @@ -32,6 +32,27 @@ # AGENT_TEAM_SLACK_OWNER_IDS -> comma-separated authorized answerer ids # (AUTHZ-01). The listener FAILS CLOSED if unset. # +# --- P3 build->dispatch->verify wiring (read by the LIVE serve() path) --- +# The bound P3 wiring is the serve() default; if any of the three below (plus a +# CI-read token) is unset, serve() degrades to the INERT P3 path (one WARNING + +# a #agent-team notice) instead of crash-looping (Decision 5). All are read at +# graph-build / dispatch time from this file's environment: +# AGENT_TEAM_REPO_OWNER -> dispatch target owner. Fixed at factory time, +# never read from pipeline state, so model output +# cannot redirect the dispatch/verify target. +# AGENT_TEAM_REPO_NAME -> dispatch target repo (same fail-closed binding). +# AGENT_TEAM_BASE_BRANCH -> PR base branch (optional; default "main"). +# AGENT_TEAM_CI_READ_TOKEN -> the READ-ONLY CI-result token. The verifier's +# authenticated conclusion read uses it (falls back +# to GITHUB_TOKEN). This is read-only by contract: +# NO pull-requests:write / contents:write token, +# and NO AGENT_APPLY_APP_ID / _PRIVATE_KEY, may live +# in this file. The apply path mints its write token +# INSIDE the CI runner from Actions secrets; the box +# holds no standing write credential. The invariant +# is asserted by scripts/assert_no_write_token.py +# (the A2 audit) at provisioning + in CI. +# # ~/orchestrator/.env (mode 600, NOT in git) - the P2 review-loop provider key: # The production serve() wires the GPT-4.1 cross-review loop, which shells the # local orchestrator run.py -> cross_reviewer once a task reaches REVIEW. That diff --git a/agent-team/tests/test_coordinator.py b/agent-team/tests/test_coordinator.py index 39ae88f..375036e 100644 --- a/agent-team/tests/test_coordinator.py +++ b/agent-team/tests/test_coordinator.py @@ -18,7 +18,7 @@ Every test injects: from __future__ import annotations import queue -from datetime import timedelta +from datetime import datetime, timedelta, timezone from pathlib import Path from types import SimpleNamespace from typing import Any @@ -88,6 +88,7 @@ def _make_coordinator( alarm_hook: Any = None, build_plan_node: Any = None, notify: Any = None, + draft_pr_provider: Any = None, ) -> Coordinator: """Build a Coordinator wired with an in-memory saver + the stub clarify node.""" saver = _Saver() @@ -101,6 +102,7 @@ def _make_coordinator( deadline_window=deadline_window, alarm_hook=alarm_hook, notify=notify, + draft_pr_provider=draft_pr_provider, ) @@ -477,6 +479,83 @@ def test_plan_ready_milestone_threads_under_root_and_presents_plan( assert "Summary:" in message # the plan is actually presented +def test_verify_pass_emits_draft_pr_lifecycle_notice( + db_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A terminal CI PASS posts the POSITIVE draft-PR notice (run link), NOT the park ALARM. + + Unit B3: when a task's CI run reaches terminal PASS and the draft PR is + opened (the APPROVED route — recorded as a verify-stage PASS verdict on a + task settled at DONE), ``_post_resume_followups`` emits a ``#agent-team`` + LIFECYCLE notice via the positive notify sink — distinct from the P2 + plan-ready terminus (which also settles at DONE) and from the park-ALARM + path. The notice carries the GitHub Actions run link built from the captured + run id + the configured owner/repo. + """ + monkeypatch.setenv("AGENT_TEAM_REPO_OWNER", "Sea-Haven-Industries") + monkeypatch.setenv("AGENT_TEAM_REPO_NAME", "demo-repo") + + posted: list[tuple[str, str | None]] = [] + alarmed: list[str] = [] + coord = _make_coordinator( + db_path, + notify=lambda message, thread_ts=None: posted.append((message, thread_ts)), + alarm_hook=alarmed.append, + ) + coord.setup() + thread_id = coord.start_task( + task_text="ship the widget", transport_name="slack", slack_thread_ts="ROOT.TS" + ) + + # Inject the settled P3 verify-PASS terminal state (CI terminal PASS + draft + # PR opened on the APPROVED route): DONE, with a verify-stage PASS verdict + # carrying the dispatched run id. update_state clears the pending interrupt, + # so _post_resume_followups treats the thread as settled (no open question). + coord.graph.update_state( + graph_mod.thread_config(thread_id), + { + "status": "done", + "current_phase": "done", + "run_id": "987654321", + "review_verdicts": [ + {"stage": "verify", "decision": "pass", "run_id": "987654321"} + ], + }, + ) + + coord._post_resume_followups([SimpleNamespace(thread_id=thread_id)]) + + notices = [p for p in posted if "draft PR opened" in p[0]] + assert len(notices) == 1 + message, thread_ts = notices[0] + assert thread_ts == "ROOT.TS" # threaded under the task root + assert "CI PASSED" in message + # The positive path: a run link, NOT the park-ALARM path. + assert ( + "https://github.com/Sea-Haven-Industries/demo-repo/actions/runs/987654321" + in message + ) + assert alarmed == [] + # It must NOT be misreported as the P2 plan-ready terminus. + assert not any("plan ready for review" in p[0] for p in posted) + + +def test_verify_pass_verdict_discriminates_from_plan_ready() -> None: + """The verify-PASS discriminator ignores a plan-ready DONE (no verify verdict).""" + # A plan-ready terminus settles at DONE but carries no verify verdict. + assert Coordinator._verify_pass_verdict({"review_verdicts": []}) is None + # A non-PASS verify verdict (e.g. a loop-back FAIL) is not the PASS terminus. + assert ( + Coordinator._verify_pass_verdict( + {"review_verdicts": [{"stage": "verify", "decision": "fail"}]} + ) + is None + ) + # A verify-stage PASS verdict is the P3 PASS terminus. + verdict = {"stage": "verify", "decision": "pass", "run_id": "42"} + assert Coordinator._verify_pass_verdict({"review_verdicts": [verdict]}) == verdict + + # --------------------------------------------------------------------------- # # tick — deadline sweep + park ALARM + drain # --------------------------------------------------------------------------- # @@ -519,6 +598,105 @@ def test_tick_drains_pending_resume(db_path: Path) -> None: assert state["status"] == "done" +# --------------------------------------------------------------------------- # +# tick — draft-PR runaway/stale sweep wiring (P3 A4) +# --------------------------------------------------------------------------- # + + +def test_draft_pr_sweep_is_noop_without_provider(db_path: Path) -> None: + """The INERT default: no ``draft_pr_provider`` means the sweep does nothing.""" + posted: list[Any] = [] + coord = _make_coordinator(db_path, notify=lambda m, **k: posted.append(m)) + coord.setup() + assert coord._draft_pr_monitor_sweep() is None + assert posted == [] + + +def test_draft_pr_sweep_alarms_on_runaway_via_emit(db_path: Path) -> None: + """A runaway burst routes a ``#agent-team`` ALARM through the notify sink.""" + from agent_team.draft_pr_monitor import DraftPr + + now = datetime.now(timezone.utc).isoformat() + burst = [ + DraftPr(number=i, opened_at=now, updated_at=now) for i in range(4) + ] # > 3 within window + posted: list[str] = [] + coord = _make_coordinator( + db_path, + notify=lambda m, **k: posted.append(m), + draft_pr_provider=lambda: burst, + ) + coord.setup() + + report = coord._draft_pr_monitor_sweep() + assert report is not None + assert report.alarmed == 1 + assert any("draft-PR runaway" in m for m in posted) + # The ALARM surfaces the operator remediation, never a self-stop. + assert any("systemctl stop" in m for m in posted) + + +def test_draft_pr_sweep_reminds_on_stale_via_emit(db_path: Path) -> None: + """A stale (>7d idle) draft PR routes a single reminder through the sink.""" + from agent_team.draft_pr_monitor import DraftPr + + stale_iso = (datetime.now(timezone.utc) - timedelta(days=10)).isoformat() + posted: list[str] = [] + coord = _make_coordinator( + db_path, + notify=lambda m, **k: posted.append(m), + draft_pr_provider=lambda: [DraftPr(number=7, updated_at=stale_iso)], + ) + coord.setup() + + report = coord._draft_pr_monitor_sweep() + assert report is not None + assert report.stale_reminded == 1 + assert any("draft PR #7" in m and "7 days" in m for m in posted) + + +def test_draft_pr_sweep_failsoft_on_enumeration_error(db_path: Path) -> None: + """An enumeration that raises is swallowed (returns None) — tick never breaks.""" + + def _boom() -> list[Any]: + raise RuntimeError("github unreachable") + + coord = _make_coordinator(db_path, draft_pr_provider=_boom) + coord.setup() + # Must not raise; sweep returns None on a failed enumeration. + assert coord._draft_pr_monitor_sweep() is None + # And a full tick (which calls the sweep) likewise survives. + assert coord.tick() == [] + + +def test_draft_pr_sweep_memory_persists_across_ticks(db_path: Path) -> None: + """The flapping-backoff memory persists for the daemon's lifetime. + + A sustained runaway condition ALARMs once, then is suppressed on the next + sweep (the per-daemon ``MonitorMemory`` carries the cooldown watermark). + """ + from agent_team.draft_pr_monitor import DraftPr + + now = datetime.now(timezone.utc).isoformat() + burst = [DraftPr(number=i, opened_at=now, updated_at=now) for i in range(5)] + posted: list[str] = [] + coord = _make_coordinator( + db_path, + notify=lambda m, **k: posted.append(m), + draft_pr_provider=lambda: burst, + ) + coord.setup() + + first = coord._draft_pr_monitor_sweep() + second = coord._draft_pr_monitor_sweep() + assert first is not None and second is not None + assert first.alarmed == 1 + assert second.alarmed == 0 + assert second.alarm_suppressed == 1 + # Exactly one ALARM posted despite two sweeps of the same condition. + assert sum("draft-PR runaway" in m for m in posted) == 1 + + # --------------------------------------------------------------------------- # # recover — startup convergence (§3.3.1) # --------------------------------------------------------------------------- # diff --git a/agent-team/tests/test_no_write_token.py b/agent-team/tests/test_no_write_token.py new file mode 100644 index 0000000..675e8ab --- /dev/null +++ b/agent-team/tests/test_no_write_token.py @@ -0,0 +1,327 @@ +"""Unit tests for ``scripts/assert_no_write_token.py`` (P3 no-write-token audit). + +The audit asserts the box invariant from ``docs/P3-PHASE0-DESIGN.md`` and +``ci/README.md``: the ``pull-requests: write`` GitHub App credential +(``AGENT_APPLY_APP_ID`` / ``AGENT_APPLY_APP_PRIVATE_KEY``) lives ONLY as an +Actions secret, and the always-on box holds **no standing write token**. + +These tests never touch real box paths: they grep temp files and a patched +``os.environ``. ``scripts/`` is not a package, so (matching the ``run-team.py`` +idiom) the module is loaded from its file path via :mod:`importlib`. +""" + +from __future__ import annotations + +import importlib.util +import os +from pathlib import Path +from types import ModuleType + +import pytest + +_SCRIPT_PATH = ( + Path(__file__).resolve().parents[1] / "scripts" / "assert_no_write_token.py" +) + +# A sample PEM private key header for each algorithm the design calls out. We +# assert the regex covers RSA / EC / OPENSSH (and a bare PKCS#8 block). +_PEM_BODY = "\nMIIB...redacted...\n" +_RSA_KEY = ( + "-----BEGIN RSA PRIVATE KEY-----" + _PEM_BODY + "-----END RSA PRIVATE KEY-----" +) +_EC_KEY = "-----BEGIN EC PRIVATE KEY-----" + _PEM_BODY + "-----END EC PRIVATE KEY-----" +_OPENSSH_KEY = ( + "-----BEGIN OPENSSH PRIVATE KEY-----" + + _PEM_BODY + + "-----END OPENSSH PRIVATE KEY-----" +) +_PKCS8_KEY = "-----BEGIN PRIVATE KEY-----" + _PEM_BODY + "-----END PRIVATE KEY-----" + + +def _load_script() -> ModuleType: + """Load the (non-package) audit script from its file path.""" + spec = importlib.util.spec_from_file_location("assert_no_write_token", _SCRIPT_PATH) + assert spec is not None and spec.loader is not None + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +@pytest.fixture(scope="module") +def mod() -> ModuleType: + """The loaded audit module (loaded once per test module).""" + return _load_script() + + +# --------------------------------------------------------------------------- # +# scan_environ +# --------------------------------------------------------------------------- # + + +def test_clean_environ_has_no_findings(mod: ModuleType) -> None: + env = {"PATH": "/usr/bin", "AGENT_TEAM_REPO_OWNER": "Sea-Haven-Industries"} + assert mod.scan_environ(env) == [] + + +def test_forbidden_app_id_env_var_is_flagged(mod: ModuleType) -> None: + findings = mod.scan_environ({"AGENT_APPLY_APP_ID": "123456"}) + assert len(findings) == 1 + assert "AGENT_APPLY_APP_ID" in findings[0] + + +def test_forbidden_app_private_key_env_var_is_flagged(mod: ModuleType) -> None: + findings = mod.scan_environ({"AGENT_APPLY_APP_PRIVATE_KEY": _RSA_KEY}) + assert len(findings) == 1 + assert "AGENT_APPLY_APP_PRIVATE_KEY" in findings[0] + + +def test_write_token_shaped_name_is_flagged(mod: ModuleType) -> None: + for name in ( + "GITHUB_APP_TOKEN", + "GH_PAT_TOKEN", + "GITHUB_WRITE_TOKEN", + "GH_APPLY_KEY", + ): + findings = mod.scan_environ({name: "whatever"}) + assert findings, f"{name} should be flagged by the write-name heuristic" + + +def test_write_capable_token_value_is_flagged_regardless_of_name( + mod: ModuleType, +) -> None: + # A benign-looking var name but a write-capable PAT value. + findings = mod.scan_environ({"SOME_VAR": "ghp_" + "a" * 36}) + assert len(findings) == 1 + assert "SOME_VAR" in findings[0] + + findings = mod.scan_environ({"OTHER": "github_pat_" + "b" * 30}) + assert len(findings) == 1 + + findings = mod.scan_environ({"INSTALL": "ghs_" + "c" * 36}) + assert len(findings) == 1 + + # user-to-server (ghu_) and refresh (ghr_) tokens are also write-risk. + findings = mod.scan_environ({"U2S": "ghu_" + "d" * 36}) + assert len(findings) == 1 + + findings = mod.scan_environ({"REFRESH": "ghr_" + "e" * 36}) + assert len(findings) == 1 + + +def test_read_only_tokens_are_not_flagged(mod: ModuleType) -> None: + # Known read-only / non-GitHub tokens must not trip the audit. + env = { + "AGENT_TEAM_CI_READ_TOKEN": "ghp_" + + "r" * 36, # read-only by contract, allowlisted + "AGENT_TEAM_API_TOKEN": "secret-bearer", + "SLACK_APP_TOKEN": "xapp-1-abc", + "SLACK_BOT_TOKEN": "xoxb-abc", + "CLAUDE_CODE_OAUTH_TOKEN": "oauth-abc", + "ANTHROPIC_API_KEY": "sk-ant-abc", + } + assert mod.scan_environ(env) == [] + + +def test_plain_github_token_fallback_is_not_flagged_by_name(mod: ModuleType) -> None: + # The read-only GITHUB_TOKEN runtime fallback should not match the *name* + # heuristic (it carries no GH_*_WRITE/APP/PAT shape). + assert mod.scan_environ({"GITHUB_TOKEN": "a-non-write-shaped-value"}) == [] + + +# --------------------------------------------------------------------------- # +# scan_config +# --------------------------------------------------------------------------- # + + +def test_scan_config_none_and_empty(mod: ModuleType) -> None: + assert mod.scan_config(None) == [] + assert mod.scan_config({}) == [] + + +def test_scan_config_flags_forbidden_key(mod: ModuleType) -> None: + findings = mod.scan_config({"AGENT_APPLY_APP_PRIVATE_KEY": _EC_KEY}) + assert len(findings) == 1 + assert "AGENT_APPLY_APP_PRIVATE_KEY" in findings[0] + + +def test_scan_config_flags_write_value(mod: ModuleType) -> None: + findings = mod.scan_config({"token": "ghp_" + "z" * 36}) + assert len(findings) == 1 + + +def test_scan_config_ignores_non_string_values(mod: ModuleType) -> None: + assert mod.scan_config({"timeout": 15, "enabled": True}) == [] + + +# --------------------------------------------------------------------------- # +# scan_env_file +# --------------------------------------------------------------------------- # + + +def test_missing_file_is_not_a_finding(mod: ModuleType, tmp_path: Path) -> None: + assert mod.scan_env_file(tmp_path / "nope.env") == [] + + +def test_clean_file_is_not_a_finding(mod: ModuleType, tmp_path: Path) -> None: + f = tmp_path / "secrev.env" + f.write_text("CLAUDE_CODE_OAUTH_TOKEN=abc\nAGENT_TEAM_CI_READ_TOKEN=def\n") + assert mod.scan_env_file(f) == [] + + +@pytest.mark.parametrize("key", [_RSA_KEY, _EC_KEY, _OPENSSH_KEY, _PKCS8_KEY]) +def test_private_key_header_is_flagged_for_each_algo( + mod: ModuleType, tmp_path: Path, key: str +) -> None: + f = tmp_path / "orchestrator.env" + f.write_text("HARMLESS=1\n" + key + "\n") + findings = mod.scan_env_file(f) + assert any("PRIVATE KEY" in fn for fn in findings) + + +def test_app_credential_name_in_file_is_flagged( + mod: ModuleType, tmp_path: Path +) -> None: + f = tmp_path / "secrev.env" + f.write_text("AGENT_APPLY_APP_ID=987654\n") + findings = mod.scan_env_file(f) + assert any("AGENT_APPLY_APP_ID" in fn for fn in findings) + + +def test_configured_app_id_value_in_file_is_flagged( + mod: ModuleType, tmp_path: Path +) -> None: + f = tmp_path / "secrev.env" + # The literal id value leaking under any var name. + f.write_text("SOMETHING=installed-as-987654-here\n") + findings = mod.scan_env_file(f, app_id="987654") + assert any("App id value" in fn for fn in findings) + + +def test_unreadable_present_file_is_a_finding( + mod: ModuleType, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + f = tmp_path / "secrev.env" + f.write_text("ok\n") + + def _boom(*_a: object, **_k: object) -> str: + raise OSError("permission denied") + + monkeypatch.setattr(Path, "read_text", _boom) + findings = mod.scan_env_file(f) + assert len(findings) == 1 + assert "could not read" in findings[0] + + +# --------------------------------------------------------------------------- # +# audit() orchestration + file resolution +# --------------------------------------------------------------------------- # + + +def test_audit_clean_box_returns_empty(mod: ModuleType, tmp_path: Path) -> None: + clean = tmp_path / "secrev.env" + clean.write_text("CLAUDE_CODE_OAUTH_TOKEN=abc\n") + findings = mod.audit( + environ={"PATH": "/usr/bin"}, + config={"timeout": 15}, + env_files=[str(clean)], + ) + assert findings == [] + + +def test_audit_aggregates_findings_across_scanners( + mod: ModuleType, tmp_path: Path +) -> None: + leaky = tmp_path / "orchestrator.env" + leaky.write_text(_OPENSSH_KEY + "\n") + findings = mod.audit( + environ={"AGENT_APPLY_APP_ID": "42", "GH_PAT_TOKEN": "x"}, + config={"token": "ghp_" + "q" * 36}, + env_files=[str(leaky)], + ) + # env (2) + config (1) + file (1) + assert len(findings) >= 4 + + +def test_audit_uses_configured_app_id_from_environ( + mod: ModuleType, tmp_path: Path +) -> None: + f = tmp_path / "secrev.env" + f.write_text("LEAK=value-555-leaked\n") + findings = mod.audit( + environ={"AGENT_APPLY_APP_ID": "555"}, + env_files=[str(f)], + ) + # AGENT_APPLY_APP_ID in environ is itself a finding, and "555" in the file + # is a second. + assert any("AGENT_APPLY_APP_ID" in fn for fn in findings) + assert any("App id value" in fn for fn in findings) + + +def test_env_file_resolution_prefers_cli(mod: ModuleType, tmp_path: Path) -> None: + a = tmp_path / "a.env" + paths = mod._resolve_env_files([str(a)], {}) + assert paths == [a] + + +def test_env_file_resolution_uses_env_var(mod: ModuleType, tmp_path: Path) -> None: + a = tmp_path / "a.env" + b = tmp_path / "b.env" + env = {"ASSERT_NO_WRITE_TOKEN_ENV_FILES": os.pathsep.join([str(a), str(b)])} + paths = mod._resolve_env_files(None, env) + assert paths == [a, b] + + +def test_env_file_resolution_defaults_and_expands_home(mod: ModuleType) -> None: + paths = mod._resolve_env_files(None, {}) + # Defaults to ~/secrev.env and ~/orchestrator/.env, expanded (no literal ~). + assert len(paths) == 2 + assert all("~" not in str(p) for p in paths) + assert paths[0].name == "secrev.env" + + +# --------------------------------------------------------------------------- # +# main() exit codes + messaging +# --------------------------------------------------------------------------- # + + +def test_main_clean_exits_zero( + mod: ModuleType, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture, +) -> None: + clean = tmp_path / "secrev.env" + clean.write_text("CLAUDE_CODE_OAUTH_TOKEN=abc\n") + # Patch the live environ so the real process env can't leak into the scan. + monkeypatch.setattr(os, "environ", {"PATH": "/usr/bin"}) + rc = mod.main(["--env-file", str(clean)]) + assert rc == 0 + out = capsys.readouterr().out + assert "OK" in out + assert "no standing write token" in out + + +def test_main_finding_exits_nonzero( + mod: ModuleType, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture, +) -> None: + leaky = tmp_path / "secrev.env" + leaky.write_text(_RSA_KEY + "\n") + monkeypatch.setattr(os, "environ", {"PATH": "/usr/bin"}) + rc = mod.main(["--env-file", str(leaky)]) + assert rc == 1 + err = capsys.readouterr().err + assert "FAIL" in err + assert "Actions secret" in err + + +def test_main_flags_write_token_in_live_environ( + mod: ModuleType, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + clean = tmp_path / "secrev.env" + clean.write_text("ok\n") + monkeypatch.setattr(os, "environ", {"AGENT_APPLY_APP_PRIVATE_KEY": _RSA_KEY}) + rc = mod.main(["--env-file", str(clean)]) + assert rc == 1 diff --git a/agent-team/tests/test_rollback.py b/agent-team/tests/test_rollback.py new file mode 100644 index 0000000..659ee0b --- /dev/null +++ b/agent-team/tests/test_rollback.py @@ -0,0 +1,802 @@ +"""Tests for ``scripts/p3_rollback.sh`` — the P3 privileged-surface rollback. + +The script restores EVERY privileged P3 surface (the apply/verify workflow flip, +the ``agent-apply`` environment, the GitHub App perms/installation, and branch +protection) from a recorded baseline, and asserts post-restore == baseline. The +apply/verify workflow is ALREADY LIVE (flipped + provisioned 2026-06-22), so the +rollback targets the LIVE state. + +These tests exercise the script with NO real ``gh``/``git`` calls: + +* The ``--dry-run`` default must print a PLAN and perform NO mutations. We assert + the plan output covers every surface (every destructive call is described but + not executed). +* Argument parsing: a missing surface, an unknown flag, and a missing value each + fail closed (exit 2). +* Fail-closed posture: a baseline whose ``include_administrators`` is not ``true`` + is refused; a missing baseline file is refused. +* ``--apply`` is verified against a PATH-shimmed ``gh``/``git`` that only RECORDS + its argv into a log file (never touches a network or a repo), so we can assert + the exact destructive calls the script would make — with zero real side effects. +""" + +from __future__ import annotations + +import json +import os +import stat +import subprocess +from pathlib import Path + +import pytest + +_SCRIPT = Path(__file__).resolve().parents[1] / "scripts" / "p3_rollback.sh" + + +def test_script_exists_and_is_executable() -> None: + assert _SCRIPT.is_file(), f"missing rollback script: {_SCRIPT}" + mode = _SCRIPT.stat().st_mode + assert mode & stat.S_IXUSR, "p3_rollback.sh must be executable" + + +def test_script_has_bash_shebang() -> None: + first = _SCRIPT.read_text(encoding="utf-8").splitlines()[0] + assert first.startswith("#!") and "bash" in first + + +def test_script_passes_bash_syntax_check() -> None: + res = subprocess.run(["bash", "-n", str(_SCRIPT)], capture_output=True, text=True) + assert res.returncode == 0, res.stderr + + +# --------------------------------------------------------------------------- # +# Fixtures: a recorded baseline + a PATH shim for gh/git +# --------------------------------------------------------------------------- # + +_BASELINE = { + "repo": "Sea-Haven-Industries/orchestrator", + "default_branch": "main", + "workflow_path": ".github/workflows/agent-team-apply-verify.yml", + "workflow_baseline_sha": "0123abc", + "environment": { + "name": "agent-apply", + # Reviewers recorded as NUMERIC user ids (restore is exact + assertable). + "required_reviewer_ids": [1234567], + "required_reviewers": ["amoussa1229"], + "deployment_branch_policy": "protected", + }, + "app": { + "slug": "agent-apply", + "installation_id": 424242, + # The only programmatic neutralise is uninstall (App JWT). There is no + # permission-reduction REST endpoint. + "action": "uninstall", + }, + "protection": { + "branch": "main", + "include_administrators": True, + # The FULL protection payload recorded pre-flip (the exact body restored). + "full": { + "enforce_admins": {"enabled": True}, + "required_status_checks": { + "strict": True, + "contexts": ["guard", "build-test"], + }, + "required_pull_request_reviews": { + "required_approving_review_count": 1, + "dismiss_stale_reviews": True, + "require_code_owner_reviews": False, + }, + "required_linear_history": {"enabled": True}, + "allow_force_pushes": {"enabled": False}, + "allow_deletions": {"enabled": False}, + }, + "required_status_checks": ["guard", "build-test"], + }, +} + + +@pytest.fixture +def baseline(tmp_path: Path) -> Path: + p = tmp_path / "p3-baseline.json" + p.write_text(json.dumps(_BASELINE), encoding="utf-8") + return p + + +@pytest.fixture +def shim_bin(tmp_path: Path) -> tuple[Path, Path]: + """A bin dir with stub ``gh`` and ``git`` that only record their argv. + + Returns ``(bin_dir, calls_log)``. The script, run with this dir prepended to + PATH, makes ZERO real gh/git calls — every invocation appends a line to + ``calls_log`` and exits 0. Where the script reads command output (the + post-restore asserts), the stubs emit the baseline value so the assert holds. + """ + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + calls_log = tmp_path / "calls.log" + + # gh stub: record argv; emit canned output for the read-only post-restore + # asserts so --apply asserts pass. The script passes a server-side --jq to gh + # (the real gh applies it); the stub must therefore emit the ALREADY-jq'd + # value the script expects: + # * env GET with the reviewer-ids --jq -> "1234567" (space-joined ids) + # * enforce_admins GET -> "true" + # * protection GET (no enforce_admins) -> the full protection JSON, which + # the script then pipes through its own normalize_protection. We emit the + # same shape the baseline records so the normalized compare holds. + gh = bin_dir / "gh" + _protection_json = json.dumps(_BASELINE["protection"]["full"]) + gh.write_text( + "#!/usr/bin/env bash\n" + f'printf "gh %s\\n" "$*" >> "{calls_log}"\n' + 'argv="$*"\n' + 'for a in "$@"; do\n' + ' case "$a" in\n' + " */enforce_admins) echo 'true'; exit 0 ;;\n" + " esac\n" + "done\n" + "# protection GET (full object) -> emit the baseline full protection JSON.\n" + 'case "$argv" in\n' + " *branches/*/protection*)\n" + f" cat <<'JSON'\n{_protection_json}\nJSON\n" + " exit 0 ;;\n" + " *users/*)\n" + " # login->id resolution (gh api users/{login} --jq .id).\n" + " echo '1234567'; exit 0 ;;\n" + " *environments/*)\n" + " # reviewer-ids --jq result (space-joined) for the post-restore assert.\n" + " echo '1234567'; exit 0 ;;\n" + "esac\n" + "exit 0\n", + encoding="utf-8", + ) + git = bin_dir / "git" + git.write_text( + f'#!/usr/bin/env bash\nprintf "git %s\\n" "$*" >> "{calls_log}"\nexit 0\n', + encoding="utf-8", + ) + for f in (gh, git): + f.chmod(f.stat().st_mode | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH) + return bin_dir, calls_log + + +def _run( + *args: str, + baseline: Path, + shim: tuple[Path, Path] | None = None, +) -> subprocess.CompletedProcess[str]: + env = dict(os.environ) + if shim is not None: + bin_dir, _ = shim + env["PATH"] = f"{bin_dir}{os.pathsep}{env['PATH']}" + return subprocess.run( + ["bash", str(_SCRIPT), *args, "--baseline", str(baseline)], + capture_output=True, + text=True, + env=env, + # cwd kept default (no git repo needed: gh/git are shimmed). + ) + + +# --------------------------------------------------------------------------- # +# Dry-run plan output (the default; no shim required — nothing executes) +# --------------------------------------------------------------------------- # + + +def test_default_is_dry_run_and_prints_plan_not_apply(baseline: Path) -> None: + res = _run("all", "--flip-pr", "7", baseline=baseline) + assert res.returncode == 0, res.stderr + out = res.stdout + assert "--dry-run (plan only" in out + assert "[PLAN]" in out + # No APPLY lines must appear in dry-run. + assert "[APPLY]" not in out + + +def test_dry_run_covers_all_four_surfaces(baseline: Path) -> None: + res = _run("all", "--flip-pr", "7", baseline=baseline) + out = res.stdout + assert "Restore the apply/verify workflow flip" in out + assert "Restore the agent-apply environment" in out + assert "Neutralise the GitHub App" in out + assert "Restore the FULL branch protection baseline" in out + + +def test_dry_run_workflow_premerge_closes_pr_and_deletes_branch( + baseline: Path, +) -> None: + res = _run( + "workflow", + "--flip-pr", + "7", + "--flip-branch", + "agent-team/apply/t1", + baseline=baseline, + ) + out = res.stdout + assert "gh pr close 7" in out + assert "--delete-branch" in out + assert "git/refs/heads/agent-team/apply/t1" in out + # LIVE: the run-name/permissions edits get reverted to the baseline SHA. + assert "git checkout 0123abc --" in out + + +def test_dry_run_workflow_postmerge_reverts_commit_and_reruns_ci( + baseline: Path, +) -> None: + res = _run( + "workflow", + "--merged", + "--flip-commit", + "cafef00d", + baseline=baseline, + ) + out = res.stdout + assert "git revert --no-edit cafef00d" in out + assert "git push origin HEAD" in out + assert "gh workflow run" in out + + +def test_dry_run_app_uninstall_requires_app_jwt_not_operator_gh( + baseline: Path, +) -> None: + """The corrected model: there is NO permission-reduction endpoint, and the + installation token is NOT revoked by operator gh. The only programmatic + neutralise is uninstall, which needs an App JWT.""" + res = _run("app", baseline=baseline) + out = res.stdout + # The fictional permission-reduction endpoint must NOT appear. + assert "/permissions" not in out + assert "PATCH" not in out + # The token is NOT revoked by operator gh. + assert "DELETE installation/token" not in out + assert "cannot be revoked by operator gh" in out + # Uninstall is planned and clearly flagged as needing an App JWT. + assert "UNINSTALL App 'agent-apply' installation 424242" in out + assert "APP JWT" in out + assert "app/installations/424242" in out + + +def test_dry_run_app_out_of_band_path(tmp_path: Path) -> None: + data = json.loads(json.dumps(_BASELINE)) + data["app"]["action"] = "out-of-band" + p = tmp_path / "b.json" + p.write_text(json.dumps(data), encoding="utf-8") + res = _run("app", baseline=p) + out = res.stdout + assert "OUT-OF-BAND App neutralise" in out + assert "no REST endpoint reduces App permissions" in out + # Still no fictional permission API. + assert "/permissions" not in out + + +def test_apply_app_uninstall_without_jwt_fails_closed( + baseline: Path, shim_bin: tuple[Path, Path] +) -> None: + """--apply uninstall with no AGENT_APPLY_APP_JWT must refuse — operator gh + cannot perform DELETE /app/installations/{id} (it needs an App JWT).""" + env = dict(os.environ) + env.pop("AGENT_APPLY_APP_JWT", None) + bin_dir, _ = shim_bin + env["PATH"] = f"{bin_dir}{os.pathsep}{env['PATH']}" + res = subprocess.run( + ["bash", str(_SCRIPT), "app", "--apply", "--baseline", str(baseline)], + capture_output=True, + text=True, + env=env, + ) + assert res.returncode != 0 + assert "needs an APP JWT" in res.stderr + + +def test_apply_app_uninstall_with_jwt_invokes_delete( + baseline: Path, shim_bin: tuple[Path, Path] +) -> None: + """With an App JWT present, --apply uninstall issues the DELETE to the + (shimmed) gh with a Bearer Authorization header.""" + env = dict(os.environ) + env["AGENT_APPLY_APP_JWT"] = "jwt-token-abc" + bin_dir, calls_log = shim_bin + env["PATH"] = f"{bin_dir}{os.pathsep}{env['PATH']}" + res = subprocess.run( + ["bash", str(_SCRIPT), "app", "--apply", "--baseline", str(baseline)], + capture_output=True, + text=True, + env=env, + ) + assert res.returncode == 0, res.stderr + res.stdout + calls = calls_log.read_text(encoding="utf-8") + assert "DELETE app/installations/424242" in calls + assert "Authorization: Bearer jwt-token-abc" in calls + + +def test_dry_run_protection_restores_full_baseline(baseline: Path) -> None: + res = _run("protection", baseline=baseline) + out = res.stdout + # The FULL protection object is restored (not just enforce_admins). + assert "PUT full branch protection on main from baseline protection.full" in out + # And the post-restore asserts both enforce_admins and the full object. + assert "assert enforce_admins.enabled == true" in out + assert "assert LIVE protection == baseline protection.full" in out + + +def test_dry_run_protection_without_full_falls_back_to_enforce_admins( + tmp_path: Path, +) -> None: + data = json.loads(json.dumps(_BASELINE)) + del data["protection"]["full"] + p = tmp_path / "nofull.json" + p.write_text(json.dumps(data), encoding="utf-8") + res = _run("protection", baseline=p) + out = res.stdout + assert "no protection.full recorded" in out + assert "branches/main/protection/enforce_admins" in out + + +def test_dry_run_incident_path_full_sequence(baseline: Path) -> None: + res = _run( + "incident", + "--flip-pr", + "13", + "--flip-branch", + "agent-team/apply/t9", + baseline=baseline, + ) + assert res.returncode == 0, res.stderr + out = res.stdout + # a. neutralise App b. revert draft PR/branch c. audit Checks d. restore e. note + assert "Neutralise the GitHub App" in out + # The corrected model: NOT an operator-gh token revoke. + assert "DELETE installation/token" not in out + assert "UNINSTALL App 'agent-apply' installation 424242" in out + assert "gh pr close 13" in out + assert "git/refs/heads/agent-team/apply/t9" in out + assert "Audit the Checks trail" in out + assert "gh run list" in out + assert "Restore the agent-apply environment" in out + assert "Restore the FULL branch protection baseline" in out + assert "incident note" in out + + +# --------------------------------------------------------------------------- # +# Argument parsing — fail closed +# --------------------------------------------------------------------------- # + + +def test_missing_surface_exits_2(baseline: Path) -> None: + res = _run(baseline=baseline) + assert res.returncode == 2 + assert "a surface is required" in res.stderr + + +def test_unknown_flag_exits_2(baseline: Path) -> None: + res = _run("workflow", "--bogus", baseline=baseline) + assert res.returncode == 2 + assert "unknown argument: --bogus" in res.stderr + + +def test_two_surfaces_is_rejected(baseline: Path) -> None: + res = _run("workflow", "protection", baseline=baseline) + assert res.returncode == 2 + assert "surface already set" in res.stderr + + +def test_flag_missing_value_exits_2(baseline: Path) -> None: + # --flip-pr with no following value (the trailing --baseline is consumed as + # the value, but then --baseline has no value -> still a parse error path). + res = subprocess.run( + ["bash", str(_SCRIPT), "workflow", "--flip-pr"], + capture_output=True, + text=True, + ) + assert res.returncode == 2 + + +def test_help_exits_0_and_lists_surfaces() -> None: + res = subprocess.run( + ["bash", str(_SCRIPT), "--help"], capture_output=True, text=True + ) + assert res.returncode == 0 + for surface in ("workflow", "environment", "app", "protection", "incident"): + assert surface in res.stdout + + +# --------------------------------------------------------------------------- # +# Fail-closed posture +# --------------------------------------------------------------------------- # + + +def test_missing_baseline_file_is_refused(tmp_path: Path) -> None: + missing = tmp_path / "nope.json" + res = _run("protection", baseline=missing) + assert res.returncode != 0 + assert "baseline file not found" in res.stderr + + +def test_protection_baseline_without_admins_on_is_refused(tmp_path: Path) -> None: + data = json.loads(json.dumps(_BASELINE)) + data["protection"]["include_administrators"] = False + p = tmp_path / "weak.json" + p.write_text(json.dumps(data), encoding="utf-8") + res = _run("protection", baseline=p) + assert res.returncode != 0 + assert "include_administrators is not true" in res.stderr + + +def test_repo_mismatch_is_refused(baseline: Path) -> None: + res = _run("protection", "--repo", "evil/other", baseline=baseline) + assert res.returncode != 0 + assert "repo mismatch" in res.stderr + + +def test_workflow_premerge_requires_flip_pr(baseline: Path) -> None: + res = _run("workflow", baseline=baseline) + assert res.returncode != 0 + assert "pre-merge path needs --flip-pr" in res.stderr + + +def test_workflow_postmerge_requires_flip_commit(baseline: Path) -> None: + res = _run("workflow", "--merged", baseline=baseline) + assert res.returncode != 0 + assert "post-merge path needs --flip-commit" in res.stderr + + +# --------------------------------------------------------------------------- # +# --apply against a PATH-shimmed gh/git (records argv, no real side effects) +# --------------------------------------------------------------------------- # + + +def test_apply_protection_invokes_gh_and_asserts( + baseline: Path, shim_bin: tuple[Path, Path] +) -> None: + _, calls_log = shim_bin + res = _run("protection", "--apply", baseline=baseline, shim=shim_bin) + assert res.returncode == 0, res.stderr + res.stdout + assert "[APPLY]" in res.stdout + # The post-restore assert ran and held (shim emits 'true'). + assert "[OK]" in res.stdout + calls = calls_log.read_text(encoding="utf-8") + # The FULL protection object was PUT to the (shimmed) gh, then asserted. + assert ( + "PUT repos/Sea-Haven-Industries/orchestrator/branches/main/protection" in calls + ) + assert "branches/main/protection/enforce_admins" in calls + + +def test_apply_environment_puts_reviewer_ids_and_asserts( + baseline: Path, shim_bin: tuple[Path, Path] +) -> None: + _, calls_log = shim_bin + res = _run("environment", "--apply", baseline=baseline, shim=shim_bin) + assert res.returncode == 0, res.stderr + res.stdout + calls = calls_log.read_text(encoding="utf-8") + assert "environments/agent-apply" in calls + # Reviewers are sent as proper typed JSON fields, by NUMERIC id. + assert "reviewers[][type]=User" in calls + assert "reviewers[][id]=1234567" in calls + # The post-restore assert compared live reviewer ids to the baseline and held. + assert "[OK]" in res.stdout + + +def test_apply_workflow_premerge_records_close_and_branch_delete( + baseline: Path, shim_bin: tuple[Path, Path] +) -> None: + _, calls_log = shim_bin + res = _run( + "workflow", + "--apply", + "--flip-pr", + "7", + "--flip-branch", + "agent-team/apply/t1", + baseline=baseline, + shim=shim_bin, + ) + assert res.returncode == 0, res.stderr + res.stdout + calls = calls_log.read_text(encoding="utf-8") + assert "pr close 7" in calls + assert "git/refs/heads/agent-team/apply/t1" in calls + assert "checkout 0123abc" in calls + + +def test_dry_run_environment_resolves_login_to_id_when_no_ids_recorded( + tmp_path: Path, +) -> None: + """A baseline that recorded only logins (no required_reviewer_ids) plans a + login->id resolution via 'gh api users/{login} --jq .id'.""" + data = json.loads(json.dumps(_BASELINE)) + del data["environment"]["required_reviewer_ids"] + p = tmp_path / "logins.json" + p.write_text(json.dumps(data), encoding="utf-8") + res = _run("environment", baseline=p) + out = res.stdout + assert "resolve reviewer login 'amoussa1229'" in out + assert "gh api users/amoussa1229 --jq .id" in out + + +def test_apply_environment_resolves_login_to_id(tmp_path: Path) -> None: + """--apply with a login-only baseline resolves the login to a numeric id via + the shimmed 'gh api users/{login}' and sends it as a typed reviewer field.""" + # Build a login-only baseline. + data = json.loads(json.dumps(_BASELINE)) + del data["environment"]["required_reviewer_ids"] + bpath = tmp_path / "logins.json" + bpath.write_text(json.dumps(data), encoding="utf-8") + + # Build a shim bin in this tmp_path. + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + calls_log = tmp_path / "calls.log" + gh = bin_dir / "gh" + gh.write_text( + "#!/usr/bin/env bash\n" + f'printf "gh %s\\n" "$*" >> "{calls_log}"\n' + 'argv="$*"\n' + 'for a in "$@"; do\n' + ' case "$a" in\n' + " */enforce_admins) echo 'true'; exit 0 ;;\n" + " esac\n" + "done\n" + 'case "$argv" in\n' + " *users/*) echo '7654321'; exit 0 ;;\n" + " *environments/*) echo '7654321'; exit 0 ;;\n" + "esac\n" + "exit 0\n", + encoding="utf-8", + ) + gh.chmod(gh.stat().st_mode | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH) + + env = dict(os.environ) + env["PATH"] = f"{bin_dir}{os.pathsep}{env['PATH']}" + res = subprocess.run( + ["bash", str(_SCRIPT), "environment", "--apply", "--baseline", str(bpath)], + capture_output=True, + text=True, + env=env, + ) + assert res.returncode == 0, res.stderr + res.stdout + calls = calls_log.read_text(encoding="utf-8") + assert "api users/amoussa1229" in calls + assert "reviewers[][id]=7654321" in calls + assert "[OK]" in res.stdout + + +# --------------------------------------------------------------------------- # +# --record-baseline mode +# --------------------------------------------------------------------------- # + + +def test_record_baseline_rejects_a_surface(tmp_path: Path) -> None: + out = tmp_path / "p3-baseline.json" + res = subprocess.run( + [ + "bash", + str(_SCRIPT), + "--record-baseline", + "protection", + "--baseline", + str(out), + "--repo", + "Sea-Haven-Industries/orchestrator", + ], + capture_output=True, + text=True, + ) + assert res.returncode == 2 + assert "does not take a surface" in res.stderr + + +def test_record_baseline_dry_run_prints_plan(tmp_path: Path) -> None: + out = tmp_path / "p3-baseline.json" + res = subprocess.run( + [ + "bash", + str(_SCRIPT), + "--record-baseline", + "--baseline", + str(out), + "--repo", + "Sea-Haven-Industries/orchestrator", + ], + capture_output=True, + text=True, + ) + assert res.returncode == 0, res.stderr + assert "RECORD BASELINE" in res.stdout + assert "[PLAN]" in res.stdout + # Dry-run captures nothing. + assert not out.exists() + + +def test_record_baseline_requires_repo(tmp_path: Path) -> None: + out = tmp_path / "p3-baseline.json" + # Clear the env-derived repo default so no repo is resolvable. + env = dict(os.environ) + env.pop("AGENT_TEAM_REPO_OWNER", None) + env.pop("AGENT_TEAM_REPO_NAME", None) + res = subprocess.run( + ["bash", str(_SCRIPT), "--record-baseline", "--baseline", str(out)], + capture_output=True, + text=True, + env=env, + ) + assert res.returncode != 0 + assert "no target repo" in res.stderr + + +def test_record_baseline_apply_writes_baseline_from_live_state( + tmp_path: Path, +) -> None: + """--record-baseline --apply captures the live workflow SHA, env reviewer + ids, full branch protection and App installation id into the baseline JSON, + creating the directory if absent. The recorded file is then a valid restore + target whose protection.include_administrators is True.""" + out = tmp_path / ".security-review" / "p3-baseline.json" # dir absent on purpose + + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + calls_log = tmp_path / "calls.log" + protection_json = json.dumps(_BASELINE["protection"]["full"]) + gh = bin_dir / "gh" + gh.write_text( + "#!/usr/bin/env bash\n" + f'printf "gh %s\\n" "$*" >> "{calls_log}"\n' + 'argv="$*"\n' + 'case "$argv" in\n' + " *contents/*) echo 'deadbeefsha'; exit 0 ;;\n" + " *branches/*/protection*)\n" + f" cat <<'JSON'\n{protection_json}\nJSON\n" + " exit 0 ;;\n" + " *environments/*) echo '[1234567]'; exit 0 ;;\n" + "esac\n" + "exit 0\n", + encoding="utf-8", + ) + gh.chmod(gh.stat().st_mode | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH) + + env = dict(os.environ) + env["PATH"] = f"{bin_dir}{os.pathsep}{env['PATH']}" + res = subprocess.run( + [ + "bash", + str(_SCRIPT), + "--record-baseline", + "--apply", + "--baseline", + str(out), + "--repo", + "Sea-Haven-Industries/orchestrator", + "--app-installation-id", + "424242", + ], + capture_output=True, + text=True, + env=env, + ) + assert res.returncode == 0, res.stderr + res.stdout + assert out.exists(), "baseline file (and its dir) must be created" + recorded = json.loads(out.read_text(encoding="utf-8")) + assert recorded["repo"] == "Sea-Haven-Industries/orchestrator" + assert recorded["workflow_baseline_sha"] == "deadbeefsha" + assert recorded["environment"]["required_reviewer_ids"] == [1234567] + assert recorded["app"]["installation_id"] == 424242 + assert recorded["app"]["action"] == "uninstall" + assert recorded["protection"]["include_administrators"] is True + assert recorded["protection"]["full"]["enforce_admins"]["enabled"] is True + + +def test_recorded_baseline_is_a_valid_restore_target(tmp_path: Path) -> None: + """A baseline produced by --record-baseline --apply can be fed straight back + into a restore (dry-run) without error — closing the record->restore loop.""" + out = tmp_path / ".security-review" / "p3-baseline.json" + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + calls_log = tmp_path / "calls.log" + protection_json = json.dumps(_BASELINE["protection"]["full"]) + gh = bin_dir / "gh" + gh.write_text( + "#!/usr/bin/env bash\n" + f'printf "gh %s\\n" "$*" >> "{calls_log}"\n' + 'argv="$*"\n' + 'case "$argv" in\n' + " *contents/*) echo 'deadbeefsha'; exit 0 ;;\n" + " *branches/*/protection*)\n" + f" cat <<'JSON'\n{protection_json}\nJSON\n" + " exit 0 ;;\n" + " *environments/*) echo '[1234567]'; exit 0 ;;\n" + "esac\n" + "exit 0\n", + encoding="utf-8", + ) + gh.chmod(gh.stat().st_mode | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH) + env = dict(os.environ) + env["PATH"] = f"{bin_dir}{os.pathsep}{env['PATH']}" + rec = subprocess.run( + [ + "bash", + str(_SCRIPT), + "--record-baseline", + "--apply", + "--baseline", + str(out), + "--repo", + "Sea-Haven-Industries/orchestrator", + "--app-installation-id", + "424242", + ], + capture_output=True, + text=True, + env=env, + ) + assert rec.returncode == 0, rec.stderr + rec.stdout + + # Now restore (dry-run) from the recorded baseline. + res = subprocess.run( + ["bash", str(_SCRIPT), "protection", "--baseline", str(out)], + capture_output=True, + text=True, + ) + assert res.returncode == 0, res.stderr + res.stdout + assert "PUT full branch protection" in res.stdout + + +def test_apply_protection_DETECTS_divergent_live_restore( + baseline: Path, tmp_path: Path +) -> None: + """Regression for the normalize_protection heredoc bug (vacuous assert). + + Previously ``normalize_protection`` ran ``python3 - <<'PY'`` and read + ``json.load(sys.stdin)`` — but the heredoc IS stdin, so it parsed the program + text, raised, and the bare except swallowed it to "" for EVERY input. Both + live and baseline normalized to "" and the post-restore assert was always + "" == "" -> a broken protection restore reported SUCCESS. The fix reads the + JSON from argv[1]. This test feeds a LIVE protection that DIFFERS from the + baseline and asserts the script now FAILS (non-zero) instead of falsely + passing. + """ + import copy + + divergent = copy.deepcopy(_BASELINE["protection"]["full"]) + # Flip a projected field so the normalized live != normalized baseline. + divergent["allow_force_pushes"] = {"enabled": True} + + bin_dir = tmp_path / "divbin" + bin_dir.mkdir() + calls_log = tmp_path / "divcalls.log" + div_json = json.dumps(divergent) + gh = bin_dir / "gh" + gh.write_text( + "#!/usr/bin/env bash\n" + f'printf "gh %s\\n" "$*" >> "{calls_log}"\n' + 'argv="$*"\n' + 'for a in "$@"; do\n' + ' case "$a" in\n' + " */enforce_admins) echo 'true'; exit 0 ;;\n" + " esac\n" + "done\n" + 'case "$argv" in\n' + " *branches/*/protection*)\n" + f" cat <<'JSON'\n{div_json}\nJSON\n" + " exit 0 ;;\n" + " *users/*) echo '1234567'; exit 0 ;;\n" + " *environments/*) echo '1234567'; exit 0 ;;\n" + "esac\n" + "exit 0\n", + encoding="utf-8", + ) + git = bin_dir / "git" + git.write_text( + f'#!/usr/bin/env bash\nprintf "git %s\\n" "$*" >> "{calls_log}"\nexit 0\n', + encoding="utf-8", + ) + for f in (gh, git): + f.chmod(f.stat().st_mode | stat.S_IEXEC | stat.S_IXGRP | stat.S_IXOTH) + + res = _run("protection", "--apply", baseline=baseline, shim=(bin_dir, calls_log)) + assert res.returncode != 0, ( + "divergent live protection must FAIL the post-restore assert, not pass:\n" + + res.stdout + + res.stderr + ) + assert "protection.full" in (res.stdout + res.stderr) diff --git a/agent-team/tests/test_run_team.py b/agent-team/tests/test_run_team.py index f278b22..ec9c559 100644 --- a/agent-team/tests/test_run_team.py +++ b/agent-team/tests/test_run_team.py @@ -673,6 +673,9 @@ class _FakeCoordinator: self.ci_poller = ci_poller self.ci_timeout = ci_timeout self._ci_pending_provider: Any = None + # Draft-PR runaway/stale monitor provider (P3 A4): the live serve path + # binds a read-only enumerator here post-construction; inert leaves None. + self._draft_pr_provider: Any = None self.setup_called = False self.start_kwargs: dict[str, Any] | None = None self.new_task_callback: Any = None @@ -924,6 +927,9 @@ def test_serve_binds_inert_p3_wiring_when_env_unset( assert coord.ci_poller is None assert coord.ci_timeout is None assert coord._ci_pending_provider is None + # A4 draft-PR monitor stays inert too, gated on the same signal as ci_poller: + # the sweep is a NO-OP, so no production runaway/stale sweep ever fires. + assert coord._draft_pr_provider is None def test_serve_binds_live_p3_wiring_when_env_set( @@ -951,6 +957,109 @@ def test_serve_binds_live_p3_wiring_when_env_set( assert callable(coord.ci_poller) assert coord.ci_timeout == timedelta(minutes=30) assert coord._ci_pending_provider == coord._enumerate_ci_pending + # A4 draft-PR monitor: the live serve path binds a read-only provider so the + # sweep has a real snapshot to ALARM / remind on (gated on the same live-pair + # signal as ci_poller). Without this the wired sweep would always see no PRs. + assert callable(coord._draft_pr_provider) + + +def test_serve_draft_pr_provider_reads_open_apply_draft_prs( + cli: ModuleType, + db_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The bound draft-PR provider is READ-ONLY: it shells a scoped ``gh pr list`` + (no write/close/dispatch) and maps the JSON into ``DraftPr`` snapshots.""" + from agent_team.draft_pr_monitor import DraftPr + + monkeypatch.setenv("AGENT_TEAM_REPO_OWNER", "Sea-Haven-Industries") + monkeypatch.setenv("AGENT_TEAM_REPO_NAME", "orchestrator") + monkeypatch.setenv("AGENT_TEAM_CI_READ_TOKEN", "ghp_test") + _FakeCoordinator.instances.clear() + monkeypatch.setattr( + "agent_team.coordinator.Coordinator", _FakeCoordinator, raising=True + ) + + captured: dict[str, Any] = {} + + class _FakeProc: + stdout = json.dumps( + [ + { + "number": 7, + "createdAt": "2026-06-23T10:00:00Z", + "updatedAt": "2026-06-23T10:05:00Z", + }, + { + "number": 9, + "createdAt": "2026-06-10T09:00:00Z", + "updatedAt": "2026-06-10T09:00:00Z", + }, + ] + ) + + def _fake_run(args: Any, **kwargs: Any) -> _FakeProc: + # The provider must shell a READ-ONLY, namespace-scoped enumeration — + # never a write/close/merge subcommand. + captured["args"] = args + captured["kwargs"] = kwargs + return _FakeProc() + + monkeypatch.setattr("subprocess.run", _fake_run, raising=True) + + coord = cli._build_coordinator(_serve_args(db_path, "serve")) + assert callable(coord._draft_pr_provider) + + prs = coord._draft_pr_provider() + + # READ-ONLY + scoped: it is a `gh pr list` over the apply/ head namespace, not + # a mutating subcommand, and never carries --shell. + assert captured["args"][:3] == ["gh", "pr", "list"] + assert "--draft" in captured["args"] + assert f"head:{cli._DRAFT_PR_HEAD_PREFIX}" in captured["args"] + assert "Sea-Haven-Industries/orchestrator" in captured["args"] + assert not any( + tok in captured["args"] for tok in ("close", "merge", "edit", "ready") + ) + # The JSON rows map field-for-field into the snapshot the monitor expects. + assert prs == [ + DraftPr( + number=7, + opened_at="2026-06-23T10:00:00Z", + updated_at="2026-06-23T10:05:00Z", + ), + DraftPr( + number=9, + opened_at="2026-06-10T09:00:00Z", + updated_at="2026-06-10T09:00:00Z", + ), + ] + + +def test_serve_draft_pr_provider_fails_soft_on_gh_error( + cli: ModuleType, + db_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A non-zero ``gh`` exit yields an empty snapshot (the sweep no-ops) rather + than raising and breaking the tick loop.""" + import subprocess + + monkeypatch.setenv("AGENT_TEAM_REPO_OWNER", "Sea-Haven-Industries") + monkeypatch.setenv("AGENT_TEAM_REPO_NAME", "orchestrator") + monkeypatch.setenv("AGENT_TEAM_CI_READ_TOKEN", "ghp_test") + _FakeCoordinator.instances.clear() + monkeypatch.setattr( + "agent_team.coordinator.Coordinator", _FakeCoordinator, raising=True + ) + + def _boom(args: Any, **kwargs: Any) -> Any: + raise subprocess.CalledProcessError(returncode=1, cmd=args) + + monkeypatch.setattr("subprocess.run", _boom, raising=True) + + coord = cli._build_coordinator(_serve_args(db_path, "serve")) + assert coord._draft_pr_provider() == [] def test_start_does_not_bind_p3_wiring_even_when_env_set( diff --git a/agent-team/tests/test_runaway_monitor.py b/agent-team/tests/test_runaway_monitor.py new file mode 100644 index 0000000..2b07efc --- /dev/null +++ b/agent-team/tests/test_runaway_monitor.py @@ -0,0 +1,400 @@ +"""Unit tests for agent_team.draft_pr_monitor (P3 box-integration, A4). + +Covers the draft-PR runaway/stale sweep: it ALARMs when > 3 draft PRs open +within 15 minutes, reminds on a draft PR idle > 7 days, applies flapping backoff +(one ALARM per cooldown, one reminder per PR per cooldown), and never auto-closes +or self-stops. No network: every side effect (alarm, reminder) is an injected +callable, exactly as the module's contract promises. +""" + +from __future__ import annotations + +from datetime import datetime, timedelta, timezone + +from agent_team.draft_pr_monitor import ( + DEFAULT_ALARM_COOLDOWN, + DEFAULT_STALE_COOLDOWN, + DraftPr, + MonitorAction, + MonitorMemory, + run_draft_pr_monitor, +) + +# A fixed "now"; ISO strings mirror the GitHub API / ledger (UTC). +_NOW = datetime(2026, 6, 23, 12, 0, 0, tzinfo=timezone.utc) + + +def _ago(**kwargs: float) -> str: + return (_NOW - timedelta(**kwargs)).isoformat() + + +class _Recorder: + """Records the runaway counts / stale PRs handed to the side-effect hooks.""" + + def __init__(self) -> None: + self.alarms: list[int] = [] + self.reminders: list[int] = [] + + def on_alarm(self, opened_in_window: int) -> None: + self.alarms.append(opened_in_window) + + def on_stale_reminder(self, pr: DraftPr) -> None: + self.reminders.append(pr.number) + + +def _open_burst(count: int, *, within_minutes: int = 5) -> list[DraftPr]: + """``count`` draft PRs all opened ``within_minutes`` ago (inside the window).""" + return [ + DraftPr( + number=i, + opened_at=_ago(minutes=within_minutes), + updated_at=_NOW.isoformat(), + ) + for i in range(count) + ] + + +# --------------------------------------------------------------------------- # +# Runaway ALARM threshold (> 3 within 15 min) +# --------------------------------------------------------------------------- # + + +def test_runaway_alarm_fires_above_threshold() -> None: + rec = _Recorder() + report = run_draft_pr_monitor( + _open_burst(4), + on_alarm=rec.on_alarm, + on_stale_reminder=rec.on_stale_reminder, + now=_NOW, + ) + assert report.alarmed == 1 + assert rec.alarms == [4] + assert report.outcomes[0].action is MonitorAction.ALARMED + + +def test_exactly_threshold_does_not_alarm() -> None: + """The rule is STRICTLY greater than 3 — exactly 3 must NOT alarm.""" + rec = _Recorder() + report = run_draft_pr_monitor( + _open_burst(3), + on_alarm=rec.on_alarm, + on_stale_reminder=rec.on_stale_reminder, + now=_NOW, + ) + assert report.alarmed == 0 + assert rec.alarms == [] + + +def test_prs_opened_outside_window_are_not_counted() -> None: + """5 PRs but only 2 inside the 15-min window -> no alarm.""" + rec = _Recorder() + prs = [ + DraftPr(number=1, opened_at=_ago(minutes=2)), + DraftPr(number=2, opened_at=_ago(minutes=10)), + DraftPr(number=3, opened_at=_ago(minutes=20)), + DraftPr(number=4, opened_at=_ago(minutes=40)), + DraftPr(number=5, opened_at=_ago(hours=3)), + ] + report = run_draft_pr_monitor( + prs, + on_alarm=rec.on_alarm, + on_stale_reminder=rec.on_stale_reminder, + now=_NOW, + ) + assert report.opened_in_window == 2 + assert report.alarmed == 0 + assert rec.alarms == [] + + +def test_unparseable_opened_at_is_skipped_for_runaway() -> None: + """A PR with a junk/missing opened_at is not counted and does not crash.""" + rec = _Recorder() + prs = _open_burst(4) + [DraftPr(number=99, opened_at="not-a-date")] + report = run_draft_pr_monitor( + prs, + on_alarm=rec.on_alarm, + on_stale_reminder=rec.on_stale_reminder, + now=_NOW, + ) + # The 4 valid PRs still breach; the junk one is just ignored for the count. + assert report.opened_in_window == 4 + assert report.alarmed == 1 + + +# --------------------------------------------------------------------------- # +# Flapping backoff — ALARM at most once per cooldown +# --------------------------------------------------------------------------- # + + +def test_runaway_alarm_suppressed_within_cooldown() -> None: + rec = _Recorder() + mem = MonitorMemory() + prs = _open_burst(5) + + first = run_draft_pr_monitor( + prs, + on_alarm=rec.on_alarm, + on_stale_reminder=rec.on_stale_reminder, + memory=mem, + now=_NOW, + ) + assert first.alarmed == 1 + + # A second tick a minute later, condition still breaching -> suppressed. + second = run_draft_pr_monitor( + prs, + on_alarm=rec.on_alarm, + on_stale_reminder=rec.on_stale_reminder, + memory=mem, + now=_NOW + timedelta(minutes=1), + ) + assert second.alarmed == 0 + assert second.alarm_suppressed == 1 + assert rec.alarms == [5] # only the first post + + +def test_runaway_alarm_refires_after_cooldown() -> None: + rec = _Recorder() + mem = MonitorMemory() + prs = _open_burst(5, within_minutes=1) + + run_draft_pr_monitor( + prs, + on_alarm=rec.on_alarm, + on_stale_reminder=rec.on_stale_reminder, + memory=mem, + now=_NOW, + ) + # Past the cooldown, a still-breaching condition ALARMs again. Re-anchor the + # PRs so they remain inside the window relative to the later "now". + later = _NOW + DEFAULT_ALARM_COOLDOWN + timedelta(seconds=1) + prs_later = [ + DraftPr(number=p.number, opened_at=(later - timedelta(minutes=1)).isoformat()) + for p in prs + ] + report = run_draft_pr_monitor( + prs_later, + on_alarm=rec.on_alarm, + on_stale_reminder=rec.on_stale_reminder, + memory=mem, + now=later, + ) + assert report.alarmed == 1 + assert len(rec.alarms) == 2 + + +# --------------------------------------------------------------------------- # +# Stale reminder (> 7 days idle), never auto-closes +# --------------------------------------------------------------------------- # + + +def test_stale_pr_reminds_once() -> None: + rec = _Recorder() + prs = [DraftPr(number=7, opened_at=_ago(days=30), updated_at=_ago(days=8))] + report = run_draft_pr_monitor( + prs, + on_alarm=rec.on_alarm, + on_stale_reminder=rec.on_stale_reminder, + now=_NOW, + ) + assert report.stale_reminded == 1 + assert rec.reminders == [7] + + +def test_fresh_pr_does_not_remind() -> None: + rec = _Recorder() + prs = [DraftPr(number=7, opened_at=_ago(days=30), updated_at=_ago(days=6))] + report = run_draft_pr_monitor( + prs, + on_alarm=rec.on_alarm, + on_stale_reminder=rec.on_stale_reminder, + now=_NOW, + ) + assert report.stale_reminded == 0 + assert rec.reminders == [] + + +def test_exactly_7_days_does_not_remind() -> None: + """The rule is strictly MORE than 7 days idle.""" + rec = _Recorder() + prs = [DraftPr(number=7, updated_at=_ago(days=7))] + report = run_draft_pr_monitor( + prs, + on_alarm=rec.on_alarm, + on_stale_reminder=rec.on_stale_reminder, + now=_NOW, + ) + assert report.stale_reminded == 0 + + +def test_stale_reminder_suppressed_within_cooldown() -> None: + rec = _Recorder() + mem = MonitorMemory() + prs = [DraftPr(number=7, updated_at=_ago(days=10))] + + run_draft_pr_monitor( + prs, + on_alarm=rec.on_alarm, + on_stale_reminder=rec.on_stale_reminder, + memory=mem, + now=_NOW, + ) + second = run_draft_pr_monitor( + prs, + on_alarm=rec.on_alarm, + on_stale_reminder=rec.on_stale_reminder, + memory=mem, + now=_NOW + timedelta(hours=1), + ) + assert second.stale_reminded == 0 + assert second.stale_suppressed == 1 + assert rec.reminders == [7] # only the first + + +def test_stale_reminder_refires_after_cooldown() -> None: + rec = _Recorder() + mem = MonitorMemory() + + run_draft_pr_monitor( + [DraftPr(number=7, updated_at=_ago(days=10))], + on_alarm=rec.on_alarm, + on_stale_reminder=rec.on_stale_reminder, + memory=mem, + now=_NOW, + ) + later = _NOW + DEFAULT_STALE_COOLDOWN + timedelta(seconds=1) + report = run_draft_pr_monitor( + [DraftPr(number=7, updated_at=(later - timedelta(days=10)).isoformat())], + on_alarm=rec.on_alarm, + on_stale_reminder=rec.on_stale_reminder, + memory=mem, + now=later, + ) + assert report.stale_reminded == 1 + assert len(rec.reminders) == 2 + + +def test_unparseable_updated_at_is_skipped_for_stale() -> None: + rec = _Recorder() + prs = [DraftPr(number=7, updated_at="garbage")] + report = run_draft_pr_monitor( + prs, + on_alarm=rec.on_alarm, + on_stale_reminder=rec.on_stale_reminder, + now=_NOW, + ) + assert report.stale_reminded == 0 + assert rec.reminders == [] + + +# --------------------------------------------------------------------------- # +# Side-effect isolation + watermark-not-advanced-on-error +# --------------------------------------------------------------------------- # + + +def test_alarm_side_effect_error_is_isolated_and_retried() -> None: + """A raising on_alarm yields ERRORED and does NOT advance the watermark.""" + calls: list[int] = [] + + def boom(opened_in_window: int) -> None: + calls.append(opened_in_window) + raise RuntimeError("slack down") + + mem = MonitorMemory() + prs = _open_burst(5) + report = run_draft_pr_monitor( + prs, + on_alarm=boom, + on_stale_reminder=lambda pr: None, + memory=mem, + now=_NOW, + ) + assert report.errored == 1 + assert report.alarmed == 0 + # Watermark not advanced -> a retry the very next tick (no cooldown lock-in). + assert mem.last_alarm_at is None + report2 = run_draft_pr_monitor( + prs, + on_alarm=boom, + on_stale_reminder=lambda pr: None, + memory=mem, + now=_NOW + timedelta(seconds=30), + ) + assert report2.errored == 1 + assert len(calls) == 2 + + +def test_stale_side_effect_error_does_not_abort_sweep() -> None: + """A reminder that raises for one PR does not stop the runaway ALARM.""" + rec = _Recorder() + + def boom(pr: DraftPr) -> None: + raise RuntimeError("slack down") + + prs = _open_burst(5) + [DraftPr(number=50, updated_at=_ago(days=10))] + report = run_draft_pr_monitor( + prs, + on_alarm=rec.on_alarm, + on_stale_reminder=boom, + now=_NOW, + ) + # Runaway still alarms; the stale reminder errored but was isolated. + assert report.alarmed == 1 + assert report.errored == 1 + + +# --------------------------------------------------------------------------- # +# Never auto-closes / self-stops; memory hygiene +# --------------------------------------------------------------------------- # + + +def test_monitor_takes_no_infrastructure_action() -> None: + """The only effects are the two injected notify callables — nothing else. + + There is no close/stop/dispatch seam on the monitor; this asserts the sweep + exposes ONLY the alarm + reminder hooks (a regression guard against adding a + self-acting side effect). + """ + import inspect + + sig = inspect.signature(run_draft_pr_monitor) + effect_params = {p for p in sig.parameters if p.startswith("on_")} + assert effect_params == {"on_alarm", "on_stale_reminder"} + + +def test_memory_prunes_gone_prs() -> None: + """A PR no longer present has its backoff watermark pruned (bounded growth).""" + rec = _Recorder() + mem = MonitorMemory() + + run_draft_pr_monitor( + [DraftPr(number=7, updated_at=_ago(days=10))], + on_alarm=rec.on_alarm, + on_stale_reminder=rec.on_stale_reminder, + memory=mem, + now=_NOW, + ) + assert 7 in mem.last_reminded_at + + # Next pass: PR #7 is gone (merged/closed by a human). Its watermark prunes. + run_draft_pr_monitor( + [], + on_alarm=rec.on_alarm, + on_stale_reminder=rec.on_stale_reminder, + memory=mem, + now=_NOW + timedelta(minutes=1), + ) + assert 7 not in mem.last_reminded_at + + +def test_no_draft_prs_is_quiet() -> None: + rec = _Recorder() + report = run_draft_pr_monitor( + [], + on_alarm=rec.on_alarm, + on_stale_reminder=rec.on_stale_reminder, + now=_NOW, + ) + assert report.outcomes == [] + assert rec.alarms == [] + assert rec.reminders == [] diff --git a/docs/provisioning/OPERATOR-RUNBOOK.md b/docs/provisioning/OPERATOR-RUNBOOK.md index 98f1f38..e039885 100644 --- a/docs/provisioning/OPERATOR-RUNBOOK.md +++ b/docs/provisioning/OPERATOR-RUNBOOK.md @@ -308,6 +308,114 @@ follow the escalation ladder below. --- +## Incident 7 — P3 rollback (unwind the apply/verify flip) + +The P3 apply/verify CI workflow is **already LIVE** (flipped + provisioned +2026-06-22): the `agent-apply` environment with its required reviewer, the GitHub +App installation (`pull-requests: write`), the run-name/permissions edits on +`.github/workflows/agent-team-apply-verify.yml`, and `enforce_admins` branch +protection on `main`. `agent-team/scripts/p3_rollback.sh` is the inverse — it +restores those four privileged surfaces to a recorded baseline and asserts +post-restore == baseline (fail-closed: a mismatched restore aborts non-zero). + +> **Run from the operator host (the Mac), NOT the box.** No standing box token is +> used; the script relies on `gh` being authenticated on the operator host (plus +> `git` for the post-merge revert path). The R720 daemon holds no write token by +> design (P3-PHASE0-DESIGN.md, "Out of scope"). + +**When to trigger:** +- The flip must be unwound (P3 apply/verify is being backed out), OR +- A **premature flip that RAN** — an apply/verify run executed before it should + have and opened a draft PR / branch that must not exist (use the `incident` + surface, below). + +**Prerequisites:** +- A baseline JSON recorded **BEFORE** the flip (the restore target). Default path + `.security-review/p3-baseline.json` or `$P3_ROLLBACK_BASELINE`, override with + `--baseline FILE`. The script refuses to run if the baseline file is missing + (`record it BEFORE the flip`) — there is no inferred baseline. +- Target repo from the baseline's `repo` key, `--repo OWNER/NAME`, or + `$AGENT_TEAM_REPO_OWNER/$AGENT_TEAM_REPO_NAME`. If both the baseline and `--repo` + are set they must agree (fail-closed on mismatch). + +**The four privileged surfaces it restores:** +1. **`workflow`** — the apply/verify flip itself. Pre-merge: close the flip PR + + delete its branch (`--flip-pr N [--flip-branch REF]`). Post-merge: `git revert` + the flip commit + push + re-run CI (`--merged --flip-commit SHA`). LIVE: also + reverts the workflow YAML to the recorded `workflow_baseline_sha` so the live + file matches baseline. +2. **`environment`** — the `agent-apply` environment: re-PUTs the recorded required + reviewer(s) + deployment branch policy, then asserts the environment exists. +3. **`app`** — the GitHub App: **always rotates (revokes) the installation token + first** (any minted token is now suspect), then `reduce`s permissions to the + baseline or `uninstall`s the installation per `app.action`. +4. **`protection`** — branch protection on `main`: re-enables `enforce_admins` + (`include_administrators`) and asserts it is ON. **Refuses to run if the + baseline's `include_administrators` is not `true`** — a rollback must never + leave a weaker posture than baseline. + +`all` runs surfaces 1→4 in order. + +**How to run — always `--dry-run` first, then `--apply`:** + +The script is **destructive-safe by default**: with no `--apply` it only PRINTS +the plan (`[PLAN] ...` lines). `--apply` is the only thing that performs +mutations. Inspect the dry-run plan, confirm it targets the right repo + baseline, +then re-run with `--apply`. + +```bash +cd ~/Documents/repositories/orchestrator/agent-team # operator host, gh authed + +# 1. Dry-run: print the plan only, no mutations. +scripts/p3_rollback.sh all --baseline .security-review/p3-baseline.json + +# 2. Apply, once the plan looks right (pre-merge flip example): +scripts/p3_rollback.sh all --apply \ + --baseline .security-review/p3-baseline.json \ + --flip-pr 123 --flip-branch agent-team/apply/flip + +# Post-merge workflow revert instead of pre-merge close: +scripts/p3_rollback.sh workflow --apply --merged --flip-commit + +# A single surface at a time is fine too: +scripts/p3_rollback.sh protection --apply +``` + +**Premature-flip-that-RAN incident (the `incident` surface):** + +Use this when a flip executed prematurely and opened a draft PR/branch. It runs +the full incident sequence — rotate, revert, audit (read-only), restore, note: + +```bash +# Dry-run first (the audit/list steps run read-only either way): +scripts/p3_rollback.sh incident --baseline .security-review/p3-baseline.json + +# Apply, with the premature draft PR if known: +scripts/p3_rollback.sh incident --apply \ + --flip-pr --flip-branch agent-team/apply/ +``` + +It performs, in order: +1. **Rotate the App installation token first** — anything the premature run minted + is suspect. +2. **Revert the draft PR / branch the App opened** — close `--flip-pr` + delete its + branch; if no `--flip-pr` is given it lists open `agent-team/apply/*` draft PRs + to triage. +3. **Audit the Checks trail** (read-only — always runs) — recent + `agent-team-apply-verify.yml` runs with conclusions. +4. **Restore the `agent-apply` environment + branch protection** to baseline + (surfaces 2 + 4). +5. **File an incident note** under + `.security-review/incidents/p3-premature-flip-.md` recording the + repo, baseline, flip PR/branch, and actions taken. + +After any rollback, confirm the post-restore `[OK]` asserts printed (the script +exits non-zero if any assert failed), and record the action — for the `incident` +path the note is written automatically; otherwise note it on the Jira ticket per +the escalation ladder. + +--- + ## Escalation ladder (design §5 / §6.6, resolves Q3) Anything that does not clear on the first ALARM escalates — but **ALARM-only in @@ -337,3 +445,46 @@ non-CLI recoveries note it on the Jira ticket. PROVISIONING-RUNBOOK steps. - Update `project_r720_agent_team` memory if the incident revealed a durable fact (a new failure mode, a config that must change). + +## P3 box env wiring (build → dispatch → verify) + +The coordinator's environment is loaded from `EnvironmentFile=-/home/adam/secrev.env` +(declared in the unit; the leading `-` makes it optional so a missing file does +not fail the unit). The **live** P3 path — gated build → dispatch (trigger CI, +capture `run_id`) → verify (read the CI conclusion) — reads four variables from +that file at graph-build / dispatch time: + +- `AGENT_TEAM_REPO_OWNER` — dispatch/verify target owner (fixed at factory time, + never read from pipeline state, so model output cannot redirect the target). +- `AGENT_TEAM_REPO_NAME` — dispatch/verify target repo (same fail-closed binding). +- `AGENT_TEAM_BASE_BRANCH` — PR base branch; optional, defaults to `main`. +- `AGENT_TEAM_CI_READ_TOKEN` — the **read-only** CI-result token used for the + verifier's authenticated conclusion read (falls back to `GITHUB_TOKEN`). + +If owner, repo, or the CI-read token is unset, `serve()` degrades to the INERT P3 +path (one WARNING + a `#agent-team` notice) rather than crash-looping the daemon +(Phase-0 Decision 5). No `pull-requests:write` / `contents:write` token and no +`AGENT_APPLY_APP_ID` / `AGENT_APPLY_APP_PRIVATE_KEY` may live in `~/secrev.env`: +the apply path mints its write token **inside** the CI runner from Actions +secrets, so the box holds no standing write credential. That invariant is +enforced by `scripts/assert_no_write_token.py` (the A2 audit) at provisioning and +in CI — run it before any deploy. + +### Verifying the vars load + +After installing/editing `~/secrev.env` and `systemctl daemon-reload` + +`systemctl restart agent-team-coordinator.service`, confirm systemd resolved the +P3 environment into the unit by reading its merged `Environment` property: + +``` +systemctl show agent-team-coordinator.service -p Environment +``` + +The output should list `AGENT_TEAM_REPO_OWNER`, `AGENT_TEAM_REPO_NAME`, +`AGENT_TEAM_BASE_BRANCH` (if set), and `AGENT_TEAM_CI_READ_TOKEN` (the value is +the read-only token — treat the command output as sensitive). If any of the three +required vars is absent here, the daemon is running the INERT P3 path; fix +`~/secrev.env`, reload, and restart. It must NOT show `AGENT_APPLY_APP_ID`, +`AGENT_APPLY_APP_PRIVATE_KEY`, or any `pull-requests:write` token — if it does, +the box is mis-provisioned (re-run `python -m scripts.assert_no_write_token` to +confirm and remediate before continuing). diff --git a/docs/provisioning/P3-LIVE-FLIP-PLAN.md b/docs/provisioning/P3-LIVE-FLIP-PLAN.md index 9f5c68e..bced8cb 100644 --- a/docs/provisioning/P3-LIVE-FLIP-PLAN.md +++ b/docs/provisioning/P3-LIVE-FLIP-PLAN.md @@ -2,8 +2,21 @@ Formal phased plan to take the agent-team Plane-2 pipeline from **clarify+plan only** to **producing reviewable draft PRs**, while keeping the always-on R720 box -read-only and the apply path zero-AWS. Status as of 2026-06-22: **NOT STARTED** -(P3 is built but inert). This plan is the input to `/sh-plan-review` before any build. +read-only and the apply path zero-AWS. + +**Status as of 2026-06-23: PARTIALLY LIVE.** The split-CI apply/verify workflow +and its provisioning (the scoped GitHub App, the `agent-apply` environment + Actions +secrets, and the dispatched runs) are **LIVE since 2026-06-22** — i.e. the +workflow-authoring + provisioning phases below (Phases 1/1b/2) are **DONE**. The +remaining work is the **box-side build → dispatch → verify integration** on +`feat/agent-team-p3-box-integration` (BUILD → DISPATCH → VERIFY wiring; +operator-initiated dispatch; run-name correlation for run_id capture; async +CI-watch resume-on-complete; fail-safe serve default) — see `docs/P3-PHASE0-DESIGN.md` +for the recorded design decisions. The two remaining **human gates** are: (C1) re-run +`/sh-security-review` + the GPT-4.1 cross-family review against the *enabled* workflow ++ the bound box-side wiring (Phase 3 / B5), and (D) deploy → smoke-test → merge +(Phase 4). This plan was the input to `/sh-plan-review` before the build began; that +loop is complete. > **Prerequisite reading:** `docs/r720-agent-team-design.md` §3.3.2 (CI-as-verifier > trust boundary, "B4"), `PROVISIONING-RUNBOOK.md` (the P3-live-flip section), @@ -13,16 +26,29 @@ read-only and the apply path zero-AWS. Status as of 2026-06-22: **NOT STARTED** ## 1. Objective & current state -**Today (inert):** the pipeline runs `INTAKE → CLARIFIER → PLANNER → REVIEW`, but -`serve` passes `build_verify_wiring=None`, the Tier-3 fixer is `--dry-run` only, and -`agent-team/ci/agent-team-apply-verify.yml` has its privileged steps disabled with -`if: ${{ false }}` and `pull-requests:write` / `environment:` commented out. So it -clarifies + plans but writes no code and opens no PR. +**Live infrastructure (since 2026-06-22):** the split-CI apply/verify workflow +(`agent-team/ci/agent-team-apply-verify.yml`) is authored, enabled, and **provisioned** +— the scoped **GitHub App** (`pull-requests:write` + minimal contents), the +`agent-apply` **GitHub Actions Environment** (Adam as required reviewer + branch +protection), and the App credentials as **Actions secrets** all exist, and dispatched +runs have executed against it. Org CI can build/test/security-review an untrusted diff +in a sandbox, a pure-code gate confirms green from authenticated Checks-API results, +and the privileged job opens a **draft PR** — all without the box ever holding a write +token. -**After P3:** the pipeline can emit a diff, have **org CI** build/test/security-review -it in an untrusted sandbox, a pure-code gate confirm green from authenticated -Checks-API results, and a scoped **GitHub App** open a **draft PR** for human review. -The box never gains a write token. +**Not yet wired (the remaining build, on `feat/agent-team-p3-box-integration`):** the +**box-side integration** that makes a task flow BUILD → DISPATCH (trigger that live CI, +capture the run_id) → VERIFY (read its conclusion) automatically. As-built, `serve` +binds the fail-safe gated P3 wiring (degrading to the INERT path if the dispatch target +/ CI-read token is unset — Phase-0 Decision 5), dispatch is **operator-initiated** +(branch push + `gh workflow run` on operator-host credentials; the box holds no standing +write token), run_id capture is **run-name / correlation-tag** based, and the CI wait is +**async resume-on-complete** via a `tick()`-driven CI-watcher (not a blocking poll). The +Tier-3 fixer remains `--dry-run` only until the Phase-4 smoke test passes. + +**After the box-side integration + the remaining human gates:** the pipeline emits a +diff end-to-end into a reviewable **draft PR** for human review. The box never gains a +write token. ## 2. Locked decisions (carried in — do not relitigate here) @@ -35,16 +61,19 @@ The box never gains a write token. ## 3. Hard gates (must clear before the flip — these block everything) -| Gate | Why | Owner | -|---|---|---| -| `/sh-plan-review` on THIS plan | adversarial plan audit before build | me → GPT-4.1 | -| `/sh-security-review` on the apply/verify CI surface | auth + untrusted-input + CI trust boundary | me | -| GPT-4.1 cross-family review on the apply/verify CI + any permission change | mandatory for the trust-boundary / permissions surface | orchestrator | -| ~~`GH_TOKEN`→`GITHUB_TOKEN` resolved~~ ✅ DONE 2026-06-22 | transport reads `GITHUB_TOKEN`; box now has a `GITHUB_TOKEN` alias of `GH_TOKEN` in `~/secrev.env` | me | +| Gate | Why | Owner | Status | +|---|---|---|---| +| `/sh-plan-review` on THIS plan | adversarial plan audit before build | me → GPT-4.1 | ✅ DONE (Round 1 + 2, 2026-06-22) | +| `/sh-security-review` on the apply/verify CI surface | auth + untrusted-input + CI trust boundary | me | ⏳ RE-RUN on the *enabled* workflow + bound box-side wiring (C1 / Phase 3 / B5) | +| GPT-4.1 cross-family review on the apply/verify CI + any permission change | mandatory for the trust-boundary / permissions surface | orchestrator | ⏳ RE-RUN on the *enabled* workflow + bound box-side wiring (C1 / Phase 3 / B5) | +| ~~`GH_TOKEN`→`GITHUB_TOKEN` resolved~~ ✅ DONE 2026-06-22 | transport reads `GITHUB_TOKEN`; box now has a `GITHUB_TOKEN` alias of `GH_TOKEN` in `~/secrev.env` | me | ✅ DONE | -The flip does NOT proceed until `/sh-security-review` AND the GPT-4.1 cross-review on -the CI surface both pass — **and these gates are re-run against the ACTUAL enabled -workflow (Phase 3), not only the inert version** (see Phase 3). +**The apply/verify CI workflow + its provisioning are already LIVE (2026-06-22).** The +two `/sh-security-review` + GPT-4.1 cross-review gates were satisfied against the +authored workflow during build; per B5 they are **RE-RUN against the ACTUAL enabled +workflow AND the bound box-side build→dispatch→verify wiring** before the box-side +integration is deployed/merged (Phase 3). The box-side integration does NOT deploy/merge +until that re-run passes — **hard stop** (see Phase 3). > **Plan-review disposition (GPT-4.1 cross-family, 2026-06-22 — REQUEST CHANGES).** Findings > folded into §4 and Phases 1/1b: expanded denylist vectors, runner-trust, concrete @@ -117,40 +146,44 @@ workflow (Phase 3), not only the inert version** (see Phase 3). ## 5. Phases -### Phase 0 — Plan review & pre-reqs 🤖/🧑 +### Phase 0 — Plan review & pre-reqs 🤖/🧑 — ✅ DONE +> **DONE.** Plan review complete; pre-reqs resolved. (Task label: **C0**.) - [x] Run `/sh-plan-review` on this doc; fold BLOCK/FIX items in. (Round 1 + Round 2 done; this doc is the result.) -- [ ] Confirm a clean revert point (git tag main; Hyper-V snapshot of sh-secrev). +- [x] Confirm a clean revert point (git tag main; Hyper-V snapshot of sh-secrev). **Snapshot retention:** keep the pre-P3 snapshot until P3 has run clean for one full cycle (Phase 4 DoD), then prune — recorded here so it is not an open-ended snapshot. - [x] Resolve `GH_TOKEN`→`GITHUB_TOKEN` (box `~/secrev.env` now has a `GITHUB_TOKEN` alias). - **Rollback:** none (no state changed). -### Phase 1 — Author/verify the split-CI apply/verify workflow 🤖 (review-gated) -- [ ] Reconcile `agent-team/ci/agent-team-apply-verify.yml` with §4. **First confirm** the +### Phase 1 — Author/verify the split-CI apply/verify workflow 🤖 (review-gated) — ✅ DONE (2026-06-22) +> **DONE.** The split-CI apply/verify workflow is authored, enabled, and provisioned. +> The deliverables below were built and shipped; they are retained for the record and +> for the Phase-3 re-run gates to verify against. (Task label: this is part of **C2**.) +- [x] Reconcile `agent-team/ci/agent-team-apply-verify.yml` with §4. **First confirm** the PR-#17 controls are present (canonicalized denylist, egress restriction, SHA-pins, empty-hash fail-closed, Checks-API consumption); only then add the new §4 items. -- [ ] **Add denylist vectors** (§4.2): submodules/`.gitmodules`, git hooks/`core.hooksPath`/`.husky`, +- [x] **Add denylist vectors** (§4.2): submodules/`.gitmodules`, git hooks/`core.hooksPath`/`.husky`, `.gitattributes` filters, lockfile postinstall/preinstall, generated/build artifacts. Add a test suite proving canonicalization resists symlink/rename/traversal. -- [ ] **Runner-trust assertion** (§4.1): test that privileged jobs cannot run on a +- [x] **Runner-trust assertion** (§4.1): test that privileged jobs cannot run on a self-hosted/user-provided runner. -- [ ] **Concretize + threat-model the diff transport** (§4.3): pick content-addressed signed +- [x] **Concretize + threat-model the diff transport** (§4.3): pick content-addressed signed artifact (shared HMAC secret) or short-lived branch-only token; add per-task nonce anti-replay; document and test it. -- [ ] **Gate-weakening detector** (§4.5): CI step that fails on a diff adding +- [x] **Gate-weakening detector** (§4.5): CI step that fails on a diff adding `noqa`/`type: ignore`/skip/xfail/excludes/`--no-verify` or editing the gate config. The detector's pattern list is **reviewed/expanded each time a new bypass vector is found** (FIX) — record the list in code with a comment pointing here, and update it + memory when a vector is added (same discipline as the denylist below). -- [ ] **PR-metadata sanitization** (§4.6) and **ledger anti-tamper** (§4.7) implemented + tested. -- [ ] `agent_team/ci_fetcher.py` (read-only Checks-API fetcher; fails closed) + +- [x] **PR-metadata sanitization** (§4.6) and **ledger anti-tamper** (§4.7) implemented + tested. +- [x] `agent_team/ci_fetcher.py` (read-only Checks-API fetcher; fails closed) + `ci_gate.py` (pure-code green). Add a mechanism for the gate to **discover the correct required check names per repo/branch** (avoid hardcoded check-name drift across repos), **with a test that exercises discovery against every intended target repo/branch** (FIX). -- [ ] **Memory/doc update when denylist vectors change** (FIX): adding a denylist vector (here +- [x] **Memory/doc update when denylist vectors change** (FIX): adding a denylist vector (here or later) updates `project_r720_agent_team` memory + the Confluence host page in the SAME change — the denied set is operational/security-critical, not tribal knowledge. -- [ ] **Deploy-before-merge enforcement — CONCRETE, CI-enforced (B1). REQUIRED Phase-1 +- [x] **Deploy-before-merge enforcement — CONCRETE, CI-enforced (B1). REQUIRED Phase-1 deliverable; the flip does not proceed without it (no manual fallback).** "Deploy" of this change = the privileged path is *proven on the live box before the workflow PR merges*. Build, in this phase: @@ -171,63 +204,92 @@ workflow (Phase 3), not only the inert version** (see Phase 3). the Phase-1 `/sh-security-review` + cross-review hard stop. The check's implementation is itself a Phase-1 build task — that it is not yet physically built is expected for a pre-build plan; what matters is it is non-optional and flip-blocking, enforced at the Phase-1 hard stop.) -- [ ] **Concrete "no write token on the box" audit (B-QUESTION → a real check):** a +- [x] **Concrete "no write token on the box" audit (B-QUESTION → a real check):** a test/script asserting the box env + coordinator config hold no `pull-requests:write` / contents-write token (grep the live env names + assert the App token is only an Actions secret), runnable on the box and in CI. Not a prose claim. -- [ ] `/sh-security-review` + GPT-4.1 cross-review on this surface. **Hard stop until both pass.** - (NOTE: these are RE-RUN on the *enabled* workflow in Phase 3 — see B5 there.) -- **Rollback:** workflow file stays inert (`if: ${{ false }}` not yet flipped); delete the file. +- [x] `/sh-security-review` + GPT-4.1 cross-review on this surface (against the authored + workflow during build). **Hard stop until both pass.** (These are RE-RUN on the *enabled* + workflow + the bound box-side wiring in Phase 3 — the C1 human gate, see B5 there. That + re-run is the genuine outstanding gate; the in-build pass is satisfied.) +- **Rollback:** the apply/verify workflow is now LIVE; rollback is no longer "delete an inert + file" — use the Phase-1b tested rollback script (revert the workflow SHA → inert, restore + the environment + branch protection from baseline). See Phase 1b / Phase 3 rollback. -### Phase 1b — Recovery for an accidentally-merged/applied privileged change 🤖/🧑 -- [ ] **Rollback as a TESTED SCRIPT covering ALL privileged surfaces (B3)** — not a one-time +### Phase 1b — Recovery for an accidentally-merged/applied privileged change 🤖/🧑 — ✅ DONE (2026-06-22) +> **DONE.** The privileged surfaces are LIVE, so their recovery tooling shipped with them. +> The tested rollback script + the runaway/stale-PR monitoring below are built and in place; +> the Phase-3 rollback exercises the script against the live state. (Task label: part of **C2**.) +- [x] **Rollback as a TESTED SCRIPT covering ALL privileged surfaces (B3)** — not a one-time manual exercise. One scripted, re-runnable rollback that, per surface, restores from a recorded baseline: (1) the apply/verify **workflow** (revert the SHA → inert), (2) the **`agent-apply` environment** (required-reviewer + protection rules), (3) the **GitHub App permissions/installation** (rotate token, reduce/uninstall), (4) **branch protection**. The script asserts the post-restore state matches the baseline. Exercised in Phase 3 rollback AND re-runnable on demand. -- [ ] **Premature-flip-that-RAN rollback (B2).** Distinct from an accidental *merge*: cover the +- [x] **Premature-flip-that-RAN rollback (B2).** Distinct from an accidental *merge*: cover the case where `if:${{ false }}` is flipped early (or the environment gate is misconfigured) and the privileged job **actually runs** — incident steps: rotate the GitHub App token immediately, close/revert any draft PR (or branch) it opened, confirm via the Checks/PR audit trail exactly what ran in the window, restore the environment + branch protection from baseline, and file the incident. This is the "it executed" path, not just "it merged." -- [ ] Add light **monitoring on draft-PR creation rate** (runaway-volume alarm) and an +- [x] Add light **monitoring on draft-PR creation rate** (runaway-volume alarm) and an **orphaned/stale draft-PR cleanup** step. -### Phase 2 — Provision the GitHub App + environment 🧑 OPERATOR (browser/admin) -- [ ] Create a dedicated **GitHub App** with **`pull-requests:write`** (+ minimal contents to +### Phase 2 — Provision the GitHub App + environment 🧑 OPERATOR (browser/admin) — ✅ DONE (2026-06-22) +> **DONE.** Provisioning is LIVE: the scoped GitHub App, the `agent-apply` environment, and +> the App-credential Actions secrets all exist, and dispatched runs have executed against +> them. (Task label: **C0** complete.) No write token landed on the box — the App token lives +> only as an Actions secret; this is enforced by `scripts/assert_no_write_token.py`. +- [x] Create a dedicated **GitHub App** with **`pull-requests:write`** (+ minimal contents to open a branch/PR); install on the org. Token lives in **CI**, never on the box. -- [ ] Create the **`agent-apply` GitHub Actions Environment** with a **required reviewer** +- [x] Create the **`agent-apply` GitHub Actions Environment** with a **required reviewer** (Adam) + branch-protection so the privileged job cannot run unreviewed. -- [ ] Store the App credentials as repo/org **Actions secrets** (not on the box). -- **Rollback:** uninstall the App; delete the environment + secrets. +- [x] Store the App credentials as repo/org **Actions secrets** (not on the box). +- **Rollback:** uninstall the App; delete the environment + secrets (via the Phase-1b script). -### Phase 3 — Bind the live wiring (still gated by the environment) 🤖 -- [ ] **PRECONDITION (B4): Phase 2 fully complete + verified before ANY flip.** Do not proceed - until the GitHub App exists with `pull-requests:write` (+ minimal contents) and is installed, - the `agent-apply` environment exists with Adam as required reviewer + branch protection, and - the App credentials are stored as Actions secrets (NOT on the box). Verify each before the - next step; the flip is blocked otherwise. -- [ ] In the workflow: uncomment `permissions: pull-requests: write` and - `environment: agent-apply`; flip the two `if: ${{ false }}` → enabled. -- [ ] **RE-RUN BOTH GATES ON THE ENABLED WORKFLOW (B5).** `/sh-security-review` + the GPT-4.1 - cross-family review are run again against the *actual enabled* `agent-team-apply-verify.yml` - (permissions live, `if:` true) and the bound `gated_build_verify_wiring` — NOT only the inert - Phase-1 version. **Hard stop until both pass on the enabled file.** (Permissions changed → - the mandatory cross-family review is independently required here against the real diff.) -- [ ] Bind `agent_team.coordinator.gated_build_verify_wiring(...)` (real diff builder + - read-only CI-result fetcher) so a leaf calls it only **after** the gate clears. -- [ ] Set the box-side apply env vars the live path reads (read-only CI-result token + - dispatch target). Confirm **no** write token lands on the box (run the Phase-1 no-write-token audit). -- [ ] **Incremental docs (B6):** update `OPERATOR-RUNBOOK.md` + memory NOW that the flip is live +### Phase 3 — Bind the box-side build→dispatch→verify wiring 🤖 — IN PROGRESS (the remaining build) +> **Workflow flip already DONE (2026-06-22):** `permissions: pull-requests: write` and +> `environment: agent-apply` are uncommented and the two `if: ${{ false }}` are enabled — the +> workflow is LIVE. The PRECONDITION (B4) below is **satisfied** (Phase 2 provisioning is +> complete + verified). The remaining Phase-3 work is the **box-side integration** built on +> `feat/agent-team-p3-box-integration` (BUILD → DISPATCH → VERIFY; operator-initiated dispatch; +> run-name correlation; async CI-watch; fail-safe serve default — see `docs/P3-PHASE0-DESIGN.md`) +> plus the C1 re-run gates and the box env wiring. **C1 (the re-run gates) and D (deploy/smoke/ +> merge, Phase 4) remain the outstanding HUMAN gates.** +- [x] **PRECONDITION (B4): Phase 2 fully complete + verified before ANY flip.** ✅ SATISFIED — + the GitHub App exists with `pull-requests:write` (+ minimal contents) and is installed, the + `agent-apply` environment exists with Adam as required reviewer + branch protection, and the + App credentials are stored as Actions secrets (NOT on the box). +- [x] In the workflow: uncomment `permissions: pull-requests: write` and + `environment: agent-apply`; flip the two `if: ${{ false }}` → enabled. ✅ DONE 2026-06-22. +- [ ] **C1 — RE-RUN BOTH GATES ON THE ENABLED WORKFLOW + BOUND WIRING (B5). 🧑 OUTSTANDING HUMAN + GATE.** `/sh-security-review` + the GPT-4.1 cross-family review are run again against the + *actual enabled* `agent-team-apply-verify.yml` (permissions live, `if:` true) **and** the + bound box-side `gated_build_verify_wiring` (BUILD → DISPATCH → VERIFY) — NOT only the inert + Phase-1 version. **Hard stop until both pass.** (Permissions are live + the dispatch/verify + wiring is new → the mandatory cross-family review is independently required here against the + real diff.) +- [ ] Bind `agent_team.coordinator.gated_build_verify_wiring(...)` (real diff builder + dispatch + with run-name correlation run_id capture + read-only CI-result fetcher) as the **fail-safe + `serve` default** (Decision 5: degrade to the INERT P3 path if the dispatch target / CI-read + token is unset, never crash-loop). The leaf path is BUILD → DISPATCH (trigger the live CI, + suspend) → [CI-watcher resumes on terminal conclusion] → VERIFY (pure-code gate). *(Built on + `feat/agent-team-p3-box-integration`.)* +- [ ] Set the box-side apply env vars the live path reads (read-only CI-result token + dispatch + target: `AGENT_TEAM_REPO_OWNER`/`_NAME`/`_BASE_BRANCH`/`_CI_READ_TOKEN`). Confirm **no** write + token lands on the box (run the no-write-token audit, `scripts/assert_no_write_token.py`). +- [ ] **Incremental docs (B6):** update `OPERATOR-RUNBOOK.md` + memory as the box-side flip lands (do not wait for Phase 6) — what the apply path can/can't do, the denylist, the rollback. + *(The P3 box env wiring is already documented in `docs/provisioning/OPERATOR-RUNBOOK.md`.)* - **Rollback:** run the Phase-1b tested rollback script (re-set `if: ${{ false }}`, re-comment `environment:`, set `build_verify_wiring=None`, restart the coordinator). **Exercise it once here** to prove it works before relying on it. -### Phase 4 — Smoke test to a first draft PR 🧑/🤖 +### Phase 4 — Deploy, smoke test to a first draft PR, merge 🧑/🤖 — D (OUTSTANDING HUMAN GATE) +> **D — the remaining HUMAN gate.** After C1 passes, deploy the box-side integration to the live +> coordinator (`sh-secrev`, via `/sh-deploy-r720`), drive the smoke test below, then merge. Gated +> on Phase 3 (box-side wiring bound + C1 re-run gates green). - [ ] Drive one trivial, in-scope task end-to-end → confirm: untrusted job builds/tests with no secrets, denylist rejects an out-of-scope diff, pure-code gate gates on real Checks results, privileged job opens a **draft PR** with required checks attached, **nothing merged**. @@ -282,8 +344,13 @@ workflow (Phase 3), not only the inert version** (see Phase 3). | Runaway PR volume | start with Tier-3 only + one finding at a time; required-reviewer environment gates each | ## 8. Definition of done -- [ ] `/sh-plan-review`, `/sh-security-review`, and GPT-4.1 cross-review on the CI surface all passed. -- [ ] Phase-4 smoke test produced a draft PR; nothing auto-merged; rollback exercised once. -- [ ] No write token on the box (verified); apply path is zero-AWS. -- [ ] Docs + Confluence + memory updated. -- [ ] Snapshot retained until P3 runs clean for one cycle, then pruned. +- [x] `/sh-plan-review` passed; `/sh-security-review` + GPT-4.1 cross-review passed on the CI + surface during build. ⏳ **C1 outstanding:** both are RE-RUN against the *enabled* workflow + + the bound box-side wiring before the box-side integration deploys/merges (Phase 3 / B5). +- [ ] **D:** Phase-4 deploy + smoke test produced a draft PR; nothing auto-merged; rollback + exercised once. +- [x] No write token on the box (verified via `scripts/assert_no_write_token.py`); apply path is + zero-AWS. *(Re-confirm after the box-side env vars are set in Phase 3.)* +- [ ] Docs + Confluence + memory updated *(this plan + `OPERATOR-RUNBOOK.md` reflect the live + infra; Confluence + memory final reconciliation is Phase 6)*. +- [x] Snapshot retained until P3 runs clean for one cycle, then pruned.