diff --git a/docs/rag/lifecycle/intent-verbs.md b/docs/rag/lifecycle/intent-verbs.md index 8f22c414..6ebc348f 100644 --- a/docs/rag/lifecycle/intent-verbs.md +++ b/docs/rag/lifecycle/intent-verbs.md @@ -173,7 +173,7 @@ Cell PM bubbles a finished cell-scope task up to Main PM. **Composes:** submit_pm_review -**Side effects:** create_pr +**Pre side effects:** create_pr ## triage diff --git a/panel/lib/lifecycle.json b/panel/lib/lifecycle.json index 73286997..f4f1066f 100644 --- a/panel/lib/lifecycle.json +++ b/panel/lib/lifecycle.json @@ -30,6 +30,7 @@ "composes": [], "description": "Claim awaiting_documentation. Returns evidence inline.", "name": "claim_doc_task", + "pre_side_effects": [], "side_effects": [] }, { @@ -39,6 +40,7 @@ "composes": [], "description": "Claim a task in awaiting_qa for review. Returns evidence inline.", "name": "claim_review", + "pre_side_effects": [], "side_effects": [] }, { @@ -51,6 +53,7 @@ ], "description": "Cell PM merges leaf PR + transitions to completed; Main PM merges root PR + escalates to CEO.", "name": "complete", + "pre_side_effects": [], "side_effects": [ "pr_merge" ] @@ -65,6 +68,7 @@ ], "description": "Create a subtask under the current task. Validates the delegation chain (main_pm->cell_pm; cell_pm->its team's devs) and the assignee-vs-task_type rule (Cell PMs get planning-typed tasks; devs get code/documentation).", "name": "delegate", + "pre_side_effects": [], "side_effects": [] }, { @@ -78,6 +82,7 @@ ], "description": "Escalate to CEO with reason. Transitions to awaiting_ceo_approval.", "name": "escalate_to_ceo", + "pre_side_effects": [], "side_effects": [] }, { @@ -88,6 +93,7 @@ "composes": [], "description": "Escalate to your role's escalation_target.", "name": "escalate_up", + "pre_side_effects": [], "side_effects": [] }, { @@ -99,6 +105,7 @@ ], "description": "Fail QA with concrete issues. Transitions to needs_revision.", "name": "fail_review", + "pre_side_effects": [], "side_effects": [] }, { @@ -112,6 +119,7 @@ "composes": [], "description": "Return your most-actionable task or signal idle.", "name": "give_me_work", + "pre_side_effects": [], "side_effects": [] }, { @@ -125,6 +133,7 @@ ], "description": "Escalate to PM. Logs a struggle journal entry.", "name": "i_am_blocked", + "pre_side_effects": [], "side_effects": [] }, { @@ -137,6 +146,7 @@ ], "description": "Submit work for QA. Auto-runs in_progress->verifying then verifying->awaiting_qa. Strict - PR must be open (call open_pr first) and >=1 commit.", "name": "i_am_done", + "pre_side_effects": [], "side_effects": [] }, { @@ -153,6 +163,7 @@ "composes": [], "description": "Signal you have no active work. PMs auto-pause owned in_progress tasks.", "name": "i_am_idle", + "pre_side_effects": [], "side_effects": [] }, { @@ -164,6 +175,7 @@ ], "description": "Signal docs complete. Transitions to awaiting_pm_review.", "name": "i_documented", + "pre_side_effects": [], "side_effects": [] }, { @@ -178,6 +190,7 @@ ], "description": "PM mirror of i_will_work_on for parent tasks. Claim, plan, transition to in_progress; from there delegate subtasks.", "name": "i_will_plan", + "pre_side_effects": [], "side_effects": [] }, { @@ -191,6 +204,7 @@ ], "description": "Claim a task, set the plan, and transition to in_progress. Atomic - preconditions checked before any state mutation.", "name": "i_will_work_on", + "pre_side_effects": [], "side_effects": [] }, { @@ -200,6 +214,7 @@ "composes": [], "description": "Push the branch and open a PR. Atomic - preconditions (assignee, >=1 commit, no prior PR) checked BEFORE any git operation. After success, call i_am_done.", "name": "open_pr", + "pre_side_effects": [], "side_effects": [ "push_branch", "create_pr" @@ -214,6 +229,7 @@ ], "description": "Pass QA. Transitions awaiting_qa -> awaiting_documentation.", "name": "pass_review", + "pre_side_effects": [], "side_effects": [] }, { @@ -229,6 +245,7 @@ ], "description": "Resume a paused task you own. paused -> in_progress.", "name": "resume", + "pre_side_effects": [], "side_effects": [] }, { @@ -240,9 +257,10 @@ ], "description": "Cell PM bubbles a finished cell-scope task up to Main PM.", "name": "submit_up", - "side_effects": [ + "pre_side_effects": [ "create_pr" - ] + ], + "side_effects": [] }, { "allowed_roles": [ @@ -255,6 +273,7 @@ "composes": [], "description": "List actionable tasks in your scope.", "name": "triage", + "pre_side_effects": [], "side_effects": [] }, { @@ -264,6 +283,7 @@ "composes": [], "description": "List actionable tasks across all teams (Main PM only).", "name": "triage_all", + "pre_side_effects": [], "side_effects": [] }, { @@ -276,6 +296,7 @@ ], "description": "PM unblocks a blocked task; restores pre-block state.", "name": "unblock", + "pre_side_effects": [], "side_effects": [] }, { @@ -289,6 +310,7 @@ "composes": [], "description": "Voluntarily release a claim back to pending. The work-in-progress branch is preserved.", "name": "unclaim", + "pre_side_effects": [], "side_effects": [] } ], diff --git a/roboco/foundation/_generators.py b/roboco/foundation/_generators.py index a2eaa8c3..5610d854 100644 --- a/roboco/foundation/_generators.py +++ b/roboco/foundation/_generators.py @@ -37,6 +37,8 @@ def _intent_verb_section(iv: IntentSpec) -> list[str]: f"**Allowed roles:** {', '.join(roles)}\n", _composes_line(iv), ] + if iv.pre_side_effects: + section.append(f"**Pre side effects:** {', '.join(iv.pre_side_effects)}\n") if iv.side_effects: section.append(f"**Side effects:** {', '.join(iv.side_effects)}\n") if iv.extra_preconditions: @@ -86,6 +88,7 @@ def _intent_to_panel_dict(iv: IntentSpec) -> dict[str, Any]: "description": iv.description, "allowed_roles": sorted(r.value for r in iv.allowed_roles), "composes": list(iv.composes), + "pre_side_effects": list(iv.pre_side_effects), "side_effects": list(iv.side_effects), } diff --git a/roboco/foundation/policy/lifecycle.py b/roboco/foundation/policy/lifecycle.py index 6d904156..2d9f7c21 100644 --- a/roboco/foundation/policy/lifecycle.py +++ b/roboco/foundation/policy/lifecycle.py @@ -186,6 +186,10 @@ class IntentSpec: don't cover (e.g. open_pr's "no PR already open" check). `side_effects` is a tuple of named git/branch/PR operations the runner invokes after the DB savepoint commits. + `pre_side_effects` are git/branch/PR operations the runner invokes + BEFORE the composing actions — for transitions that depend on a git + op having already run (e.g. submit_up must open the cell→root PR + before submit_pm_review's pr_created gate can pass). """ name: str @@ -195,6 +199,7 @@ class IntentSpec: extra_preconditions: tuple[Precondition, ...] side_effects: tuple[str, ...] next_hint: Callable[[Any], str] + pre_side_effects: tuple[str, ...] = () @dataclass(frozen=True) @@ -971,7 +976,13 @@ _INTENT_VERBS: dict[str, IntentSpec] = { description="Cell PM bubbles a finished cell-scope task up to Main PM.", composes=("submit_pm_review",), extra_preconditions=(), - side_effects=("create_pr",), + # The cell→root PR must exist BEFORE submit_pm_review runs — its + # pr_created gate rejects (returning None) otherwise, which then + # crashed the trailing create_pr on a None task. create_pr persists + # pr_number onto the task row, so submit_pm_review (which re-fetches) + # sees pr_created=True. Mirrors the dev's open_pr→i_am_done split. + pre_side_effects=("create_pr",), + side_effects=(), next_hint=lambda _t: "idle until Main PM reviews", ), "unblock": IntentSpec( diff --git a/roboco/services/gateway/choreographer/_verb_runner.py b/roboco/services/gateway/choreographer/_verb_runner.py index 3fd30434..fe8d43fe 100644 --- a/roboco/services/gateway/choreographer/_verb_runner.py +++ b/roboco/services/gateway/choreographer/_verb_runner.py @@ -41,13 +41,19 @@ class VerbRunner: agent: Any, context: spec.Context, ) -> Any: - """Run composed atomic actions in order, then side effects. + """Run pre-side-effects, then composed atomic actions, then side effects. + + Most verbs only need composes→side_effects. A few (submit_up) need a + git op to run BEFORE the DB transition it gates — those declare + ``pre_side_effects`` which run first, outside the savepoint. Returns the final task object (post-composition). Raises whatever the underlying TaskService methods raise; the savepoint context rolls the DB back on raise. """ intent = spec._INTENT_VERBS[intent_name] + for side_effect_name in intent.pre_side_effects: + await self._dispatch_side_effect(side_effect_name, task, agent) async with self.task_service.session.begin_nested(): for action_name in intent.composes: task = await self._dispatch_atomic(action_name, task, agent, context) diff --git a/tests/unit/gateway/test_verb_runner.py b/tests/unit/gateway/test_verb_runner.py index 9199b2c5..bc1d1f93 100644 --- a/tests/unit/gateway/test_verb_runner.py +++ b/tests/unit/gateway/test_verb_runner.py @@ -80,6 +80,60 @@ async def test_runner_runs_side_effects_after_db_commit() -> None: git_svc.create_pr.assert_awaited_once() +@pytest.mark.asyncio +async def test_submit_up_creates_pr_before_transition() -> None: + """#180: submit_up's create_pr (pre_side_effect) runs BEFORE the + submit_pm_review transition. + + submit_pm_review rejects (returns None) unless pr_created is already + set; create_pr persists pr_number onto the task row. With the old + composes→side_effects ordering the transition ran first, returned + None, and the trailing create_pr crashed on ``None.branch_name``. + """ + calls: list[str] = [] + + task_svc = AsyncMock() + task_svc.session.begin_nested = MagicMock( + return_value=MagicMock(__aenter__=AsyncMock(), __aexit__=AsyncMock()) + ) + + def _submit_pm_review(*_args: object, **_kwargs: object) -> MagicMock: + calls.append("submit_pm_review") + return MagicMock(status="awaiting_pm_review") + + task_svc.submit_pm_review = AsyncMock(side_effect=_submit_pm_review) + + git_svc = AsyncMock() + + def _create_pr(*_args: object, **_kwargs: object) -> dict[str, int]: + calls.append("create_pr") + return {"pr_number": 31} + + git_svc.create_pr = AsyncMock(side_effect=_create_pr) + runner = VerbRunner(task_service=task_svc, git_service=git_svc) + + task = MagicMock( + id=uuid4(), + status="in_progress", + branch_name="feature/backend/ABC12345--DEF67890", + ) + agent = MagicMock(id=uuid4(), role="cell_pm") + ctx = spec.Context(notes="cell scope complete; bubbling up to main pm") + + await runner.run_intent("submit_up", task, agent, ctx) + assert calls == ["create_pr", "submit_pm_review"], ( + f"create_pr must precede submit_pm_review for submit_up; got {calls}" + ) + + +def test_submit_up_spec_pins_pr_before_transition() -> None: + """The submit_up spec declares create_pr as a pre_side_effect, not a + trailing side_effect — the ordering fix lives in the spec.""" + submit_up = spec._INTENT_VERBS["submit_up"] + assert submit_up.pre_side_effects == ("create_pr",) + assert "create_pr" not in submit_up.side_effects + + @pytest.mark.asyncio async def test_runner_does_not_run_side_effects_if_compose_fails() -> None: """If a composed atomic action raises, side effects must NOT run."""