Files
roboco/tests/unit/gateway/test_sandbox_release_hook.py
T
cea3e56628 feat(lifecycle): revision findings ledger — structured failure feedback, persisted and delivered down the chain (#486)
* feat(lifecycle): revision findings ledger — structured QA/PR/PM/CEO failure feedback, persisted and delivered down the chain

Every bounce used to survive only as flattened prose: rounds overwrote each
other in notes_structured, request_changes persisted nothing, two raw
dev_notes appends were silently destroyed by the next handoff note, and the
dev prompt pointed at fields (qa_notes via evidence(), pm_notes) the API
never delivered. Agents re-interpreted and re-discovered every failure
before they could start fixing it.

- task_review_findings (migration 071, append-only): file/line/severity/
  criterion(AC-id-validated)/expected/actual/fix/evidence per finding, with
  origin (qa|pr_gate|pm|ceo), round, and an open->addressed->verified
  lifecycle (waived reserved); new tasks.pm_notes + PmReviewContent give
  request_changes a structured home
- producers: fail_review/pr_fail/request_changes take findings=[...] (prose
  issues shimmed+merged for one release, deprecation-logged); ceo_reject
  validates its reason (no 500), lands an origin=ceo finding, and bumps
  round+audit on branchless coordination roots; guardrails at the verb
  chokepoint (nudge >5, hard reject >10, field caps, traversal-safe file);
  the dev_notes data-loss appends are removed; new task.request_changes +
  task.ceo_reject audit events close rework attribution
- delivery: qa_notes/pr_reviewer_notes/pm_notes carry the deterministic
  [F-id8] rendering; claim briefings, evidence(), the REVISION_REQUIRED
  spawn prompt, PM triage bounced-blocks, and A2A bodies deliver open
  findings; round-N+1 QA and gate reviewers get the full prior ledger;
  panel Findings tab + bounced-xN chip; metrics pm_rejects/ceo_rejects +
  findings counts; vault task notes render a Findings section (fail-open)
- resolution closes for every origin: i_am_done and submit_up/submit_root
  take resolved_findings gated by FINDINGS_ADDRESSED (owner-gated so a
  stale non-owner PM can never mutate the ledger); pass_review/pr_pass/
  complete verify-stamp same-transaction; ceo_approve stamps best-effort
- 24 real-DB integration tests drive the full loop through the real
  choreographer; full suite 12856 green

* docs: revision findings ledger sweep — CLAUDE.md, map, RAG corpus

- CLAUDE.md: new ledger section + corrected request_changes row
- docs/map/review-findings.md (new subsystem map) + surgical updates to
  task-service/pr-gate-review/metrics-observability/vault/panel maps
- docs/rag: producers' findings contract across qa/pr-reviewer/developer/
  cell-pm/main-pm/ceo role docs (the PM docs were missing request_changes
  entirely), verb references, and a new architecture/review-findings.md
  disambiguating ledger findings from convention findings

* test(e2e): resubmit resolves the pr_fail finding per the ledger contract

The scripted pr_fail revision loop resubmitted submit_up without
resolved_findings — correctly rejected now that FINDINGS_ADDRESSED gates
the PM resubmit verbs (green locally, red only in CI since the e2e suite
skips without ROBOCO_E2E_SMOKE=1). The scripted PM now reads the open
ledger row pr_fail persisted (new open_finding_ids arc helper) and
resolves it on resubmit, asserting the open set drains — exercising the
coordinator half of the new contract end to end.

---------

Co-authored-by: Renn F <rennf93@users.noreply.github.com>
2026-07-11 22:54:42 +02:00

524 lines
17 KiB
Python

"""Sandbox release hook — Choreographer._teardown_sandbox_best_effort.
CEO directive: sandboxes must die when an agent's engagement with its work
ends, not only when its container is removed. Ties the on-demand
request_sandbox subsystem's teardown to the SUCCESSFUL exit of the six
verbs whose completion means exactly that: i_am_done, unclaim, i_am_idle,
pass_review, fail_review, i_documented. A rejected/failed verb call must
NOT release (the work isn't done), and a release failure must never fail
the verb — best-effort, backstopped by the container-removal teardown +
janitor sweep that already exist.
Two layers:
- the helper itself (`_teardown_sandbox_best_effort`), tested directly
against a fake orchestrator;
- the six call sites, tested by spying on the helper (patched onto the
instance) so each verb's fixture stays the minimal one already proven
by its own dedicated test file (test_choreographer_dev.py,
test_unclaim.py, test_choreographer_idle_guards.py,
test_choreographer_qa.py, test_choreographer_doc.py).
"""
from __future__ import annotations
from datetime import UTC, datetime
from typing import Any
from unittest.mock import AsyncMock, MagicMock
from uuid import uuid4
import pytest
from roboco.services.gateway.choreographer import Choreographer, ChoreographerDeps
from structlog.testing import capture_logs
def _make_deps(**overrides: Any) -> ChoreographerDeps:
base: dict[str, Any] = {
"task": AsyncMock(),
"work_session": AsyncMock(),
"git": AsyncMock(),
"a2a": AsyncMock(),
"journal": AsyncMock(),
"audit": AsyncMock(),
"evidence_repo": AsyncMock(),
}
base.update(overrides)
task = base["task"]
# Covers both VerbRunner's savepoint (i_am_done/pass_review/fail_review/
# i_documented) and i_documented's own session.flush() — one shared
# setup for every verb exercised in this file.
task.session = MagicMock()
task.session.begin_nested = MagicMock(
return_value=MagicMock(
__aenter__=AsyncMock(return_value=None),
__aexit__=AsyncMock(return_value=False),
)
)
task.session.flush = AsyncMock()
# pass_review's verified-stamp (ReviewFindingsRepository.list_for_task)
# reads via session.execute — an empty scalars result (no findings).
task.session.execute = AsyncMock(
return_value=MagicMock(
scalars=MagicMock(return_value=MagicMock(all=MagicMock(return_value=[])))
)
)
repo = base["evidence_repo"]
for method in (
"list_unread_a2a",
"list_unread_mentions",
"list_pending_notifications",
"task_metadata_gaps",
"recent_team_activity",
"blockers_in_lane",
"journal_highlights_for_task",
):
getattr(repo, method).return_value = []
_ldef = base["journal"].latest_decision_at.return_value
if type(_ldef).__name__ in ("MagicMock", "AsyncMock"):
base["journal"].latest_decision_at.return_value = datetime.now(UTC)
return ChoreographerDeps(**base)
# ---------------------------------------------------------------------------
# The helper itself
# ---------------------------------------------------------------------------
@pytest.mark.asyncio
async def test_helper_noop_when_orchestrator_missing() -> None:
c = Choreographer(_make_deps()) # orchestrator defaults to None
await c._teardown_sandbox_best_effort(uuid4()) # must not raise
@pytest.mark.asyncio
async def test_helper_calls_release_sandbox_with_resolved_slug() -> None:
agent_id = uuid4()
orch = AsyncMock()
c = Choreographer(_make_deps(orchestrator=orch))
await c._teardown_sandbox_best_effort(agent_id)
# Not seeded in AGENT_UUIDS, so _resolve_to_slug falls back to identity —
# mirrors test_request_sandbox_verb.py's cross-agent-isolation assertion.
orch.release_sandbox.assert_awaited_once_with(str(agent_id))
@pytest.mark.asyncio
async def test_helper_swallows_release_failure_and_logs() -> None:
agent_id = uuid4()
orch = AsyncMock()
orch.release_sandbox.side_effect = RuntimeError("docker down")
c = Choreographer(_make_deps(orchestrator=orch))
with capture_logs() as logs:
await c._teardown_sandbox_best_effort(agent_id) # must not raise
assert any("sandbox_release_failed" in str(e.get("event", "")) for e in logs)
# ---------------------------------------------------------------------------
# i_am_done
# ---------------------------------------------------------------------------
def _ready_i_am_done_task(task_id: Any, agent_id: Any, **overrides: Any) -> MagicMock:
base = {
"id": task_id,
"status": "in_progress",
"assigned_to": agent_id,
"plan": {"x": 1},
"branch_name": "feature/backend/abc",
"work_session_id": uuid4(),
"self_verified": False,
"pr_number": 8,
"pr_url": "https://x/pr/8",
"team": "backend",
"progress_updates": [{"message": "did x"}],
"acceptance_criteria": [],
"acceptance_criteria_status": [],
"commits": [{"sha": "deadbeef"}],
"documents": [],
"dev_notes": "Implemented the change and added tests covering the new path.",
"quick_context": None,
}
base.update(overrides)
return MagicMock(**base)
@pytest.mark.asyncio
async def test_i_am_done_success_releases_sandbox(
monkeypatch: pytest.MonkeyPatch,
) -> None:
agent_id = uuid4()
task_id = uuid4()
t = _ready_i_am_done_task(task_id, agent_id)
after_verify = MagicMock(
**{**t.__dict__, "self_verified": True, "status": "verifying"}
)
after_submit = MagicMock(**{**after_verify.__dict__, "status": "awaiting_qa"})
task_svc = AsyncMock()
task_svc.get.return_value = t
task_svc.agent_for.return_value = MagicMock(
id=agent_id, role="developer", team="backend", slug=None
)
task_svc.submit_verification.return_value = after_verify
task_svc.submit_qa.return_value = after_submit
task_svc.qa_agent_for_team.return_value = MagicMock(
id=uuid4(), skills=[{"id": "code_review"}]
)
journal_svc = AsyncMock()
journal_svc.has_reflect_for_task.return_value = True
journal_svc.has_decision_for_task.return_value = True
journal_svc.has_learning_for_task.return_value = False
journal_svc.has_struggle_for_task.return_value = False
deps = _make_deps(task=task_svc, journal=journal_svc)
c = Choreographer(deps)
release = AsyncMock()
monkeypatch.setattr(c, "_teardown_sandbox_best_effort", release)
env = await c.i_am_done(agent_id, task_id, "done")
assert env.error is None
release.assert_awaited_once_with(agent_id)
@pytest.mark.asyncio
async def test_i_am_done_rejection_does_not_release_sandbox(
monkeypatch: pytest.MonkeyPatch,
) -> None:
agent_id = uuid4()
task_id = uuid4()
task_svc = AsyncMock()
task_svc.get.return_value = None
deps = _make_deps(task=task_svc)
c = Choreographer(deps)
release = AsyncMock()
monkeypatch.setattr(c, "_teardown_sandbox_best_effort", release)
env = await c.i_am_done(agent_id, task_id, "done")
assert env.error == "not_found"
release.assert_not_awaited()
# ---------------------------------------------------------------------------
# unclaim
# ---------------------------------------------------------------------------
@pytest.mark.asyncio
async def test_unclaim_success_releases_sandbox(
monkeypatch: pytest.MonkeyPatch,
) -> None:
agent_id = uuid4()
task_id = uuid4()
t = MagicMock(id=task_id, status="claimed", assigned_to=agent_id)
task_svc = AsyncMock()
task_svc.get.return_value = t
task_svc.agent_for.return_value = MagicMock(
id=agent_id, role="developer", team="backend", slug=None
)
task_svc.unclaim_for_agent.return_value = MagicMock(
id=task_id, status="pending", assigned_to=None
)
deps = _make_deps(task=task_svc)
c = Choreographer(deps)
release = AsyncMock()
monkeypatch.setattr(c, "_teardown_sandbox_best_effort", release)
env = await c.unclaim(agent_id, task_id)
assert env.error is None
release.assert_awaited_once_with(agent_id)
@pytest.mark.asyncio
async def test_unclaim_rejection_does_not_release_sandbox(
monkeypatch: pytest.MonkeyPatch,
) -> None:
agent_id = uuid4()
task_id = uuid4()
task_svc = AsyncMock()
task_svc.get.return_value = None
deps = _make_deps(task=task_svc)
c = Choreographer(deps)
release = AsyncMock()
monkeypatch.setattr(c, "_teardown_sandbox_best_effort", release)
env = await c.unclaim(agent_id, task_id)
assert env.error == "not_found"
release.assert_not_awaited()
# ---------------------------------------------------------------------------
# i_am_idle
# ---------------------------------------------------------------------------
@pytest.mark.asyncio
async def test_i_am_idle_success_releases_sandbox(
monkeypatch: pytest.MonkeyPatch,
) -> None:
agent_id = uuid4()
task_svc = AsyncMock()
task_svc.list_assigned_for_agent.return_value = []
task_svc.list_in_progress_for_agent.return_value = []
deps = _make_deps(task=task_svc)
c = Choreographer(deps)
release = AsyncMock()
monkeypatch.setattr(c, "_teardown_sandbox_best_effort", release)
env = await c.i_am_idle(agent_id)
assert env.error is None
assert env.status == "idle"
release.assert_awaited_once_with(agent_id)
@pytest.mark.asyncio
async def test_i_am_idle_with_unread_does_not_release_sandbox(
monkeypatch: pytest.MonkeyPatch,
) -> None:
"""idle_with_unread sends the agent back to work — not a real exit, so
the container does not shut down and the sandbox must survive."""
agent_id = uuid4()
deps = _make_deps()
deps.evidence_repo.list_unread_a2a.return_value = [{"from": "x", "task_id": "t1"}]
c = Choreographer(deps)
release = AsyncMock()
monkeypatch.setattr(c, "_teardown_sandbox_best_effort", release)
env = await c.i_am_idle(agent_id)
assert env.status == "idle_with_unread"
release.assert_not_awaited()
@pytest.mark.asyncio
async def test_i_am_idle_guard_rejection_does_not_release_sandbox(
monkeypatch: pytest.MonkeyPatch,
) -> None:
agent_id = uuid4()
pending = MagicMock(id=uuid4(), status="pending")
task_svc = AsyncMock()
task_svc.list_assigned_for_agent.return_value = [pending]
task_svc.list_in_progress_for_agent.return_value = []
deps = _make_deps(task=task_svc)
c = Choreographer(deps)
release = AsyncMock()
monkeypatch.setattr(c, "_teardown_sandbox_best_effort", release)
env = await c.i_am_idle(agent_id)
assert env.error == "invalid_state"
release.assert_not_awaited()
# ---------------------------------------------------------------------------
# pass_review / fail_review
# ---------------------------------------------------------------------------
def _qa_owned_task(task_id: Any, qa_id: Any, **overrides: Any) -> MagicMock:
base = {
"id": task_id,
"status": "awaiting_qa",
"task_type": "code",
"team": "backend",
"assigned_to": qa_id,
"qa_evidence_inspected": True,
"quick_context": None,
}
base.update(overrides)
return MagicMock(**base)
def _qa_agent_mock(qa_id: Any) -> MagicMock:
return MagicMock(id=qa_id, role="qa", team="backend", slug=None)
@pytest.mark.asyncio
async def test_pass_review_success_releases_sandbox(
monkeypatch: pytest.MonkeyPatch,
) -> None:
qa_id = uuid4()
task_id = uuid4()
t = _qa_owned_task(task_id, qa_id)
after = MagicMock(
id=task_id,
status="awaiting_documentation",
assigned_to=qa_id,
team="backend",
pr_url="https://x/pr/8",
qa_evidence_inspected=True,
)
task_svc = AsyncMock()
task_svc.get.return_value = t
task_svc.agent_for.return_value = _qa_agent_mock(qa_id)
task_svc.qa_pass.return_value = after
task_svc.documenter_for_team.return_value = MagicMock(id=uuid4())
journal_svc = AsyncMock()
journal_svc.has_learning_for_task.return_value = True
deps = _make_deps(task=task_svc, journal=journal_svc)
c = Choreographer(deps)
release = AsyncMock()
monkeypatch.setattr(c, "_teardown_sandbox_best_effort", release)
notes = (
"Reviewed PR carefully. Branch convention correct. Commit prefix "
"verified. README diff matches spec. All acceptance criteria met."
)
env = await c.pass_review(qa_id, task_id, notes=notes)
assert env.error is None
release.assert_awaited_once_with(qa_id)
@pytest.mark.asyncio
async def test_pass_review_rejection_does_not_release_sandbox(
monkeypatch: pytest.MonkeyPatch,
) -> None:
qa_id = uuid4()
task_id = uuid4()
task_svc = AsyncMock()
task_svc.get.return_value = None
deps = _make_deps(task=task_svc)
c = Choreographer(deps)
release = AsyncMock()
monkeypatch.setattr(c, "_teardown_sandbox_best_effort", release)
env = await c.pass_review(qa_id, task_id, notes="x")
assert env.error == "not_found"
release.assert_not_awaited()
@pytest.mark.asyncio
async def test_fail_review_success_releases_sandbox(
monkeypatch: pytest.MonkeyPatch,
) -> None:
qa_id = uuid4()
task_id = uuid4()
dev_id = uuid4()
t = _qa_owned_task(task_id, qa_id)
after = MagicMock(
id=task_id,
status="needs_revision",
assigned_to=dev_id,
team="backend",
)
task_svc = AsyncMock()
task_svc.get.return_value = t
task_svc.agent_for.return_value = _qa_agent_mock(qa_id)
task_svc.qa_fail.return_value = after
task_svc.session = MagicMock()
task_svc.session.add = MagicMock()
task_svc.session.flush = AsyncMock()
journal_svc = AsyncMock()
journal_svc.has_learning_for_task.return_value = True
deps = _make_deps(task=task_svc, journal=journal_svc)
c = Choreographer(deps)
release = AsyncMock()
monkeypatch.setattr(c, "_teardown_sandbox_best_effort", release)
issues = [
"Missing unit test coverage for /healthz endpoint — add an assertion",
"Lint errors in /api/foo.py: unused import and missing return type",
]
env = await c.fail_review(qa_id, task_id, issues)
assert env.error is None
release.assert_awaited_once_with(qa_id)
@pytest.mark.asyncio
async def test_fail_review_rejection_does_not_release_sandbox(
monkeypatch: pytest.MonkeyPatch,
) -> None:
qa_id = uuid4()
task_id = uuid4()
task_svc = AsyncMock()
task_svc.get.return_value = None
deps = _make_deps(task=task_svc)
c = Choreographer(deps)
release = AsyncMock()
monkeypatch.setattr(c, "_teardown_sandbox_best_effort", release)
env = await c.fail_review(qa_id, task_id, ["some issue"])
assert env.error == "not_found"
release.assert_not_awaited()
# ---------------------------------------------------------------------------
# i_documented
# ---------------------------------------------------------------------------
def _doc_owned_task(task_id: Any, doc_id: Any, **overrides: Any) -> MagicMock:
base = {
"id": task_id,
"status": "awaiting_documentation",
"task_type": "code",
"team": "backend",
"assigned_to": doc_id,
"quick_context": None,
}
base.update(overrides)
return MagicMock(**base)
def _doc_agent_mock(doc_id: Any) -> MagicMock:
return MagicMock(id=doc_id, role="documenter", team="backend", slug=None)
@pytest.mark.asyncio
async def test_i_documented_success_releases_sandbox(
monkeypatch: pytest.MonkeyPatch,
) -> None:
doc_id = uuid4()
task_id = uuid4()
t = _doc_owned_task(task_id, doc_id)
after = MagicMock(
id=task_id, status="awaiting_pm_review", assigned_to=doc_id, team="backend"
)
task_svc = AsyncMock()
task_svc.get.return_value = t
task_svc.agent_for.return_value = _doc_agent_mock(doc_id)
task_svc.docs_complete.return_value = after
task_svc.cell_pm_for_team.return_value = MagicMock(id=uuid4())
journal_svc = AsyncMock()
journal_svc.has_reflect_for_task.return_value = True
deps = _make_deps(task=task_svc, journal=journal_svc)
c = Choreographer(deps)
release = AsyncMock()
monkeypatch.setattr(c, "_teardown_sandbox_best_effort", release)
notes = "Wrote backend/guides/feature-x.md with usage examples and config notes."
files = ["backend/guides/feature-x.md"]
env = await c.i_documented(doc_id, task_id, notes=notes, files=files)
assert env.error is None
release.assert_awaited_once_with(doc_id)
@pytest.mark.asyncio
async def test_i_documented_rejection_does_not_release_sandbox(
monkeypatch: pytest.MonkeyPatch,
) -> None:
doc_id = uuid4()
task_id = uuid4()
task_svc = AsyncMock()
task_svc.get.return_value = None
deps = _make_deps(task=task_svc)
c = Choreographer(deps)
release = AsyncMock()
monkeypatch.setattr(c, "_teardown_sandbox_best_effort", release)
env = await c.i_documented(doc_id, task_id, notes="x" * 30, files=["docs.md"])
assert env.error == "not_found"
release.assert_not_awaited()