mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
refactor(gateway): consolidate commit + notify role gates into verb_gates
Replaces hardcoded role-string-constants in content_actions.py (_COMMIT_ALLOWED_ROLES, _NOTIFY_ALLOWED_ROLES) with calls into verb_gates.is_verb_allowed against a synthetic in-progress task probe. Pre-fix the same role lists lived in both content_actions and verb_gates; if one drifted the other would mask it. Now there's one table. Adds `notify` to verb_gates._ALWAYS_AVAILABLE for cell_pm, main_pm, product_owner, head_marketing. Note: i_will_plan / delegate role checks INTENTIONALLY stay as explicit `role not in (cell_pm, main_pm)` checks, not is_verb_allowed. Their state checks must surface as `invalid_state` (a different agent-side error code) — conflating them with role-state combo checks breaks the rejection-code semantics agents rely on. Tests: 3127 passing, 100% coverage, ruff clean.
This commit is contained in:
@@ -1087,12 +1087,17 @@ class Choreographer:
|
||||
producing the cycle smoke 2026-05-04 captured.
|
||||
"""
|
||||
agent = await self.task.agent_for(pm_agent_id)
|
||||
if agent is None or agent.role not in ("cell_pm", "main_pm"):
|
||||
role = str(agent.role) if agent is not None else ""
|
||||
# Role-only gate: i_will_plan is principle-level reserved for PMs.
|
||||
# The status check below ((pending → in_progress) is a separate gate
|
||||
# whose rejection must surface as `invalid_state`, not
|
||||
# `not_authorized` — agents react to those two errors differently.
|
||||
if role not in ("cell_pm", "main_pm"):
|
||||
return Envelope.not_authorized(
|
||||
message="only cell_pm or main_pm may call i_will_plan",
|
||||
remediate="this verb is reserved for PMs",
|
||||
context_briefing=await self._briefing_for(pm_agent_id, task_id),
|
||||
)
|
||||
).with_introspection(task=t, role=role)
|
||||
status = str(t.status)
|
||||
if status != "pending":
|
||||
# Idempotent re-entry: caller already owns this task in a
|
||||
@@ -1295,7 +1300,8 @@ class Choreographer:
|
||||
if guard := await self._delegate_role_guards(
|
||||
pm_agent_id, parent_task_id, agent, inputs
|
||||
):
|
||||
return guard
|
||||
role = str(agent.role) if agent is not None else ""
|
||||
return guard.with_introspection(task=parent, role=role)
|
||||
if guard := await self._delegate_static_guards(
|
||||
pm_agent_id, parent_task_id, parent, inputs
|
||||
):
|
||||
@@ -1312,7 +1318,13 @@ class Choreographer:
|
||||
agent: Any,
|
||||
inputs: DelegateInputs,
|
||||
) -> Envelope | None:
|
||||
"""Role + delegation-chain guards (the original two)."""
|
||||
"""Role + delegation-chain guards (the original two).
|
||||
|
||||
Role gate stays role-only here (not via is_verb_allowed) — the
|
||||
parent-status check is a separate gate that must surface as
|
||||
`invalid_state`, not `not_authorized`. See _i_will_plan_preflight
|
||||
for the same rationale.
|
||||
"""
|
||||
if agent is None or agent.role not in ("cell_pm", "main_pm"):
|
||||
return Envelope.not_authorized(
|
||||
message="only cell_pm or main_pm may delegate",
|
||||
|
||||
@@ -18,6 +18,7 @@ from typing import TYPE_CHECKING, Any
|
||||
from roboco.services.gateway.commit_validator import validate_commit_message
|
||||
from roboco.services.gateway.envelope import Envelope
|
||||
from roboco.services.gateway.evidence_builder import build_evidence_for_task
|
||||
from roboco.services.gateway.verb_gates import is_verb_allowed
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from uuid import UUID
|
||||
@@ -59,19 +60,24 @@ class ContentActionsDeps:
|
||||
notifications: Any
|
||||
|
||||
|
||||
# Roles authorized to issue formal ack-required notifications via `notify`.
|
||||
# Pre-gateway, NotificationService callers were gated by the same set
|
||||
# (PMs and Board members); the gateway re-asserts that gate at the verb
|
||||
# layer because the do.py router is shared by all roles.
|
||||
_NOTIFY_ALLOWED_ROLES: frozenset[str] = frozenset(
|
||||
{"cell_pm", "main_pm", "product_owner", "head_marketing"}
|
||||
)
|
||||
# Notification authorization is sourced from verb_gates._ALWAYS_AVAILABLE
|
||||
# (which lists `notify` for cell_pm, main_pm, product_owner, head_marketing).
|
||||
# Pre-gateway this lived as a `_NOTIFY_ALLOWED_ROLES` constant here; merging
|
||||
# into verb_gates removes the risk that the two sets disagree.
|
||||
_VALID_NOTIFY_PRIORITIES: frozenset[str] = frozenset({"normal", "high", "urgent"})
|
||||
|
||||
# Only roles whose manifest includes "commit" should reach the verb body.
|
||||
# Server-side gate is defense-in-depth in case the MCP manifest filter ever
|
||||
# misroutes the call (smoke 2026-05-03 saw main-pm hit the git layer).
|
||||
_COMMIT_ALLOWED_ROLES: frozenset[str] = frozenset({"developer", "documenter"})
|
||||
# Synthetic task probe for role-only gate checks: when the verb body
|
||||
# wants to fast-fail on role BEFORE loading the agent's active task,
|
||||
# we hand verb_gates an in-progress code-typed shape so it consults
|
||||
# the same _STATE_VERBS row a real in-progress task would.
|
||||
class _RoleProbeTask:
|
||||
"""Minimal task-shaped object for role-only is_verb_allowed checks."""
|
||||
|
||||
status: str = "in_progress"
|
||||
task_type: str = "code"
|
||||
|
||||
|
||||
_ROLE_PROBE = _RoleProbeTask()
|
||||
|
||||
|
||||
class ContentActions:
|
||||
@@ -119,8 +125,8 @@ class ContentActions:
|
||||
records progress entry from the commit message.
|
||||
"""
|
||||
agent = await self.task.agent_for(agent_id)
|
||||
caller_role = agent.role if agent is not None else None
|
||||
if caller_role not in _COMMIT_ALLOWED_ROLES:
|
||||
caller_role = str(agent.role) if agent is not None else ""
|
||||
if not is_verb_allowed(caller_role, "commit", _ROLE_PROBE):
|
||||
return Envelope.not_authorized(
|
||||
message=(
|
||||
f"role '{caller_role}' may not commit code; only"
|
||||
@@ -347,8 +353,8 @@ class ContentActions:
|
||||
context_briefing={},
|
||||
)
|
||||
agent = await self.task.agent_for(agent_id)
|
||||
caller_role = agent.role if agent is not None else None
|
||||
if caller_role not in _NOTIFY_ALLOWED_ROLES:
|
||||
caller_role = str(agent.role) if agent is not None else ""
|
||||
if not is_verb_allowed(caller_role, "notify", _ROLE_PROBE):
|
||||
return Envelope.not_authorized(
|
||||
message=(
|
||||
f"role {caller_role!r} cannot send formal notifications; "
|
||||
|
||||
@@ -19,14 +19,18 @@ from typing import Any
|
||||
_PM_ROLES: frozenset[str] = frozenset({"cell_pm", "main_pm"})
|
||||
|
||||
# Always-available verbs (don't depend on task state).
|
||||
#
|
||||
# `notify` (formal acked notifications) is on the PM/Board roles only —
|
||||
# pre-gateway it was gated by `_NOTIFY_ALLOWED_ROLES` in
|
||||
# `content_actions.py`; now consolidated here.
|
||||
_ALWAYS_AVAILABLE: dict[str, frozenset[str]] = {
|
||||
"developer": frozenset({"i_am_idle", "give_me_work"}),
|
||||
"qa": frozenset({"i_am_idle", "give_me_work"}),
|
||||
"documenter": frozenset({"i_am_idle", "give_me_work"}),
|
||||
"cell_pm": frozenset({"i_am_idle", "give_me_work", "triage"}),
|
||||
"main_pm": frozenset({"i_am_idle", "give_me_work", "triage_all"}),
|
||||
"product_owner": frozenset({"i_am_idle", "triage"}),
|
||||
"head_marketing": frozenset({"i_am_idle", "triage"}),
|
||||
"cell_pm": frozenset({"i_am_idle", "give_me_work", "triage", "notify"}),
|
||||
"main_pm": frozenset({"i_am_idle", "give_me_work", "triage_all", "notify"}),
|
||||
"product_owner": frozenset({"i_am_idle", "triage", "notify"}),
|
||||
"head_marketing": frozenset({"i_am_idle", "triage", "notify"}),
|
||||
"auditor": frozenset({"i_am_idle", "triage"}),
|
||||
}
|
||||
|
||||
|
||||
@@ -171,3 +171,37 @@ def test_is_verb_allowed_false_for_blocked_verb() -> None:
|
||||
|
||||
def test_is_verb_allowed_false_for_unknown_role() -> None:
|
||||
assert is_verb_allowed("nope", "i_am_idle", _task("pending")) is False
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------
|
||||
# Task 4: verb_gates is the single source of truth for content tools too
|
||||
# ---------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_commit_allowed_for_developer_and_documenter_only() -> None:
|
||||
"""Was a hardcoded set in content_actions; now lives in verb_gates."""
|
||||
task = _task("in_progress", task_type="code")
|
||||
assert is_verb_allowed("developer", "commit", task) is True
|
||||
assert is_verb_allowed("documenter", "commit", task) is True
|
||||
assert is_verb_allowed("qa", "commit", task) is False
|
||||
assert is_verb_allowed("cell_pm", "commit", task) is False
|
||||
assert is_verb_allowed("main_pm", "commit", task) is False
|
||||
|
||||
|
||||
def test_commit_not_offered_when_task_is_not_in_progress() -> None:
|
||||
"""commit is a per-state verb — not offered when state forbids it."""
|
||||
task = _task("completed", task_type="code")
|
||||
assert is_verb_allowed("developer", "commit", task) is False
|
||||
|
||||
|
||||
def test_notify_allowed_for_pm_and_board_only() -> None:
|
||||
"""Was a hardcoded set in content_actions; now lives in verb_gates."""
|
||||
task = _task("in_progress", task_type="code")
|
||||
assert is_verb_allowed("cell_pm", "notify", task) is True
|
||||
assert is_verb_allowed("main_pm", "notify", task) is True
|
||||
assert is_verb_allowed("product_owner", "notify", task) is True
|
||||
assert is_verb_allowed("head_marketing", "notify", task) is True
|
||||
assert is_verb_allowed("developer", "notify", task) is False
|
||||
assert is_verb_allowed("qa", "notify", task) is False
|
||||
assert is_verb_allowed("documenter", "notify", task) is False
|
||||
assert is_verb_allowed("auditor", "notify", task) is False
|
||||
|
||||
Reference in New Issue
Block a user