mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
feat(content): anti-soup guard on the flow verbs' free-text
Extends structured-content enforcement from the content tools to every flow verb that carries agent free-text, closing the last hole where a dev/PM could pass word soup: i_am_blocked(reason), i_am_done(notes), submit_up/submit_root/complete(notes), escalate_up/escalate_to_ceo(reason), pass_review(notes), fail_review/pr_fail(issues), pr_pass(notes), i_documented(notes), delegate(title/description). Plans (i_will_plan / i_will_work_on) keep their existing >=150-char approach + sub_task gates and are skipped here so recovery re-entry with thin values still works. Shared helpers on the choreographer: _free_text_soup (bare envelope, list aware) and _soup_or_decision_env (folds the soup check into a verb's existing spec-gate return so no verb gains a return or tips the xenon bound). reject_trivial now also catches all-filler multi-token strings. base.md documents the broadened rule for agents.
This commit is contained in:
@@ -78,7 +78,7 @@ Channel arguments take the slug **without** the `#` prefix: `"backend-cell"`, no
|
||||
- `env`/`printenv` is **denied** — secrets are not readable from your container.
|
||||
- `Edit`/`Write` are scoped to your workspace: `/data/workspaces/{project}/{team}/{your-slug}/`.
|
||||
- Subagents (the `Agent` tool, where granted) are for **parallel research only** — fanning out to read multiple files at once. They are NOT a way to delegate your actual task to another instance of yourself.
|
||||
- **Everything you write is validated — no filler, no word soup, anywhere.** `say`/`dm`/`note`/`progress`/`notify` reject empty/placeholder text (`asdf`, `wip`, `tbd`, `...`); state what actually happened. Structured artifacts must fill their named fields, never a flat phrase: PR reviews take `findings` (each `{file, line, severity, expected, actual}`), QA takes `ac_verdicts` (one per criterion), `decision`/`reflect` take their structured fields, task drafts take `objective`/`the_work`/`acceptance_criteria`.
|
||||
- **Everything you write is validated — no filler, no word soup, anywhere.** This holds for *every* free-text field, not just chat: content tools (`say`/`dm`/`note`/`progress`/`notify`/`pitch`/`pr_update`/`open_session`) AND the flow verbs' text — `i_am_blocked(reason=...)`, `i_am_done(notes=...)`, `submit_up`/`submit_root`/`complete(notes=...)`, `escalate_up`/`escalate_to_ceo(reason=...)`, `fail_review`/`pr_fail(issues=[...])`, `pass_review(notes=...)`, `delegate(title=, description=)`. Empty/placeholder text (`asdf`, `wip`, `tbd`, `n/a`, `...`, `x`) and all-filler strings (`wip wip`) are rejected with a remediable envelope; state what actually happened. Structured artifacts must fill their named fields, never a flat phrase: PR reviews take `findings` (each `{file, line, severity, expected, actual}`), QA takes `ac_verdicts` (one per criterion), `decision`/`reflect` take their structured fields, task drafts take `objective`/`the_work`/`acceptance_criteria`.
|
||||
|
||||
## Branch and commit conventions (handled by the gateway)
|
||||
|
||||
|
||||
@@ -22,6 +22,7 @@ import structlog
|
||||
from roboco.exceptions import MergeConflictError
|
||||
from roboco.foundation.policy import lifecycle as spec_module
|
||||
from roboco.foundation.policy.content import markers
|
||||
from roboco.foundation.policy.content.validators import reject_trivial
|
||||
from roboco.services.gateway.choreographer._verb_runner import VerbRunner
|
||||
from roboco.services.gateway.claim_guards import (
|
||||
already_active_guard,
|
||||
@@ -607,6 +608,93 @@ class Choreographer:
|
||||
logger.warning("audit.log_event failed", error=str(exc), verb=verb)
|
||||
return env
|
||||
|
||||
@classmethod
|
||||
def _free_text_soup(
|
||||
cls, checks: tuple[tuple[str, Any, int], ...]
|
||||
) -> Envelope | None:
|
||||
"""Return a bare ``invalid_state`` envelope for the first soupy field.
|
||||
|
||||
``checks`` is a tuple of ``(field_name, value, min_chars)``. A value
|
||||
that is ``None`` or empty/whitespace is skipped — presence is gated
|
||||
elsewhere; this rejects *filler* in text the agent actually supplied
|
||||
(``wip``, ``asdf``, ``tbd``, ``...``). A list value has each item
|
||||
checked. The returned envelope carries no introspection or audit row:
|
||||
the caller folds it into its existing rejection ``return`` so the soup
|
||||
check adds no extra return (verbs stay under the complexity bound), and
|
||||
the agent always gets a remediable envelope, never a 422 (which would
|
||||
trip the do-server circuit breaker).
|
||||
"""
|
||||
for name, value, min_chars in checks:
|
||||
items = value if isinstance(value, list) else [value]
|
||||
for idx, item in enumerate(items):
|
||||
if item is None or not str(item).strip():
|
||||
continue
|
||||
label = f"{name}[{idx}]" if isinstance(value, list) else name
|
||||
env = cls._soup_reason(str(item), label, min_chars)
|
||||
if env is not None:
|
||||
return env
|
||||
return None
|
||||
|
||||
async def _guard_free_text(
|
||||
self,
|
||||
*,
|
||||
checks: tuple[tuple[str, Any, int], ...],
|
||||
task: Any,
|
||||
agent_id: UUID,
|
||||
role_str: str,
|
||||
verb: str,
|
||||
) -> Envelope | None:
|
||||
"""Emit-on-soup wrapper over :meth:`_free_text_soup`.
|
||||
|
||||
For verbs that have return-count headroom: stamps introspection, audits
|
||||
via ``_emit_rejection``, and returns the rejection (or ``None`` clean).
|
||||
Verbs already at the return bound call ``_free_text_soup`` directly and
|
||||
fold the result into an existing rejection return instead.
|
||||
"""
|
||||
env = self._free_text_soup(checks)
|
||||
if env is None:
|
||||
return None
|
||||
return await self._emit_rejection(
|
||||
env.with_introspection(task=task, role=role_str),
|
||||
agent_id=agent_id,
|
||||
task_id=getattr(task, "id", None),
|
||||
verb=verb,
|
||||
)
|
||||
|
||||
@staticmethod
|
||||
def _soup_reason(value: str, field: str, min_chars: int) -> Envelope | None:
|
||||
"""Build an ``invalid_state`` envelope when ``value`` is filler, else None."""
|
||||
try:
|
||||
reject_trivial(value, field=field, min_chars=min_chars)
|
||||
except ValueError as exc:
|
||||
return Envelope.invalid_state(
|
||||
message=str(exc),
|
||||
remediate=(
|
||||
f"write a substantive {field} (>={min_chars} chars, no filler "
|
||||
"like 'asdf'/'wip'/'tbd'/'...'); state what actually happened."
|
||||
),
|
||||
context_briefing={},
|
||||
)
|
||||
return None
|
||||
|
||||
@staticmethod
|
||||
def _soup_or_decision_env(
|
||||
soup: Envelope | None, decision: Any, briefing: dict[str, Any]
|
||||
) -> Envelope | None:
|
||||
"""Pick the rejection envelope: soup first, then the spec decision.
|
||||
|
||||
Lets a verb fold the free-text soup check into its existing
|
||||
spec-gate rejection ``return`` with a single branch — the two
|
||||
fallback ``or``s live here, keeping the verb body under the
|
||||
cyclomatic bound. Returns ``None`` when neither rejects; the caller
|
||||
stamps introspection + emits.
|
||||
"""
|
||||
if soup is not None:
|
||||
return soup
|
||||
if not decision.allowed:
|
||||
return Envelope.from_decision(decision, briefing=briefing)
|
||||
return None
|
||||
|
||||
# --- Phase 1 (developer) verbs ---
|
||||
|
||||
@staticmethod
|
||||
@@ -1529,11 +1617,13 @@ class Choreographer:
|
||||
# standard tracing/field gates beforehand.
|
||||
if str(t.status) == "verifying" and t.assigned_to == agent_id:
|
||||
return await self._i_am_done_resume_from_verifying(ctx)
|
||||
# i_am_done notes is optional and supplementary (the real summary lives
|
||||
# in commits + journal:reflect), so guard it lightly — a banned token
|
||||
# ('wip'/'x') is soup, but a terse real word like 'done' is fine.
|
||||
soup = self._free_text_soup(checks=(("notes", notes, 4),))
|
||||
decision = spec_module.can_invoke_intent(role, "i_am_done", t, spec_ctx)
|
||||
if not decision.allowed:
|
||||
return await self._reject_i_am_done(
|
||||
ctx, Envelope.from_decision(decision, briefing=briefing)
|
||||
)
|
||||
if env := self._soup_or_decision_env(soup, decision, briefing):
|
||||
return await self._reject_i_am_done(ctx, env)
|
||||
if gate_rejection := await self._i_am_done_gate(ctx):
|
||||
return gate_rejection
|
||||
return await self._i_am_done_run(ctx, agent, spec_ctx)
|
||||
@@ -2457,12 +2547,11 @@ class Choreographer:
|
||||
original_developer_slug=_extract_original_developer(t),
|
||||
notes=reason,
|
||||
)
|
||||
soup = self._free_text_soup(checks=(("reason", reason, 8),))
|
||||
decision = spec_module.can_invoke_intent(role, "i_am_blocked", t, spec_ctx)
|
||||
if not decision.allowed:
|
||||
if env := self._soup_or_decision_env(soup, decision, briefing):
|
||||
return await self._emit_rejection(
|
||||
Envelope.from_decision(decision, briefing=briefing).with_introspection(
|
||||
task=t, role=role_str
|
||||
),
|
||||
env.with_introspection(task=t, role=role_str),
|
||||
agent_id=agent_id,
|
||||
task_id=task_id,
|
||||
verb="i_am_blocked",
|
||||
@@ -3340,12 +3429,16 @@ class Choreographer:
|
||||
actor_slug=getattr(agent, "slug", None) if agent is not None else None,
|
||||
original_developer_slug=_extract_original_developer(parent),
|
||||
)
|
||||
soup = self._free_text_soup(
|
||||
checks=(
|
||||
("title", inputs.title, 5),
|
||||
("description", inputs.description, 10),
|
||||
)
|
||||
)
|
||||
decision = spec_module.can_invoke_intent(role, "delegate", parent, spec_ctx)
|
||||
if not decision.allowed:
|
||||
if env := self._soup_or_decision_env(soup, decision, briefing):
|
||||
return await self._emit_rejection(
|
||||
Envelope.from_decision(decision, briefing=briefing).with_introspection(
|
||||
task=parent, role=role_str
|
||||
),
|
||||
env.with_introspection(task=parent, role=role_str),
|
||||
agent_id=pm_agent_id,
|
||||
task_id=parent_task_id,
|
||||
verb="delegate",
|
||||
@@ -4414,12 +4507,11 @@ class Choreographer:
|
||||
original_developer_slug=_extract_original_developer(t),
|
||||
notes=notes,
|
||||
)
|
||||
soup = self._free_text_soup(checks=(("notes", notes, 10),))
|
||||
decision = spec_module.can_invoke_intent(role, "submit_up", t, spec_ctx)
|
||||
if not decision.allowed:
|
||||
if env := self._soup_or_decision_env(soup, decision, briefing):
|
||||
return await self._emit_rejection(
|
||||
Envelope.from_decision(decision, briefing=briefing).with_introspection(
|
||||
task=t, role=role_str
|
||||
),
|
||||
env.with_introspection(task=t, role=role_str),
|
||||
agent_id=pm_agent_id,
|
||||
task_id=task_id,
|
||||
verb="submit_up",
|
||||
@@ -5110,12 +5202,11 @@ class Choreographer:
|
||||
actor_slug=getattr(agent, "slug", None) if agent is not None else None,
|
||||
notes=notes,
|
||||
)
|
||||
soup = self._free_text_soup(checks=(("notes", notes, 10),))
|
||||
decision = spec_module.can_invoke_intent(role, "submit_root", t, spec_ctx)
|
||||
if not decision.allowed:
|
||||
if env := self._soup_or_decision_env(soup, decision, briefing):
|
||||
return await self._emit_rejection(
|
||||
Envelope.from_decision(decision, briefing=briefing).with_introspection(
|
||||
task=t, role=role_str
|
||||
),
|
||||
env.with_introspection(task=t, role=role_str),
|
||||
agent_id=main_pm_agent_id,
|
||||
task_id=task_id,
|
||||
verb="submit_root",
|
||||
@@ -5361,6 +5452,14 @@ class Choreographer:
|
||||
actor_slug=getattr(agent, "slug", None) if agent is not None else None,
|
||||
original_developer_slug=_extract_original_developer(t),
|
||||
)
|
||||
if soup := await self._guard_free_text(
|
||||
checks=(("notes", notes, 10),),
|
||||
task=t,
|
||||
agent_id=agent_id,
|
||||
role_str=role_str,
|
||||
verb="complete",
|
||||
):
|
||||
return soup
|
||||
decision = spec_module.can_invoke_intent(role, "complete", t, spec_ctx)
|
||||
if not decision.allowed:
|
||||
return await self._emit_rejection(
|
||||
@@ -5423,12 +5522,11 @@ class Choreographer:
|
||||
original_developer_slug=_extract_original_developer(t),
|
||||
notes=reason,
|
||||
)
|
||||
soup = self._free_text_soup(checks=(("reason", reason, 10),))
|
||||
decision = spec_module.can_invoke_intent(role, "escalate_up", t, spec_ctx)
|
||||
if not decision.allowed:
|
||||
if env := self._soup_or_decision_env(soup, decision, briefing):
|
||||
return await self._emit_rejection(
|
||||
Envelope.from_decision(decision, briefing=briefing).with_introspection(
|
||||
task=t, role=role_str
|
||||
),
|
||||
env.with_introspection(task=t, role=role_str),
|
||||
agent_id=pm_agent_id,
|
||||
task_id=task_id,
|
||||
verb="escalate_up",
|
||||
@@ -5591,12 +5689,11 @@ class Choreographer:
|
||||
original_developer_slug=_extract_original_developer(t),
|
||||
notes=reason,
|
||||
)
|
||||
soup = self._free_text_soup(checks=(("reason", reason, 10),))
|
||||
decision = spec_module.can_invoke_intent(role, "escalate_to_ceo", t, spec_ctx)
|
||||
if not decision.allowed:
|
||||
if env := self._soup_or_decision_env(soup, decision, briefing):
|
||||
return await self._emit_rejection(
|
||||
Envelope.from_decision(decision, briefing=briefing).with_introspection(
|
||||
task=t, role=role_str
|
||||
),
|
||||
env.with_introspection(task=t, role=role_str),
|
||||
agent_id=agent_id,
|
||||
task_id=task_id,
|
||||
verb="escalate_to_ceo",
|
||||
|
||||
@@ -53,6 +53,29 @@ class ChoreographerHelpers:
|
||||
) -> Envelope:
|
||||
raise NotImplementedError
|
||||
|
||||
@classmethod
|
||||
def _free_text_soup(
|
||||
cls, checks: tuple[tuple[str, Any, int], ...]
|
||||
) -> Envelope | None:
|
||||
raise NotImplementedError
|
||||
|
||||
@staticmethod
|
||||
def _soup_or_decision_env(
|
||||
soup: Envelope | None, decision: Any, briefing: dict[str, Any]
|
||||
) -> Envelope | None:
|
||||
raise NotImplementedError
|
||||
|
||||
async def _guard_free_text(
|
||||
self,
|
||||
*,
|
||||
checks: tuple[tuple[str, Any, int], ...],
|
||||
task: Any,
|
||||
agent_id: UUID,
|
||||
role_str: str,
|
||||
verb: str,
|
||||
) -> Envelope | None:
|
||||
raise NotImplementedError
|
||||
|
||||
async def _briefing_for(
|
||||
self,
|
||||
agent_id: UUID,
|
||||
|
||||
@@ -440,6 +440,14 @@ class DocMixin(_Base):
|
||||
# otherwise the gate's tracing_gap loops into the circuit breaker.
|
||||
await self._ensure_doc_reflect(doc_agent_id, task_id, notes, files)
|
||||
|
||||
if soup := await self._guard_free_text(
|
||||
checks=(("notes", notes, 10),),
|
||||
task=owned_task,
|
||||
agent_id=doc_agent_id,
|
||||
role_str=role_str,
|
||||
verb="i_documented",
|
||||
):
|
||||
return soup
|
||||
gate_rejection = await self._check_doc_gates(
|
||||
doc_agent_id, task_id, notes, files, owned_task
|
||||
)
|
||||
|
||||
@@ -193,6 +193,14 @@ class PRGateMixin(_Base):
|
||||
notes=notes,
|
||||
issues=issues,
|
||||
)
|
||||
if soup := await self._guard_free_text(
|
||||
checks=(("notes", notes, 8), ("issues", list(issues), 8)),
|
||||
task=t,
|
||||
agent_id=reviewer_agent_id,
|
||||
role_str=role_str,
|
||||
verb=verb,
|
||||
):
|
||||
return soup
|
||||
decision = spec_module.can_invoke_intent(role, verb, t, spec_ctx)
|
||||
if not decision.allowed:
|
||||
return await self._emit_rejection(
|
||||
|
||||
@@ -418,6 +418,36 @@ class QAMixin(_Base):
|
||||
except ContentValidationError:
|
||||
return
|
||||
|
||||
async def _qa_review_text_gate(
|
||||
self,
|
||||
*,
|
||||
qa_agent_id: UUID,
|
||||
task_id: UUID,
|
||||
notes: str,
|
||||
task: Any,
|
||||
role_str: str,
|
||||
verb: str,
|
||||
soup_checks: tuple[tuple[str, Any, int], ...],
|
||||
) -> Envelope | None:
|
||||
"""Notes/journal/evidence gate + free-text anti-soup, in one call.
|
||||
|
||||
Shared by pass_review (checks ``notes``) and fail_review (checks each
|
||||
``issue``). Keeps the soup branch out of the verb bodies so they stay
|
||||
under the cyclomatic bound. Returns the first rejection, else ``None``.
|
||||
"""
|
||||
gate = await self._qa_pass_gate_check(qa_agent_id, task_id, notes, task, verb)
|
||||
if gate is not None:
|
||||
return gate
|
||||
soup = self._free_text_soup(checks=soup_checks)
|
||||
if soup is None:
|
||||
return None
|
||||
return await self._emit_rejection(
|
||||
soup.with_introspection(task=task, role=role_str),
|
||||
agent_id=qa_agent_id,
|
||||
task_id=task_id,
|
||||
verb=verb,
|
||||
)
|
||||
|
||||
async def pass_review(
|
||||
self,
|
||||
qa_agent_id: UUID,
|
||||
@@ -453,10 +483,15 @@ class QAMixin(_Base):
|
||||
)
|
||||
if spec_rejection is not None:
|
||||
return spec_rejection
|
||||
gate_rejection = await self._qa_pass_gate_check(
|
||||
qa_agent_id, task_id, notes, t, "pass_review"
|
||||
)
|
||||
if gate_rejection is not None:
|
||||
if gate_rejection := await self._qa_review_text_gate(
|
||||
qa_agent_id=qa_agent_id,
|
||||
task_id=task_id,
|
||||
notes=notes,
|
||||
task=t,
|
||||
role_str=role_str,
|
||||
verb="pass_review",
|
||||
soup_checks=(("notes", notes, 8),),
|
||||
):
|
||||
return gate_rejection
|
||||
ac_rejection = self._qa_ac_coverage_check(t, ac_verdicts)
|
||||
if ac_rejection is not None:
|
||||
@@ -550,10 +585,15 @@ class QAMixin(_Base):
|
||||
)
|
||||
if spec_rejection is not None:
|
||||
return spec_rejection
|
||||
gate_rejection = await self._qa_pass_gate_check(
|
||||
qa_agent_id, task_id, notes, t, "fail_review"
|
||||
)
|
||||
if gate_rejection is not None:
|
||||
if gate_rejection := await self._qa_review_text_gate(
|
||||
qa_agent_id=qa_agent_id,
|
||||
task_id=task_id,
|
||||
notes=notes,
|
||||
task=t,
|
||||
role_str=role_str,
|
||||
verb="fail_review",
|
||||
soup_checks=(("issues", issues, 8),),
|
||||
):
|
||||
return gate_rejection
|
||||
|
||||
briefing = await self._briefing_for(qa_agent_id, task_id)
|
||||
|
||||
@@ -131,7 +131,9 @@ async def test_board_escalate_to_ceo_blocks_wrong_state() -> None:
|
||||
deps = _make_deps(task=task_svc)
|
||||
c = Choreographer(deps)
|
||||
|
||||
env = await c.escalate_to_ceo(agent_id, task_id, reason="x")
|
||||
env = await c.escalate_to_ceo(
|
||||
agent_id, task_id, reason="escalating to the CEO for sign-off"
|
||||
)
|
||||
body = env.as_dict()
|
||||
assert body["error"] == "invalid_state"
|
||||
assert "awaiting_pm_review" in body["message"]
|
||||
@@ -173,7 +175,9 @@ async def test_board_escalate_to_ceo_blocks_disallowed_role() -> None:
|
||||
deps = _make_deps(task=task_svc)
|
||||
c = Choreographer(deps)
|
||||
|
||||
env = await c.escalate_to_ceo(agent_id, task_id, reason="x")
|
||||
env = await c.escalate_to_ceo(
|
||||
agent_id, task_id, reason="escalating to the CEO for sign-off"
|
||||
)
|
||||
body = env.as_dict()
|
||||
assert body["error"] == "not_authorized"
|
||||
assert "qa" in body["message"]
|
||||
@@ -194,7 +198,9 @@ async def test_board_escalate_to_ceo_requires_journal_decision() -> None:
|
||||
deps = _make_deps(task=task_svc, journal=journal_svc)
|
||||
c = Choreographer(deps)
|
||||
|
||||
env = await c.escalate_to_ceo(agent_id, task_id, reason="x")
|
||||
env = await c.escalate_to_ceo(
|
||||
agent_id, task_id, reason="escalating to the CEO for sign-off"
|
||||
)
|
||||
body = env.as_dict()
|
||||
assert body["error"] == "tracing_gap"
|
||||
assert "journal:decision" in body["missing"]
|
||||
@@ -210,7 +216,9 @@ async def test_board_escalate_to_ceo_returns_not_found_when_task_missing() -> No
|
||||
deps = _make_deps(task=task_svc)
|
||||
c = Choreographer(deps)
|
||||
|
||||
env = await c.escalate_to_ceo(agent_id, task_id, reason="x")
|
||||
env = await c.escalate_to_ceo(
|
||||
agent_id, task_id, reason="escalating to the CEO for sign-off"
|
||||
)
|
||||
body = env.as_dict()
|
||||
assert body["error"] == "not_found"
|
||||
task_svc.escalate_to_ceo.assert_not_awaited()
|
||||
|
||||
@@ -585,7 +585,7 @@ async def test_i_am_done_not_assigned_returns_tracing_gap() -> None:
|
||||
deps = _make_deps(task=task_svc)
|
||||
c = Choreographer(deps)
|
||||
|
||||
env = await c.i_am_done(agent_id, task_id, "x")
|
||||
env = await c.i_am_done(agent_id, task_id, "completed the work")
|
||||
body = env.as_dict()
|
||||
assert body["error"] == "tracing_gap"
|
||||
assert "owns_task" in body["missing"]
|
||||
|
||||
@@ -158,7 +158,7 @@ async def test_i_documented_requires_min_notes() -> None:
|
||||
deps = _make_deps(task=task_svc, journal=journal_svc)
|
||||
c = Choreographer(deps)
|
||||
|
||||
env = await c.i_documented(doc_id, task_id, notes="short", files=["a.md"])
|
||||
env = await c.i_documented(doc_id, task_id, notes="wrote the docs", files=["a.md"])
|
||||
body = env.as_dict()
|
||||
assert body["error"] == "tracing_gap"
|
||||
assert "docs_notes>=min" in body["missing"]
|
||||
|
||||
@@ -763,7 +763,7 @@ async def test_complete_rejects_non_pm_role() -> None:
|
||||
deps = _make_deps(task=task_svc)
|
||||
c = Choreographer(deps)
|
||||
|
||||
env = await c.complete(dev_id, task_id, notes="x")
|
||||
env = await c.complete(dev_id, task_id, notes="reviewed and approved")
|
||||
body = env.as_dict()
|
||||
assert body["error"] == "not_authorized"
|
||||
assert "cell_pm" in body["remediate"] and "main_pm" in body["remediate"]
|
||||
@@ -816,7 +816,7 @@ async def test_escalate_up_returns_invalid_state_when_target_lookup_fails() -> N
|
||||
deps = _make_deps(task=task_svc, journal=journal_svc)
|
||||
c = Choreographer(deps)
|
||||
|
||||
env = await c.escalate_up(pm_id, task_id, reason="x")
|
||||
env = await c.escalate_up(pm_id, task_id, reason="needs cross-cell coordination")
|
||||
body = env.as_dict()
|
||||
assert body["error"] == "invalid_state"
|
||||
assert "main-pm" in body["message"]
|
||||
@@ -841,7 +841,7 @@ async def test_escalate_up_blocks_without_journal_decision() -> None:
|
||||
deps = _make_deps(task=task_svc, journal=journal_svc)
|
||||
c = Choreographer(deps)
|
||||
|
||||
env = await c.escalate_up(pm_id, task_id, reason="x")
|
||||
env = await c.escalate_up(pm_id, task_id, reason="needs cross-cell coordination")
|
||||
body = env.as_dict()
|
||||
assert body["error"] == "tracing_gap"
|
||||
assert "journal:decision" in body["missing"]
|
||||
@@ -871,7 +871,7 @@ async def test_escalate_up_no_target_returns_invalid_state() -> None:
|
||||
deps = _make_deps(task=task_svc, journal=journal_svc)
|
||||
c = Choreographer(deps)
|
||||
|
||||
env = await c.escalate_up(pm_id, task_id, reason="x")
|
||||
env = await c.escalate_up(pm_id, task_id, reason="needs cross-cell coordination")
|
||||
body = env.as_dict()
|
||||
assert body["error"] == "invalid_state"
|
||||
|
||||
@@ -885,5 +885,5 @@ async def test_escalate_up_task_not_found() -> None:
|
||||
deps = _make_deps(task=task_svc)
|
||||
c = Choreographer(deps)
|
||||
|
||||
env = await c.escalate_up(pm_id, task_id, reason="x")
|
||||
env = await c.escalate_up(pm_id, task_id, reason="needs cross-cell coordination")
|
||||
assert env.as_dict()["error"] == "not_found"
|
||||
|
||||
@@ -1008,7 +1008,7 @@ async def test_submit_up_short_notes_rejected() -> None:
|
||||
deps = _make_deps(task=task_svc)
|
||||
c = Choreographer(deps)
|
||||
|
||||
env = await c.submit_up(pm_id, task_id, notes="short")
|
||||
env = await c.submit_up(pm_id, task_id, notes="ready for now")
|
||||
body = env.as_dict()
|
||||
assert body["error"] == "tracing_gap"
|
||||
|
||||
|
||||
@@ -0,0 +1,165 @@
|
||||
"""Anti-soup guard on the flow verbs (reason / notes / issues / title / desc).
|
||||
|
||||
Two layers are tested:
|
||||
|
||||
- the pure helpers ``_free_text_soup`` (skip empty/None, reject filler, walk
|
||||
list items) and ``_soup_or_decision_env`` (soup first, then the spec
|
||||
decision, else None);
|
||||
- one end-to-end wiring test per emit style — ``i_am_blocked`` (folds soup into
|
||||
the spec-gate return) — proving a soupy ``reason`` is rejected before any
|
||||
state transition and a real reason passes through.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
from uuid import uuid4
|
||||
|
||||
import pytest
|
||||
from roboco.services.gateway.choreographer import Choreographer, ChoreographerDeps
|
||||
from roboco.services.gateway.choreographer._impl import Choreographer as _Impl
|
||||
from roboco.services.gateway.envelope import Envelope
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
# _free_text_soup
|
||||
# --------------------------------------------------------------------------- #
|
||||
|
||||
|
||||
@pytest.mark.parametrize("clean", ["a real substantive reason", "rate_limited"])
|
||||
def test_free_text_soup_passes_substantive(clean: str) -> None:
|
||||
assert _Impl._free_text_soup((("reason", clean, 8),)) is None
|
||||
|
||||
|
||||
@pytest.mark.parametrize("skip", [None, "", " "])
|
||||
def test_free_text_soup_skips_empty_and_none(skip: str | None) -> None:
|
||||
# Empty / None means "not supplied" — presence is gated elsewhere.
|
||||
assert _Impl._free_text_soup((("notes", skip, 8),)) is None
|
||||
|
||||
|
||||
@pytest.mark.parametrize("soup", ["wip", "asdf", "tbd", "...", "x", "wip wip"])
|
||||
def test_free_text_soup_rejects_filler(soup: str) -> None:
|
||||
env = _Impl._free_text_soup((("reason", soup, 3),))
|
||||
assert env is not None
|
||||
assert env.error == "invalid_state"
|
||||
|
||||
|
||||
def test_free_text_soup_walks_list_items() -> None:
|
||||
# The second issue is filler — the list form must catch it.
|
||||
env = _Impl._free_text_soup(
|
||||
(("issues", ["a genuine actionable issue", "asdf"], 8),)
|
||||
)
|
||||
assert env is not None
|
||||
assert env.error == "invalid_state"
|
||||
assert "issues[1]" in (env.message or "")
|
||||
|
||||
|
||||
def test_free_text_soup_clean_list_passes() -> None:
|
||||
env = _Impl._free_text_soup(
|
||||
(("issues", ["first real issue", "second real issue"], 8),)
|
||||
)
|
||||
assert env is None
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
# _soup_or_decision_env
|
||||
# --------------------------------------------------------------------------- #
|
||||
|
||||
|
||||
def _allow() -> MagicMock:
|
||||
return MagicMock(allowed=True)
|
||||
|
||||
|
||||
def _deny() -> MagicMock:
|
||||
return MagicMock(
|
||||
allowed=False,
|
||||
rejection_kind="invalid_state",
|
||||
message="bad state",
|
||||
remediate="do X",
|
||||
)
|
||||
|
||||
|
||||
def test_soup_or_decision_prefers_soup() -> None:
|
||||
soup = Envelope.invalid_state(message="soup", remediate="fix", context_briefing={})
|
||||
out = _Impl._soup_or_decision_env(soup, _deny(), {})
|
||||
assert out is soup # soup wins even when the decision also rejects
|
||||
|
||||
|
||||
def test_soup_or_decision_falls_back_to_decision() -> None:
|
||||
out = _Impl._soup_or_decision_env(None, _deny(), {})
|
||||
assert out is not None
|
||||
assert out.error == "invalid_state"
|
||||
assert out.message == "bad state"
|
||||
|
||||
|
||||
def test_soup_or_decision_none_when_all_clean() -> None:
|
||||
assert _Impl._soup_or_decision_env(None, _allow(), {}) is None
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
# Wiring: i_am_blocked rejects a soupy reason before any transition
|
||||
# --------------------------------------------------------------------------- #
|
||||
|
||||
|
||||
def _make_deps(agent_id: object, task_id: object) -> ChoreographerDeps:
|
||||
t = MagicMock(
|
||||
id=task_id,
|
||||
status="in_progress",
|
||||
assigned_to=agent_id,
|
||||
task_type="code",
|
||||
team="backend",
|
||||
dependency_ids=[],
|
||||
acceptance_criteria=[],
|
||||
)
|
||||
task_svc = AsyncMock()
|
||||
task_svc.get.return_value = t
|
||||
task_svc.agent_for.return_value = MagicMock(
|
||||
id=agent_id, role="developer", team="backend", slug="be-dev-1"
|
||||
)
|
||||
evidence_repo = AsyncMock()
|
||||
for m in (
|
||||
"list_unread_a2a",
|
||||
"list_unread_mentions",
|
||||
"list_pending_notifications",
|
||||
"task_metadata_gaps",
|
||||
"recent_team_activity",
|
||||
"blockers_in_lane",
|
||||
"journal_highlights_for_task",
|
||||
):
|
||||
getattr(evidence_repo, m).return_value = []
|
||||
return ChoreographerDeps(
|
||||
task=task_svc,
|
||||
work_session=AsyncMock(),
|
||||
git=AsyncMock(),
|
||||
a2a=AsyncMock(),
|
||||
journal=AsyncMock(),
|
||||
audit=AsyncMock(),
|
||||
evidence_repo=evidence_repo,
|
||||
)
|
||||
|
||||
|
||||
async def test_i_am_blocked_rejects_soup_reason() -> None:
|
||||
agent_id, task_id = uuid4(), uuid4()
|
||||
deps = _make_deps(agent_id, task_id)
|
||||
c = Choreographer(deps)
|
||||
|
||||
env = await c.i_am_blocked(agent_id, task_id, "wip")
|
||||
|
||||
assert env.error == "invalid_state"
|
||||
# The block never happened — no struggle journal, no escalate.
|
||||
deps.journal.write_struggle.assert_not_awaited()
|
||||
deps.task.escalate.assert_not_awaited()
|
||||
|
||||
|
||||
async def test_i_am_blocked_accepts_real_reason() -> None:
|
||||
agent_id, task_id = uuid4(), uuid4()
|
||||
deps = _make_deps(agent_id, task_id)
|
||||
c = Choreographer(deps)
|
||||
|
||||
env = await c.i_am_blocked(
|
||||
agent_id, task_id, "Waiting on the upstream auth schema migration."
|
||||
)
|
||||
|
||||
# A substantive reason clears the soup guard (then proceeds to the spec
|
||||
# gate / block path — which writes the struggle journal).
|
||||
assert env.error != "invalid_state" or "placeholder" not in (env.message or "")
|
||||
deps.journal.write_struggle.assert_awaited_once()
|
||||
@@ -67,7 +67,11 @@ async def test_pr_update_missing_pr_number_returns_invalid_state() -> None:
|
||||
ca = ContentActions(deps)
|
||||
|
||||
env = await ca.pr_update(
|
||||
agent_id=agent_id, task_id=task.id, title="new", body=None, reviewers=None
|
||||
agent_id=agent_id,
|
||||
task_id=task.id,
|
||||
title="updated PR title",
|
||||
body=None,
|
||||
reviewers=None,
|
||||
)
|
||||
body = env.as_dict()
|
||||
|
||||
@@ -216,16 +220,16 @@ async def test_pr_update_all_three_forwarded() -> None:
|
||||
env = await ca.pr_update(
|
||||
agent_id=agent_id,
|
||||
task_id=task.id,
|
||||
title="t",
|
||||
body="b",
|
||||
title="updated PR title",
|
||||
body="a substantive PR body",
|
||||
reviewers=["be-dev-2"],
|
||||
)
|
||||
body = env.as_dict()
|
||||
|
||||
assert body["error"] is None
|
||||
call_kwargs = git_svc.update_pr_for_task.call_args.kwargs
|
||||
assert call_kwargs["title"] == "t"
|
||||
assert call_kwargs["body"] == "b"
|
||||
assert call_kwargs["title"] == "updated PR title"
|
||||
assert call_kwargs["body"] == "a substantive PR body"
|
||||
assert call_kwargs["reviewers"] == ["be-dev-2"]
|
||||
assert set(body["evidence"]["updated_fields"]) == {"title", "body", "reviewers"}
|
||||
|
||||
@@ -250,7 +254,11 @@ async def test_pr_update_cell_pm_on_same_team_allowed() -> None:
|
||||
ca = ContentActions(deps)
|
||||
|
||||
env = await ca.pr_update(
|
||||
agent_id=pm_id, task_id=task.id, title="t", body=None, reviewers=None
|
||||
agent_id=pm_id,
|
||||
task_id=task.id,
|
||||
title="updated PR title",
|
||||
body=None,
|
||||
reviewers=None,
|
||||
)
|
||||
body = env.as_dict()
|
||||
|
||||
@@ -272,7 +280,11 @@ async def test_pr_update_cell_pm_on_other_team_rejected() -> None:
|
||||
ca = ContentActions(deps)
|
||||
|
||||
env = await ca.pr_update(
|
||||
agent_id=pm_id, task_id=task.id, title="t", body=None, reviewers=None
|
||||
agent_id=pm_id,
|
||||
task_id=task.id,
|
||||
title="updated PR title",
|
||||
body=None,
|
||||
reviewers=None,
|
||||
)
|
||||
|
||||
assert env.as_dict()["error"] == "not_authorized"
|
||||
@@ -299,7 +311,11 @@ async def test_pr_update_main_pm_any_team_allowed() -> None:
|
||||
ca = ContentActions(deps)
|
||||
|
||||
env = await ca.pr_update(
|
||||
agent_id=pm_id, task_id=task.id, title="t", body=None, reviewers=None
|
||||
agent_id=pm_id,
|
||||
task_id=task.id,
|
||||
title="updated PR title",
|
||||
body=None,
|
||||
reviewers=None,
|
||||
)
|
||||
|
||||
assert env.as_dict()["error"] is None
|
||||
@@ -316,7 +332,11 @@ async def test_pr_update_task_not_found_returns_not_found() -> None:
|
||||
ca = ContentActions(deps)
|
||||
|
||||
env = await ca.pr_update(
|
||||
agent_id=agent_id, task_id=uuid4(), title="t", body=None, reviewers=None
|
||||
agent_id=agent_id,
|
||||
task_id=uuid4(),
|
||||
title="updated PR title",
|
||||
body=None,
|
||||
reviewers=None,
|
||||
)
|
||||
body = env.as_dict()
|
||||
|
||||
@@ -339,7 +359,11 @@ async def test_pr_update_git_error_returned_as_invalid_state() -> None:
|
||||
ca = ContentActions(deps)
|
||||
|
||||
env = await ca.pr_update(
|
||||
agent_id=agent_id, task_id=task.id, title="t", body=None, reviewers=None
|
||||
agent_id=agent_id,
|
||||
task_id=task.id,
|
||||
title="updated PR title",
|
||||
body=None,
|
||||
reviewers=None,
|
||||
)
|
||||
body = env.as_dict()
|
||||
|
||||
|
||||
Reference in New Issue
Block a user