mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
feat: in-path PR-review gate — per-cell + main reviewers (#229)
* feat(lifecycle): add the in-path PR-review gate status + reviewer verbs
Insert awaiting_pr_review between the assembled-PR submit and the PM merge,
giving the merge level the rejection capability it structurally lacks — today
only qa_fail and ceo_reject ever reach needs_revision, so a PM review is a
merge button with no teeth.
- New Status awaiting_pr_review + submit_for_review / pr_pass / pr_fail actions
(pr_pass -> awaiting_pm_review, pr_fail -> needs_revision, mirroring the QA gate).
- Reviewer verbs claim_gate_review / pr_pass / pr_fail, and a main-PM submit_root
verb (the root analogue of the cell PM's submit_up; opens the root->master PR).
- Extend the self-review-symmetry validator to the new sign-off actions.
- Mirror the value into the ORM TaskStatus enum + the A2A state map, and add the
postgres taskstatus enum value (migration 040, forward-only like 037).
- Regenerate the per-role verb tables; add gate spec tests.
Spec surface only; the gateway methods + dispatch are wired in follow-ups, so the
verbs are advertised but dormant (flow_server tolerates unregistered verbs).
* feat(identity): add the three cell PR-review-gate reviewers
The in-path gate needs a reviewer per cell so each cell's assembled cell->root
PR is reviewed by a stack-specialized agent, while pr-reviewer-1 serves the
root->master gate (and keeps doing inbound external PRs).
- be/fe/ux-pr-reviewer: PR_REVIEWER role, team-scoped (so dispatch routes each
cell's gate to its own reviewer); seeded identities + ROLE_TEAM_RULES + names.
AI agent count 22 -> 25.
- They reuse the existing roboco-agent-pr-reviewer image (AGENT_IMAGES maps the
three slugs to it, as be-dev-1/-2 share one image) — no new image.
- Tracing table: pr_pass/pr_fail require a learning entry (parity with
post_pr_review), submit_root mirrors submit_up, claim_gate_review is waived
(its tracing applies on pr_pass/pr_fail) — completes the verb surface added
in the prior commit.
- Update the roster-pinning identity tests.
* feat(gateway): wire the in-path PR-review gate end to end
Make the assembled-PR review gate operational across the choreographer, the
TaskService transitions, and the v1 flow surface.
- TaskService: submit_for_review (in_progress→awaiting_pr_review), pr_gate_claim
(no-transition reviewer claim), pr_pass (→awaiting_pm_review), pr_fail
(→needs_revision); mirror qa_pass/qa_fail (clear claim, actor-mismatch warn,
issues appended for the PM's revision). VerbRunner gains the matching atomic
handlers + a create_root_pr side effect.
- Repoint submit_up to compose submit_for_review (cell→root PR enters the gate),
and add a main-PM submit_root verb (opens the root→master PR, enters the gate).
- Split main_pm_complete: a code root must pass the gate first (requires
awaiting_pm_review; rejects an in_progress code root toward submit_root and no
longer reopens the PR), while a branchless coordination root still walks
straight through, ungated.
- PRGateMixin (claim_gate_review / pr_pass / pr_fail) composed onto the
Choreographer; flow_server forwarders + v1 routes (pr_reviewer + main_pm) +
request schemas.
- Tests: gate spec + the updated submit_up / main_pm_complete expectations + new
real-DB integration tests driving submit_for_review→pr_gate_claim→pr_pass and
pr_fail through the real enforcement layer.
* feat(orchestrator): dispatch the in-path PR-review gate
Make the gate live in the dispatch loop.
- _dispatch_pr_gate_work: route awaiting_pr_review tasks to reviewers by level —
a cell→root task to its cell reviewer (be/fe/ux-pr-reviewer), the root→master
task to pr-reviewer-1. The reviewer self-claims via claim_gate_review (no
pre-claim, mirroring the external-PR dispatcher); registered in
_dispatch_all_work. _select_agent_for_cell learns the pr_reviewer role.
- _build_pr_gate_prompt: anchors the reviewer to the parent objective + full
acceptance criteria + the FE<->BE contract, then pr_pass / pr_fail.
- _readiness_check_role_for_status: awaiting_pr_review -> pr_reviewer.
- Fail routing: pr_fail reassigns the failed assembled task to its PM
(_revision_pm_for_task: cell PM for a cell team, Main PM for the root), and the
revision dispatcher is generalized from coordination-roots-only to any
PM-owned needs_revision task so the gate-failed task is re-coordinated instead
of deadlocking.
* docs: document the in-path PR-review gate + the cell reviewers (22→25)
Reflect the shipped gate across the canonical + RAG docs.
- CLAUDE.md: agent count 22→25, the cell reviewers in the org chart, an
awaiting_pr_review state + the gate transitions + a gate note in the lifecycle
section, and submit_root / claim_gate_review / pr_pass / pr_fail in the verb
surface table.
- docs/rag/architecture: org-structure (count, cell-reviewer roster, cells
table), agent-uuids (be/fe/ux-pr-reviewer rows), agent-model (role + team
rows).
- docs/rag/roles/pr-reviewer: the in-path gate section + the gate verbs.
- Wrap reviewer.id with UUID(str(...)) in the gate DB tests for mypy.
* docs: finish the gate doc sweep across README + RAG + generated artifacts
Catch the remaining surfaces beyond the canonical docs.
- README + how-to: agent count 22→25, the 6-agent cells (+ PR Reviewer), the
main reviewer's root→master gate role.
- RAG: permissions + tool-permissions + task-tools list the gate verbs
(claim_gate_review / pr_pass / pr_fail) for pr_reviewer; regenerate the
lifecycle artifacts (intent-verbs, status-transitions, the per-role
lifecycle-*.md prompts, panel lifecycle.json) from the spec via
build_lifecycle_artifacts.py so they carry the new status + verbs.
* fix(migration): shorten the 040 revision id to fit alembic_version VARCHAR(32)
The revision id '040_taskstatus_awaiting_pr_review' is 33 chars; alembic's
alembic_version.version_num column is VARCHAR(32), so recording the migration on
a real 'alembic upgrade head' failed with 'value too long for type character
varying(32)' (surfaced on the NAS deploy). The test suite missed it: the test DB
is built via Base.metadata.create_all and the parity test only renders SQL
offline, so nothing actually applied the migration chain.
- Rename to '040_awaiting_pr_review' (22 chars).
- Add a guard test asserting every revision id fits the VARCHAR(32) column.
- Verified by applying the full chain 001->040 against real Postgres: it now
reaches head and records '040_awaiting_pr_review' without truncation.
* fix(migration): land the actual 040 revision-id shortening + guard test
The prior commit captured only the file rename (git add aborted on the deleted
old path), leaving the long revision id and missing the guard test. This commit
carries the real content: revision id '040_awaiting_pr_review' (22 chars) and the
revision-id length guard. Re-verified against real Postgres — the full chain
reaches head and records the short id without truncation.
* fix(product): flush cell deletes before inserts when re-mapping projects
Editing a product's cell->project map (PATCH /api/products/{id}) 409'd with
'duplicate key value violates unique constraint uq_product_projects_product_team'
whenever a team already had a mapping. _replace_cells clears the old rows and
appends the new ones, but within a single flush SQLAlchemy orders INSERTs before
DELETEs for the same table, so the new (product_id, team) rows collided with the
not-yet-deleted old ones. Flush the deletes first.
Pre-existing bug (unrelated to the PR-review gate); surfaced on the NAS. New
real-Postgres regression test re-maps all three cells to different projects —
it fails with the unique violation without the fix and passes with it. The
existing update test only changed WHICH team was mapped, so it never collided.
* fix(gateway): let main_pm submit_root past the shared submit-up guard
submit_root reused the cell PM's _submit_up_ownership_guard, which
hardcoded agent.role != cell_pm and rejected the Main PM with
"submit_up is reserved for cell_pm". A branch-bearing code root could
then never close: submit_root bounced to complete, while complete
required awaiting_pm_review (reachable only via submit_root) and bounced
back — a circular rejection.
Both callers already run the spec gate (can_invoke_intent), which
enforces submit_up→cell_pm and submit_root→main_pm, so the guard's role
re-check was redundant for submit_up and wrong for submit_root. Broaden
it to accept either PM role as a defense-in-depth non-PM reject.
Adds the first choreographer-level submit_root test (the gap that let
this ship).
* fix(gateway): proactively steer both PMs to their bubble-up verb
The submit_root deadlock had a sibling steering gap: nothing told a PM
which verb opens the gate. The delegate next-hint said only 'i_am_idle
when done', and complete's in_progress rejection named submit_root for
the Main PM but left the Cell PM with a bare 'not ready for completion'
— no submit_up pointer, the same guess-the-verb trap.
- delegate hint now names the role-correct verb (root → submit_root,
cell parent → submit_up) proactively, before any rejection.
- cell_pm_complete's in_progress rejection now steers to submit_up,
mirroring the Main PM's submit_root gate hint.
Tests cover both the cell-PM steer and the role-aware delegate hint.
* docs: correct who-merges-which-PR across the gate docs + complete description
Audit of the gate docs found the merge actors mis-stated in several
places — the exact ambiguity that risks 'the reviewer/PM merges the root
PR' confusion:
- complete IntentSpec description said 'Main PM merges root PR' — false;
main_pm_complete escalates and the CEO merges root→master. Corrected
(propagated to intent-verbs.md, lifecycle.json, generated role prompts
via build_lifecycle_artifacts.py).
- task-tools.md: submit_up target was awaiting_pm_review (should be
awaiting_pr_review); Main PM flow had no submit_root — added it.
- README.md: lifecycle diagram now shows the awaiting_pr_review gate.
- cell-pm.md / main-pm.md: dropped the stale 'submit_up hands work to the
Main PM who merges your cell branch' model — the cell PM merges its own
gated cell→root PR; the Main PM owns the root + submit_root; the CEO
merges master. Added submit_root to the main-pm manifest.
- git-commits.md, pr-creation.md, tool-permissions.md, git-tools.md:
stopped attributing root→master PR opening to complete (it's submit_root).
No behavior change; verb wiring + state machine verified gap-free this
session (the pr_fail→needs_revision→PM respawn loop closes correctly).
* fix(orchestrator): stop closure respawn waiting the reaper window
A PM that finished its subtasks and idled left its parent 'paused' with a
fresh last_heartbeat_at. _is_recently_paused gated closure respawn on
_claim_heartbeat_ttl — the REAPER window (stale_claim_reap_seconds: 600s
default, 1800s on the NAS) — so the parent sat untouched for up to 10-30
minutes before its PM was respawned to close it. The whole chain stalled
behind it.
The race that guard actually protects against (i_am_idle auto-pauses, then
the agent is marked IDLE + its container tears down) is seconds, and the
live-session case is already covered by _is_agent_active. Introduce a
dedicated short debounce (pm_closure_recently_paused_seconds, default 45s)
and gate closure on that instead.
The existing test fixture masked this by setting _claim_heartbeat_ttl to
claim_stale_seconds (180s), not the production reaper value. Fixture now
mirrors production; adds a regression test that a parent paused past the
debounce but within the reaper window respawns immediately.
* feat(gate): post the in-path review verdict on the assembled PR
The in-path gate previously left no trace on the PR it gated — pr_pass /
pr_fail were pure status transitions. Now each verdict is posted as a
GitHub review on the assembled PR itself (server-side, bot account), so
the decision is visible on the very PR the PM merges.
- pr_pass → APPROVE, pr_fail → REQUEST_CHANGES on a cell→root PR.
- The root→master PR ALWAYS gets a plain COMMENT, never APPROVE/REQUEST_
CHANGES: only the CEO acts on master, so the gate must never leave an
approval that could satisfy branch protection (letting someone else
merge) nor a blocking review that could impede the CEO's merge.
- Best-effort and AFTER the DB transition — a GitHub failure is logged,
never rolls back the gate decision. Reuses git.post_pr_review's existing
self-review→COMMENT downgrade for the org's own PRs.
Adds _project_slug_for to the ChoreographerHelpers protocol (mypy) and a
unit suite covering event selection, the master-bound COMMENT rule, the
no-PR skip, and failure-swallowing. Docs updated (pr-reviewer, task-tools).
---------
Co-authored-by: Renn F <rennf93@users.noreply.github.com>
This commit is contained in:
Renzo F
@@ -22,12 +22,13 @@ from roboco.services.gateway.choreographer._impl import (
|
||||
)
|
||||
from roboco.services.gateway.choreographer.board import BoardMixin
|
||||
from roboco.services.gateway.choreographer.doc import DocMixin
|
||||
from roboco.services.gateway.choreographer.pr_gate import PRGateMixin
|
||||
from roboco.services.gateway.choreographer.pr_review import PRReviewerMixin
|
||||
from roboco.services.gateway.choreographer.qa import QAMixin
|
||||
|
||||
|
||||
class Choreographer(
|
||||
BoardMixin, DocMixin, QAMixin, PRReviewerMixin, _LegacyChoreographer
|
||||
BoardMixin, DocMixin, QAMixin, PRReviewerMixin, PRGateMixin, _LegacyChoreographer
|
||||
):
|
||||
"""Composed choreographer.
|
||||
|
||||
|
||||
@@ -4512,14 +4512,25 @@ class Choreographer:
|
||||
async def _submit_up_ownership_guard(
|
||||
self, pm_agent_id: UUID, task_id: UUID, t: Any, notes: str
|
||||
) -> Envelope | None:
|
||||
"""Role + assignment + notes-length guards for submit_up."""
|
||||
"""Role + assignment + notes-length guards shared by submit_up + submit_root.
|
||||
|
||||
Both callers run the spec gate (``can_invoke_intent``) first, which
|
||||
already restricts submit_up→cell_pm and submit_root→main_pm, so this
|
||||
guard must accept either PM role — a hardcoded cell_pm-only check would
|
||||
reject the Main PM's submit_root and deadlock root closure (submit_root
|
||||
and complete then point at each other). The check stays as a
|
||||
defense-in-depth reject of any non-PM actor that reaches here.
|
||||
"""
|
||||
from roboco.config import settings as roboco_settings
|
||||
|
||||
agent = await self.task.agent_for(pm_agent_id)
|
||||
if agent is None or agent.role != "cell_pm":
|
||||
if agent is None or agent.role not in ("cell_pm", "main_pm"):
|
||||
return Envelope.not_authorized(
|
||||
message="submit_up is reserved for cell_pm",
|
||||
remediate="main_pm should call complete on root tasks instead",
|
||||
message="submit_up / submit_root are reserved for PM roles",
|
||||
remediate=(
|
||||
"only a cell PM (submit_up) or main PM (submit_root)"
|
||||
" may bubble work up"
|
||||
),
|
||||
context_briefing=await self._briefing_for(pm_agent_id, task_id),
|
||||
)
|
||||
if t.assigned_to != pm_agent_id:
|
||||
@@ -4811,13 +4822,25 @@ class Choreographer:
|
||||
context_briefing=await self._briefing_for(pm_agent_id, task_id),
|
||||
)
|
||||
if str(t.status) != "awaiting_pm_review":
|
||||
# An in_progress cell task must enter the in-path gate first:
|
||||
# submit_up opens the cell→root PR and moves it to
|
||||
# awaiting_pr_review, then the cell reviewer pr_passes it to
|
||||
# awaiting_pm_review where complete merges. Name the verb so the
|
||||
# cell PM isn't left guessing (the parallel of the main PM's
|
||||
# submit_root steer).
|
||||
gate_hint = (
|
||||
"open the cell→root PR and enter review first:"
|
||||
" submit_up(task_id, notes='...'). After the cell reviewer"
|
||||
" pr_passes it, complete merges the cell→root PR."
|
||||
if str(t.status) == "in_progress"
|
||||
else "this task is not ready for completion."
|
||||
)
|
||||
return Envelope.invalid_state(
|
||||
message=(
|
||||
f"task {task_id} is in {t.status}, expected awaiting_pm_review"
|
||||
),
|
||||
remediate=(
|
||||
"this task is not ready for completion."
|
||||
+ await self._own_review_hint(pm_agent_id, task_id)
|
||||
gate_hint + await self._own_review_hint(pm_agent_id, task_id)
|
||||
),
|
||||
context_briefing=await self._briefing_for(pm_agent_id, task_id),
|
||||
)
|
||||
@@ -5049,6 +5072,96 @@ class Choreographer:
|
||||
return
|
||||
await self.task.reassign(parent_task_id, pm_agent.id)
|
||||
|
||||
async def submit_root(
|
||||
self, main_pm_agent_id: UUID, task_id: UUID, notes: str
|
||||
) -> Envelope:
|
||||
"""Main PM opens the root→master PR and enters the in-path review gate.
|
||||
|
||||
The root analogue of the cell PM's submit_up: the spec gate (main_pm +
|
||||
in_progress) runs first, then the same preflight guard (ownership, notes
|
||||
length, journal:decision, subtasks-terminal, branch present). Then
|
||||
``VerbRunner.run_intent("submit_root", ...)`` opens the root→master PR
|
||||
(the ``create_root_pr`` pre-side-effect) and transitions in_progress →
|
||||
awaiting_pr_review. The main reviewer then reviews the assembled
|
||||
root→master diff; after pr_pass, ``complete`` escalates to the CEO.
|
||||
"""
|
||||
t = await self.task.get(task_id)
|
||||
briefing = await self._briefing_for(
|
||||
main_pm_agent_id, task_id, task=t, include_ac_coverage=True
|
||||
)
|
||||
if t is None:
|
||||
return await self._emit_rejection(
|
||||
Envelope.not_found(message=f"task {task_id} not found"),
|
||||
agent_id=main_pm_agent_id,
|
||||
task_id=task_id,
|
||||
verb="submit_root",
|
||||
)
|
||||
agent = await self.task.agent_for(main_pm_agent_id)
|
||||
role_str = str(agent.role) if agent is not None else "main_pm"
|
||||
try:
|
||||
role = spec_module.Role(role_str)
|
||||
except ValueError:
|
||||
return await self._emit_rejection(
|
||||
Envelope.not_authorized(
|
||||
message=f"unknown role '{role_str}'",
|
||||
remediate="role is not declared in the lifecycle spec",
|
||||
context_briefing=briefing,
|
||||
).with_introspection(task=t, role=role_str),
|
||||
agent_id=main_pm_agent_id,
|
||||
task_id=task_id,
|
||||
verb="submit_root",
|
||||
)
|
||||
spec_ctx = spec_module.Context(
|
||||
actor_id=main_pm_agent_id,
|
||||
actor_slug=getattr(agent, "slug", None) if agent is not None else None,
|
||||
notes=notes,
|
||||
)
|
||||
decision = spec_module.can_invoke_intent(role, "submit_root", t, spec_ctx)
|
||||
if not decision.allowed:
|
||||
return await self._emit_rejection(
|
||||
Envelope.from_decision(decision, briefing=briefing).with_introspection(
|
||||
task=t, role=role_str
|
||||
),
|
||||
agent_id=main_pm_agent_id,
|
||||
task_id=task_id,
|
||||
verb="submit_root",
|
||||
)
|
||||
# Same preflight as submit_up — ownership + notes-length +
|
||||
# journal:decision + subtasks-terminal + branch-present. submit_root's
|
||||
# tracing requirements mirror submit_up's, so the shared guard applies.
|
||||
guard = await self._submit_up_guard(main_pm_agent_id, task_id, t, notes)
|
||||
if guard is not None:
|
||||
guard.with_introspection(task=t, role=role_str)
|
||||
return await self._emit_rejection(
|
||||
guard,
|
||||
agent_id=main_pm_agent_id,
|
||||
task_id=task_id,
|
||||
verb="submit_root",
|
||||
)
|
||||
runner = self._verb_runner()
|
||||
try:
|
||||
t = await runner.run_intent("submit_root", t, agent, spec_ctx)
|
||||
except Exception as exc:
|
||||
return await self._emit_rejection(
|
||||
Envelope.invalid_state(
|
||||
message=f"verb runner failed: {exc}",
|
||||
remediate=(
|
||||
"ensure all subtasks are terminal + the branch exists,"
|
||||
" then retry submit_root"
|
||||
),
|
||||
context_briefing=briefing,
|
||||
).with_introspection(task=t, role=role_str),
|
||||
agent_id=main_pm_agent_id,
|
||||
task_id=task_id,
|
||||
verb="submit_root",
|
||||
)
|
||||
return Envelope.ok(
|
||||
status=str(t.status),
|
||||
task_id=str(task_id),
|
||||
next=spec_module._INTENT_VERBS["submit_root"].next_hint(t),
|
||||
context_briefing=briefing,
|
||||
).with_introspection(task=t, role=role_str)
|
||||
|
||||
async def _main_pm_complete_guard(
|
||||
self, main_pm_agent_id: UUID, root_task_id: UUID, t: Any, notes: str
|
||||
) -> Envelope | None:
|
||||
@@ -5072,19 +5185,35 @@ class Choreographer:
|
||||
main_pm_agent_id, root_task_id
|
||||
),
|
||||
)
|
||||
# Accept in_progress too. A root resumed from paused (its
|
||||
# subtasks all done) sits in in_progress — there is no submit_up for
|
||||
# roots to move it to awaiting_pm_review. main_pm_complete itself
|
||||
# opens the root→master PR and walks it through awaiting_pm_review
|
||||
# before escalating; the CEO is the root's reviewer.
|
||||
if str(t.status) not in ("awaiting_pm_review", "in_progress"):
|
||||
# A code root must pass the in-path PR-review gate first: submit_root
|
||||
# opens the root→master PR and moves it in_progress → awaiting_pr_review,
|
||||
# then the main reviewer pr_passes it to awaiting_pm_review. So complete
|
||||
# accepts only awaiting_pm_review for a code root. A branchless
|
||||
# coordination root (product fan-out, no repo/PR) skips the gate, so it
|
||||
# may still be walked from in_progress here.
|
||||
root_is_branchless = not bool(t.branch_name)
|
||||
allowed_statuses = (
|
||||
("awaiting_pm_review", "in_progress")
|
||||
if root_is_branchless
|
||||
else ("awaiting_pm_review",)
|
||||
)
|
||||
if str(t.status) not in allowed_statuses:
|
||||
gate_hint = (
|
||||
"this task is not ready for main-PM completion."
|
||||
if root_is_branchless
|
||||
else (
|
||||
"open the root→master PR and enter review first:"
|
||||
" submit_root(task_id, notes='...'). After the main reviewer"
|
||||
" pr_passes it, complete escalates to the CEO."
|
||||
)
|
||||
)
|
||||
return Envelope.invalid_state(
|
||||
message=(
|
||||
f"task {root_task_id} is in {t.status}, expected"
|
||||
" awaiting_pm_review or in_progress"
|
||||
f" {' or '.join(allowed_statuses)}"
|
||||
),
|
||||
remediate=(
|
||||
"this task is not ready for main-PM completion."
|
||||
gate_hint
|
||||
+ await self._own_review_hint(main_pm_agent_id, root_task_id)
|
||||
),
|
||||
context_briefing=await self._briefing_for(
|
||||
@@ -5144,44 +5273,36 @@ class Choreographer:
|
||||
verb="main_pm_complete",
|
||||
)
|
||||
|
||||
needs_pr = t.pr_number is None
|
||||
if not needs_pr:
|
||||
current_target = await self.git.pr_target(t.pr_number)
|
||||
needs_pr = current_target != "master"
|
||||
if needs_pr:
|
||||
await self.git.create_pr(t.branch_name, parent="master", is_root_pr=True)
|
||||
|
||||
# escalate_to_ceo requires source=awaiting_pm_review, but a root
|
||||
# resumed from paused is in_progress and nothing else moves it there
|
||||
# (submit_up is cell-PM-only). The root→master PR now exists, so walk
|
||||
# the root through awaiting_pm_review here. Uses the TaskService
|
||||
# transition directly (no gateway team-match) — submit_pm_review's
|
||||
# gates (in_progress + branch + pr_created + subtasks terminal) all
|
||||
# hold at this point.
|
||||
refreshed = await self.task.get(root_task_id)
|
||||
if refreshed is not None and str(refreshed.status) == "in_progress":
|
||||
advanced = await self.task.submit_pm_review(
|
||||
main_pm_agent_id, root_task_id, notes
|
||||
)
|
||||
if advanced is None:
|
||||
return await self._emit_rejection(
|
||||
Envelope.invalid_state(
|
||||
message=(
|
||||
"could not move root to awaiting_pm_review for CEO"
|
||||
" escalation"
|
||||
),
|
||||
remediate=(
|
||||
"ensure the root→master PR is open and all subtasks"
|
||||
" are terminal, then retry complete"
|
||||
),
|
||||
context_briefing=await self._briefing_for(
|
||||
main_pm_agent_id, root_task_id
|
||||
),
|
||||
).with_introspection(task=refreshed, role="main_pm"),
|
||||
agent_id=main_pm_agent_id,
|
||||
task_id=root_task_id,
|
||||
verb="main_pm_complete",
|
||||
# A branchless coordination root (product fan-out, no repo/PR) skips the
|
||||
# in-path gate: it never went through submit_root / pr_pass, so walk it
|
||||
# in_progress → awaiting_pm_review here, exactly as before. A code root
|
||||
# reaches this point already in awaiting_pm_review — submit_root opened
|
||||
# the root→master PR and the main reviewer pr_passed it — so no PR
|
||||
# creation or status walk is needed.
|
||||
if not bool(t.branch_name):
|
||||
refreshed = await self.task.get(root_task_id)
|
||||
if refreshed is not None and str(refreshed.status) == "in_progress":
|
||||
advanced = await self.task.submit_pm_review(
|
||||
main_pm_agent_id, root_task_id, notes
|
||||
)
|
||||
if advanced is None:
|
||||
return await self._emit_rejection(
|
||||
Envelope.invalid_state(
|
||||
message=(
|
||||
"could not move coordination root to"
|
||||
" awaiting_pm_review for CEO escalation"
|
||||
),
|
||||
remediate=(
|
||||
"ensure all subtasks are terminal, then retry complete"
|
||||
),
|
||||
context_briefing=await self._briefing_for(
|
||||
main_pm_agent_id, root_task_id
|
||||
),
|
||||
).with_introspection(task=refreshed, role="main_pm"),
|
||||
agent_id=main_pm_agent_id,
|
||||
task_id=root_task_id,
|
||||
verb="main_pm_complete",
|
||||
)
|
||||
|
||||
# Use kwargs — service signature is (task_id, agent_role="cell_pm",
|
||||
# notes=None). Positional was passing agent_id as task_id and the
|
||||
|
||||
@@ -63,6 +63,9 @@ class ChoreographerHelpers:
|
||||
) -> dict[str, Any]:
|
||||
raise NotImplementedError
|
||||
|
||||
async def _project_slug_for(self, t: Any) -> str | None:
|
||||
raise NotImplementedError
|
||||
|
||||
@staticmethod
|
||||
def _with_briefing(
|
||||
env: Envelope,
|
||||
|
||||
@@ -123,6 +123,21 @@ class VerbRunner:
|
||||
agent.id, task.id, ctx.notes or ""
|
||||
)
|
||||
|
||||
async def _do_submit_for_review(
|
||||
self, task: Any, agent: Any, ctx: spec.Context
|
||||
) -> Any:
|
||||
return await self.task_service.submit_for_review(
|
||||
agent.id, task.id, ctx.notes or ""
|
||||
)
|
||||
|
||||
async def _do_pr_pass(self, task: Any, agent: Any, ctx: spec.Context) -> Any:
|
||||
return await self.task_service.pr_pass(agent.id, task.id, ctx.notes or "")
|
||||
|
||||
async def _do_pr_fail(self, task: Any, agent: Any, ctx: spec.Context) -> Any:
|
||||
return await self.task_service.pr_fail(
|
||||
agent.id, task.id, ctx.notes or "", list(ctx.issues)
|
||||
)
|
||||
|
||||
async def _do_escalate_to_ceo(
|
||||
self, task: Any, agent: Any, ctx: spec.Context
|
||||
) -> Any:
|
||||
@@ -170,6 +185,9 @@ class VerbRunner:
|
||||
"docs_complete": cls._do_docs_complete,
|
||||
"complete": cls._do_complete,
|
||||
"submit_pm_review": cls._do_submit_pm_review,
|
||||
"submit_for_review": cls._do_submit_for_review,
|
||||
"pr_pass": cls._do_pr_pass,
|
||||
"pr_fail": cls._do_pr_fail,
|
||||
"escalate_to_ceo": cls._do_escalate_to_ceo,
|
||||
"block": cls._do_block,
|
||||
"unblock": cls._do_unblock,
|
||||
@@ -191,6 +209,13 @@ class VerbRunner:
|
||||
task.branch_name, parent=parent, is_root_pr=False
|
||||
)
|
||||
|
||||
async def _do_create_root_pr(self, task: Any, _agent: Any) -> Any:
|
||||
# Root→master PR for the in-path gate's root level (submit_root). The
|
||||
# base is always master and is_root_pr marks it for the CEO-merge path.
|
||||
return await self.git_service.create_pr(
|
||||
task.branch_name, parent="master", is_root_pr=True
|
||||
)
|
||||
|
||||
async def _do_pr_merge(self, task: Any, agent: Any) -> Any:
|
||||
from roboco.services.gateway.merge_chain import resolve_parent_branch
|
||||
|
||||
@@ -204,5 +229,6 @@ class VerbRunner:
|
||||
return {
|
||||
"push_branch": cls._do_push_branch,
|
||||
"create_pr": cls._do_create_pr,
|
||||
"create_root_pr": cls._do_create_root_pr,
|
||||
"pr_merge": cls._do_pr_merge,
|
||||
}
|
||||
|
||||
@@ -0,0 +1,362 @@
|
||||
"""In-path PR-review gate verbs (assembled cell→root + root→master PRs).
|
||||
|
||||
Mixin for ``claim_gate_review``, ``pr_pass`` and ``pr_fail`` — the reviewer
|
||||
side of the in-path gate. Distinct from ``PRReviewerMixin`` (inbound
|
||||
external/fork PRs): these GATE an internal delivery task between the PM's
|
||||
submit (submit_up / submit_root) and the PM's merge. The reviewer reads the
|
||||
assembled diff and either passes it on to awaiting_pm_review or fails it back to
|
||||
needs_revision — exactly like QA's pass_review / fail_review, but at the
|
||||
assembled-PR level.
|
||||
|
||||
Inherits ``ChoreographerHelpers`` under ``TYPE_CHECKING`` only so mypy resolves
|
||||
``self.task`` etc.; at runtime the composed ``Choreographer`` supplies the real
|
||||
attributes via MRO (same pattern as ``QAMixin`` / ``PRReviewerMixin``).
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from types import SimpleNamespace
|
||||
from typing import TYPE_CHECKING, Any
|
||||
|
||||
import structlog
|
||||
|
||||
from roboco.foundation.policy import lifecycle as spec_module
|
||||
from roboco.foundation.policy import tracing as _tr
|
||||
from roboco.services.gateway.envelope import Envelope
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from uuid import UUID
|
||||
|
||||
from roboco.services.gateway.choreographer._protocol import ChoreographerHelpers
|
||||
|
||||
_Base = ChoreographerHelpers
|
||||
else:
|
||||
_Base = object
|
||||
|
||||
logger = structlog.get_logger()
|
||||
|
||||
|
||||
class PRGateMixin(_Base):
|
||||
"""In-path PR-review-gate verbs (assembled cell→root + root→master PRs)."""
|
||||
|
||||
async def claim_gate_review(
|
||||
self, reviewer_agent_id: UUID, task_id: UUID
|
||||
) -> Envelope:
|
||||
"""Reviewer claims an awaiting_pr_review task without transitioning it.
|
||||
|
||||
Mirrors QA's claim_review: the spec gate enforces role (pr_reviewer) +
|
||||
the claim source-status (AWAITING_PR_REVIEW); the verb body then claims
|
||||
without transition (status stays awaiting_pr_review) so the downstream
|
||||
pr_pass / pr_fail source-status still matches. The assembled PR's diff is
|
||||
returned inline (read-only) so the reviewer inspects it before deciding.
|
||||
"""
|
||||
t = await self.task.get(task_id)
|
||||
if t is None:
|
||||
return await self._emit_rejection(
|
||||
Envelope.not_found(message=f"task {task_id} not found"),
|
||||
agent_id=reviewer_agent_id,
|
||||
task_id=task_id,
|
||||
verb="claim_gate_review",
|
||||
)
|
||||
agent = await self.task.agent_for(reviewer_agent_id)
|
||||
role_str = str(agent.role) if agent is not None else "pr_reviewer"
|
||||
briefing = await self._briefing_for(reviewer_agent_id, task_id)
|
||||
role = await self._gate_role_or_rejection(
|
||||
t, role_str, briefing, reviewer_agent_id, task_id, "claim_gate_review"
|
||||
)
|
||||
if isinstance(role, Envelope):
|
||||
return role
|
||||
spec_ctx = spec_module.Context(
|
||||
actor_id=reviewer_agent_id,
|
||||
actor_slug=getattr(agent, "slug", None) if agent is not None else None,
|
||||
)
|
||||
decision = spec_module.can_invoke_intent(role, "claim_gate_review", t, spec_ctx)
|
||||
if not decision.allowed:
|
||||
return await self._emit_rejection(
|
||||
Envelope.from_decision(decision, briefing=briefing).with_introspection(
|
||||
task=t, role=role_str
|
||||
),
|
||||
agent_id=reviewer_agent_id,
|
||||
task_id=task_id,
|
||||
verb="claim_gate_review",
|
||||
)
|
||||
guard = await self._run_claim_guards(agent_id=reviewer_agent_id, task=t)
|
||||
if guard:
|
||||
guard.with_introspection(task=t, role=role_str)
|
||||
return await self._emit_rejection(
|
||||
self._with_briefing(guard, briefing),
|
||||
agent_id=reviewer_agent_id,
|
||||
task_id=task_id,
|
||||
verb="claim_gate_review",
|
||||
)
|
||||
claimed = await self.task.pr_gate_claim(reviewer_agent_id, task_id)
|
||||
if claimed is None:
|
||||
return await self._emit_rejection(
|
||||
Envelope.invalid_state(
|
||||
message="this assembled-PR review task is no longer claimable",
|
||||
remediate="it may already be claimed; give_me_work for the next",
|
||||
context_briefing=briefing,
|
||||
).with_introspection(task=t, role=role_str),
|
||||
agent_id=reviewer_agent_id,
|
||||
task_id=task_id,
|
||||
verb="claim_gate_review",
|
||||
)
|
||||
t = claimed
|
||||
evidence = await self._build_gate_review_evidence(t)
|
||||
return Envelope.ok(
|
||||
status=str(t.status),
|
||||
task_id=str(task_id),
|
||||
next=spec_module._INTENT_VERBS["claim_gate_review"].next_hint(t),
|
||||
evidence=evidence,
|
||||
context_briefing=briefing,
|
||||
).with_introspection(task=t, role=role_str)
|
||||
|
||||
async def pr_pass(
|
||||
self, reviewer_agent_id: UUID, task_id: UUID, notes: str
|
||||
) -> Envelope:
|
||||
"""Pass the assembled PR; awaiting_pr_review → awaiting_pm_review."""
|
||||
return await self._gate_decision(
|
||||
reviewer_agent_id, task_id, "pr_pass", notes=notes, issues=()
|
||||
)
|
||||
|
||||
async def pr_fail(
|
||||
self, reviewer_agent_id: UUID, task_id: UUID, issues: list[str]
|
||||
) -> Envelope:
|
||||
"""Fail the assembled PR with concrete issues; → needs_revision."""
|
||||
if not issues:
|
||||
t = await self.task.get(task_id)
|
||||
return await self._emit_rejection(
|
||||
Envelope.invalid_state(
|
||||
message="pr_fail requires at least one issue",
|
||||
remediate="pass issues=['<concrete actionable issue>', ...]",
|
||||
context_briefing=await self._briefing_for(
|
||||
reviewer_agent_id, task_id
|
||||
),
|
||||
).with_introspection(task=t, role="pr_reviewer"),
|
||||
agent_id=reviewer_agent_id,
|
||||
task_id=task_id,
|
||||
verb="pr_fail",
|
||||
)
|
||||
notes = "Issues:\n" + "\n".join(f"- {issue}" for issue in issues)
|
||||
return await self._gate_decision(
|
||||
reviewer_agent_id, task_id, "pr_fail", notes=notes, issues=tuple(issues)
|
||||
)
|
||||
|
||||
# -- helpers ----------------------------------------------------------
|
||||
|
||||
async def _gate_preflight(
|
||||
self,
|
||||
reviewer_agent_id: UUID,
|
||||
task_id: UUID,
|
||||
verb: str,
|
||||
*,
|
||||
notes: str,
|
||||
issues: tuple[str, ...],
|
||||
) -> Any:
|
||||
"""Ownership + role + spec gate for pr_pass / pr_fail.
|
||||
|
||||
Returns a rejection ``Envelope`` or the
|
||||
``(t, agent, role_str, briefing, spec_ctx)`` tuple on pass.
|
||||
"""
|
||||
t = await self.task.get(task_id)
|
||||
if t is None:
|
||||
return await self._emit_rejection(
|
||||
Envelope.not_found(message=f"task {task_id} not found"),
|
||||
agent_id=reviewer_agent_id,
|
||||
task_id=task_id,
|
||||
verb=verb,
|
||||
)
|
||||
if t.assigned_to != reviewer_agent_id:
|
||||
return await self._emit_rejection(
|
||||
Envelope.not_authorized(
|
||||
message="not assigned to you",
|
||||
remediate="claim it via claim_gate_review(task_id) first",
|
||||
context_briefing=await self._briefing_for(
|
||||
reviewer_agent_id, task_id
|
||||
),
|
||||
).with_introspection(task=t, role="pr_reviewer"),
|
||||
agent_id=reviewer_agent_id,
|
||||
task_id=task_id,
|
||||
verb=verb,
|
||||
)
|
||||
agent = await self.task.agent_for(reviewer_agent_id)
|
||||
role_str = str(agent.role) if agent is not None else "pr_reviewer"
|
||||
briefing = await self._briefing_for(reviewer_agent_id, task_id)
|
||||
role = await self._gate_role_or_rejection(
|
||||
t, role_str, briefing, reviewer_agent_id, task_id, verb
|
||||
)
|
||||
if isinstance(role, Envelope):
|
||||
return role
|
||||
spec_ctx = spec_module.Context(
|
||||
actor_id=reviewer_agent_id,
|
||||
actor_slug=getattr(agent, "slug", None) if agent is not None else None,
|
||||
notes=notes,
|
||||
issues=issues,
|
||||
)
|
||||
decision = spec_module.can_invoke_intent(role, verb, t, spec_ctx)
|
||||
if not decision.allowed:
|
||||
return await self._emit_rejection(
|
||||
Envelope.from_decision(decision, briefing=briefing).with_introspection(
|
||||
task=t, role=role_str
|
||||
),
|
||||
agent_id=reviewer_agent_id,
|
||||
task_id=task_id,
|
||||
verb=verb,
|
||||
)
|
||||
return (t, agent, role_str, briefing, spec_ctx)
|
||||
|
||||
async def _gate_decision(
|
||||
self,
|
||||
reviewer_agent_id: UUID,
|
||||
task_id: UUID,
|
||||
verb: str,
|
||||
*,
|
||||
notes: str,
|
||||
issues: tuple[str, ...],
|
||||
) -> Envelope:
|
||||
"""Shared body for pr_pass / pr_fail: preflight + tracing + run."""
|
||||
pre = await self._gate_preflight(
|
||||
reviewer_agent_id, task_id, verb, notes=notes, issues=issues
|
||||
)
|
||||
if isinstance(pre, Envelope):
|
||||
return pre
|
||||
t, agent, role_str, briefing, spec_ctx = pre
|
||||
gate = await self._gate_tracing(reviewer_agent_id, task_id, t, role_str, verb)
|
||||
if gate is not None:
|
||||
return gate
|
||||
runner = self._verb_runner()
|
||||
try:
|
||||
t = await runner.run_intent(verb, t, agent, spec_ctx)
|
||||
except Exception as exc:
|
||||
return await self._emit_rejection(
|
||||
Envelope.invalid_state(
|
||||
message=f"verb runner failed: {exc}",
|
||||
remediate="retry; if persistent, unclaim and notify the CEO",
|
||||
context_briefing=briefing,
|
||||
).with_introspection(task=t, role=role_str),
|
||||
agent_id=reviewer_agent_id,
|
||||
task_id=task_id,
|
||||
verb=verb,
|
||||
)
|
||||
# Leave the gate verdict on the PR itself so there's a visible trail on
|
||||
# the very PR the PM (or CEO) merges. Best-effort and AFTER the DB
|
||||
# transition — a GitHub failure must not roll back the gate decision.
|
||||
reviewer_slug = getattr(agent, "slug", None) or role_str
|
||||
await self._post_gate_review_to_pr(t, verb, reviewer_slug, notes)
|
||||
return Envelope.ok(
|
||||
status=str(t.status),
|
||||
task_id=str(task_id),
|
||||
next=spec_module._INTENT_VERBS[verb].next_hint(t),
|
||||
context_briefing=briefing,
|
||||
).with_introspection(task=t, role=role_str)
|
||||
|
||||
async def _post_gate_review_to_pr(
|
||||
self, t: Any, verb: str, reviewer_slug: str, notes: str
|
||||
) -> None:
|
||||
"""Post the gate verdict as a review on the assembled PR (best-effort).
|
||||
|
||||
``pr_pass`` posts an APPROVE, ``pr_fail`` a REQUEST_CHANGES — EXCEPT on
|
||||
the root→master PR (a root task has no ``parent_task_id``), which always
|
||||
gets a plain COMMENT: only the CEO acts on master, so the gate never
|
||||
leaves an approval that could satisfy branch protection and let anyone
|
||||
else merge, nor a blocking review that could impede the CEO's merge.
|
||||
For the org's own PRs ``git.post_pr_review`` already downgrades a
|
||||
forbidden self-review to a COMMENT, so the verdict lands regardless.
|
||||
"""
|
||||
slug = await self._project_slug_for(t)
|
||||
pr_number = getattr(t, "pr_number", None)
|
||||
if not slug or not pr_number:
|
||||
return
|
||||
is_root = getattr(t, "parent_task_id", None) is None
|
||||
if verb == "pr_pass":
|
||||
event = "COMMENT" if is_root else "APPROVE"
|
||||
verdict = "PASSED ✅"
|
||||
else:
|
||||
event = "COMMENT" if is_root else "REQUEST_CHANGES"
|
||||
verdict = "CHANGES REQUESTED 🔴"
|
||||
body_lines = [
|
||||
f"## In-path PR-review gate — {verdict}",
|
||||
"",
|
||||
f"Reviewed by **{reviewer_slug}** (RoboCo PR reviewer). Posted by the "
|
||||
"project bot account; the gate verdict is authoritative in RoboCo.",
|
||||
"",
|
||||
(notes or "").strip() or "_(no additional notes)_",
|
||||
]
|
||||
if is_root:
|
||||
body_lines += ["", "_Only the CEO merges this PR into `master`._"]
|
||||
try:
|
||||
await self.git.post_pr_review(
|
||||
slug, int(pr_number), "\n".join(body_lines), event=event
|
||||
)
|
||||
except Exception:
|
||||
logger.exception(
|
||||
"gate review PR post failed", task_id=str(getattr(t, "id", ""))
|
||||
)
|
||||
|
||||
async def _gate_role_or_rejection(
|
||||
self,
|
||||
t: Any,
|
||||
role_str: str,
|
||||
briefing: dict[str, Any],
|
||||
agent_id: UUID,
|
||||
task_id: UUID,
|
||||
verb: str,
|
||||
) -> Any:
|
||||
"""Parse the role enum, or return a not_authorized rejection Envelope."""
|
||||
try:
|
||||
return spec_module.Role(role_str)
|
||||
except ValueError:
|
||||
return await self._emit_rejection(
|
||||
Envelope.not_authorized(
|
||||
message=f"unknown role '{role_str}'",
|
||||
remediate="role is not declared in the lifecycle spec",
|
||||
context_briefing=briefing,
|
||||
).with_introspection(task=t, role=role_str),
|
||||
agent_id=agent_id,
|
||||
task_id=task_id,
|
||||
verb=verb,
|
||||
)
|
||||
|
||||
async def _gate_tracing(
|
||||
self,
|
||||
reviewer_agent_id: UUID,
|
||||
task_id: UUID,
|
||||
t: Any,
|
||||
role_str: str,
|
||||
verb: str,
|
||||
) -> Envelope | None:
|
||||
"""pr_pass / pr_fail require a journal:learning entry (parity with QA)."""
|
||||
has_learning = await self.journal.has_learning_for_task(
|
||||
reviewer_agent_id, task_id
|
||||
)
|
||||
ctx = _tr.GateContext(journal_learning_present=has_learning)
|
||||
result = _tr.check_requirements(
|
||||
task=SimpleNamespace(),
|
||||
requirements=list(_tr.requirements_for(verb)),
|
||||
ctx=ctx,
|
||||
)
|
||||
if result.passed:
|
||||
return None
|
||||
return await self._emit_rejection(
|
||||
(
|
||||
await self._build_tracing_gap(
|
||||
reviewer_agent_id, task_id, result.missing
|
||||
)
|
||||
).with_introspection(task=t, role=role_str),
|
||||
agent_id=reviewer_agent_id,
|
||||
task_id=task_id,
|
||||
verb=verb,
|
||||
)
|
||||
|
||||
async def _build_gate_review_evidence(self, t: Any) -> dict[str, Any]:
|
||||
"""Inline evidence for claim_gate_review: the assembled diff + criteria."""
|
||||
diff = ""
|
||||
if t.branch_name:
|
||||
diff = await self.git.diff(branch_name=t.branch_name)
|
||||
return {
|
||||
"pr_number": t.pr_number,
|
||||
"pr_url": t.pr_url,
|
||||
"pr_diff": diff,
|
||||
"acceptance_criteria": list(getattr(t, "acceptance_criteria", None) or []),
|
||||
"is_assembled_pr": True,
|
||||
}
|
||||
Reference in New Issue
Block a user