mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(gateway): four PM-lifecycle smoke regressions
* choreographer.escalate_up: AttributeError when target lookup returned
None. Switched from role-based escalate_up_to_role (which mishandled
slug-shaped escalation_target like "main-pm" because AgentRole only
accepts underscore form) to slug-based task.escalate, with explicit
None-handling that returns invalid_state instead of 500.
* prompts/roles/cell_pm.md + main_pm.md: enumerate the new lifecycle
verbs (i_will_plan, delegate, submit_up, give_me_work, i_am_idle).
Without this, PM agents fell back to calling i_will_work_on (the dev
verb) and 404'd at /api/v2/flow/cell_pm/i_will_work_on. Workflow
walkthroughs included.
* messaging.get_channel_by_slug + get_or_create_channel_by_slug: strip
leading "#" so "#main-pm-board" resolves to the row stored as
"main-pm-board". Agents follow Slack convention; gateway must accept
it.
* agent_sdk session-end post-mortem hook: corrected payload shape from
{content, kind:"reflect"} to {type:"task_reflection", title, content}
so /api/journals/me/entries validates. Added pad-to-min-length so the
50-char content gate doesn't reject thin post-mortems.
* test_choreographer_pm: updated escalate_up test to assert task.escalate
is awaited, plus regression test for the None-target invalid_state path.
380 unit tests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.7
parent
4520293def
commit
27dccc7215
@@ -7,18 +7,30 @@ You triage your cell's work, unblock blocked tasks, and complete (merge) tasks r
|
|||||||
- Escalation target: main-pm
|
- Escalation target: main-pm
|
||||||
|
|
||||||
## Your verbs (already loaded — no ToolSearch needed)
|
## Your verbs (already loaded — no ToolSearch needed)
|
||||||
- `triage()` — returns the highest-priority task to act on (blocked > awaiting_pm_review)
|
- `give_me_work()` — returns your highest-priority task (your own pending PM task or a subtask awaiting your review)
|
||||||
- `unblock(task_id, restore=True)` — unblock. With restore=True (default), task returns to its pre-block state.
|
- `i_will_plan(task_id, plan)` — claim YOUR cell-PM task, record your plan, transition pending → in_progress. Always call this before delegating subtasks.
|
||||||
- `complete(task_id, notes)` — mark a task complete. **Auto-merges the leaf PR into the parent task branch.**
|
- `delegate(parent_task_id, title, description, assigned_to, team, task_type, acceptance_criteria, estimated_complexity)` — create a subtask under your cell-PM task and assign it to a developer **in your cell** (e.g. `be-dev-1`, never another cell's PM). Repeat 2–5 times for focused subtasks.
|
||||||
|
- `submit_up(task_id, notes)` — once all subtasks are terminal, opens your cell-level PR up to Main PM's branch and transitions YOUR task to awaiting_pm_review. Notes ≥ 20 chars + journal:decision required.
|
||||||
|
- `triage()` — see what your cell needs next (blocked > awaiting_pm_review)
|
||||||
|
- `unblock(task_id, restore=True)` — unblock a dev's blocked subtask. With restore=True (default), task returns to its pre-block state.
|
||||||
|
- `complete(task_id, notes)` — review a SUBTASK in awaiting_pm_review. **Auto-merges the leaf PR into your task's branch.**
|
||||||
- `escalate_up(task_id, reason)` — escalate to Main PM
|
- `escalate_up(task_id, reason)` — escalate to Main PM
|
||||||
- `note(text, scope?)` — journal. Required: `scope='decision'` before unblock/complete/escalate_up.
|
- `note(text, scope?, task_id?)` — journal. Required: `scope='decision'` before i_will_plan / delegate / unblock / complete / submit_up / escalate_up.
|
||||||
- `say(channel, text)` / `dm(recipient, text)` — comms
|
- `say(channel, text)` / `dm(recipient, text)` — comms (channel name without `#` prefix, e.g. `"backend-cell"`)
|
||||||
- `evidence(task_id)` — inspect a task's PR + commits + diff
|
- `evidence(task_id)` — inspect a task's PR + commits + diff
|
||||||
- `give_me_work()` / `i_am_idle()` — like other roles
|
- `i_am_idle()` — exit cleanly; pauses any in_progress tasks you own so you'll be respawned at the right moment.
|
||||||
|
|
||||||
|
## Workflow (root delegation by Main PM → your cell-PM task)
|
||||||
|
1. `evidence(task_id="<your-task>")`
|
||||||
|
2. `note(scope='decision', task_id="<your-task>", text="<approach + subtask breakdown>")`
|
||||||
|
3. `i_will_plan(task_id="<your-task>", plan="<scope, subtasks, sequencing, risks>")`
|
||||||
|
4. `delegate(parent_task_id="<your-task>", assigned_to="<dev-slug>", ...)` — repeat per subtask
|
||||||
|
5. `i_am_idle()` — wait. You'll be respawned to: (a) review a subtask in awaiting_pm_review → `complete(subtask_id, ...)`, (b) once all subtasks terminal → `submit_up(your_task_id, ...)`.
|
||||||
|
|
||||||
## Ground rules
|
## Ground rules
|
||||||
- **You do not implement tasks yourself.** Implementation tasks belong to developers. If a cell task needs implementation, ensure a developer is assigned (or escalate_up to Main PM if no developer is available) — never write code, run `commit`, or open PRs from this seat.
|
- **You do not implement tasks yourself.** Implementation tasks belong to developers. If a cell task needs implementation, delegate it (never write code, run `commit`, or open PRs from this seat).
|
||||||
- **Do not use `Bash curl http://...orchestrator...` or `Bash git ...` for actions the gateway covers** — triage/unblock/complete/escalate/journal/comms all go through the gateway verbs. Direct API calls bypass tracing and will be rejected by the role gates.
|
- **Never call `i_will_work_on`** — that's a developer verb. Yours is `i_will_plan`.
|
||||||
- Complete is irreversible (merge happens). Verify the task is ready: subtasks all terminal, journal:decision recorded.
|
- **Do not use `Bash curl http://...orchestrator...` or `Bash git ...` for actions the gateway covers** — i_will_plan/delegate/triage/unblock/complete/submit_up/escalate/journal/comms all go through the gateway verbs. Direct API calls bypass tracing and will be rejected by the role gates.
|
||||||
|
- Complete is irreversible (merge happens). Verify the subtask is ready: PR open, journal:decision recorded.
|
||||||
|
- Subtasks MUST go to a developer slug in YOUR cell, not another cell's PM and not Main PM.
|
||||||
- Errors include a `remediate` field — follow it.
|
- Errors include a `remediate` field — follow it.
|
||||||
- Don't bypass the gate. The system catches missing tracing.
|
|
||||||
|
|||||||
@@ -7,18 +7,29 @@ You coordinate across cells, open root-task PRs to master, and escalate to CEO.
|
|||||||
- Escalation target: ceo
|
- Escalation target: ceo
|
||||||
|
|
||||||
## Your verbs (already loaded)
|
## Your verbs (already loaded)
|
||||||
|
- `give_me_work()` — returns your highest-priority task (your root in pending or a cell-PM task in awaiting_pm_review for you to merge)
|
||||||
|
- `i_will_plan(task_id, plan)` — claim YOUR root task, record your cell-distribution plan, transition pending → in_progress. Always call this before delegating to cells.
|
||||||
|
- `delegate(parent_task_id, title, description, assigned_to, team, task_type, acceptance_criteria, estimated_complexity)` — create a subtask under your root and assign it to a **Cell PM** (`be-pm`, `fe-pm`, `ux-pm`). Never assign directly to a developer slug. One subtask per cell that needs work.
|
||||||
- `triage_all()` — across all teams (blocked > awaiting_pm_review)
|
- `triage_all()` — across all teams (blocked > awaiting_pm_review)
|
||||||
- `unblock(task_id, restore=True)` — same as Cell PM
|
- `unblock(task_id, restore=True)` — unblock a cell-PM task. With restore=True (default), task returns to its pre-block state.
|
||||||
- `complete(task_id, notes)` — for root tasks: opens master PR if not already open, then escalates to CEO
|
- `complete(task_id, notes)` — for cell-PM tasks in awaiting_pm_review: merges the cell PR into your root branch. For ROOT tasks once all cell-PM subtasks are terminal: opens master PR + transitions root to awaiting_ceo_approval.
|
||||||
- `escalate_up(task_id, reason)` — escalate to CEO directly
|
- `escalate_up(task_id, reason)` — escalate to CEO via your chain
|
||||||
- `note(text, scope?)` — journal. Required: `scope='decision'` before complete/escalate_up.
|
- `escalate_to_ceo(task_id, reason)` — escalate root tasks to CEO directly (only valid in awaiting_pm_review)
|
||||||
- `say(channel, text)` / `dm(recipient, text)` — comms
|
- `note(text, scope?, task_id?)` — journal. Required: `scope='decision'` before i_will_plan / delegate / complete / escalate_*.
|
||||||
|
- `say(channel, text)` / `dm(recipient, text)` — comms (channel name without `#` prefix, e.g. `"main-pm-board"`)
|
||||||
- `evidence(task_id)` — inspect a task
|
- `evidence(task_id)` — inspect a task
|
||||||
- `give_me_work()` / `i_am_idle()`
|
- `i_am_idle()` — exit cleanly; pauses any in_progress tasks you own so you'll be respawned at the right moment.
|
||||||
|
|
||||||
|
## Workflow (CEO assigns a root task to you)
|
||||||
|
1. `evidence(task_id="<root>")`
|
||||||
|
2. `note(scope='decision', task_id="<root>", text="<plan summary: cells X/Y get subtasks A/B>")`
|
||||||
|
3. `i_will_plan(task_id="<root>", plan="<scope, cell breakdown, sequencing, risks>")`
|
||||||
|
4. `delegate(parent_task_id="<root>", assigned_to="be-pm"|"fe-pm"|"ux-pm", team="backend"|"frontend"|"ux_ui", ...)` — repeat per cell needing work.
|
||||||
|
5. `i_am_idle()` — wait. You'll be respawned to: (a) review a cell-PM task in awaiting_pm_review → `complete(cell_pm_task_id, ...)` (merges cell PR into root branch), (b) once all subtasks terminal → `complete(root_id, ...)` (opens master PR + escalates to CEO).
|
||||||
|
|
||||||
## Ground rules
|
## Ground rules
|
||||||
- **You do not implement tasks yourself.** Implementation tasks belong to developers. If a root task needs implementation, ensure a developer is assigned (escalate_up to a Cell PM if needed) — never `commit` or write code from this seat.
|
- **You do not implement tasks yourself.** Implementation tasks belong to developers. If a root task needs implementation, delegate it to a Cell PM (never `commit` or write code from this seat).
|
||||||
- **Do not use `Bash curl http://...orchestrator...` or `Bash git ...` for actions the gateway covers** — triage/unblock/complete/escalate/journal/comms all go through the gateway verbs. Direct API calls bypass tracing and will be rejected by the role gates.
|
- **Never call `i_will_work_on`** — that's a developer verb. Yours is `i_will_plan`.
|
||||||
- Main PM only completes ROOT tasks (no parent_task_id). Cell PMs complete their own scope.
|
- **Never assign a code subtask directly to a developer slug** — always to a Cell PM. The Cell PM breaks it down further.
|
||||||
- After your `complete`, the task is in awaiting_ceo_approval — CEO acts via UI.
|
- **Do not use `Bash curl http://...orchestrator...` or `Bash git ...` for actions the gateway covers** — i_will_plan/delegate/triage_all/unblock/complete/escalate/journal/comms all go through the gateway verbs.
|
||||||
- Errors include a `remediate` field — follow it.
|
- Errors include a `remediate` field — follow it.
|
||||||
|
|||||||
+18
-10
@@ -697,17 +697,25 @@ def _terminal_snapshot() -> TerminalStatus:
|
|||||||
async def journal_post_mortem(req: PostMortemRequest) -> dict[str, str]:
|
async def journal_post_mortem(req: PostMortemRequest) -> dict[str, str]:
|
||||||
"""SessionEnd hook submits a post-mortem; we log it and flush to the main API."""
|
"""SessionEnd hook submits a post-mortem; we log it and flush to the main API."""
|
||||||
duration = req.duration_seconds or (time.time() - _state.started_at)
|
duration = req.duration_seconds or (time.time() - _state.started_at)
|
||||||
|
content = (
|
||||||
|
"[post-mortem]\n"
|
||||||
|
f"terminal_tool: {req.terminal_tool}\n"
|
||||||
|
f"duration_seconds: {duration:.1f}\n"
|
||||||
|
f"tools_called: {req.tools_called or _state.total_calls}\n"
|
||||||
|
f"loop_triggered: {req.loop_triggered or _state.loop_triggered}\n"
|
||||||
|
f"halt_triggered: {req.halt_triggered or _state.halt_triggered}\n"
|
||||||
|
f"reason: {req.reason}"
|
||||||
|
)
|
||||||
|
# Pad short content to clear the journal min-length gate
|
||||||
|
# (task_reflection requires >= 50 chars). Pad with a small margin so
|
||||||
|
# the gate doesn't reject borderline post-mortems.
|
||||||
|
_MIN_REFLECTION_CHARS = 60
|
||||||
|
if len(content) < _MIN_REFLECTION_CHARS:
|
||||||
|
content = content + "\n" + ("-" * (_MIN_REFLECTION_CHARS - len(content)))
|
||||||
payload = {
|
payload = {
|
||||||
"content": (
|
"type": "task_reflection",
|
||||||
"[post-mortem]\n"
|
"title": f"session_end: {req.reason or 'unknown'}",
|
||||||
f"terminal_tool: {req.terminal_tool}\n"
|
"content": content,
|
||||||
f"duration_seconds: {duration:.1f}\n"
|
|
||||||
f"tools_called: {req.tools_called or _state.total_calls}\n"
|
|
||||||
f"loop_triggered: {req.loop_triggered or _state.loop_triggered}\n"
|
|
||||||
f"halt_triggered: {req.halt_triggered or _state.halt_triggered}\n"
|
|
||||||
f"reason: {req.reason}"
|
|
||||||
),
|
|
||||||
"kind": "reflect",
|
|
||||||
}
|
}
|
||||||
try:
|
try:
|
||||||
async with httpx.AsyncClient() as client:
|
async with httpx.AsyncClient() as client:
|
||||||
|
|||||||
@@ -1427,21 +1427,31 @@ class Choreographer:
|
|||||||
)
|
)
|
||||||
|
|
||||||
me = await self.task.agent_for(pm_agent_id)
|
me = await self.task.agent_for(pm_agent_id)
|
||||||
target_role = me.escalation_target
|
target_slug = me.escalation_target if me else None
|
||||||
if not target_role:
|
if not target_slug:
|
||||||
return Envelope.invalid_state(
|
return Envelope.invalid_state(
|
||||||
message="no escalation target configured for your role",
|
message="no escalation target configured for your role",
|
||||||
remediate="check agents_config.py for your role's escalation_target",
|
remediate="check agents_config.py ESCALATION_CHAIN for your slug",
|
||||||
context_briefing=await self._briefing_for(pm_agent_id, task_id),
|
context_briefing=await self._briefing_for(pm_agent_id, task_id),
|
||||||
)
|
)
|
||||||
|
|
||||||
t = await self.task.escalate_up_to_role(
|
t = await self.task.escalate(pm_agent_id, task_id, reason)
|
||||||
pm_agent_id, task_id, target_role, reason
|
if t is None:
|
||||||
)
|
return Envelope.invalid_state(
|
||||||
|
message=(
|
||||||
|
f"could not escalate task {task_id} to {target_slug}: "
|
||||||
|
"target agent not found or task missing"
|
||||||
|
),
|
||||||
|
remediate=(
|
||||||
|
f"verify {target_slug} exists in agents table and that the "
|
||||||
|
"task is still present"
|
||||||
|
),
|
||||||
|
context_briefing=await self._briefing_for(pm_agent_id, task_id),
|
||||||
|
)
|
||||||
return Envelope.ok(
|
return Envelope.ok(
|
||||||
status=str(t.status),
|
status=str(t.status),
|
||||||
task_id=str(task_id),
|
task_id=str(task_id),
|
||||||
next=f"escalated to {target_role}; idle until they respond",
|
next=f"escalated to {target_slug}; idle until they respond",
|
||||||
context_briefing=await self._briefing_for(pm_agent_id, task_id),
|
context_briefing=await self._briefing_for(pm_agent_id, task_id),
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|||||||
@@ -263,9 +263,15 @@ class MessagingService(BaseService):
|
|||||||
await self.session.flush()
|
await self.session.flush()
|
||||||
|
|
||||||
async def get_channel_by_slug(self, slug: str) -> ChannelTable | None:
|
async def get_channel_by_slug(self, slug: str) -> ChannelTable | None:
|
||||||
"""Get a channel by slug."""
|
"""Get a channel by slug.
|
||||||
|
|
||||||
|
Strips a leading ``#`` so agents passing channel names with the
|
||||||
|
Slack-style ``#`` prefix (e.g. ``#main-pm-board``) resolve to the
|
||||||
|
same row stored without it.
|
||||||
|
"""
|
||||||
|
normalized = slug.lstrip("#") if slug else slug
|
||||||
result = await self.session.execute(
|
result = await self.session.execute(
|
||||||
select(ChannelTable).where(ChannelTable.slug == slug)
|
select(ChannelTable).where(ChannelTable.slug == normalized)
|
||||||
)
|
)
|
||||||
return result.scalar_one_or_none()
|
return result.scalar_one_or_none()
|
||||||
|
|
||||||
@@ -284,8 +290,9 @@ class MessagingService(BaseService):
|
|||||||
Returns:
|
Returns:
|
||||||
Channel if found or created, None if not a valid channel
|
Channel if found or created, None if not a valid channel
|
||||||
"""
|
"""
|
||||||
|
normalized = slug.lstrip("#") if slug else slug
|
||||||
# First try database
|
# First try database
|
||||||
channel = await self.get_channel_by_slug(slug)
|
channel = await self.get_channel_by_slug(normalized)
|
||||||
if channel:
|
if channel:
|
||||||
return channel
|
return channel
|
||||||
|
|
||||||
@@ -294,7 +301,7 @@ class MessagingService(BaseService):
|
|||||||
from roboco.seeds import DEFAULT_CHANNELS
|
from roboco.seeds import DEFAULT_CHANNELS
|
||||||
|
|
||||||
channel_data = next(
|
channel_data = next(
|
||||||
(c for c in DEFAULT_CHANNELS if c["slug"] == slug),
|
(c for c in DEFAULT_CHANNELS if c["slug"] == normalized),
|
||||||
None,
|
None,
|
||||||
)
|
)
|
||||||
if not channel_data:
|
if not channel_data:
|
||||||
|
|||||||
@@ -551,9 +551,9 @@ async def test_escalate_up_routes_by_escalation_target() -> None:
|
|||||||
task_svc.get.return_value = t
|
task_svc.get.return_value = t
|
||||||
task_svc.agent_for.return_value = MagicMock(
|
task_svc.agent_for.return_value = MagicMock(
|
||||||
role="cell_pm",
|
role="cell_pm",
|
||||||
escalation_target="main_pm",
|
escalation_target="main-pm",
|
||||||
)
|
)
|
||||||
task_svc.escalate_up_to_role.return_value = after
|
task_svc.escalate.return_value = after
|
||||||
journal_svc = AsyncMock()
|
journal_svc = AsyncMock()
|
||||||
journal_svc.has_decision_for_task.return_value = True
|
journal_svc.has_decision_for_task.return_value = True
|
||||||
deps = _make_deps(task=task_svc, journal=journal_svc)
|
deps = _make_deps(task=task_svc, journal=journal_svc)
|
||||||
@@ -561,14 +561,37 @@ async def test_escalate_up_routes_by_escalation_target() -> None:
|
|||||||
|
|
||||||
env = await c.escalate_up(pm_id, task_id, reason="cross-cell coordination needed")
|
env = await c.escalate_up(pm_id, task_id, reason="cross-cell coordination needed")
|
||||||
assert env.error is None
|
assert env.error is None
|
||||||
task_svc.escalate_up_to_role.assert_awaited_once_with(
|
task_svc.escalate.assert_awaited_once_with(
|
||||||
pm_id,
|
pm_id,
|
||||||
task_id,
|
task_id,
|
||||||
"main_pm",
|
|
||||||
"cross-cell coordination needed",
|
"cross-cell coordination needed",
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_escalate_up_returns_invalid_state_when_target_lookup_fails() -> None:
|
||||||
|
"""Regression: escalate_up_to_role returning None used to crash on t.status."""
|
||||||
|
pm_id = uuid4()
|
||||||
|
task_id = uuid4()
|
||||||
|
t = MagicMock(id=task_id, status="blocked", assigned_to=pm_id, team="backend")
|
||||||
|
task_svc = AsyncMock()
|
||||||
|
task_svc.get.return_value = t
|
||||||
|
task_svc.agent_for.return_value = MagicMock(
|
||||||
|
role="cell_pm",
|
||||||
|
escalation_target="main-pm",
|
||||||
|
)
|
||||||
|
task_svc.escalate.return_value = None # target slug not found in DB
|
||||||
|
journal_svc = AsyncMock()
|
||||||
|
journal_svc.has_decision_for_task.return_value = True
|
||||||
|
deps = _make_deps(task=task_svc, journal=journal_svc)
|
||||||
|
c = Choreographer(deps)
|
||||||
|
|
||||||
|
env = await c.escalate_up(pm_id, task_id, reason="x")
|
||||||
|
body = env.as_dict()
|
||||||
|
assert body["error"] == "invalid_state"
|
||||||
|
assert "main-pm" in body["message"]
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_escalate_up_blocks_without_journal_decision() -> None:
|
async def test_escalate_up_blocks_without_journal_decision() -> None:
|
||||||
pm_id = uuid4()
|
pm_id = uuid4()
|
||||||
|
|||||||
Reference in New Issue
Block a user