diff --git a/agent-team/agent_team/graph.py b/agent-team/agent_team/graph.py index 685ee5c..c01576e 100644 --- a/agent-team/agent_team/graph.py +++ b/agent-team/agent_team/graph.py @@ -40,6 +40,7 @@ from __future__ import annotations import uuid from collections.abc import Callable +from contextlib import contextmanager from datetime import datetime, timedelta, timezone from pathlib import Path from typing import TYPE_CHECKING, Any @@ -56,11 +57,29 @@ from agent_team.task_model import ( from agent_team.transport import QuestionSet if TYPE_CHECKING: # pragma: no cover - typing only + from collections.abc import Iterator from contextlib import AbstractContextManager from langgraph.checkpoint.base import BaseCheckpointSaver from langgraph.graph.state import CompiledStateGraph +# The custom (non-builtin) types that travel inside a LangGraph checkpoint and +# must therefore be on the msgpack deserialization allowlist (D9, §3.3.1). Today +# the only such type is the clarifier's QuestionSet, which rides in the +# ``interrupt()`` payload (graph.clarify_node) and so is msgpack-encoded by the +# checkpoint serializer as ``(module, name, kwargs)`` and reconstructed on +# resume. Without an explicit allowlist the serializer runs in permissive mode +# and logs "Deserializing unregistered type agent_team.transport.base.QuestionSet +# ... will be blocked in a future version" on every checkpoint load; passing the +# allowlist registers the type so it deserializes silently AND keeps working when +# LangGraph flips the future default to block-unregistered. Every PipelineState +# field is a primitive/dict/list and TaskStatus/Phase are stored as ``.value`` +# strings, so QuestionSet is the complete set; add any new custom checkpoint type +# here when one is introduced (otherwise it would be blocked under strict mode). +_CHECKPOINT_ALLOWED_MSGPACK_TYPES: tuple[tuple[str, str], ...] = ( + ("agent_team.transport.base", "QuestionSet"), +) + __all__ = [ "APPROVED_ROUTE", "BUILD_NODE", @@ -73,6 +92,7 @@ __all__ = [ "PLAN", "REVIEW", "VERIFY_NODE", + "build_checkpoint_serde", "build_graph", "build_sqlite_checkpointer", "clarify_node", @@ -428,22 +448,61 @@ def build_graph( return builder.compile(checkpointer=checkpointer) +def build_checkpoint_serde() -> Any: + """Build the checkpoint serializer with the QuestionSet msgpack allowlist. + + Returns a :class:`~langgraph.checkpoint.serde.jsonplus.JsonPlusSerializer` + constructed with an explicit ``allowed_msgpack_modules`` covering every + custom type that rides inside a checkpoint (today only + :class:`~agent_team.transport.QuestionSet`; see + :data:`_CHECKPOINT_ALLOWED_MSGPACK_TYPES`). + + Why this exists: the default serializer runs in *permissive* msgpack mode + (``allowed_msgpack_modules=True``), which deserializes any type but logs + ``"Deserializing unregistered type agent_team.transport.base.QuestionSet ... + This will be blocked in a future version"`` on every checkpoint load — and a + future LangGraph release will turn that into a hard block, breaking durable + resume. Passing the explicit allowlist is the registration path that warning + recommends: the listed type deserializes silently, and the config is + already block-clean for when the default flips. (Per + ``langgraph/checkpoint/serde/jsonplus.py``: ``_create_msgpack_ext_hook`` + only emits the warning while the allowlist is the ``True`` sentinel; once an + explicit collection is supplied, an allowlisted ``(module, name)`` returns + True with no warning.) + + The ``langgraph.checkpoint.serde.jsonplus`` import is deferred to call time + so this module still imports cleanly where the optional checkpoint package + is absent (pre-deploy scaffolding). + """ + from langgraph.checkpoint.serde.jsonplus import JsonPlusSerializer + + return JsonPlusSerializer( + allowed_msgpack_modules=list(_CHECKPOINT_ALLOWED_MSGPACK_TYPES) + ) + + def build_sqlite_checkpointer( db_path: Path | str, ) -> AbstractContextManager[BaseCheckpointSaver]: """Construct the production SQLite checkpointer over ``db_path`` (D9, §3.3). - Returns a **context manager**, not an entered saver: in - ``langgraph-checkpoint-sqlite`` ``SqliteSaver.from_conn_string`` is a - ``@contextmanager`` classmethod, so the caller MUST enter it (``with`` it, - or ``__enter__`` and retain it for the graph's lifetime) before passing the - yielded saver to :func:`build_graph`. The live coordinator owns that - lifecycle (it enters the CM at setup and holds it for the daemon's life); - passing the raw return value straight into ``build_graph`` would compile a - graph whose checkpointer is an un-entered CM and break ``get_state`` / - ``invoke`` at runtime. The earlier ``-> BaseCheckpointSaver`` annotation - mis-stated this contract (review FIX); the type now matches reality so a - direct caller cannot be silently misled. + Returns a **context manager**, not an entered saver: the caller MUST enter + it (``with`` it, or ``__enter__`` and retain it for the graph's lifetime) + before passing the yielded saver to :func:`build_graph`. The live + coordinator owns that lifecycle (it enters the CM at setup and holds it for + the daemon's life); passing the raw return value straight into + ``build_graph`` would compile a graph whose checkpointer is an un-entered CM + and break ``get_state`` / ``invoke`` at runtime. + + We open the connection and construct ``SqliteSaver(conn, serde=...)`` + ourselves rather than using ``SqliteSaver.from_conn_string`` because the + latter has no seam to inject a serializer (it always builds the default, + warning-emitting one). The injected serde is :func:`build_checkpoint_serde`, + whose msgpack allowlist registers :class:`QuestionSet` so resume no longer + logs the "unregistered type" warning and stays forward-compatible with + LangGraph's coming block-by-default. The connection is opened with + ``check_same_thread=False`` (matching ``from_conn_string``) and closed when + the context manager exits. The import of ``langgraph.checkpoint.sqlite`` is deferred to call time so this module imports cleanly in environments where that optional package is @@ -466,7 +525,20 @@ def build_sqlite_checkpointer( db_path = Path(db_path) db_path.parent.mkdir(parents=True, exist_ok=True) - return SqliteSaver.from_conn_string(str(db_path)) + serde = build_checkpoint_serde() + + @contextmanager + def _saver_cm() -> Iterator[BaseCheckpointSaver]: + import sqlite3 + from contextlib import closing + + # Mirror SqliteSaver.from_conn_string's connection settings, but build + # the saver with our allowlisted serde (from_conn_string offers no serde + # seam). closing() guarantees the connection is released on exit. + with closing(sqlite3.connect(str(db_path), check_same_thread=False)) as conn: + yield SqliteSaver(conn, serde=serde) + + return _saver_cm() # --- Driver seam (thread_id-keyed). ----------------------------------------- diff --git a/agent-team/tests/test_graph.py b/agent-team/tests/test_graph.py index acc413a..2a29dbc 100644 --- a/agent-team/tests/test_graph.py +++ b/agent-team/tests/test_graph.py @@ -29,6 +29,7 @@ from agent_team.graph import ( INTAKE, P1_PHASE_SEQUENCE, PLAN, + build_checkpoint_serde, build_graph, build_sqlite_checkpointer, clarify_node, @@ -293,6 +294,107 @@ def test_build_sqlite_checkpointer_builds_when_dep_present(tmp_path) -> None: assert hasattr(saver, "get_next_version") +# --- Checkpoint serializer / QuestionSet msgpack allowlist (D9). ------------ +# QuestionSet rides in the clarifier interrupt payload and so is msgpack-encoded +# into every checkpoint. The default serializer deserializes it but logs +# "Deserializing unregistered type agent_team.transport.base.QuestionSet ... will +# be blocked in a future version" on each load, and the coming LangGraph default +# turns that warning into a hard block (breaking durable resume). These pin that +# build_checkpoint_serde registers the type so it round-trips silently AND is +# already block-clean. + +_QSET_MSGPACK_KEY = ("agent_team.transport.base", "QuestionSet") + + +def _capture_serde_warnings(): + """Attach a capturing handler to the serde logger; return (handler, buffer).""" + import io + import logging + + buf = io.StringIO() + handler = logging.StreamHandler(buf) + handler.setLevel(logging.WARNING) + logger = logging.getLogger("langgraph.checkpoint.serde.jsonplus") + logger.addHandler(handler) + logger.setLevel(logging.WARNING) + return logger, handler, buf + + +def _reset_serde_warning_dedup() -> None: + """Clear the serializer's process-wide warn-once dedup set. + + jsonplus dedups "unregistered type" warnings across the process lifetime, so + an earlier test (or the assertion below) could mask a regression. Clearing the + set makes each assertion observe the live behavior, not a stale dedup. + """ + from langgraph.checkpoint.serde import jsonplus as _jp + + _jp._warned_unregistered_types.clear() + _jp._warned_blocked_types.clear() + + +def test_build_checkpoint_serde_allowlists_questionset() -> None: + # The serde must carry QuestionSet on its msgpack allowlist (an explicit + # collection, NOT the permissive ``True`` sentinel) — that is the registration + # path the warning recommends and what makes resume forward-compatible. + pytest.importorskip("langgraph.checkpoint.serde.jsonplus") + serde = build_checkpoint_serde() + allowed = serde._allowed_msgpack_modules + assert allowed is not True # not the warn-on-everything permissive default + assert _QSET_MSGPACK_KEY in allowed + + +def test_questionset_round_trips_through_serde_without_warning() -> None: + # The configured serializer must round-trip a QuestionSet AND emit no + # "unregistered type" warning on deserialize. + pytest.importorskip("langgraph.checkpoint.serde.jsonplus") + _reset_serde_warning_dedup() + logger, handler, buf = _capture_serde_warnings() + try: + serde = build_checkpoint_serde() + qset = QuestionSet( + thread_id="t-1", + question_id="q-1", + turn=0, + questions=["What is in scope?"], + context={"phase": Phase.CLARIFY.value}, + ) + encoded = serde.dumps_typed(qset) + restored = serde.loads_typed(encoded) + finally: + logger.removeHandler(handler) + + assert restored == qset + assert "unregistered type" not in buf.getvalue() + + +def test_questionset_round_trips_through_sqlite_checkpointer_without_warning( + tmp_path, +) -> None: + # End-to-end against the REAL production saver: drive the graph to the + # clarifier suspend (which checkpoints a QuestionSet), force a checkpoint + # load, and resume — asserting no "unregistered type" warning prints and the + # task still completes. This is the runtime regression guard for the warning. + pytest.importorskip("langgraph.checkpoint.sqlite") + _reset_serde_warning_dedup() + logger, handler, buf = _capture_serde_warnings() + try: + cm = build_sqlite_checkpointer(tmp_path / "state.db") + with cm as saver: + graph = build_graph(saver) + thread_id, _ = start_task(graph, transport="slack") + pending = pending_question(graph, thread_id=thread_id) + assert isinstance(pending["question_set"], QuestionSet) + # Force a fresh checkpoint deserialize (the warning's trigger point). + get_pipeline_state(graph, thread_id=thread_id) + resumed = resume_task(graph, thread_id=thread_id, answer="scope it") + finally: + logger.removeHandler(handler) + + assert resumed["status"] == TaskStatus.DONE.value + assert "unregistered type" not in buf.getvalue() + + # --- P2 review-loop wiring. ------------------------------------------------- diff --git a/docs/provisioning/P3-LIVE-FLIP-PLAN.md b/docs/provisioning/P3-LIVE-FLIP-PLAN.md deleted file mode 100644 index 793091d..0000000 --- a/docs/provisioning/P3-LIVE-FLIP-PLAN.md +++ /dev/null @@ -1,148 +0,0 @@ -# P3-LIVE-FLIP PLAN — agent-team build → verify → draft-PR - -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. - -> **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), -> and memory `project_r720_agent_team` (locked decisions). - ---- - -## 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. - -**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. - -## 2. Locked decisions (carried in — do not relitigate here) - -- **D-OIDC:** apply path uses a **GitHub App `pull-requests:write` token**, **ZERO AWS, no OIDC**. -- **D2:** box stays **read-only / no standing write token**; **org CI does the applying**. -- **B4:** the LLM-proposed diff is **untrusted code**; the CI trust boundary (below) is mandatory. -- Output is **draft PRs only** — nothing auto-merges; human approval is the merge gate. -- (Separate, not part of this plan: `aws-posture` resident access via step-ca + IAM Roles - Anywhere — that IAM is already cross-review-approved and is its own sub-task.) - -## 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 | the GitHub transport reads `GITHUB_TOKEN`; box has `GH_TOKEN` | me (folded in here) | - -The flip does NOT proceed until `/sh-security-review` AND the GPT-4.1 cross-review on -the CI surface both pass. - -## 4. The CI trust boundary (design B4 — what the workflow must enforce) - -1. **Split CI.** An **untrusted build/test job**: `contents: read` only, **no secrets / no - OIDC / no write token**, egress-restricted. A **separate privileged job** that **never - checks out the patch code** (no `pull_request_target` + head checkout) opens the draft PR. -2. **Denylist.** A diff touching `.github/workflows/**`, IAM/permission IaC, - branch-protection / `CODEOWNERS` / Dependabot config, or out-of-scope files is - **rejected / escalated to a human — never auto-built**. -3. **Diff-hash integrity.** The box records the diff hash in its ledger; CI verifies the - hash before apply. The diff reaches CI as a signed artifact / short-lived branch-only - token (the box has no write token). -4. **Pure-code green gate.** Pass/fail is owned by a **pure-code gate** reading - **authenticated Checks-API results** (run id + diff hash). The **LLM verifier may propose - fixes but can never declare a build green**. -5. **Merge gate.** Draft PR + required checks + `/sh-security-review` + Claude Code App - review + **human approval**. - -## 5. Phases - -### Phase 0 — Plan review & pre-reqs 🤖/🧑 -- [ ] Run `/sh-plan-review` on this doc; fold BLOCK/FIX items in. -- [ ] Confirm a clean revert point (git tag main; Hyper-V snapshot of sh-secrev). -- [ ] Resolve `GH_TOKEN`→`GITHUB_TOKEN` (transport accepts both / box env updated). -- **Rollback:** none (no state changed). - -### Phase 1 — Author the split-CI apply/verify workflow 🤖 (review-gated) -- [ ] Write `agent-team/ci/agent-team-apply-verify.yml` per §4: split jobs, denylist, - diff-hash verification, pure-code gate reading Checks-API. **SHA-pin all actions.** -- [ ] Implement/confirm `agent_team/ci_fetcher.py` (read-only Checks-API result fetcher; - fails closed: missing token / 404 / auth fail → `None` → gate BLOCKs, task parks) - and `agent_team/ci_gate.py` (pure-code green decision). -- [ ] `/sh-security-review` + GPT-4.1 cross-review on this surface. **Hard stop until both pass.** -- **Rollback:** workflow file stays inert (`if: ${{ false }}` not yet flipped); delete the file. - -### Phase 2 — Provision the GitHub App + environment 🧑 OPERATOR (browser/admin) -- [ ] 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** - (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. - -### Phase 3 — Bind the live wiring (still gated by the environment) 🤖 -- [ ] In the workflow: uncomment `permissions: pull-requests: write` and - `environment: agent-apply`; flip the two `if: ${{ false }}` → enabled. -- [ ] 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. -- **Rollback:** re-set `if: ${{ false }}`, re-comment `environment:`, set - `build_verify_wiring=None`; restart the coordinator. (Exercise this rollback once.) - -### Phase 4 — Smoke test to a first draft PR 🧑/🤖 -- [ ] 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**. -- [ ] Flip the **Tier-3 fixer** off `--dry-run` only after the smoke test passes; verify a - dependency-CVE bump produces a draft PR. -- **Rollback:** close the draft PR; Phase-3 rollback. - -### Phase 5 — Enable the cross-plane loop 🤖 -- [ ] Allow confirmed Plane-1 checker findings (`intake-checker`) to flow into pipeline tasks - that end in draft-PR fixes (start conservative: highest-severity, one at a time). -- **Rollback:** revert intake-checker wiring to report-only. - -### Phase 6 — Docs & memory 🤖 -- [ ] Update `OPERATOR-RUNBOOK.md`, the Confluence host page, and memory: P3 is LIVE, - what the apply path can/can't do, the denylist, the merge gate, the rollback. - -## 6. What changes (and what does NOT) - -**Changes:** -- The pipeline produces **draft PRs** for well-scoped work (dep bumps, small fixes, docs, - checker-surfaced issues). Your role shifts from author → reviewer/approver for that class. -- Tier-3 fixer goes live (dep-CVE bumps → draft PRs). -- Plane-1 findings can become draft-PR fixes (cross-plane loop). - -**Does NOT change:** -- Box stays read-only; **CI applies, not the box**. No AWS / no OIDC in the apply path. -- **Denylisted, high-blast-radius changes** (workflows, IAM, branch protection, CODEOWNERS, - Dependabot) are **never auto-built** — always escalated. -- Human gates stay: the clarifier question-set AND the PR approval are both yours. -- **Drafts, not merges** — you remain the merge authority. - -## 7. Risks & mitigations - -| Risk | Mitigation | -|---|---| -| Untrusted diff exfiltrates secrets / escalates in CI | split CI: build job has no secrets/OIDC/write, egress-restricted; privileged job never checks out patch code | -| Malicious diff edits CI/IAM/branch-protection to self-escalate | denylist → reject/escalate, never auto-build | -| LLM "declares" a broken build green | pure-code gate reads authenticated Checks-API only; LLM can't set status | -| Diff tampered between box and CI | diff-hash recorded in ledger, verified before apply | -| Standing write capability on the always-on box | there is none — App token lives in CI; box holds only a read-only CI-result token | -| 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.