From 5fe1e6df58471a9bc1bdfcaa7baafd6a1f4e5193 Mon Sep 17 00:00:00 2001 From: Renzo F <45401804+rennf93@users.noreply.github.com> Date: Sat, 20 Jun 2026 09:27:29 +0200 Subject: [PATCH] =?UTF-8?q?feat:=20in-path=20PR-review=20gate=20=E2=80=94?= =?UTF-8?q?=20per-cell=20+=20main=20reviewers=20(#229)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 --- CLAUDE.md | 20 +- README.md | 24 +- .../prompts/_generated/lifecycle-cell_pm.md | 4 +- .../prompts/_generated/lifecycle-main_pm.md | 3 +- .../_generated/lifecycle-pr_reviewer.md | 3 + agents/prompts/_generated/main_pm.md | 1 + agents/prompts/_generated/pr_reviewer.md | 3 + agents/prompts/_generated/verbs.md | 4 + alembic/versions/040_awaiting_pr_review.py | 37 ++ docs/how-to/README.md | 2 +- docs/rag/architecture/agent-model.md | 8 +- docs/rag/architecture/agent-uuids.md | 3 + docs/rag/architecture/org-structure.md | 21 +- docs/rag/architecture/permissions.md | 2 +- docs/rag/architecture/tool-permissions.md | 6 +- docs/rag/lifecycle/intent-verbs.md | 44 ++- docs/rag/lifecycle/status-transitions.md | 5 + docs/rag/roles/cell-pm.md | 30 +- docs/rag/roles/main-pm.md | 13 +- docs/rag/roles/pr-reviewer.md | 11 +- docs/rag/tools/git-tools.md | 4 +- docs/rag/tools/task-tools.md | 23 +- docs/rag/workflows/git-commits.md | 2 +- docs/rag/workflows/pr-creation.md | 4 +- panel/lib/lifecycle.json | 95 ++++- roboco/api/routes/v1/flow_main_pm.py | 12 + roboco/api/routes/v1/flow_pr_reviewer.py | 36 ++ roboco/api/schemas/v1/flow.py | 19 + roboco/config.py | 18 + roboco/foundation/_validate_lifecycle.py | 21 +- roboco/foundation/identity.py | 22 ++ roboco/foundation/policy/lifecycle.py | 177 ++++++++- roboco/foundation/policy/tracing.py | 16 + roboco/mcp/flow_server.py | 25 ++ roboco/models/a2a.py | 1 + roboco/models/base.py | 1 + roboco/runtime/orchestrator.py | 134 ++++++- roboco/seeds/initial_data.py | 3 + .../gateway/choreographer/__init__.py | 3 +- .../services/gateway/choreographer/_impl.py | 223 ++++++++--- .../gateway/choreographer/_protocol.py | 3 + .../gateway/choreographer/_verb_runner.py | 26 ++ .../services/gateway/choreographer/pr_gate.py | 362 ++++++++++++++++++ roboco/services/product.py | 9 +- roboco/services/task.py | 137 +++++++ tests/foundation/test_identity.py | 7 +- tests/foundation/test_lifecycle_spec.py | 1 + tests/foundation/test_pr_review_gate.py | 111 ++++++ tests/integration/test_lifecycle_real_db.py | 95 +++++ tests/integration/test_product_service.py | 44 +++ tests/unit/gateway/test_choreographer_pm.py | 143 ++++++- .../gateway/test_choreographer_pm_extras.py | 17 +- .../unit/gateway/test_pr_gate_posts_review.py | 105 +++++ tests/unit/gateway/test_verb_runner.py | 24 +- tests/unit/runtime/test_idle_vs_respawn.py | 76 +++- tests/unit/test_enum_migration_parity.py | 24 ++ 56 files changed, 2061 insertions(+), 206 deletions(-) create mode 100644 alembic/versions/040_awaiting_pr_review.py create mode 100644 roboco/services/gateway/choreographer/pr_gate.py create mode 100644 tests/foundation/test_pr_review_gate.py create mode 100644 tests/unit/gateway/test_pr_gate_posts_review.py diff --git a/CLAUDE.md b/CLAUDE.md index 668c431c..9fcadd55 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -10,7 +10,7 @@ Contributions require a signed **Contributor License Agreement** (`CLA.md`), aut ## Project Overview -**RoboCo** is an AI Agentic Company - a virtual organization of 22 AI agents + 1 human CEO, designed to operate as a complete software development workforce. The system implements a structured organizational hierarchy with formal communication protocols, task management, and quality controls. +**RoboCo** is an AI Agentic Company - a virtual organization of 25 AI agents + 1 human CEO, designed to operate as a complete software development workforce. The system implements a structured organizational hierarchy with formal communication protocols, task management, and quality controls. ### Core Architecture @@ -19,7 +19,7 @@ CEO (Renzo - Human) | +-- Intake (on-demand interviewer: chats only with the CEO to draft a task) +-- Secretary (on-demand chief-of-staff: reads company state, runs gated CEO directives) - +-- PR Reviewer (read-only: reviews inbound external/fork + internal PRs, posts on the PR itself) + +-- PR Reviewer (read-only: the main reviewer — inbound external/fork + internal PRs, and the root→master in-path gate) | +-- Board (3 agents) +-- Product Owner @@ -28,9 +28,9 @@ CEO (Renzo - Human) | +-- Main PM (coordinates all cells) | - +-- Backend Cell (5 agents: 2 Devs, 1 QA, 1 PM, 1 Documenter) - +-- Frontend Cell (5 agents: 2 Devs, 1 QA, 1 PM, 1 Documenter) - +-- UX/UI Cell (5 agents: 2 Devs, 1 QA, 1 PM, 1 Documenter) + +-- Backend Cell (6 agents: 2 Devs, 1 QA, 1 PM, 1 Documenter, 1 PR Reviewer) + +-- Frontend Cell (6 agents: 2 Devs, 1 QA, 1 PM, 1 Documenter, 1 PR Reviewer) + +-- UX/UI Cell (6 agents: 2 Devs, 1 QA, 1 PM, 1 Documenter, 1 PR Reviewer) ``` ### Hardware Infrastructure @@ -200,6 +200,8 @@ backlog -> pending -> claimed -> in_progress -> [blocked|paused] -> verifying completed completed ``` +**In-path PR-review gate** (`awaiting_pr_review`): each assembled PR is reviewed before the PM merges. The cell PM's `submit_up` opens the cell→root PR and the Main PM's `submit_root` opens the root→master PR; both enter `awaiting_pr_review`, where a reviewer `pr_pass`es it on to `awaiting_pm_review` or `pr_fail`s it back to `needs_revision` — the merge-level reject the PM otherwise lacks. Leaf dev tasks and branchless coordination roots skip the gate. + **States:** | State | Description | |-------|-------------| @@ -213,6 +215,7 @@ backlog -> pending -> claimed -> in_progress -> [blocked|paused] -> verifying | `needs_revision` | QA or CEO requested changes | | `awaiting_qa` | Submitted for QA review — PR must already exist | | `awaiting_documentation` | Documentation phase — PR already open from pre-QA; doc writes docs | +| `awaiting_pr_review` | In-path PR-review gate: a reviewer checks the assembled cell→root / root→master PR before the PM merges (assembled, PR-bearing tasks only) | | `awaiting_pm_review` | Docs complete, PM reviews + merges | | `awaiting_ceo_approval` | Major tasks escalated for CEO final approval | | `completed` | Terminal state - work done and merged | @@ -230,6 +233,9 @@ All status transitions are validated through the enforcement layer. Key restrict | `awaiting_qa` → `awaiting_documentation` (pass) | QA only | | `awaiting_qa` → `needs_revision` (fail) | QA only | | `awaiting_documentation` → `awaiting_pm_review` | Documenter or Developer (parallel completion) | +| `in_progress` → `awaiting_pr_review` (submit_up / submit_root) | PM roles (opens the assembled cell→root / root→master PR) | +| `awaiting_pr_review` → `awaiting_pm_review` (pr_pass) | PR reviewer only | +| `awaiting_pr_review` → `needs_revision` (pr_fail) | PR reviewer only | | `awaiting_pm_review` → `completed` | PM roles only | | `awaiting_pm_review` → `awaiting_ceo_approval` | PM roles only | | `awaiting_ceo_approval` → `completed/needs_revision/cancelled` | CEO only | @@ -329,8 +335,8 @@ Each agent gets a **spawn manifest** at `/app/tool-manifest.json` listing the ve | qa | `give_me_work`, `claim_review`, `pass_review`, `fail_review`, `i_am_blocked`, `resume`, `unclaim` | | documenter | `give_me_work`, `claim_doc_task`, `i_documented`, `i_am_blocked`, `resume`, `unclaim` | | cell_pm | `give_me_work`, `i_will_plan`, `delegate`, `complete`, `submit_up`, `triage`, `unblock`, `escalate_up`, `reassign`, `resume`, `unclaim` | -| main_pm | `give_me_work`, `i_will_plan`, `delegate`, `complete`, `triage`, `triage_all`, `unblock`, `escalate_up`, `escalate_to_ceo`, `resume`, `unclaim` | -| pr_reviewer | `give_me_work`, `claim_pr_review`, `post_pr_review` (read-only reviewer; inbound external/fork PRs first) | +| main_pm | `give_me_work`, `i_will_plan`, `delegate`, `complete`, `submit_root`, `triage`, `triage_all`, `unblock`, `escalate_up`, `escalate_to_ceo`, `resume`, `unclaim` | +| pr_reviewer | `give_me_work`, `claim_pr_review`, `post_pr_review` (inbound external/fork PRs), `claim_gate_review`, `pr_pass`, `pr_fail` (in-path assembled-PR gate) | | product_owner | `triage`, `escalate_to_ceo` | | head_marketing| `triage`, `escalate_to_ceo` | | auditor | `triage` (read-only — no `say`/`dm`) | diff --git a/README.md b/README.md index a58078f1..5191aec9 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ # RoboCo -AI Agents Company - A virtual organization of 22 AI agents + 1 human CEO, designed to operate as a complete software development workforce. +AI Agents Company - A virtual organization of 25 AI agents + 1 human CEO, designed to operate as a complete software development workforce. @@ -39,7 +39,7 @@ CEO (You, the human) │ ├── Intake (on-demand interviewer: chats only with you to draft a task) ├── Secretary (on-demand chief-of-staff: reads company state, runs gated directives) - ├── PR Reviewer (read-only: reviews inbound external/fork + internal PRs) + ├── PR Reviewer (read-only main reviewer: inbound external/fork + internal PRs, and the root→master in-path gate) │ └── Board (3 agents) ├── Product Owner @@ -48,12 +48,12 @@ CEO (You, the human) │ └── Main PM (coordinates all cells) │ - ├── Backend Cell (5 agents: 2 Devs, 1 QA, 1 PM, 1 Documenter) - ├── Frontend Cell (5 agents: 2 Devs, 1 QA, 1 PM, 1 Documenter) - └── UX/UI Cell (5 agents: 2 Devs, 1 QA, 1 PM, 1 Documenter) + ├── Backend Cell (6 agents: 2 Devs, 1 QA, 1 PM, 1 Documenter, 1 PR Reviewer) + ├── Frontend Cell (6 agents: 2 Devs, 1 QA, 1 PM, 1 Documenter, 1 PR Reviewer) + └── UX/UI Cell (6 agents: 2 Devs, 1 QA, 1 PM, 1 Documenter, 1 PR Reviewer) ``` -The 22 agents = Intake + Secretary + PR Reviewer + the Board (3) + Main PM + the three 5-agent cells (15). Agents run on Anthropic Claude by default, or on xAI Grok (the official `grok` CLI on a SuperGrok subscription) — see the provider note under Configuration. +The 25 agents = Intake + Secretary + PR Reviewer + the Board (3) + Main PM + the three 6-agent cells (18). Agents run on Anthropic Claude by default, or on xAI Grok (the official `grok` CLI on a SuperGrok subscription) — see the provider note under Configuration. ## How it works @@ -208,6 +208,16 @@ cancelled blocked needs_revision awaiting_documentat completed ``` +Assembled, PR-bearing tasks pass through one extra stage — the in-path PR-review gate — before the PM merges: + +``` +in_progress → awaiting_pr_review → awaiting_pm_review + (submit_up / (pr_pass) + submit_root) (pr_fail → needs_revision) +``` + +The cell PM's `submit_up` (cell→root PR) and the Main PM's `submit_root` (root→master PR) open the assembled PR and enter the gate; a PR reviewer `pr_pass`es it on to the PM merge or `pr_fail`s it back. Leaf dev tasks (reviewed by QA) and branchless coordination roots skip the gate. + ## API Endpoints Domain routes are mounted under `/api`: @@ -276,7 +286,7 @@ uv run mypy roboco/ - [x] Database ORM (SQLAlchemy async) - [x] Task lifecycle state machine - [x] Multi-agent workspace management -- [x] Agent prompts (22 agents) +- [x] Agent prompts (25 agents) - [x] Messaging API - [x] Task API with full lifecycle - [x] Git operations API diff --git a/agents/prompts/_generated/lifecycle-cell_pm.md b/agents/prompts/_generated/lifecycle-cell_pm.md index e4e8d7e4..eb11e39c 100644 --- a/agents/prompts/_generated/lifecycle-cell_pm.md +++ b/agents/prompts/_generated/lifecycle-cell_pm.md @@ -3,7 +3,7 @@ These are the only verbs the gateway will accept from you. Calling any other verb will be rejected with a Decision telling you the right one. -- **complete**: Cell PM merges leaf PR + transitions to completed; Main PM merges root PR + escalates to CEO. +- **complete**: Cell PM merges the PR (leaf into the cell branch, or the gated cell→root PR into the root branch) + transitions to completed; Main PM escalates the root to the CEO (who merges root→master). - **delegate**: 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/research, UX devs also design). documentation is NOT delegatable — the lifecycle auto-creates the doc phase after the code subtask passes QA. - **escalate_up**: Escalate to your role's escalation_target. - **give_me_work**: Return your most-actionable task or signal idle. @@ -11,7 +11,7 @@ other verb will be rejected with a Decision telling you the right one. - **i_will_plan**: PM mirror of i_will_work_on for parent tasks. Claim, plan, transition to in_progress; from there delegate subtasks. - **reassign**: Hand a claimed/in_progress task to another developer in your own cell. The branch is keyed to the task (not the agent), so it is preserved — the new developer continues the work-in-progress. No status change. - **resume**: Resume a paused task you own. paused -> in_progress. -- **submit_up**: Cell PM opens the cell→root PR and moves the cell task to awaiting_pm_review. The same Cell PM then completes it. +- **submit_up**: Cell PM opens the cell→root PR and moves the cell task into the PR-review gate (awaiting_pr_review). The cell reviewer reviews the assembled diff; after pr_pass the same Cell PM completes it. - **triage**: List actionable tasks in your scope. - **unblock**: PM unblocks a blocked task; restores pre-block state. - **unclaim**: Voluntarily release a claim back to pending. The work-in-progress branch is preserved. diff --git a/agents/prompts/_generated/lifecycle-main_pm.md b/agents/prompts/_generated/lifecycle-main_pm.md index 35c98f74..9d54960d 100644 --- a/agents/prompts/_generated/lifecycle-main_pm.md +++ b/agents/prompts/_generated/lifecycle-main_pm.md @@ -3,7 +3,7 @@ These are the only verbs the gateway will accept from you. Calling any other verb will be rejected with a Decision telling you the right one. -- **complete**: Cell PM merges leaf PR + transitions to completed; Main PM merges root PR + escalates to CEO. +- **complete**: Cell PM merges the PR (leaf into the cell branch, or the gated cell→root PR into the root branch) + transitions to completed; Main PM escalates the root to the CEO (who merges root→master). - **delegate**: 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/research, UX devs also design). documentation is NOT delegatable — the lifecycle auto-creates the doc phase after the code subtask passes QA. - **escalate_to_ceo**: Escalate to CEO with reason. Transitions to awaiting_ceo_approval. - **escalate_up**: Escalate to your role's escalation_target. @@ -11,6 +11,7 @@ other verb will be rejected with a Decision telling you the right one. - **i_am_idle**: Signal you have no active work. PMs auto-pause owned in_progress tasks. - **i_will_plan**: PM mirror of i_will_work_on for parent tasks. Claim, plan, transition to in_progress; from there delegate subtasks. - **resume**: Resume a paused task you own. paused -> in_progress. +- **submit_root**: Main PM opens the root→master PR and moves the root task to awaiting_pr_review for the main reviewer (the root analogue of the cell PM's submit_up). After pr_pass, call complete to escalate to the CEO. Only for code roots; branchless coordination roots skip the gate and complete directly. - **triage**: List actionable tasks in your scope. - **triage_all**: List actionable tasks across all teams (Main PM only). - **unblock**: PM unblocks a blocked task; restores pre-block state. diff --git a/agents/prompts/_generated/lifecycle-pr_reviewer.md b/agents/prompts/_generated/lifecycle-pr_reviewer.md index 18f425f0..3f23e8f6 100644 --- a/agents/prompts/_generated/lifecycle-pr_reviewer.md +++ b/agents/prompts/_generated/lifecycle-pr_reviewer.md @@ -3,7 +3,10 @@ These are the only verbs the gateway will accept from you. Calling any other verb will be rejected with a Decision telling you the right one. +- **claim_gate_review**: Claim an assembled-PR review task (awaiting_pr_review) WITHOUT transitioning it — mirrors QA's claim_review. The assembled diff and the parent task's acceptance criteria are returned inline. - **claim_pr_review**: Claim an inbound external-PR review task and start work. pending -> claimed -> in_progress. - **give_me_work**: Return your most-actionable task or signal idle. - **i_am_idle**: Signal you have no active work. PMs auto-pause owned in_progress tasks. - **post_pr_review**: Post one complete change-request to the external PR and finish the review task. in_progress -> completed. +- **pr_fail**: Fail the assembled-PR review with concrete issues. Transitions awaiting_pr_review -> needs_revision, routed back like a QA fail. +- **pr_pass**: Pass the assembled-PR review. Transitions awaiting_pr_review -> awaiting_pm_review so the PM can merge. diff --git a/agents/prompts/_generated/main_pm.md b/agents/prompts/_generated/main_pm.md index 77e7434e..0cbdbb1c 100644 --- a/agents/prompts/_generated/main_pm.md +++ b/agents/prompts/_generated/main_pm.md @@ -15,6 +15,7 @@ | `i_am_idle` | `i_am_idle()` | | `i_will_plan` | `i_will_plan(task_id: UUID, plan: str, approach: str, sub_tasks: list[str | str] = PydanticUndefined, technical_considerations: list[str] = PydanticUndefined, risks: list[str | str] = PydanticUndefined, open_questions: list[str | str | bool] = PydanticUndefined)` | | `resume` | `resume(task_id: UUID)` | +| `submit_root` | `submit_root(task_id: UUID, notes: str)` | | `triage` | `triage()` | | `triage_all` | `triage_all()` | | `unblock` | `unblock(task_id: UUID, restore: bool = True)` | diff --git a/agents/prompts/_generated/pr_reviewer.md b/agents/prompts/_generated/pr_reviewer.md index 039db68c..cfdc6810 100644 --- a/agents/prompts/_generated/pr_reviewer.md +++ b/agents/prompts/_generated/pr_reviewer.md @@ -7,10 +7,13 @@ | Verb | Body schema | |------|-------------| +| `claim_gate_review` | `claim_gate_review(task_id: UUID)` | | `claim_pr_review` | `claim_pr_review(task_id: UUID)` | | `give_me_work` | `give_me_work()` | | `i_am_idle` | `i_am_idle()` | | `post_pr_review` | `post_pr_review(task_id: UUID, body: str, event: str = 'REQUEST_CHANGES')` | +| `pr_fail` | `pr_fail(task_id: UUID, issues: list[str])` | +| `pr_pass` | `pr_pass(task_id: UUID, notes: str)` | ### Content (do) tools diff --git a/agents/prompts/_generated/verbs.md b/agents/prompts/_generated/verbs.md index 1a61a6e8..259921fc 100644 --- a/agents/prompts/_generated/verbs.md +++ b/agents/prompts/_generated/verbs.md @@ -153,6 +153,7 @@ real tools live in their agent_sdk drivers, not role_config. | `i_am_idle` | `i_am_idle()` | | `i_will_plan` | `i_will_plan(task_id: UUID, plan: str, approach: str, sub_tasks: list[str | str] = PydanticUndefined, technical_considerations: list[str] = PydanticUndefined, risks: list[str | str] = PydanticUndefined, open_questions: list[str | str | bool] = PydanticUndefined)` | | `resume` | `resume(task_id: UUID)` | +| `submit_root` | `submit_root(task_id: UUID, notes: str)` | | `triage` | `triage()` | | `triage_all` | `triage_all()` | | `unblock` | `unblock(task_id: UUID, restore: bool = True)` | @@ -255,10 +256,13 @@ real tools live in their agent_sdk drivers, not role_config. | Verb | Body schema | |------|-------------| +| `claim_gate_review` | `claim_gate_review(task_id: UUID)` | | `claim_pr_review` | `claim_pr_review(task_id: UUID)` | | `give_me_work` | `give_me_work()` | | `i_am_idle` | `i_am_idle()` | | `post_pr_review` | `post_pr_review(task_id: UUID, body: str, event: str = 'REQUEST_CHANGES')` | +| `pr_fail` | `pr_fail(task_id: UUID, issues: list[str])` | +| `pr_pass` | `pr_pass(task_id: UUID, notes: str)` | ### Content (do) tools diff --git a/alembic/versions/040_awaiting_pr_review.py b/alembic/versions/040_awaiting_pr_review.py new file mode 100644 index 00000000..91315759 --- /dev/null +++ b/alembic/versions/040_awaiting_pr_review.py @@ -0,0 +1,37 @@ +"""Add 'awaiting_pr_review' to the postgres taskstatus enum. + +The in-path PR-review gate (``Status.AWAITING_PR_REVIEW`` in +foundation/policy/lifecycle and ``TaskStatus`` in models/base) inserts a +reviewer sign-off between the assembled-PR submit and the PM merge. Persisting +a task in that state requires the postgres ``taskstatus`` enum to carry the +value. Mirrors migration 037's pattern (forward-only enum ADD VALUE). + +Revision ID: 040_awaiting_pr_review +Revises: 039_seed_grok_provider +Create Date: 2026-06-20 + +NOTE: the revision id is kept well under 32 chars — alembic's +``alembic_version.version_num`` column is ``VARCHAR(32)`` and a longer id raises +``value too long for type character varying(32)`` when alembic records it. +""" + +from __future__ import annotations + +from alembic import op + +revision = "040_awaiting_pr_review" +down_revision = "039_seed_grok_provider" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + # Unguarded (renders in offline --sql so the enum-migration-parity test + # sees it) and idempotent. PG 16 permits ADD VALUE inside a transaction. + op.execute("ALTER TYPE taskstatus ADD VALUE IF NOT EXISTS 'awaiting_pr_review'") + + +def downgrade() -> None: + # Postgres does not support removing enum values without a destructive + # type recreation. Forward-only by design (see migrations 034 / 037). + pass diff --git a/docs/how-to/README.md b/docs/how-to/README.md index 3d6ceb42..ca0d4dcb 100644 --- a/docs/how-to/README.md +++ b/docs/how-to/README.md @@ -2,7 +2,7 @@ ![Twelve-second looping preview of the RoboCo control panel — the org tree, a task in progress, and an approval queue.](../videos/panel-teaser.gif) -RoboCo is a virtual software company — 22 AI agents and one human: you. Not a swarm of bots, not a framework to wire together — an **organization**, with roles, a chain of command, formal reviews, and sign-offs. You don't micromanage it; you run it like a CEO. Drop work in at the top and the company carries it all the way through planning, building, review, and documentation, then brings it back to your desk for the final word. You act at the two ends; the organization fills in everything between. +RoboCo is a virtual software company — 25 AI agents and one human: you. Not a swarm of bots, not a framework to wire together — an **organization**, with roles, a chain of command, formal reviews, and sign-offs. You don't micromanage it; you run it like a CEO. Drop work in at the top and the company carries it all the way through planning, building, review, and documentation, then brings it back to your desk for the final word. You act at the two ends; the organization fills in everything between. And the proof is this guide. The screenshots throughout aren't a mock-up: they follow RoboCo building one of its *own* features — the **Prompter**, the task-authoring page now living in this very control panel. RoboCo's agents scoped it, built it across three cells, failed and re-ran its QA, documented it, and opened the real pull request you'll see at the end. RoboCo builds RoboCo — that is the whole proof of concept. diff --git a/docs/rag/architecture/agent-model.md b/docs/rag/architecture/agent-model.md index f382d65a..efb68739 100644 --- a/docs/rag/architecture/agent-model.md +++ b/docs/rag/architecture/agent-model.md @@ -24,7 +24,7 @@ | `developer` | Writes code | | `qa` | Reviews and tests | | `documenter` | Writes documentation | -| `pr_reviewer` | Read-only reviewer of inbound external/fork + internal PRs (agent `pr-reviewer-1`) | +| `pr_reviewer` | Read-only reviewer: inbound external/fork + internal PRs, and the in-path assembled-PR gate (`pr-reviewer-1` main + `be/fe/ux-pr-reviewer` per cell) | | `prompter` | On-demand intake interviewer, human-only (agent `intake-1`) | | `secretary` | On-demand chief-of-staff, human-only (agent `secretary-1`) | | `system` | Internal orchestrator | @@ -33,9 +33,9 @@ | Team | Agents | |------|--------| -| `backend` | be-pm, be-dev-*, be-qa, be-doc | -| `frontend` | fe-pm, fe-dev-*, fe-qa, fe-doc | -| `ux_ui` | ux-pm, ux-dev-*, ux-qa, ux-doc | +| `backend` | be-pm, be-dev-*, be-qa, be-doc, be-pr-reviewer | +| `frontend` | fe-pm, fe-dev-*, fe-qa, fe-doc, fe-pr-reviewer | +| `ux_ui` | ux-pm, ux-dev-*, ux-qa, ux-doc, ux-pr-reviewer | | `main_pm` | main-pm | | `board` | product-owner, head-marketing, auditor | | `marketing` | head-marketing | diff --git a/docs/rag/architecture/agent-uuids.md b/docs/rag/architecture/agent-uuids.md index d9f0e68b..f325c908 100644 --- a/docs/rag/architecture/agent-uuids.md +++ b/docs/rag/architecture/agent-uuids.md @@ -35,6 +35,7 @@ delegate(assigned_to="00000000-0000-0000-0001-000000000001", ...) | `be-qa` | `00000000-0000-0000-0001-000000000003` | | `be-pm` | `00000000-0000-0000-0001-000000000004` | | `be-doc` | `00000000-0000-0000-0001-000000000005` | +| `be-pr-reviewer` | `00000000-0000-0000-0001-000000000006` | ## Frontend Cell (0002) @@ -45,6 +46,7 @@ delegate(assigned_to="00000000-0000-0000-0001-000000000001", ...) | `fe-qa` | `00000000-0000-0000-0002-000000000003` | | `fe-pm` | `00000000-0000-0000-0002-000000000004` | | `fe-doc` | `00000000-0000-0000-0002-000000000005` | +| `fe-pr-reviewer` | `00000000-0000-0000-0002-000000000006` | ## UX/UI Cell (0003) @@ -55,6 +57,7 @@ delegate(assigned_to="00000000-0000-0000-0001-000000000001", ...) | `ux-qa` | `00000000-0000-0000-0003-000000000003` | | `ux-pm` | `00000000-0000-0000-0003-000000000004` | | `ux-doc` | `00000000-0000-0000-0003-000000000005` | +| `ux-pr-reviewer` | `00000000-0000-0000-0003-000000000006` | ## Board/Management (0004) diff --git a/docs/rag/architecture/org-structure.md b/docs/rag/architecture/org-structure.md index af96fcf7..d4269747 100644 --- a/docs/rag/architecture/org-structure.md +++ b/docs/rag/architecture/org-structure.md @@ -22,23 +22,24 @@ CEO (Renzo - Human) | Role | Count | |------|-------| -| CEO | 1 (human — not counted in the 22) | +| CEO | 1 (human — not counted in the 25) | | Product Owner | 1 | | Head of Marketing | 1 | | Auditor | 1 | -| PR Reviewer | 1 | +| Main PR Reviewer | 1 (pr-reviewer-1; external/fork + internal PRs + the root→master gate) | | Main PM | 1 | | Cell PMs | 3 | | Developers | 6 (2 per cell) | | QAs | 3 (1 per cell) | | Documenters | 3 (1 per cell) | +| Cell PR Reviewers | 3 (1 per cell; the in-path cell→root gate) | | Prompter (Intake) | 1 (on-demand) | | Secretary | 1 (on-demand) | -| **Total AI agents** | **22** | +| **Total AI agents** | **25** | ## On-Demand Roles (Human-Facing) -Two of the 22 — the Prompter (Intake) and the Secretary — sit outside the standing delivery org above. They are **human-only** and **spawned on demand** as live chat sessions; they are counted among the 22, but unlike the standing org they have no lifecycle verbs and no outward agent comms: +Two of the 25 — the Prompter (Intake) and the Secretary — sit outside the standing delivery org above. They are **human-only** and **spawned on demand** as live chat sessions; they are counted among the 25, but unlike the standing org they have no lifecycle verbs and no outward agent comms: | Role | Purpose | |------|---------| @@ -49,11 +50,13 @@ Neither has lifecycle verbs or outward agent comms. See [Prompter](../roles/prom ## Cells -| Cell | PM | Developers | QA | Documenter | -|------|-----|------------|-----|------------| -| Backend | be-pm | be-dev-1, be-dev-2 | be-qa | be-doc | -| Frontend | fe-pm | fe-dev-1, fe-dev-2 | fe-qa | fe-doc | -| UX/UI | ux-pm | ux-dev-1, ux-dev-2 | ux-qa | ux-doc | +| Cell | PM | Developers | QA | Documenter | PR Reviewer | +|------|-----|------------|-----|------------|-------------| +| Backend | be-pm | be-dev-1, be-dev-2 | be-qa | be-doc | be-pr-reviewer | +| Frontend | fe-pm | fe-dev-1, fe-dev-2 | fe-qa | fe-doc | fe-pr-reviewer | +| UX/UI | ux-pm | ux-dev-1, ux-dev-2 | ux-qa | ux-doc | ux-pr-reviewer | + +The cell PR reviewer runs the **in-path PR-review gate** on its cell's assembled cell→root PR (`claim_gate_review` → `pr_pass` / `pr_fail`); `pr-reviewer-1` runs the same gate on the root→master PR plus the inbound external/fork + internal PR review. ## Teams diff --git a/docs/rag/architecture/permissions.md b/docs/rag/architecture/permissions.md index e68aa3e8..fdb8a922 100644 --- a/docs/rag/architecture/permissions.md +++ b/docs/rag/architecture/permissions.md @@ -15,7 +15,7 @@ What each role can do in the system. | (read-only reviewer) | pr_reviewer | | (human-only) | prompter, secretary | -`pr_reviewer` is a board-adjacent, read-only role (QA level): it claims and posts inbound-PR reviews (`claim_pr_review` / `post_pr_review`) but creates, assigns, completes, and notifies nothing. `prompter` (intake) and `secretary` are **human-only** — they chat with the CEO and have only `note` + `evidence`, with no task or notification permissions; they don't appear in the action tables below. +`pr_reviewer` is a read-only role (QA level): the main reviewer (`pr-reviewer-1`) is board-adjacent and the three cell reviewers are team-scoped. It claims and posts inbound-PR reviews (`claim_pr_review` / `post_pr_review`) and runs the in-path assembled-PR gate (`claim_gate_review` / `pr_pass` / `pr_fail`), but creates, assigns, merges, and notifies nothing. `prompter` (intake) and `secretary` are **human-only** — they chat with the CEO and have only `note` + `evidence`, with no task or notification permissions; they don't appear in the action tables below. ## Task Permissions diff --git a/docs/rag/architecture/tool-permissions.md b/docs/rag/architecture/tool-permissions.md index 77466bcb..0050997c 100644 --- a/docs/rag/architecture/tool-permissions.md +++ b/docs/rag/architecture/tool-permissions.md @@ -64,7 +64,7 @@ The canonical source of role → verb mapping is `roboco/services/gateway/role_c **Read-only git:** all 4 -**Workspace writes:** none. `complete` on a root parent task opens the master PR via the choreographer and escalates to CEO. +**Workspace writes:** none. `submit_root` on a root parent task opens the root→master PR (entering the `awaiting_pr_review` gate); after the main reviewer `pr_pass`es it, `complete` escalates to the CEO. The Main PM never merges to master — only the CEO does. ## Board (Product Owner, Head of Marketing) @@ -84,13 +84,13 @@ The canonical source of role → verb mapping is `roboco/services/gateway/role_c ## PR Reviewer -**Flow verbs:** `give_me_work`, `claim_pr_review`, `post_pr_review`, `i_am_idle` (read-only) +**Flow verbs:** `give_me_work`, `claim_pr_review`, `post_pr_review` (inbound external/fork + internal PRs), `claim_gate_review`, `pr_pass`, `pr_fail` (in-path assembled-PR gate), `i_am_idle` (read-only) **Content verbs:** `note`, `evidence`, plus notification reads (`notify_list`, `notify_get`) and channel discovery — no `say` / `dm`: the change-request is posted server-side on the PR itself. **Read-only git:** none. -**Workspace writes:** none — reviews inbound external/fork + internal PRs only. +**Workspace writes:** none — reviews inbound PRs and the org's own assembled cell→root / root→master PRs read-only; never merges. ## Prompter (Intake) & Secretary diff --git a/docs/rag/lifecycle/intent-verbs.md b/docs/rag/lifecycle/intent-verbs.md index b1db0409..73a54a91 100644 --- a/docs/rag/lifecycle/intent-verbs.md +++ b/docs/rag/lifecycle/intent-verbs.md @@ -9,6 +9,15 @@ Claim awaiting_documentation. Returns evidence inline. **Composes:** (no atomic actions) +## claim_gate_review + +Claim an assembled-PR review task (awaiting_pr_review) WITHOUT transitioning it — mirrors QA's claim_review. The assembled diff and the parent task's acceptance criteria are returned inline. + +**Allowed roles:** pr_reviewer + +**Composes:** (no atomic actions) + + ## claim_pr_review Claim an inbound external-PR review task and start work. pending -> claimed -> in_progress. @@ -29,7 +38,7 @@ Claim a task in awaiting_qa for review. Returns evidence inline. ## complete -Cell PM merges leaf PR + transitions to completed; Main PM merges root PR + escalates to CEO. +Cell PM merges the PR (leaf into the cell branch, or the gated cell→root PR into the root branch) + transitions to completed; Main PM escalates the root to the CEO (who merges root→master). **Allowed roles:** cell_pm, main_pm @@ -174,6 +183,24 @@ Post one complete change-request to the external PR and finish the review task. **Composes:** pr_review_done +## pr_fail + +Fail the assembled-PR review with concrete issues. Transitions awaiting_pr_review -> needs_revision, routed back like a QA fail. + +**Allowed roles:** pr_reviewer + +**Composes:** pr_fail + + +## pr_pass + +Pass the assembled-PR review. Transitions awaiting_pr_review -> awaiting_pm_review so the PM can merge. + +**Allowed roles:** pr_reviewer + +**Composes:** pr_pass + + ## reassign Hand a claimed/in_progress task to another developer in your own cell. The branch is keyed to the task (not the agent), so it is preserved — the new developer continues the work-in-progress. No status change. @@ -192,13 +219,24 @@ Resume a paused task you own. paused -> in_progress. **Composes:** resume +## submit_root + +Main PM opens the root→master PR and moves the root task to awaiting_pr_review for the main reviewer (the root analogue of the cell PM's submit_up). After pr_pass, call complete to escalate to the CEO. Only for code roots; branchless coordination roots skip the gate and complete directly. + +**Allowed roles:** main_pm + +**Composes:** submit_for_review + +**Pre side effects:** create_root_pr + + ## submit_up -Cell PM opens the cell→root PR and moves the cell task to awaiting_pm_review. The same Cell PM then completes it. +Cell PM opens the cell→root PR and moves the cell task into the PR-review gate (awaiting_pr_review). The cell reviewer reviews the assembled diff; after pr_pass the same Cell PM completes it. **Allowed roles:** cell_pm -**Composes:** submit_pm_review +**Composes:** submit_for_review **Pre side effects:** create_pr diff --git a/docs/rag/lifecycle/status-transitions.md b/docs/rag/lifecycle/status-transitions.md index 42082b85..d8ad1f67 100644 --- a/docs/rag/lifecycle/status-transitions.md +++ b/docs/rag/lifecycle/status-transitions.md @@ -11,6 +11,10 @@ | awaiting_pm_review | awaiting_ceo_approval | escalate_to_ceo | head_marketing, main_pm, product_owner | | awaiting_pm_review | cancelled | cancel | cell_pm, ceo, main_pm | | awaiting_pm_review | completed | complete | cell_pm, main_pm | +| awaiting_pr_review | awaiting_pm_review | pr_pass | pr_reviewer | +| awaiting_pr_review | cancelled | cancel | cell_pm, ceo, main_pm | +| awaiting_pr_review | claimed | claim | pr_reviewer | +| awaiting_pr_review | needs_revision | pr_fail | pr_reviewer | | awaiting_qa | awaiting_documentation | qa_pass | qa | | awaiting_qa | cancelled | cancel | cell_pm, ceo, main_pm | | awaiting_qa | claimed | claim | qa | @@ -24,6 +28,7 @@ | claimed | cancelled | cancel | cell_pm, ceo, main_pm | | claimed | in_progress | start | any | | in_progress | awaiting_pm_review | submit_pm_review | any | +| in_progress | awaiting_pr_review | submit_for_review | any | | in_progress | blocked | block | any | | in_progress | cancelled | cancel | cell_pm, ceo, main_pm | | in_progress | completed | pr_review_done | pr_reviewer | diff --git a/docs/rag/roles/cell-pm.md b/docs/rag/roles/cell-pm.md index cec6a6c2..b5bb1c58 100644 --- a/docs/rag/roles/cell-pm.md +++ b/docs/rag/roles/cell-pm.md @@ -22,8 +22,8 @@ - Create subtasks via `delegate(parent_task_id, title, description, body)` - Triage your cell's queue via `triage()` - Unblock blocked tasks via `unblock(task_id, restore=True)` -- Complete tasks via `complete(task_id, notes)` — this merges the leaf PR (no separate `merge_pr` tool exists; the choreographer does it) -- Submit a finished cell-scoped task up to Main PM via `submit_up(task_id, notes)` +- Complete tasks via `complete(task_id, notes)` — this merges the PR (a leaf subtask's PR into your cell branch, or your assembled cell→root PR into the root branch after it clears the gate). No separate `merge_pr` tool exists; the choreographer does it. +- Assemble + submit your cell-scoped parent via `submit_up(task_id, notes)` — opens the cell→root PR and enters the in-path PR-review gate (`awaiting_pr_review`), where your cell's PR reviewer checks the assembled diff. After `pr_pass`, you `complete` it to merge. - Send `notify` (ack-required notifications) — devs/QA/doc cannot - Read-only inspect git via `roboco_git_status / _log / _diff / _branch_list` @@ -32,7 +32,7 @@ - Access other cells' tasks → Main PM only (`triage_all`) - Pass / fail QA → QA only - Write code or commit → devs / documenters only (`commit` is in their manifest, not yours) -- Open the master PR → that's Main PM's `complete` on the root parent +- Open or merge the master PR → the Main PM's `submit_root` opens the root→master PR and only the CEO merges it to `master` - Run shell git — blocked by the bash-guard hook ## Task Flow (gateway verbs) @@ -52,12 +52,15 @@ triage() → scan your cell's queue unblock(task_id, restore=True) → unblock + restore prior status reassign(task_id, new_assignee) → hand a claimed/in_progress task to another dev in your cell (WIP survives) -complete(task_id, notes) → merges the leaf PR; transitions task - to completed (or escalates root parent - to CEO via Main PM) +complete(task_id, notes) → merges the PR (a leaf subtask into your + cell branch, or — after the gate — your + cell→root PR into the root branch); + transitions the task to completed -submit_up(task_id, notes) → bubble finished cell-scoped work up to - Main PM, who integrates it (see below) +submit_up(task_id, notes) → opens the cell→root PR and enters the + PR-review gate (awaiting_pr_review); your + cell reviewer pr_passes it, then you + complete to merge (see below) escalate_up(task_id, reason) → ask Main PM for help (cross-cell, etc.) unclaim(task_id) / resume(task_id) / i_am_idle() ``` @@ -142,11 +145,16 @@ notify(target="be-dev-1", text="Please prioritise task X by EOD.", priority="high", task_id="...") ``` -## Submitting Finished Work Up +## Assembling + Submitting Finished Work -When a cell-scoped task is done (QA green, docs landed, leaf PR merged into your cell branch via `complete`), `submit_up(task_id, notes)` hands it to **Main PM (main-pm)**. Main PM owns the integration branch and the master PR — it merges your cell branch up the chain and ultimately into master (you never open a master PR yourself). See the Main PM role doc, "Integrating cell work + completing the root." +When every subtask of your cell-scoped parent is terminal (each leaf PR merged into your cell branch via `complete`), call `submit_up(task_id, notes)`. This opens the **cell→root PR** and moves the parent into the in-path PR-review gate (`awaiting_pr_review`), where your cell's **PR reviewer** reviews the assembled diff: -`submit_up` is for finished work moving up; `escalate_up` (below) is for *help* you need while work is still in flight. +- `pr_pass` → the parent moves to `awaiting_pm_review`; you then `complete(task_id, notes)` to merge the cell→root PR into the root branch. +- `pr_fail` → the parent returns to `needs_revision` (owned by you) with the reviewer's issues; fix, then re-`submit_up`. + +You merge your own cell→root PR — the Main PM does **not** merge your cell branch. The Main PM owns the **root** task: once every cell's parent is terminal, it runs the same gate one level up (`submit_root` → main reviewer → escalate to CEO) and only the CEO merges to `master`. You never open or merge a master PR yourself. + +`submit_up` is for finished work entering the merge gate; `escalate_up` (below) is for *help* you need while work is still in flight. ## Escalating to Main PM diff --git a/docs/rag/roles/main-pm.md b/docs/rag/roles/main-pm.md index 94523ede..9d2f55e7 100644 --- a/docs/rag/roles/main-pm.md +++ b/docs/rag/roles/main-pm.md @@ -80,7 +80,7 @@ channels() # discover the pm-all channel, then read its history | MCP server | Verbs you can call | |-----------------------|--------------------| -| `roboco-flow` | `triage`, `triage_all`, `give_me_work`, `i_will_plan`, `delegate`, `unblock`, `complete`, `escalate_up`, `escalate_to_ceo`, `resume`, `unclaim`, `i_am_idle` | +| `roboco-flow` | `triage`, `triage_all`, `give_me_work`, `i_will_plan`, `delegate`, `unblock`, `submit_root`, `complete`, `escalate_up`, `escalate_to_ceo`, `resume`, `unclaim`, `i_am_idle` | | `roboco-do` | `note`, `say`, `dm`, `notify`, `evidence`, `open_session`, `link_session`, `pr_update` | | `roboco-docs` | `roboco_docs_write`, `roboco_docs_read`, `roboco_docs_list` | | `roboco-git-readonly` | `roboco_git_status`, `roboco_git_log`, `roboco_git_diff`, `roboco_git_branch_list` | @@ -103,16 +103,17 @@ This is for *help while work is in flight*. Finished cell-scoped work arrives by ## Integrating cell work + completing the root -You own the integration-branch chain and the master PR. Cell PMs `submit_up(task_id, notes)` their finished cell-scoped tasks to you; they merge only their own cell/leaf PRs and never touch master (see the Cell PM role doc, "Submitting Finished Work Up"). +You own the **root** task and the root→master PR. Each Cell PM assembles, gates, and merges its own cell→root PR into your integration branch (its `submit_up` enters the cell-level PR-review gate, not your queue) — so cell work lands on the root branch without you acting per-cell. ``` master ← feature/main_pm/{root} ← feature/{cell}/{root}/{cell-pm} ← dev branches -(you + CEO) (you) (cell PM) (devs) +(CEO) (you, via gate) (cell PM, via gate) (devs) ``` -- A cell PM's `complete` merges a leaf PR into its cell branch; `submit_up` then hands the cell-scoped result to you. -- Your `complete(root_task_id, notes)` on the **root** parent is what opens/merges the master-bound PR — once every cell's subtasks are terminal. -- For major work, escalate the finished root to the CEO with `escalate_to_ceo(root_task_id, reason)` instead; the CEO approves and merges from the panel. Only Main PM and the CEO ever act on master. +- A cell PM's `complete` merges a leaf PR into its cell branch; after the cell gate, its `complete` merges the cell→root PR into your root branch. You do not merge cell branches. +- Once every cell's parent is terminal, **`submit_root(root_task_id, notes)`** opens the root→master PR and enters the in-path gate (`awaiting_pr_review`). The **main PR reviewer** checks the assembled root diff: `pr_pass` → `awaiting_pm_review`; `pr_fail` → `needs_revision` (owned by you, fix + re-`submit_root`). +- After `pr_pass`, `complete(root_task_id, notes)` escalates the root to the CEO (`awaiting_ceo_approval`) — it does **not** merge. A branchless coordination root (product fan-out, no repo) skips the gate and `complete` escalates directly. +- The CEO approves and merges the root→master PR from the panel. Only the CEO ever merges to `master`. ## A2A diff --git a/docs/rag/roles/pr-reviewer.md b/docs/rag/roles/pr-reviewer.md index f1b63185..85964647 100644 --- a/docs/rag/roles/pr-reviewer.md +++ b/docs/rag/roles/pr-reviewer.md @@ -2,9 +2,9 @@ ## Identity -- **Agent:** pr-reviewer-1 (the single global reviewer — one review at a time) +- **Agents:** pr-reviewer-1 (the main reviewer) + be-pr-reviewer / fe-pr-reviewer / ux-pr-reviewer (one in-path reviewer per cell) - **Role:** `pr_reviewer` -- **Team:** none (board-adjacent, read-only; `RoleLevel.QA`) +- **Team:** pr-reviewer-1 is board-adjacent (team none); the cell reviewers are team-scoped (backend / frontend / ux_ui). Read-only; `RoleLevel.QA`. - **Reports to:** CEO ## Core Responsibilities @@ -13,12 +13,17 @@ 2. Read the PR diff adversarially against the project's standards and post **one** complete change-request as a real GitHub review **on the PR itself** — no agent-to-agent chatter. 3. Journal evidence of what was checked. -The org's own in-flight integration PRs are skipped — a live task already owns their branch and they pass QA + PM review. Re-review is driven by the PR's head commit: an unchanged PR is skipped, new commits open a fresh review. +The org's own in-flight integration PRs are skipped by the **inbound** poll above — a live task already owns their branch. Re-review of inbound PRs is driven by the PR's head commit: an unchanged PR is skipped, new commits open a fresh review. + +## In-path PR-review gate + +The `pr_reviewer` role also runs the **in-path gate** on the org's OWN assembled delivery PRs — the merge-level review QA does not do. When a cell PM bubbles up its cell→root PR (`submit_up`) or the Main PM opens the root→master PR (`submit_root`), the task enters `awaiting_pr_review`. The cell reviewer (be/fe/ux-pr-reviewer) reviews its cell's assembled PR; pr-reviewer-1 reviews the root→master PR for the cross-cell integration seam (the bug class that lives where the FE and BE meet). Workflow: `claim_gate_review(task_id)` → review the assembled diff against the parent objective + every acceptance criterion + the FE↔BE contract → `note(scope="learning", ...)` → `pr_pass(task_id, notes)` (moves it on to the PM merge) or `pr_fail(task_id, issues)` (sends it back to `needs_revision`, like a QA fail). Either verdict is also posted on the assembled PR itself as a GitHub review (server-side, via the bot account) so the decision is visible on the PR the PM merges: `pr_pass` posts an APPROVE and `pr_fail` a REQUEST_CHANGES — except on the root→master PR, which only ever gets a plain COMMENT because only the CEO acts on `master`. This gate gives the merge level the reject teeth the PM otherwise lacks. Leaf dev tasks and branchless coordination roots skip the gate. ## What You CAN Do - Pull an inbound-PR review task via `give_me_work()` and claim it via `claim_pr_review(task_id)`. - Post your verdict via `post_pr_review(task_id, ...)` — the change-request lands on the PR as a GitHub review (server-side; you never push to the contributor's fork). +- Run the in-path gate on the org's assembled delivery PRs: `claim_gate_review(task_id)` → `pr_pass(task_id, notes)` or `pr_fail(task_id, issues)`. - Read-only inspect git via `roboco_git_status / _log / _diff / _branch_list`. - Search the knowledge base via `roboco_ask_mentor` / `roboco_kb_search`. - Note evidence via `note(...)` and `evidence(...)`. diff --git a/docs/rag/tools/git-tools.md b/docs/rag/tools/git-tools.md index f7c15dd9..4d4bd2fa 100644 --- a/docs/rag/tools/git-tools.md +++ b/docs/rag/tools/git-tools.md @@ -47,7 +47,9 @@ There is no separate `push` step and no separate `create_pr` step. Both are side ```python # Cell PM completing a leaf task: merges the leaf PR. -# Main PM completing a parent task: opens the master PR + escalates to CEO. +# (Assembled cell→root / root→master PRs are opened by submit_up / submit_root +# and gated in awaiting_pr_review first.) After a code root's gate clears, +# the Main PM's complete escalates to the CEO — the CEO merges root→master. complete(task_id="a1b2c3d4-...", notes="QA passed; docs complete; ready to ship.") ``` diff --git a/docs/rag/tools/task-tools.md b/docs/rag/tools/task-tools.md index 64ca329a..aa0d1ca0 100644 --- a/docs/rag/tools/task-tools.md +++ b/docs/rag/tools/task-tools.md @@ -60,7 +60,9 @@ delegate(parent_task_id, title, description, assigned_to, team, task_type, # it to the parent ACs it is responsible for reassign(task_id, assigned_to) # move a subtask to a different agent unblock(task_id) # blocked -> in_progress (PM only) -submit_up(task_id, notes) # open cell->root PR; -> awaiting_pm_review +submit_up(task_id, notes) # open cell->root PR; -> awaiting_pr_review + # (the cell PR reviewer gates it; after pr_pass + # the same Cell PM completes + merges) complete(task_id, notes) # awaiting_pm_review -> completed (merges leaf PR) escalate_up(task_id, reason) # escalate to your escalation target ``` @@ -71,16 +73,19 @@ After `i_will_plan` and each `delegate`, the envelope includes a coverage view o ## Main PM flow -The Main PM shares most Cell PM verbs (`i_will_plan`, `delegate`, `complete`, `unblock`, `triage`, `escalate_up`), **adds** the two below, and — unlike a Cell PM — has **no** `submit_up` or `reassign` (there is no PM above it to submit to; it completes or escalates the root directly): +The Main PM shares most Cell PM verbs (`i_will_plan`, `delegate`, `complete`, `unblock`, `triage`, `escalate_up`), **adds** the verbs below, and — unlike a Cell PM — has **no** `submit_up` or `reassign`. Its bubble-up verb is `submit_root` (the root analogue of the Cell PM's `submit_up`): ```python triage_all() # list actionable tasks across all teams +submit_root(task_id, notes) # open root->master PR; -> awaiting_pr_review + # (the main PR reviewer gates it; after pr_pass, + # complete escalates to the CEO) escalate_to_ceo(task_id, reason) # awaiting_pm_review -> awaiting_ceo_approval give_me_work() # Main PM may also pull work directly ``` -`complete` for the Main PM merges the **root** PR. Only the CEO merges to `master`; agents stop at `escalate_to_ceo`. +For a code root the Main PM **must** `submit_root` first — that opens the root→master PR and enters the in-path gate (`awaiting_pr_review`); only after the main reviewer `pr_pass`es it does `complete` escalate to the CEO. A branchless coordination root (product fan-out, no repo) skips the gate and is completed/escalated directly. The Main PM never merges to `master` — `complete` escalates and only the CEO merges the root→master PR. ## Board flow (Product Owner / Head of Marketing) @@ -112,6 +117,18 @@ i_am_idle() The PR Reviewer reviews inbound external/fork (and, behind a flag, internal) PRs the org did not open. It is read-only: no `commit`/`open_pr`/`merge`, no `say`/`dm` — the change-request is posted server-side on the PR itself, and the CEO decides Supersede/Dismiss from the PR Review Queue. +The same role also runs the **in-path PR-review gate** on the org's own assembled delivery PRs — the merge-level review before the PM merges: + +```python +claim_gate_review(task_id) # claim an awaiting_pr_review task; returns the assembled diff +pr_pass(task_id, notes) # assembled PR is correct -> awaiting_pm_review (the PM merges) +pr_fail(task_id, issues) # send it back -> needs_revision, like a QA fail +``` + +Both verdicts are also posted on the assembled PR itself as a GitHub review (server-side, bot account) so the decision is visible on the PR the PM merges: `pr_pass` → APPROVE, `pr_fail` → REQUEST_CHANGES — except the root→master PR, which only ever gets a plain COMMENT (only the CEO acts on `master`). + +A cell reviewer (be/fe/ux-pr-reviewer) reviews its cell's assembled cell→root PR; `pr-reviewer-1` reviews the root→master PR for the cross-cell integration seam, before the CEO sees it. + ## Cancel Cancelling a task (any non-terminal status -> `cancelled`) is restricted to **PM roles and the CEO**. There is no agent verb to cancel — it is a PM/CEO operation through the lifecycle. diff --git a/docs/rag/workflows/git-commits.md b/docs/rag/workflows/git-commits.md index ad1b3ed4..44f6e44f 100644 --- a/docs/rag/workflows/git-commits.md +++ b/docs/rag/workflows/git-commits.md @@ -48,7 +48,7 @@ This automatically: You don't push or create a PR yourself. The choreographer pushed the commit during `commit()`, and the PR is opened/merged as part of the lifecycle transitions: - `open_pr(task_id)` — opens the PR (devs) -- `pass(task_id)` (QA) → `i_documented(task_id)` (doc) → `complete(task_id)` (cell PM merges the leaf PR; main PM opens the master PR) +- `pass(task_id)` (QA) → `i_documented(task_id)` (doc) → `complete(task_id)` (cell PM merges the leaf PR). Assembled PRs pass the in-path gate first: the cell PM's `submit_up` (cell→root PR) and the main PM's `submit_root` (root→master PR) open the PR and enter `awaiting_pr_review`; after a reviewer `pr_pass`, the cell PM `complete`s to merge cell→root, while the main PM's `complete` escalates the root to the CEO, who merges to master. ## Viewing Commits and History diff --git a/docs/rag/workflows/pr-creation.md b/docs/rag/workflows/pr-creation.md index af6b94ed..131a5edf 100644 --- a/docs/rag/workflows/pr-creation.md +++ b/docs/rag/workflows/pr-creation.md @@ -49,14 +49,14 @@ Task transitions to `awaiting_pm_review` when both are true. ## PM Merges via `complete` -After `awaiting_pm_review`, the Cell PM calls `complete(task_id, notes)`. The choreographer: +An assembled parent reaches `awaiting_pm_review` only after the in-path gate: the Cell PM's `submit_up` opens the cell→root PR and enters `awaiting_pr_review`, where the cell PR reviewer `pr_pass`es it. The Cell PM then calls `complete(task_id, notes)`. The choreographer: 1. Verifies all subtasks are in a terminal state 2. Verifies the PR is reviewable 3. Merges the leaf PR into the parent branch (squash by default) 4. Transitions the task to `completed` -For the root parent, **Main PM**'s `complete` opens the master PR and escalates to CEO via `escalate_to_ceo` semantics. +For the root parent, **Main PM**'s `submit_root` opens the root→master PR and enters the same gate; after the main reviewer `pr_pass`es it, the Main PM's `complete` escalates to the CEO (it does **not** merge). Only the CEO merges the root→master PR. There is no `roboco_git_merge_pr` MCP tool. diff --git a/panel/lib/lifecycle.json b/panel/lib/lifecycle.json index be0a49fc..579b9dc9 100644 --- a/panel/lib/lifecycle.json +++ b/panel/lib/lifecycle.json @@ -18,6 +18,7 @@ "pending" ], "pr_reviewer": [ + "awaiting_pr_review", "pending" ], "product_owner": [], @@ -36,6 +37,16 @@ "pre_side_effects": [], "side_effects": [] }, + { + "allowed_roles": [ + "pr_reviewer" + ], + "composes": [], + "description": "Claim an assembled-PR review task (awaiting_pr_review) WITHOUT transitioning it \u2014 mirrors QA's claim_review. The assembled diff and the parent task's acceptance criteria are returned inline.", + "name": "claim_gate_review", + "pre_side_effects": [], + "side_effects": [] + }, { "allowed_roles": [ "pr_reviewer" @@ -67,7 +78,7 @@ "composes": [ "complete" ], - "description": "Cell PM merges leaf PR + transitions to completed; Main PM merges root PR + escalates to CEO.", + "description": "Cell PM merges the PR (leaf into the cell branch, or the gated cell\u2192root PR into the root branch) + transitions to completed; Main PM escalates the root to the CEO (who merges root\u2192master).", "name": "complete", "pre_side_effects": [], "side_effects": [ @@ -264,6 +275,30 @@ "pre_side_effects": [], "side_effects": [] }, + { + "allowed_roles": [ + "pr_reviewer" + ], + "composes": [ + "pr_fail" + ], + "description": "Fail the assembled-PR review with concrete issues. Transitions awaiting_pr_review -> needs_revision, routed back like a QA fail.", + "name": "pr_fail", + "pre_side_effects": [], + "side_effects": [] + }, + { + "allowed_roles": [ + "pr_reviewer" + ], + "composes": [ + "pr_pass" + ], + "description": "Pass the assembled-PR review. Transitions awaiting_pr_review -> awaiting_pm_review so the PM can merge.", + "name": "pr_pass", + "pre_side_effects": [], + "side_effects": [] + }, { "allowed_roles": [ "cell_pm" @@ -290,14 +325,28 @@ "pre_side_effects": [], "side_effects": [] }, + { + "allowed_roles": [ + "main_pm" + ], + "composes": [ + "submit_for_review" + ], + "description": "Main PM opens the root\u2192master PR and moves the root task to awaiting_pr_review for the main reviewer (the root analogue of the cell PM's submit_up). After pr_pass, call complete to escalate to the CEO. Only for code roots; branchless coordination roots skip the gate and complete directly.", + "name": "submit_root", + "pre_side_effects": [ + "create_root_pr" + ], + "side_effects": [] + }, { "allowed_roles": [ "cell_pm" ], "composes": [ - "submit_pm_review" + "submit_for_review" ], - "description": "Cell PM opens the cell\u2192root PR and moves the cell task to awaiting_pm_review. The same Cell PM then completes it.", + "description": "Cell PM opens the cell\u2192root PR and moves the cell task into the PR-review gate (awaiting_pr_review). The cell reviewer reviews the assembled diff; after pr_pass the same Cell PM completes it.", "name": "submit_up", "pre_side_effects": [ "create_pr" @@ -438,6 +487,40 @@ "source": "awaiting_pm_review", "target": "completed" }, + { + "action": "pr_pass", + "roles": [ + "pr_reviewer" + ], + "source": "awaiting_pr_review", + "target": "awaiting_pm_review" + }, + { + "action": "cancel", + "roles": [ + "cell_pm", + "ceo", + "main_pm" + ], + "source": "awaiting_pr_review", + "target": "cancelled" + }, + { + "action": "claim", + "roles": [ + "pr_reviewer" + ], + "source": "awaiting_pr_review", + "target": "claimed" + }, + { + "action": "pr_fail", + "roles": [ + "pr_reviewer" + ], + "source": "awaiting_pr_review", + "target": "needs_revision" + }, { "action": "qa_pass", "roles": [ @@ -542,6 +625,12 @@ "source": "in_progress", "target": "awaiting_pm_review" }, + { + "action": "submit_for_review", + "roles": null, + "source": "in_progress", + "target": "awaiting_pr_review" + }, { "action": "block", "roles": null, diff --git a/roboco/api/routes/v1/flow_main_pm.py b/roboco/api/routes/v1/flow_main_pm.py index ba3115d2..d4538c5a 100644 --- a/roboco/api/routes/v1/flow_main_pm.py +++ b/roboco/api/routes/v1/flow_main_pm.py @@ -16,6 +16,7 @@ from roboco.api.schemas.v1.flow import ( IAmIdleRequest, IWillPlanRequest, ResumeRequest, + SubmitRootRequest, TriageRequest, UnblockRequest, UnclaimRequest, @@ -100,6 +101,17 @@ async def triage_all( return envelope_to_response(env, request) +@router.post("/submit_root") +async def submit_root( + request: Request, + body: SubmitRootRequest, + x_agent_id: _AgentIdHeader, + choreographer: _ChoreographerDep, +) -> dict: + env = await choreographer.submit_root(x_agent_id, body.task_id, body.notes) + return envelope_to_response(env, request) + + @router.post("/complete") async def complete( request: Request, diff --git a/roboco/api/routes/v1/flow_pr_reviewer.py b/roboco/api/routes/v1/flow_pr_reviewer.py index d5be3a4e..a3b4a07e 100644 --- a/roboco/api/routes/v1/flow_pr_reviewer.py +++ b/roboco/api/routes/v1/flow_pr_reviewer.py @@ -8,10 +8,13 @@ from fastapi import APIRouter, Depends, Header, Request from roboco.api.deps import get_choreographer from roboco.api.routes.v1._role_dep import envelope_to_response, require_pr_reviewer from roboco.api.schemas.v1.flow import ( + ClaimGateReviewRequest, ClaimPrReviewRequest, GiveMeWorkRequest, IAmIdleRequest, PostPrReviewRequest, + PrFailRequest, + PrPassRequest, ) from roboco.services.gateway.choreographer import Choreographer @@ -61,6 +64,39 @@ async def post_pr_review( return envelope_to_response(env, request) +@router.post("/claim_gate_review") +async def claim_gate_review( + request: Request, + body: ClaimGateReviewRequest, + x_agent_id: _AgentIdHeader, + choreographer: _ChoreographerDep, +) -> dict: + env = await choreographer.claim_gate_review(x_agent_id, body.task_id) + return envelope_to_response(env, request) + + +@router.post("/pr_pass") +async def pr_pass( + request: Request, + body: PrPassRequest, + x_agent_id: _AgentIdHeader, + choreographer: _ChoreographerDep, +) -> dict: + env = await choreographer.pr_pass(x_agent_id, body.task_id, body.notes) + return envelope_to_response(env, request) + + +@router.post("/pr_fail") +async def pr_fail( + request: Request, + body: PrFailRequest, + x_agent_id: _AgentIdHeader, + choreographer: _ChoreographerDep, +) -> dict: + env = await choreographer.pr_fail(x_agent_id, body.task_id, body.issues) + return envelope_to_response(env, request) + + @router.post("/i_am_idle") async def i_am_idle( request: Request, diff --git a/roboco/api/schemas/v1/flow.py b/roboco/api/schemas/v1/flow.py index 631aafca..656a9cc7 100644 --- a/roboco/api/schemas/v1/flow.py +++ b/roboco/api/schemas/v1/flow.py @@ -135,6 +135,20 @@ class PostPrReviewRequest(BaseModel): event: str = "REQUEST_CHANGES" +class ClaimGateReviewRequest(BaseModel): + task_id: UUID + + +class PrPassRequest(BaseModel): + task_id: UUID + notes: str = Field(..., min_length=1) + + +class PrFailRequest(BaseModel): + task_id: UUID + issues: list[str] = Field(..., min_length=1) + + class ClaimDocTaskRequest(BaseModel): task_id: UUID @@ -283,3 +297,8 @@ class DelegateRequest(BaseModel): class SubmitUpRequest(BaseModel): task_id: UUID notes: str = Field(..., min_length=1) + + +class SubmitRootRequest(BaseModel): + task_id: UUID + notes: str = Field(..., min_length=1) diff --git a/roboco/config.py b/roboco/config.py index 47ff77a1..030dafe4 100644 --- a/roboco/config.py +++ b/roboco/config.py @@ -646,6 +646,24 @@ class Settings(BaseSettings): ge=60, description="Claim heartbeat staleness threshold (seconds)", ) + # Debounce for respawning a PM to CLOSE a paused parent once its subtasks + # are terminal. It guards only the narrow i_am_idle race (the parent + # auto-pauses, then the agent is marked IDLE + its container tears down) — + # the live-session case is already covered by the `_is_agent_active` check. + # It must therefore be SHORT (a few dispatch ticks), NOT the multi-minute + # reaper window: a paused parent's heartbeat reflects when the PM last + # worked, so a PM that worked right up to idle leaves a fresh heartbeat and + # any large window strands the whole chain until it expires. Was wrongly + # bound to stale_claim_reap_seconds (600s default, 1800s on the NAS), which + # delayed every cell/main closure by up to 10-30 minutes. + pm_closure_recently_paused_seconds: int = Field( + default=45, + ge=5, + description=( + "Debounce (seconds) before respawning a PM to close a recently " + "paused parent; override via ROBOCO_PM_CLOSURE_RECENTLY_PAUSED_SECONDS" + ), + ) # Reaper window for stale-claim detection. Dogfooding reaped agents at # ~180s while they were actively # retrying — LLM inference + retry loops routinely exceed 3 min diff --git a/roboco/foundation/_validate_lifecycle.py b/roboco/foundation/_validate_lifecycle.py index f5bc32ee..84cd1af2 100644 --- a/roboco/foundation/_validate_lifecycle.py +++ b/roboco/foundation/_validate_lifecycle.py @@ -160,17 +160,20 @@ def _check_claim_rules_status_coverage() -> None: def _check_self_review_symmetry() -> None: - """qa_pass / qa_fail / docs_complete must agree on self_review_block.""" + """Review actions must agree on self_review_block. + + qa_pass / qa_fail / docs_complete and the in-path PR-review gate's + pr_pass / pr_fail are all reviewer sign-offs — they must all block + self-review identically so no path lets an author approve their own work. + """ from roboco.foundation.policy.lifecycle import _ATOMIC_ACTIONS - qp = _ATOMIC_ACTIONS["qa_pass"].self_review_block - qf = _ATOMIC_ACTIONS["qa_fail"].self_review_block - dc = _ATOMIC_ACTIONS["docs_complete"].self_review_block - if not (qp == qf == dc): - raise LifecycleSpecError( - f"self_review_block asymmetry:" - f" qa_pass={qp}, qa_fail={qf}, docs_complete={dc}" - ) + flags = { + name: _ATOMIC_ACTIONS[name].self_review_block + for name in ("qa_pass", "qa_fail", "docs_complete", "pr_pass", "pr_fail") + } + if len(set(flags.values())) != 1: + raise LifecycleSpecError(f"self_review_block asymmetry: {flags}") def _check_role_team_rules_slugs() -> None: diff --git a/roboco/foundation/identity.py b/roboco/foundation/identity.py index ecd49f6b..0f86ebfd 100644 --- a/roboco/foundation/identity.py +++ b/roboco/foundation/identity.py @@ -222,6 +222,28 @@ AGENTS: dict[str, AgentRow] = { Team.BOARD, _u("00000000-0000-0000-0004-000000000007"), ), + # In-path PR-review gate reviewers — one per cell, team-scoped so the + # dispatcher routes each cell's assembled cell→root PR to its own reviewer. + # Same PR_REVIEWER role/image as pr-reviewer-1 (which serves the root→master + # gate + inbound external PRs); these only ever review their cell's gate. + "be-pr-reviewer": AgentRow( + "be-pr-reviewer", + Role.PR_REVIEWER, + Team.BACKEND, + _u("00000000-0000-0000-0001-000000000006"), + ), + "fe-pr-reviewer": AgentRow( + "fe-pr-reviewer", + Role.PR_REVIEWER, + Team.FRONTEND, + _u("00000000-0000-0000-0002-000000000006"), + ), + "ux-pr-reviewer": AgentRow( + "ux-pr-reviewer", + Role.PR_REVIEWER, + Team.UX_UI, + _u("00000000-0000-0000-0003-000000000006"), + ), } diff --git a/roboco/foundation/policy/lifecycle.py b/roboco/foundation/policy/lifecycle.py index cfb3373a..b811d988 100644 --- a/roboco/foundation/policy/lifecycle.py +++ b/roboco/foundation/policy/lifecycle.py @@ -48,6 +48,7 @@ class Status(StrEnum): AWAITING_QA = "awaiting_qa" NEEDS_REVISION = "needs_revision" AWAITING_DOCUMENTATION = "awaiting_documentation" + AWAITING_PR_REVIEW = "awaiting_pr_review" AWAITING_PM_REVIEW = "awaiting_pm_review" AWAITING_CEO_APPROVAL = "awaiting_ceo_approval" COMPLETED = "completed" @@ -279,6 +280,37 @@ _STATUS_TRANSITIONS: tuple[StatusTransition, ...] = ( "docs_complete", frozenset({Role.DOCUMENTER}), ), + # In-path PR-review gate (assembled cell→root + root→master PRs). The + # assembled-PR task enters the gate via submit_for_review (composed by the + # cell PM's submit_up and the main PM's submit_root); the reviewer claims it + # without transitioning (mirrors QA's claim_review), then pr_pass moves it on + # to awaiting_pm_review for the PM merge, or pr_fail routes it back exactly + # like a QA fail. Leaf tasks and branchless coordination roots never enter + # this gate — they reach awaiting_pm_review via docs_complete / submit_pm_review. + StatusTransition( + Status.IN_PROGRESS, + Status.AWAITING_PR_REVIEW, + "submit_for_review", + None, + ), + StatusTransition( + Status.AWAITING_PR_REVIEW, + Status.CLAIMED, + "claim", + frozenset({Role.PR_REVIEWER}), + ), + StatusTransition( + Status.AWAITING_PR_REVIEW, + Status.AWAITING_PM_REVIEW, + "pr_pass", + frozenset({Role.PR_REVIEWER}), + ), + StatusTransition( + Status.AWAITING_PR_REVIEW, + Status.NEEDS_REVISION, + "pr_fail", + frozenset({Role.PR_REVIEWER}), + ), # PM completes / escalates StatusTransition( Status.AWAITING_PM_REVIEW, @@ -379,6 +411,7 @@ _ATOMIC_ACTIONS: dict[str, ActionSpec] = { Status.NEEDS_REVISION, Status.AWAITING_QA, Status.AWAITING_DOCUMENTATION, + Status.AWAITING_PR_REVIEW, } ), target_status=Status.CLAIMED, @@ -509,6 +542,38 @@ _ATOMIC_ACTIONS: dict[str, ActionSpec] = { self_review_block=True, needs_team_match=True, ), + # PR-review gate: enter the gate (PM-driven, on an assembled PR), then the + # reviewer passes or fails it. pr_pass/pr_fail mirror qa_pass/qa_fail. + "submit_for_review": ActionSpec( + name="submit_for_review", + allowed_roles=_PM_ROLES, + source_statuses=frozenset({Status.IN_PROGRESS}), + target_status=Status.AWAITING_PR_REVIEW, + allowed_task_types=None, + preconditions=(), + self_review_block=False, + needs_team_match=True, + ), + "pr_pass": ActionSpec( + name="pr_pass", + allowed_roles=frozenset({Role.PR_REVIEWER}), + source_statuses=frozenset({Status.AWAITING_PR_REVIEW}), + target_status=Status.AWAITING_PM_REVIEW, + allowed_task_types=None, + preconditions=(), + self_review_block=True, + needs_team_match=True, + ), + "pr_fail": ActionSpec( + name="pr_fail", + allowed_roles=frozenset({Role.PR_REVIEWER}), + source_statuses=frozenset({Status.AWAITING_PR_REVIEW}), + target_status=Status.NEEDS_REVISION, + allowed_task_types=None, + preconditions=(), + self_review_block=True, + needs_team_match=True, + ), "complete": ActionSpec( name="complete", allowed_roles=_PM_ROLES, @@ -606,7 +671,7 @@ CLAIM_RULES: dict[Role, frozenset[Status]] = { Role.PRODUCT_OWNER: frozenset(), Role.HEAD_MARKETING: frozenset(), Role.AUDITOR: frozenset(), - Role.PR_REVIEWER: frozenset({Status.PENDING}), + Role.PR_REVIEWER: frozenset({Status.PENDING, Status.AWAITING_PR_REVIEW}), Role.CEO: frozenset(), } @@ -637,6 +702,9 @@ ROLE_TEAM_RULES: dict[str, str | None] = { "head-marketing": None, "auditor": None, "pr-reviewer-1": None, + "be-pr-reviewer": "backend", + "fe-pr-reviewer": "frontend", + "ux-pr-reviewer": "ux_ui", "ceo": None, } @@ -668,8 +736,18 @@ def _next_hint_after_plan(_t: Any) -> str: ) -def _next_hint_continue_delegating(_t: Any) -> str: - return "continue delegating subtasks, or i_am_idle when done" +def _next_hint_continue_delegating(t: Any) -> str: + # Name the bubble-up verb proactively so PMs don't have to discover it via + # a rejection: a root (no parent) is the Main PM's submit_root (root→master + # PR); a cell parent is the Cell PM's submit_up (cell→root PR). + bubble = ( + "submit_root" if getattr(t, "parent_task_id", None) is None else "submit_up" + ) + return ( + "continue delegating subtasks; when every subtask is terminal," + f" call {bubble}(task_id, notes='...') to open the PR + enter the" + " review gate (or i_am_idle if not ready)" + ) def _next_hint_qa_review(_t: Any) -> str: @@ -702,6 +780,21 @@ def _next_hint_pm_idle(_t: Any) -> str: return "idle until subtasks finish" +def _next_hint_pr_gate_review(_t: Any) -> str: + return ( + "review the assembled PR diff against the parent objective + full" + " acceptance criteria + the cross-cell contract. Then pr_pass(notes)" + " to accept or pr_fail(issues) to send back." + ) + + +def _next_hint_submit_root(_t: Any) -> str: + return ( + "root→master PR opened; the main reviewer will review it," + " then complete(task_id) escalates to the CEO" + ) + + # --------------------------------------------------------------------------- # Context — the third arg to Precondition.check (caller-supplied state) # --------------------------------------------------------------------------- @@ -1002,6 +1095,46 @@ _INTENT_VERBS: dict[str, IntentSpec] = { side_effects=(), next_hint=_next_hint_idle, ), + # In-path PR-review gate verbs (assembled cell→root + root→master PRs). + # Distinct from the external/fork reviewer surface above: these GATE an + # internal delivery task between docs/submit and the PM merge. + "claim_gate_review": IntentSpec( + name="claim_gate_review", + allowed_roles=frozenset({Role.PR_REVIEWER}), + description=( + "Claim an assembled-PR review task (awaiting_pr_review) WITHOUT" + " transitioning it — mirrors QA's claim_review. The assembled diff" + " and the parent task's acceptance criteria are returned inline." + ), + composes=(), + extra_preconditions=(), + side_effects=(), + next_hint=_next_hint_pr_gate_review, + ), + "pr_pass": IntentSpec( + name="pr_pass", + allowed_roles=frozenset({Role.PR_REVIEWER}), + description=( + "Pass the assembled-PR review. Transitions awaiting_pr_review ->" + " awaiting_pm_review so the PM can merge." + ), + composes=("pr_pass",), + extra_preconditions=(), + side_effects=(), + next_hint=_next_hint_idle, + ), + "pr_fail": IntentSpec( + name="pr_fail", + allowed_roles=frozenset({Role.PR_REVIEWER}), + description=( + "Fail the assembled-PR review with concrete issues. Transitions" + " awaiting_pr_review -> needs_revision, routed back like a QA fail." + ), + composes=("pr_fail",), + extra_preconditions=(), + side_effects=(), + next_hint=_next_hint_dev_revise, + ), # Phase 3: documenter verbs "claim_doc_task": IntentSpec( name="claim_doc_task", @@ -1026,8 +1159,9 @@ _INTENT_VERBS: dict[str, IntentSpec] = { name="complete", allowed_roles=_PM_ROLES, description=( - "Cell PM merges leaf PR + transitions to completed; Main PM" - " merges root PR + escalates to CEO." + "Cell PM merges the PR (leaf into the cell branch, or the gated" + " cell→root PR into the root branch) + transitions to completed;" + " Main PM escalates the root to the CEO (who merges root→master)." ), composes=("complete",), extra_preconditions=(), @@ -1064,10 +1198,11 @@ _INTENT_VERBS: dict[str, IntentSpec] = { name="submit_up", allowed_roles=frozenset({Role.CELL_PM}), description=( - "Cell PM opens the cell→root PR and moves the cell task to" - " awaiting_pm_review. The same Cell PM then completes it." + "Cell PM opens the cell→root PR and moves the cell task into the" + " PR-review gate (awaiting_pr_review). The cell reviewer reviews the" + " assembled diff; after pr_pass the same Cell PM completes it." ), - composes=("submit_pm_review",), + composes=("submit_for_review",), extra_preconditions=(), # The cell→root PR must exist BEFORE submit_pm_review runs — its # pr_created gate rejects (returning None) otherwise, which then @@ -1078,7 +1213,29 @@ _INTENT_VERBS: dict[str, IntentSpec] = { side_effects=(), # The Cell PM owns cell completion — it merges the cell→root PR # via complete(). Main PM only completes the ROOT task. - next_hint=lambda _t: "complete(task_id) to merge the cell→root PR", + next_hint=lambda _t: ( + "cell→root PR opened + in review; the cell reviewer will pr_pass," + " then complete(task_id) merges it" + ), + ), + "submit_root": IntentSpec( + name="submit_root", + allowed_roles=frozenset({Role.MAIN_PM}), + description=( + "Main PM opens the root→master PR and moves the root task to" + " awaiting_pr_review for the main reviewer (the root analogue of the" + " cell PM's submit_up). After pr_pass, call complete to escalate to" + " the CEO. Only for code roots; branchless coordination roots skip" + " the gate and complete directly." + ), + composes=("submit_for_review",), + extra_preconditions=(), + # The root→master PR must exist before the reviewer can review it — + # opened here (parent=master, is_root_pr=True), mirroring submit_up's + # pre-create of the cell→root PR. + pre_side_effects=("create_root_pr",), + side_effects=(), + next_hint=_next_hint_submit_root, ), "unblock": IntentSpec( name="unblock", @@ -1337,7 +1494,7 @@ def can_invoke_intent( # Special handling for claim-like verbs with empty composition (claim_review, # claim_doc_task). These verbs don't compose "claim" action, but still need # to enforce claim-status rules via CLAIM_RULES narrowing. - elif intent in ("claim_review", "claim_doc_task"): + elif intent in ("claim_review", "claim_doc_task", "claim_gate_review"): rejection = _check_claim_rules_narrow(role, task) if rejection is not None: return rejection diff --git a/roboco/foundation/policy/tracing.py b/roboco/foundation/policy/tracing.py index 0533c459..81dcb659 100644 --- a/roboco/foundation/policy/tracing.py +++ b/roboco/foundation/policy/tracing.py @@ -265,6 +265,10 @@ VERB_REQUIREMENTS: dict[str, frozenset[Requirement]] = { ), # PR reviewer posts its change-request — must record a learning entry first. "post_pr_review": frozenset({Requirement.JOURNAL_LEARNING}), + # In-path PR-review gate: the reviewer records a learning entry before + # passing or failing the assembled PR (parity with post_pr_review). + "pr_pass": frozenset({Requirement.JOURNAL_LEARNING}), + "pr_fail": frozenset({Requirement.JOURNAL_LEARNING}), # Doc submit. "i_documented": frozenset( { @@ -282,6 +286,16 @@ VERB_REQUIREMENTS: dict[str, frozenset[Requirement]] = { Requirement.NOTES_MIN_CHARS, } ), + # Main PM submit-root — root analogue of submit_up (opens the root→master + # PR + enters the gate). Same accountability set. + "submit_root": frozenset( + { + Requirement.SUBTASKS_TERMINAL, + Requirement.JOURNAL_DECISION, + Requirement.JOURNAL_REFLECT, + Requirement.NOTES_MIN_CHARS, + } + ), # PM complete — adds JOURNAL_REFLECT (parity with submit_up). "complete": frozenset( { @@ -317,6 +331,8 @@ VERBS_WITHOUT_TRACING: frozenset[str] = frozenset( "claim_review", # claim_pr_review's tracing applies on post_pr_review. "claim_pr_review", + # claim_gate_review's tracing applies on pr_pass / pr_fail. + "claim_gate_review", # claim_doc_task's tracing applies on i_documented. "claim_doc_task", # open_pr is a mechanical push+open; preconditions are inline. diff --git a/roboco/mcp/flow_server.py b/roboco/mcp/flow_server.py index 9f4a0e79..b33e262a 100644 --- a/roboco/mcp/flow_server.py +++ b/roboco/mcp/flow_server.py @@ -531,6 +531,26 @@ def submit_up(task_id: str, notes: str) -> dict[str, Any]: return _post(_role_path("submit_up"), {"task_id": task_id, "notes": notes}) +def submit_root(task_id: str, notes: str) -> dict[str, Any]: + """Main PM: open the root→master PR and enter the in-path PR-review gate.""" + return _post(_role_path("submit_root"), {"task_id": task_id, "notes": notes}) + + +def claim_gate_review(task_id: str) -> dict[str, Any]: + """PR reviewer: claim an assembled-PR review task. Returns the diff inline.""" + return _post(_role_path("claim_gate_review"), {"task_id": task_id}) + + +def pr_pass(task_id: str, notes: str) -> dict[str, Any]: + """PR reviewer: pass the assembled PR (→ awaiting_pm_review).""" + return _post(_role_path("pr_pass"), {"task_id": task_id, "notes": notes}) + + +def pr_fail(task_id: str, issues: list[str]) -> dict[str, Any]: + """PR reviewer: fail the assembled PR with concrete issues → needs_revision.""" + return _post(_role_path("pr_fail"), {"task_id": task_id, "issues": issues}) + + # ---------- Tool registry ---------- # # Maps the verb name an agent calls (matches manifest entries and the @@ -560,6 +580,10 @@ _TOOLS: dict[str, Any] = { # pr reviewer (inbound external/fork PRs) "claim_pr_review": claim_pr_review, "post_pr_review": post_pr_review, + # pr reviewer (in-path assembled-PR gate) + "claim_gate_review": claim_gate_review, + "pr_pass": pr_pass, + "pr_fail": pr_fail, # doc "claim_doc_task": claim_doc_task, "i_documented": i_documented, @@ -572,6 +596,7 @@ _TOOLS: dict[str, Any] = { "i_will_plan": i_will_plan, "delegate": delegate, "submit_up": submit_up, + "submit_root": submit_root, # board / main pm "escalate_to_ceo": escalate_to_ceo, } diff --git a/roboco/models/a2a.py b/roboco/models/a2a.py index d1c75597..4f496661 100644 --- a/roboco/models/a2a.py +++ b/roboco/models/a2a.py @@ -412,6 +412,7 @@ def task_status_to_a2a_state(roboco_status: str) -> A2ATaskState: "needs_revision": A2ATaskState.INPUT_REQUIRED, "awaiting_qa": A2ATaskState.WORKING, "awaiting_documentation": A2ATaskState.WORKING, + "awaiting_pr_review": A2ATaskState.WORKING, "awaiting_pm_review": A2ATaskState.WORKING, "awaiting_ceo_approval": A2ATaskState.INPUT_REQUIRED, # Awaiting CEO decision "completed": A2ATaskState.COMPLETED, diff --git a/roboco/models/base.py b/roboco/models/base.py index 718dfc9d..f91d3f9a 100644 --- a/roboco/models/base.py +++ b/roboco/models/base.py @@ -41,6 +41,7 @@ class TaskStatus(StrEnum): NEEDS_REVISION = "needs_revision" AWAITING_QA = "awaiting_qa" AWAITING_DOCUMENTATION = "awaiting_documentation" # Docs + Dev PR in parallel + AWAITING_PR_REVIEW = "awaiting_pr_review" # In-path reviewer gate before PM merge AWAITING_PM_REVIEW = "awaiting_pm_review" # After docs + PR ready AWAITING_CEO_APPROVAL = "awaiting_ceo_approval" # PMs approved, CEO decides COMPLETED = "completed" diff --git a/roboco/runtime/orchestrator.py b/roboco/runtime/orchestrator.py index 9fdb9a8c..fb3ccba3 100644 --- a/roboco/runtime/orchestrator.py +++ b/roboco/runtime/orchestrator.py @@ -127,8 +127,13 @@ AGENT_IMAGES: dict[str, str] = { "auditor": "roboco-agent-pm", # PR Reviewer — read-only reviewer (diff via API, grep, post one # change-request; never runs code). Its own image for parity with the other - # agents; built FROM the base, no extra toolchain. + # agents; built FROM the base, no extra toolchain. The three cell reviewers + # are additional instances of the same role and reuse the same image (as + # be-dev-1/-2 share one dev image) — the in-path gate adds no new image. "pr-reviewer-1": "roboco-agent-pr-reviewer", + "be-pr-reviewer": "roboco-agent-pr-reviewer", + "fe-pr-reviewer": "roboco-agent-pr-reviewer", + "ux-pr-reviewer": "roboco-agent-pr-reviewer", # Intake — persistent Agent-SDK driver, not a one-shot `claude -p`. INTAKE_AGENT_ID: "roboco-agent-prompter", # Secretary — persistent Agent-SDK driver with gated CEO authority. @@ -710,6 +715,11 @@ class AgentOrchestrator: # Tests bypass `__init__` via `__new__` and set _claim_heartbeat_ttl # directly; production never uses _task_svc from __init__. self._claim_heartbeat_ttl: int = settings.stale_claim_reap_seconds + # Short debounce for closure respawn of a recently-paused parent — + # NOT the reaper window. See _is_recently_paused. + self._closure_recently_paused_ttl: int = ( + settings.pm_closure_recently_paused_seconds + ) # Longer threshold before a wedged (ACTIVE-yet-idle) GROK container is # killed + evicted so the reaper can release its task; see # _maybe_kill_wedged_grok. @@ -2566,6 +2576,7 @@ class AgentOrchestrator: role_mismatch: dict[str, str | set[str]] = { "awaiting_qa": "qa", "awaiting_documentation": "documenter", + "awaiting_pr_review": "pr_reviewer", "awaiting_pm_review": {"cell_pm", "main_pm"}, "awaiting_ceo_approval": "ceo", # Dev-owned states — only developer/documenter may claim or @@ -6084,6 +6095,8 @@ Start by: candidates = [f"{prefix}-doc"] elif role == "pm": candidates = [f"{prefix}-pm"] + elif role == "pr_reviewer": + candidates = [f"{prefix}-pr-reviewer"] else: return None @@ -6893,6 +6906,7 @@ Start now: evidence(task_id="{task_id}") ("dev_work", self._dispatch_dev_work(client)), ("qa_work", self._dispatch_qa_work(client)), ("pr_review_work", self._dispatch_pr_review_work(client)), + ("pr_gate_work", self._dispatch_pr_gate_work(client)), ("doc_work", self._dispatch_doc_work(client)), ("pm_review_work", self._dispatch_pm_review_work(client)), ("marketing_work", self._dispatch_marketing_work(client)), @@ -7410,21 +7424,22 @@ Start now: evidence(task_id="{task_id}") async def _dispatch_revision_coordination_roots( self, client: httpx.AsyncClient ) -> None: - """Re-spawn the owning PM for a CEO-rejected coordination root. + """Re-spawn the owning PM for a PM-owned needs_revision task. - A coordination root (team=main_pm, product-linked, no repo) the CEO sends - back lands in ``needs_revision``. The dev dispatcher skips it (not a cell - team) and the closure path only handles paused parents, so without this - it would sit in needs_revision forever — the deadlock. Respawn its PM so - it re-coordinates the revision. Cell/code needs_revision tasks are left - to the dev dispatcher; this handles only coordination roots. + Two cases land a task in ``needs_revision`` owned by a PM rather than a + developer: a CEO-rejected coordination root (team=main_pm, product-linked, + no repo), and a gate-failed assembled task (a cell→root or root→master PR + the in-path reviewer sent back via pr_fail). The dev dispatcher only + spawns developers and the closure path only handles paused parents, so + without this such a task would sit in needs_revision forever — the + deadlock. The PM-ownership filter below scopes this to exactly those: a + leaf dev revision stays owned by its developer and is left to the dev + dispatcher. """ tasks = await self._fetch_tasks(client, "needs_revision") for task in tasks: if self._is_task_handled_this_tick(task.get("id")): continue - if not _is_coordination_task(task): - continue owner = task.get("assigned_to") or task.get("claimed_by") agent_slug = self._resolve_agent_slug(owner) if owner else None if not agent_slug or self._is_agent_active(agent_slug): @@ -7474,24 +7489,35 @@ Start now: evidence(task_id="{task_id}") return None def _is_recently_paused(self, task: dict[str, Any]) -> bool: - """A paused task whose heartbeat is fresher than the stale cutoff. + """A paused task whose heartbeat is fresher than the closure debounce. Closes the ``i_am_idle`` vs closure-respawn race: ``i_am_idle`` auto-pauses in-flight tasks and then sets the agent IDLE. If the dispatcher ticks between those two writes it sees a paused parent and would spawn the closure PM against a session that is mid-shutdown. A fresh ``last_heartbeat_at`` (newer than - ``settings.claim_stale_seconds``) is the signal that the agent - was alive moments ago and a respawn now would race the existing - session. Genuinely-stale paused tasks (or tasks with no heartbeat - recorded) fall through and follow the regular closure path. + ``settings.pm_closure_recently_paused_seconds``) is the signal that + the agent was alive moments ago and a respawn now would race the + existing session. Genuinely-stale paused tasks (or tasks with no + heartbeat recorded) fall through and follow the regular closure path. + + This debounce is deliberately SHORT (a few dispatch ticks). It is + NOT the reaper window (``_claim_heartbeat_ttl`` / + ``stale_claim_reap_seconds``, 600s default and 1800s on the NAS): + binding it there delayed every cell/main closure by up to 10-30 + minutes, because a paused parent's heartbeat reflects when the PM + last *worked*, so a PM that worked right up to idle leaves a fresh + heartbeat. The live-session case is already covered separately by + the ``_is_agent_active`` check in ``_maybe_spawn_pm_closure``. """ if task.get("status") != "paused": return False last_hb = self._coerce_heartbeat(task.get("last_heartbeat_at")) if last_hb is None: return False - cutoff = datetime.now(UTC) - timedelta(seconds=self._claim_heartbeat_ttl) + cutoff = datetime.now(UTC) - timedelta( + seconds=self._closure_recently_paused_ttl + ) return last_hb > cutoff def _closure_pm_for_team(self, team: str | None) -> str: @@ -8031,6 +8057,36 @@ Never `commit`, never write code, never run `git`. PMs coordinate. ) break + async def _dispatch_pr_gate_work(self, client: httpx.AsyncClient) -> None: + """Dispatch in-path PR-review-gate tasks (awaiting_pr_review) to reviewers. + + Routes by level: a cell→root task (team backend/frontend/ux_ui) goes to + that cell's reviewer (be/fe/ux-pr-reviewer); the root→master task goes to + the main reviewer (pr-reviewer-1). The reviewer claims the task itself via + ``claim_gate_review`` (no pre-claim — mirrors the external-PR dispatcher); + the ``is_agent_active`` guard + one-reviewer-per-cell prevent a + double-spawn, and ``spawned`` bounds each reviewer to one task per tick. + """ + tasks = await self._fetch_tasks(client, "awaiting_pr_review") + spawned: set[str] = set() + for task in tasks: + if self._is_task_handled_this_tick(task.get("id")): + continue + team = task.get("team") + if team in ("backend", "frontend", "ux_ui"): + reviewer = self._select_agent_for_cell(team, "pr_reviewer") + else: + reviewer = "pr-reviewer-1" + if not reviewer or reviewer in spawned or self._is_agent_active(reviewer): + continue + spawned.add(reviewer) + await self.spawn_agent( + agent_id=reviewer, + task_id=task["id"], + initial_prompt=self._build_pr_gate_prompt(task), + git_context=self._task_git_context(task), + ) + async def _dispatch_doc_work(self, client: httpx.AsyncClient) -> None: """ Dispatch documentation + developer work during the parallel @@ -9062,6 +9118,52 @@ contributor's code. Do not push to their fork. You never merge. — posts ONE complete review to the PR and finishes the task. Use event="APPROVE" only if the PR is genuinely ready as-is. 5. i_am_idle() when done. +""" + + def _build_pr_gate_prompt(self, task: dict[str, Any]) -> str: + """Build the prompt for a reviewer on an in-path assembled-PR gate task.""" + task_id = task.get("id", "unknown") + title = task.get("title", "Untitled") + team = task.get("team", "unknown") + pr_number = task.get("pr_number", "?") + pr_url = task.get("pr_url", "") + criteria = task.get("acceptance_criteria") or [] + crit_block = ( + "\n".join(f" - {c}" for c in criteria) if criteria else " (none recorded)" + ) + return f"""\ +An assembled pull request is ready for review before the PM merges it. + +TASK ID: {task_id} +TITLE: {title} +TEAM: {team} +ASSEMBLED PR: #{pr_number} {pr_url} + +== WHAT YOU ARE REVIEWING == +This is the gate BEFORE the merge — the merge-level review QA does not do. You +review the ASSEMBLED diff (the whole cell→root or root→master PR), not a single +leaf, against the original intent and the contract between cells. The bug class +this catches lives in the seam (e.g. a frontend that sends a string where the +backend requires a UUID) — invisible to any single-cell QA. Read-only: you +never push or merge. + +ACCEPTANCE CRITERIA (the assembled work must satisfy ALL of these): +{crit_block} + +== REVIEW WORKFLOW == + +1. claim_gate_review(task_id="{task_id}") + — claims the review; returns the assembled diff + acceptance criteria inline. +2. Review the diff against the objective + every acceptance criterion + the + FE↔BE / cross-cell contract. Do not lose scope: the assembled thing must + actually do what was asked. +3. note(scope="learning", task_id="{task_id}", text="") + — required before you pass or fail. +4a. pr_pass(task_id="{task_id}", notes="") + — if correct and complete: moves it to the PM to merge. +4b. pr_fail(task_id="{task_id}", issues=["", ...]) + — if anything is wrong: sends it back to the PM for revision, like a QA fail. +5. i_am_idle() when done. """ def _build_doc_prompt(self, task: dict[str, Any]) -> str: diff --git a/roboco/seeds/initial_data.py b/roboco/seeds/initial_data.py index 66423756..c743f1f7 100644 --- a/roboco/seeds/initial_data.py +++ b/roboco/seeds/initial_data.py @@ -110,6 +110,9 @@ _AGENT_PRESENTATION: dict[str, dict[str, Any]] = { "intake-1": {"name": "Intake"}, "secretary-1": {"name": "Secretary"}, "pr-reviewer-1": {"name": "PR Reviewer"}, + "be-pr-reviewer": {"name": "Backend PR Reviewer"}, + "fe-pr-reviewer": {"name": "Frontend PR Reviewer"}, + "ux-pr-reviewer": {"name": "UX/UI PR Reviewer"}, } diff --git a/roboco/services/gateway/choreographer/__init__.py b/roboco/services/gateway/choreographer/__init__.py index 8a41b4c8..e8c2bb88 100644 --- a/roboco/services/gateway/choreographer/__init__.py +++ b/roboco/services/gateway/choreographer/__init__.py @@ -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. diff --git a/roboco/services/gateway/choreographer/_impl.py b/roboco/services/gateway/choreographer/_impl.py index 7ebc0768..86ab43b9 100644 --- a/roboco/services/gateway/choreographer/_impl.py +++ b/roboco/services/gateway/choreographer/_impl.py @@ -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 diff --git a/roboco/services/gateway/choreographer/_protocol.py b/roboco/services/gateway/choreographer/_protocol.py index 0d6a6909..a7b7b6fa 100644 --- a/roboco/services/gateway/choreographer/_protocol.py +++ b/roboco/services/gateway/choreographer/_protocol.py @@ -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, diff --git a/roboco/services/gateway/choreographer/_verb_runner.py b/roboco/services/gateway/choreographer/_verb_runner.py index bb0707e7..ddf541ad 100644 --- a/roboco/services/gateway/choreographer/_verb_runner.py +++ b/roboco/services/gateway/choreographer/_verb_runner.py @@ -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, } diff --git a/roboco/services/gateway/choreographer/pr_gate.py b/roboco/services/gateway/choreographer/pr_gate.py new file mode 100644 index 00000000..fe98a75a --- /dev/null +++ b/roboco/services/gateway/choreographer/pr_gate.py @@ -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=['', ...]", + 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, + } diff --git a/roboco/services/product.py b/roboco/services/product.py index 59af0119..9027a184 100644 --- a/roboco/services/product.py +++ b/roboco/services/product.py @@ -133,7 +133,14 @@ class ProductService(BaseService): synchronous lazy load and raise MissingGreenlet). """ await self.session.refresh(product, ["cells"]) - product.cells.clear() # delete-orphan removes the old mapping rows + product.cells.clear() # delete-orphan marks the old mapping rows for delete + # Flush the DELETEs before appending the new rows. Within a single flush + # SQLAlchemy orders INSERTs before DELETEs for the same table, so without + # this the new (product_id, team) rows collide with the not-yet-deleted + # old ones on uq_product_projects_product_team (a 409 on any re-mapping of + # a team that already has a project). Flushing here issues the DELETEs + # first; the appended rows insert cleanly on the next flush. + await self.session.flush() for mapping in cells: product.cells.append( ProductProjectTable(team=mapping.team, project_id=mapping.project_id) diff --git a/roboco/services/task.py b/roboco/services/task.py index 16cc699b..6d64397c 100644 --- a/roboco/services/task.py +++ b/roboco/services/task.py @@ -6688,6 +6688,143 @@ class TaskService(BaseService): await self.session.flush() return await self.fail_qa(task_id, notes=notes, agent_role="qa") + async def _revision_pm_for_task(self, task: TaskTable) -> AgentTable | None: + """The PM who owns an assembled task's revision. + + Cell PM for a cell team (the cell→root level), else the Main PM (the + root→master level). Used by pr_fail so the in-path reviewer's rejection + lands on whoever assembles the work. + """ + try: + team = Team(task.team) if task.team else None + except ValueError: + team = None + if team in (Team.BACKEND, Team.FRONTEND, Team.UX_UI): + return await self.cell_pm_for_team(team) + return await self.main_pm_agent() + + async def pr_gate_claim( + self, reviewer_agent_id: UUID, task_id: UUID + ) -> TaskTable | None: + """Reviewer claims an awaiting_pr_review task (no state transition). + + The in-path PR-review gate mirrors QA's claim_review: status stays at + awaiting_pr_review while the reviewer inspects the assembled diff; + pr_pass / pr_fail perform the transition. + """ + return await self._qa_or_doc_claim( + reviewer_agent_id, task_id, TaskStatus.AWAITING_PR_REVIEW + ) + + async def submit_for_review( + self, agent_id: UUID, task_id: UUID, notes: str + ) -> TaskTable | None: + """Enter the in-path PR-review gate: in_progress -> awaiting_pr_review. + + Composed by the cell PM's submit_up (cell→root PR) and the main PM's + submit_root (root→master PR); the assembled PR is already open by the + time this runs. Mirrors submit_pm_review but targets the gate so a + reviewer signs off before the PM merge. + """ + if notes: + await self.add_progress(task_id, agent_id, notes) + task = await self.get(task_id) + if task is None or task.status != TaskStatus.IN_PROGRESS: + return None + agent = await self.agent_for(agent_id) + agent_role = agent.role if agent else "cell_pm" + self._validate_and_set_status(task, TaskStatus.AWAITING_PR_REVIEW, agent_role) + await self.session.flush() + return task + + async def pr_pass( + self, reviewer_agent_id: UUID, task_id: UUID, notes: str + ) -> TaskTable | None: + """Reviewer passes the gate: awaiting_pr_review -> awaiting_pm_review. + + Clears the claim so the PM-closure dispatcher routes the now-unassigned + task to the owning PM to merge — the same path a leaf takes after + docs_complete. Mirrors qa_pass. + """ + task = await self.get(task_id) + if task is None or task.status != TaskStatus.AWAITING_PR_REVIEW: + return None + if ( + task.claimed_by is not None + and to_python_uuid(task.claimed_by) != reviewer_agent_id + ): + self.log.warning( + "pr_pass actor mismatch", + task_id=str(task_id), + reviewer_agent_id=str(reviewer_agent_id), + claimed_by=str(task.claimed_by), + ) + captured = to_python_uuid(task.claimed_by) + if notes: + task.qa_notes = _append_capped(task.qa_notes, "[PR REVIEW]\n" + notes) + task.assigned_to = None + task.claimed_by = None + task.active_claimant_id = cast("Any", None) + self._validate_and_set_status( + task, + TaskStatus.AWAITING_PM_REVIEW, + "pr_reviewer", + audit_agent_id=captured, + ) + await self.session.flush() + self.log.info("Assembled PR passed review", task_id=str(task_id)) + return task + + async def pr_fail( + self, + reviewer_agent_id: UUID, + task_id: UUID, + notes: str, + issues: list[str], + ) -> TaskTable | None: + """Reviewer fails the gate: awaiting_pr_review -> needs_revision. + + Appends the concrete issues for the PM's revision and clears the claim; + the revision dispatcher routes the assembled task back to its PM. + Mirrors qa_fail — an assembled PR is the PM's to revise, so there is no + original-developer reassign; routing is handled at dispatch. + """ + task = await self.get(task_id) + if task is None or task.status != TaskStatus.AWAITING_PR_REVIEW: + return None + if ( + task.claimed_by is not None + and to_python_uuid(task.claimed_by) != reviewer_agent_id + ): + self.log.warning( + "pr_fail actor mismatch", + task_id=str(task_id), + reviewer_agent_id=str(reviewer_agent_id), + claimed_by=str(task.claimed_by), + ) + captured = to_python_uuid(task.claimed_by) + if issues: + issue_block = "[PR REVIEW ISSUES]\n" + "\n".join(f"- {i}" for i in issues) + task.dev_notes = _append_capped(task.dev_notes, issue_block) + if notes: + task.qa_notes = _append_capped(task.qa_notes, "[PR REVIEW]\n" + notes) + # Hand the failed assembled task to its PM to revise (cell PM for a cell + # team, Main PM for the root); the revision dispatcher re-spawns whoever + # owns a needs_revision task. Fall back to unassigned if no PM resolves. + pm = await self._revision_pm_for_task(task) + task.assigned_to = cast("Any", pm.id) if pm is not None else None + task.claimed_by = cast("Any", pm.id) if pm is not None else None + task.active_claimant_id = cast("Any", None) + self._validate_and_set_status( + task, + TaskStatus.NEEDS_REVISION, + "pr_reviewer", + audit_agent_id=captured, + ) + await self.session.flush() + self.log.info("Assembled PR failed review", task_id=str(task_id)) + return task + async def unblock_with_restore( self, pm_agent_id: UUID, diff --git a/tests/foundation/test_identity.py b/tests/foundation/test_identity.py index ef2df6e0..c3d5a892 100644 --- a/tests/foundation/test_identity.py +++ b/tests/foundation/test_identity.py @@ -86,6 +86,9 @@ def test_agents_catalog_has_all_seed_slugs() -> None: "intake-1", "secretary-1", "pr-reviewer-1", + "be-pr-reviewer", + "fe-pr-reviewer", + "ux-pr-reviewer", } actual = set(identity.AGENTS.keys()) assert actual == expected_slugs, f"agent catalog drift: {actual ^ expected_slugs}" @@ -207,7 +210,9 @@ def test_slugs_for_role_system_returns_singleton() -> None: def test_slugs_for_team_backend() -> None: backend = identity.slugs_for_team(identity.Team.BACKEND) - assert backend == frozenset({"be-dev-1", "be-dev-2", "be-qa", "be-pm", "be-doc"}) + assert backend == frozenset( + {"be-dev-1", "be-dev-2", "be-qa", "be-pm", "be-doc", "be-pr-reviewer"} + ) def test_slugs_for_team_marketing_is_empty() -> None: diff --git a/tests/foundation/test_lifecycle_spec.py b/tests/foundation/test_lifecycle_spec.py index c4d6a929..a970281b 100644 --- a/tests/foundation/test_lifecycle_spec.py +++ b/tests/foundation/test_lifecycle_spec.py @@ -55,6 +55,7 @@ def test_status_enum_has_every_pre_gateway_status() -> None: "awaiting_qa", "needs_revision", "awaiting_documentation", + "awaiting_pr_review", "awaiting_pm_review", "awaiting_ceo_approval", "completed", diff --git a/tests/foundation/test_pr_review_gate.py b/tests/foundation/test_pr_review_gate.py new file mode 100644 index 00000000..100df20b --- /dev/null +++ b/tests/foundation/test_pr_review_gate.py @@ -0,0 +1,111 @@ +"""Tier 1 — in-path PR-review gate spec self-tests. Fast (no DB, no network). + +Covers the lifecycle surface of the assembled-PR review gate: the new +awaiting_pr_review status, the submit_for_review / pr_pass / pr_fail actions, +the reviewer verbs, and the structural invariant that a gated task cannot reach +the PM-merge stage without a pr_pass. +""" + +from __future__ import annotations + +from types import SimpleNamespace +from typing import Any + +from roboco.foundation.policy import lifecycle as spec +from roboco.foundation.policy.lifecycle import Role, Status + + +def _task(status: str, team: str = "backend", **kw: Any) -> SimpleNamespace: + return SimpleNamespace(status=status, team=team, **kw) + + +def test_submit_for_review_enters_the_gate() -> None: + assert ( + spec.status_after("submit_for_review", Status.IN_PROGRESS) + == Status.AWAITING_PR_REVIEW + ) + + +def test_pr_pass_and_pr_fail_targets() -> None: + assert ( + spec.status_after("pr_pass", Status.AWAITING_PR_REVIEW) + == Status.AWAITING_PM_REVIEW + ) + assert ( + spec.status_after("pr_fail", Status.AWAITING_PR_REVIEW) == Status.NEEDS_REVISION + ) + + +def test_reviewer_can_pass_and_fail_from_the_gate() -> None: + t = _task("awaiting_pr_review") + assert spec.can_invoke_intent(Role.PR_REVIEWER, "pr_pass", t).allowed + assert spec.can_invoke_intent(Role.PR_REVIEWER, "pr_fail", t).allowed + + +def test_non_reviewer_roles_cannot_pass_the_gate() -> None: + t = _task("awaiting_pr_review") + for role in (Role.DEVELOPER, Role.QA, Role.CELL_PM, Role.MAIN_PM): + d = spec.can_invoke_intent(role, "pr_pass", t) + assert not d.allowed + assert d.rejection_kind == "not_authorized" + + +def test_reviewer_cannot_pass_from_a_non_gate_state() -> None: + d = spec.can_invoke_intent(Role.PR_REVIEWER, "pr_pass", _task("awaiting_pm_review")) + assert not d.allowed + assert d.rejection_kind == "invalid_state" + + +def test_reviewer_claims_the_gate_without_transition() -> None: + assert spec.can_invoke_intent( + Role.PR_REVIEWER, "claim_gate_review", _task("awaiting_pr_review") + ).allowed + + +def test_reviewer_cannot_claim_gate_from_another_roles_state() -> None: + d = spec.can_invoke_intent( + Role.PR_REVIEWER, "claim_gate_review", _task("awaiting_qa") + ) + assert not d.allowed + + +def test_submit_root_is_main_pm_only_and_opens_a_pr() -> None: + iv = spec._INTENT_VERBS["submit_root"] + assert iv.allowed_roles == frozenset({Role.MAIN_PM}) + assert iv.composes == ("submit_for_review",) + assert iv.pre_side_effects == ("create_root_pr",) + + +def test_gate_cannot_skip_straight_to_terminal_or_ceo() -> None: + targets = spec.STATUS_GRAPH[Status.AWAITING_PR_REVIEW] + assert Status.COMPLETED not in targets + assert Status.AWAITING_CEO_APPROVAL not in targets + # The only forward exits are pr_pass (to PM review) and pr_fail (back). + assert Status.AWAITING_PM_REVIEW in targets + assert Status.NEEDS_REVISION in targets + + +def test_pr_pass_is_the_sole_action_out_of_the_gate_into_pm_review() -> None: + movers = [ + name + for name, a in spec._ATOMIC_ACTIONS.items() + if a.target_status == Status.AWAITING_PM_REVIEW + and Status.AWAITING_PR_REVIEW in a.source_statuses + ] + assert movers == ["pr_pass"] + + +def test_gate_actions_block_self_review() -> None: + assert spec._ATOMIC_ACTIONS["pr_pass"].self_review_block is True + assert spec._ATOMIC_ACTIONS["pr_fail"].self_review_block is True + + +def test_delegate_hint_names_the_role_correct_bubble_up_verb() -> None: + """After delegating, a PM is steered to the right bubble-up verb proactively: + a root (no parent) → submit_root; a cell parent → submit_up. Prevents the + guess-the-verb flail that deadlocked root closure.""" + hint = spec._INTENT_VERBS["delegate"].next_hint + root = _task("in_progress", parent_task_id=None) + cell_parent = _task("in_progress", parent_task_id="parent-id") + assert "submit_root" in hint(root) + assert "submit_up" in hint(cell_parent) diff --git a/tests/integration/test_lifecycle_real_db.py b/tests/integration/test_lifecycle_real_db.py index 78a11561..d4a31d2d 100644 --- a/tests/integration/test_lifecycle_real_db.py +++ b/tests/integration/test_lifecycle_real_db.py @@ -680,6 +680,101 @@ async def test_pm_complete_simple_task( assert str(final.status) == Status.COMPLETED.value +async def _seed_reviewer(db_session: AsyncSession) -> AgentTable: + """Add + flush a backend in-path PR-review-gate reviewer for the gate tests. + + Flushed here so a later ``task.assigned_to = reviewer.id`` update can't race + the reviewer INSERT in the same unit-of-work and trip the FK constraint. + """ + reviewer = AgentTable( + id=uuid4(), + name="BE PR Reviewer", + slug="be-pr-reviewer", + role=AgentRole.PR_REVIEWER, + team=Team.BACKEND, + status=AgentStatus.ACTIVE, + model_config={}, + system_prompt="reviewer", + capabilities=["review"], + permissions={}, + metrics={}, + ) + db_session.add(reviewer) + await db_session.flush() + return reviewer + + +@pytest.mark.asyncio +async def test_pr_review_gate_pass_path( + db_session: AsyncSession, lifecycle_setup: dict[str, Any] +) -> None: + """in_progress → submit_for_review → awaiting_pr_review → pr_gate_claim → + pr_pass → awaiting_pm_review. + + Drives the new TaskService transitions through the real enforcement layer + (validate_task_transition + validate_git_requirements) and the DB — the + layers the mocked unit tests can't exercise. + """ + task = lifecycle_setup["task"] + cell_pm_agent = lifecycle_setup["cell_pm_agent"] + reviewer = await _seed_reviewer(db_session) + task.status = TaskStatus.IN_PROGRESS + task.pr_number = _PR_NUMBER + task.pr_url = _PR_URL + task.pr_created = True + task.assigned_to = cell_pm_agent.id + task.claimed_by = cell_pm_agent.id + await db_session.flush() + + svc = TaskService(db_session) + reviewer_id = UUID(str(reviewer.id)) + + entered = await svc.submit_for_review( + cell_pm_agent.id, task.id, notes="cell assembled; entering review" + ) + assert entered is not None + assert str(entered.status) == Status.AWAITING_PR_REVIEW.value + + claimed = await svc.pr_gate_claim(reviewer_id, task.id) + assert claimed is not None + assert str(claimed.status) == Status.AWAITING_PR_REVIEW.value + assert claimed.assigned_to == reviewer.id + + passed = await svc.pr_pass(reviewer_id, task.id, notes="integration verified") + assert passed is not None + assert str(passed.status) == Status.AWAITING_PM_REVIEW.value + assert passed.assigned_to is None # cleared so the PM-closure dispatch routes + + final = await svc.get(task.id) + assert final is not None + assert str(final.status) == Status.AWAITING_PM_REVIEW.value + + +@pytest.mark.asyncio +async def test_pr_review_gate_fail_path( + db_session: AsyncSession, lifecycle_setup: dict[str, Any] +) -> None: + """awaiting_pr_review → pr_fail → needs_revision, with issues recorded for + the PM's revision.""" + task = lifecycle_setup["task"] + reviewer = await _seed_reviewer(db_session) + task.status = TaskStatus.AWAITING_PR_REVIEW + task.assigned_to = reviewer.id + task.claimed_by = reviewer.id + await db_session.flush() + + svc = TaskService(db_session) + failed = await svc.pr_fail( + UUID(str(reviewer.id)), + task.id, + notes="integration seam is broken", + issues=["FE sends task_id as a string where the BE requires a UUID"], + ) + assert failed is not None + assert str(failed.status) == Status.NEEDS_REVISION.value + assert "string where the BE requires a UUID" in (failed.dev_notes or "") + + # --------------------------------------------------------------------------- # 6. PM escalate: awaiting_pm_review → awaiting_ceo_approval # --------------------------------------------------------------------------- diff --git a/tests/integration/test_product_service.py b/tests/integration/test_product_service.py index 09f8367b..265aef29 100644 --- a/tests/integration/test_product_service.py +++ b/tests/integration/test_product_service.py @@ -153,6 +153,50 @@ async def test_duplicate_slug_conflicts(product_setup: dict) -> None: ) +@pytest.mark.asyncio +async def test_update_remaps_existing_cells_without_unique_collision( + product_setup: dict, +) -> None: + """Re-mapping cells that already have a project must not collide. + + ``_replace_cells`` deletes the old (product_id, team) rows and inserts the + new ones. Within a single flush SQLAlchemy orders INSERTs before DELETEs, so + without flushing the deletes first the new rows hit + ``uq_product_projects_product_team`` — the 409 seen when editing a product's + projects. Regression for that ordering bug. + """ + svc = product_setup["svc"] + projects = product_setup["projects"] + product = await svc.create( + ProductCreate( + name="Remap", + slug=f"remap-{uuid4().hex[:6]}", + cells=[ + ProductCellMapping(team=c, project_id=projects[c].id) + for c in (Team.BACKEND, Team.FRONTEND, Team.UX_UI) + ], + ), + created_by=product_setup["creator"], + ) + # Every team already has a mapping; re-map all three to different projects. + swapped = { + Team.BACKEND: projects[Team.FRONTEND].id, + Team.FRONTEND: projects[Team.UX_UI].id, + Team.UX_UI: projects[Team.BACKEND].id, + } + updated = await svc.update( + product.id, + ProductUpdate( + cells=[ + ProductCellMapping(team=t, project_id=pid) for t, pid in swapped.items() + ] + ), + ) + assert updated is not None + for team, pid in swapped.items(): + assert await svc.project_for(product.id, team) == pid + + @pytest.mark.asyncio async def test_update_replaces_cells(product_setup: dict) -> None: svc = product_setup["svc"] diff --git a/tests/unit/gateway/test_choreographer_pm.py b/tests/unit/gateway/test_choreographer_pm.py index 27e22109..807cfa2c 100644 --- a/tests/unit/gateway/test_choreographer_pm.py +++ b/tests/unit/gateway/test_choreographer_pm.py @@ -415,14 +415,49 @@ async def test_cell_pm_complete_not_assigned_returns_not_authorized() -> None: @pytest.mark.asyncio -async def test_main_pm_complete_opens_master_pr_and_escalates() -> None: +async def test_cell_pm_complete_in_progress_steers_to_submit_up() -> None: + """Mirror of the main-PM submit_root steer: a cell task still in_progress + must enter the gate via submit_up first. The rejection must NAME submit_up + so the cell PM isn't left guessing the verb (the gap that deadlocked the + main PM on submit_root).""" + pm_id = uuid4() + task_id = uuid4() + t = MagicMock( + id=task_id, + status="in_progress", + assigned_to=pm_id, + parent_task_id=uuid4(), + branch_name="feature/backend/parent123", + ) + task_svc = AsyncMock() + task_svc.get.return_value = t + task_svc.all_subtasks_terminal.return_value = True + journal_svc = AsyncMock() + journal_svc.has_decision_for_task.return_value = True + journal_svc.latest_decision_at.return_value = datetime.now(UTC) + journal_svc.has_reflect_for_task.return_value = True + deps = _make_deps(task=task_svc, journal=journal_svc) + c = Choreographer(deps) + + env = await c.cell_pm_complete( + pm_id, task_id, notes="cell scope assembled; ready to bubble up" + ) + assert env.error is not None + assert "submit_up" in (env.remediate or "") + + +@pytest.mark.asyncio +async def test_main_pm_complete_escalates_code_root_without_reopening_pr() -> None: + """A code root reaches main_pm_complete already in awaiting_pm_review — + submit_root opened the root→master PR and the main reviewer pr_passed it — + so complete just escalates to the CEO and does NOT reopen the PR.""" main_pm_id = uuid4() root_task_id = uuid4() t = MagicMock( id=root_task_id, status="awaiting_pm_review", assigned_to=main_pm_id, - pr_number=None, + pr_number=99, branch_name="feature/backend/root123", parent_task_id=None, team="backend", @@ -433,7 +468,6 @@ async def test_main_pm_complete_opens_master_pr_and_escalates() -> None: task_svc.escalate_to_ceo.return_value = after task_svc.all_subtasks_terminal.return_value = True git_svc = AsyncMock() - git_svc.create_pr.return_value = {"pr_number": 99, "pr_url": "https://x/y/pull/99"} journal_svc = AsyncMock() journal_svc.has_decision_for_task.return_value = True journal_svc.latest_decision_at.return_value = datetime.now(UTC) @@ -446,19 +480,52 @@ async def test_main_pm_complete_opens_master_pr_and_escalates() -> None: ) assert env.error is None assert env.status == "awaiting_ceo_approval" - git_svc.create_pr.assert_awaited_once_with( - "feature/backend/root123", - parent="master", - is_root_pr=True, - ) + git_svc.create_pr.assert_not_awaited() task_svc.escalate_to_ceo.assert_awaited_once() @pytest.mark.asyncio -async def test_main_pm_complete_advances_in_progress_root_to_ceo() -> None: - """#183: a root resumed to in_progress (subtasks all done) has no submit_up - to reach awaiting_pm_review. main_pm_complete opens the root→master PR, - walks the root through awaiting_pm_review, then escalates to CEO.""" +async def test_main_pm_complete_rejects_in_progress_code_root_toward_submit_root() -> ( + None +): + """A code root must pass the in-path gate first. main_pm_complete rejects it + while still in_progress and points the Main PM at submit_root.""" + main_pm_id = uuid4() + root_task_id = uuid4() + t = MagicMock( + id=root_task_id, + status="in_progress", + assigned_to=main_pm_id, + pr_number=None, + branch_name="feature/main_pm/root123", + parent_task_id=None, + team="main_pm", + ) + task_svc = AsyncMock() + task_svc.get.return_value = t + task_svc.all_subtasks_terminal.return_value = True + git_svc = AsyncMock() + journal_svc = AsyncMock() + journal_svc.has_decision_for_task.return_value = True + journal_svc.latest_decision_at.return_value = datetime.now(UTC) + journal_svc.has_reflect_for_task.return_value = True + deps = _make_deps(task=task_svc, git=git_svc, journal=journal_svc) + c = Choreographer(deps) + + env = await c.main_pm_complete( + main_pm_id, root_task_id, notes="root scope reviewed; ready for CEO sign-off" + ) + assert env.error is not None + assert "submit_root" in (env.remediate or "") + task_svc.escalate_to_ceo.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_submit_root_accepts_main_pm_and_enters_the_gate() -> None: + """submit_root is the Main PM's entry to the in-path gate. It reuses the + cell PM's _submit_up_guard for ownership/state, so the ownership guard must + NOT hardcode-reject main_pm — otherwise submit_root and complete point at + each other and a code root can never close (the circular-rejection bug).""" main_pm_id = uuid4() root_task_id = uuid4() in_prog = MagicMock( @@ -470,6 +537,48 @@ async def test_main_pm_complete_advances_in_progress_root_to_ceo() -> None: parent_task_id=None, team="main_pm", ) + gated = MagicMock(**{**in_prog.__dict__, "status": "awaiting_pr_review"}) + task_svc = AsyncMock() + task_svc.get.return_value = in_prog + task_svc.submit_for_review.return_value = gated + task_svc.all_subtasks_terminal.return_value = True + task_svc.uncovered_parent_acceptance_criteria.return_value = [] + task_svc.agent_for.return_value = MagicMock(role="main_pm", team="main_pm") + task_svc.session.begin_nested = MagicMock( + return_value=MagicMock(__aenter__=AsyncMock(), __aexit__=AsyncMock()) + ) + git_svc = AsyncMock() + journal_svc = AsyncMock() + journal_svc.has_decision_for_task.return_value = True + journal_svc.latest_decision_at.return_value = datetime.now(UTC) + journal_svc.has_reflect_for_task.return_value = True + deps = _make_deps(task=task_svc, git=git_svc, journal=journal_svc) + c = Choreographer(deps) + + env = await c.submit_root( + main_pm_id, root_task_id, notes="root scope assembled; opening root→master PR" + ) + assert env.error is None, env.as_dict() + assert env.status == "awaiting_pr_review" + task_svc.submit_for_review.assert_awaited_once() + + +@pytest.mark.asyncio +async def test_main_pm_complete_walks_branchless_coordination_root_to_ceo() -> None: + """A branchless coordination root (product fan-out, no repo/PR) skips the + in-path gate: main_pm_complete walks it in_progress→awaiting_pm_review and + escalates to the CEO. No root→master PR is created (it has no branch).""" + main_pm_id = uuid4() + root_task_id = uuid4() + in_prog = MagicMock( + id=root_task_id, + status="in_progress", + assigned_to=main_pm_id, + pr_number=None, + branch_name=None, + parent_task_id=None, + team="main_pm", + ) awaiting = MagicMock(**{**in_prog.__dict__, "status": "awaiting_pm_review"}) after = MagicMock(**{**in_prog.__dict__, "status": "awaiting_ceo_approval"}) task_svc = AsyncMock() @@ -478,7 +587,6 @@ async def test_main_pm_complete_advances_in_progress_root_to_ceo() -> None: task_svc.escalate_to_ceo.return_value = after task_svc.all_subtasks_terminal.return_value = True git_svc = AsyncMock() - git_svc.create_pr.return_value = {"pr_number": 99, "pr_url": "https://x/y/pull/99"} journal_svc = AsyncMock() journal_svc.has_decision_for_task.return_value = True journal_svc.latest_decision_at.return_value = datetime.now(UTC) @@ -487,16 +595,11 @@ async def test_main_pm_complete_advances_in_progress_root_to_ceo() -> None: c = Choreographer(deps) env = await c.main_pm_complete( - main_pm_id, root_task_id, notes="root scope reviewed; ready for CEO sign-off" + main_pm_id, root_task_id, notes="coordination root reviewed; ready for CEO" ) assert env.error is None assert env.status == "awaiting_ceo_approval" - git_svc.create_pr.assert_awaited_once_with( - "feature/main_pm/root123", - parent="master", - is_root_pr=True, - ) - # #183: the in_progress→awaiting_pm_review hop must run before escalation. + git_svc.create_pr.assert_not_awaited() task_svc.submit_pm_review.assert_awaited_once() task_svc.escalate_to_ceo.assert_awaited_once() diff --git a/tests/unit/gateway/test_choreographer_pm_extras.py b/tests/unit/gateway/test_choreographer_pm_extras.py index 24e0c0e1..0a858370 100644 --- a/tests/unit/gateway/test_choreographer_pm_extras.py +++ b/tests/unit/gateway/test_choreographer_pm_extras.py @@ -883,10 +883,11 @@ async def test_delegate_invalid_team_enum_rejected() -> None: @pytest.mark.asyncio async def test_submit_up_opens_pr_and_keeps_cell_pm_assignment() -> None: - """#182: submit_up opens the cell→root PR and moves the cell task to - awaiting_pm_review, but does NOT hand it off to Main PM. The cell PM - owns cell completion (it is respawned to `complete` the task), so the - assignment stays put — no reassign.""" + """submit_up opens the cell→root PR and moves the cell task into the + in-path PR-review gate (awaiting_pr_review), but does NOT hand it off to + Main PM. The cell PM owns cell completion (it is respawned to `complete` + after the cell reviewer pr_passes), so the assignment stays put — no + reassign.""" pm_id = uuid4() task_id = uuid4() t = MagicMock( @@ -899,7 +900,7 @@ async def test_submit_up_opens_pr_and_keeps_cell_pm_assignment() -> None: ) after = MagicMock( id=task_id, - status="awaiting_pm_review", + status="awaiting_pr_review", assigned_to=pm_id, parent_task_id=None, branch_name="feature/backend/abc123", @@ -909,7 +910,7 @@ async def test_submit_up_opens_pr_and_keeps_cell_pm_assignment() -> None: task_svc.get.return_value = t task_svc.agent_for.return_value = MagicMock(role="cell_pm", team="backend") task_svc.all_subtasks_terminal.return_value = True - task_svc.submit_pm_review.return_value = after + task_svc.submit_for_review.return_value = after git_svc = AsyncMock() git_svc.create_pr.return_value = {"pr_number": 12, "pr_url": "x"} journal_svc = AsyncMock() @@ -919,10 +920,10 @@ async def test_submit_up_opens_pr_and_keeps_cell_pm_assignment() -> None: c = Choreographer(deps) env = await c.submit_up( - pm_id, task_id, notes="cell completed all subtasks; ready for main pm" + pm_id, task_id, notes="cell completed all subtasks; ready for review" ) assert env.error is None - assert env.status == "awaiting_pm_review" + assert env.status == "awaiting_pr_review" git_svc.create_pr.assert_awaited_once() task_svc.reassign.assert_not_called() diff --git a/tests/unit/gateway/test_pr_gate_posts_review.py b/tests/unit/gateway/test_pr_gate_posts_review.py new file mode 100644 index 00000000..7929ede9 --- /dev/null +++ b/tests/unit/gateway/test_pr_gate_posts_review.py @@ -0,0 +1,105 @@ +"""The in-path gate posts its verdict on the assembled PR. + +pr_pass / pr_fail leave a review on the PR the PM (or CEO) eventually merges, +so the gate decision is visible on the PR itself — never a silent transition. +The root→master PR only ever gets a COMMENT (only the CEO acts on master). +""" + +from __future__ import annotations + +from typing import Any +from unittest.mock import AsyncMock, MagicMock +from uuid import uuid4 + +import pytest +from roboco.services.gateway.choreographer import Choreographer, ChoreographerDeps + + +def _make_choreographer(git: AsyncMock) -> Choreographer: + base: dict[str, Any] = { + "task": AsyncMock(), + "work_session": AsyncMock(), + "git": git, + "a2a": AsyncMock(), + "journal": AsyncMock(), + "audit": AsyncMock(), + "evidence_repo": AsyncMock(), + } + c = Choreographer(ChoreographerDeps(**base)) + # _project_slug_for hits the project service; stub it for the unit. + c._project_slug_for = AsyncMock(return_value="proj") # type: ignore[method-assign] + return c + + +def _task(*, parent_task_id: Any, pr_number: int | None = 77) -> MagicMock: + return MagicMock(id=uuid4(), parent_task_id=parent_task_id, pr_number=pr_number) + + +@pytest.mark.asyncio +async def test_cell_pass_posts_approve() -> None: + git = AsyncMock() + c = _make_choreographer(git) + await c._post_gate_review_to_pr( + _task(parent_task_id=uuid4()), "pr_pass", "be-pr-reviewer", "looks good" + ) + git.post_pr_review.assert_awaited_once() + _slug, _pr, body = git.post_pr_review.await_args.args + assert git.post_pr_review.await_args.kwargs["event"] == "APPROVE" + assert "PASSED" in body + assert "be-pr-reviewer" in body + + +@pytest.mark.asyncio +async def test_cell_fail_posts_request_changes() -> None: + git = AsyncMock() + c = _make_choreographer(git) + await c._post_gate_review_to_pr( + _task(parent_task_id=uuid4()), + "pr_fail", + "be-pr-reviewer", + "Issues:\n- seam mismatch", + ) + assert git.post_pr_review.await_args.kwargs["event"] == "REQUEST_CHANGES" + assert "CHANGES REQUESTED" in git.post_pr_review.await_args.args[2] + + +@pytest.mark.asyncio +async def test_root_pass_posts_comment_never_approve() -> None: + """The root→master PR must never get an APPROVE — only the CEO merges it.""" + git = AsyncMock() + c = _make_choreographer(git) + await c._post_gate_review_to_pr( + _task(parent_task_id=None), "pr_pass", "pr-reviewer-1", "root scope clean" + ) + assert git.post_pr_review.await_args.kwargs["event"] == "COMMENT" + assert "master" in git.post_pr_review.await_args.args[2] + + +@pytest.mark.asyncio +async def test_root_fail_posts_comment_never_blocks_master() -> None: + git = AsyncMock() + c = _make_choreographer(git) + await c._post_gate_review_to_pr( + _task(parent_task_id=None), "pr_fail", "pr-reviewer-1", "Issues:\n- x" + ) + assert git.post_pr_review.await_args.kwargs["event"] == "COMMENT" + + +@pytest.mark.asyncio +async def test_no_pr_number_skips_post() -> None: + git = AsyncMock() + c = _make_choreographer(git) + await c._post_gate_review_to_pr( + _task(parent_task_id=uuid4(), pr_number=None), "pr_pass", "r", "n" + ) + git.post_pr_review.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_github_failure_is_swallowed() -> None: + """A posting failure must not propagate — the gate transition already ran.""" + git = AsyncMock() + git.post_pr_review.side_effect = RuntimeError("github down") + c = _make_choreographer(git) + # Must not raise. + await c._post_gate_review_to_pr(_task(parent_task_id=uuid4()), "pr_pass", "r", "n") diff --git a/tests/unit/gateway/test_verb_runner.py b/tests/unit/gateway/test_verb_runner.py index 6676b99d..4d59cd46 100644 --- a/tests/unit/gateway/test_verb_runner.py +++ b/tests/unit/gateway/test_verb_runner.py @@ -83,13 +83,13 @@ async def test_runner_runs_side_effects_after_db_commit() -> None: @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_up's create_pr (pre_side_effect) runs BEFORE the + submit_for_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``. + submit_for_review needs the cell→root PR to already exist (the reviewer + reviews it); create_pr persists pr_number onto the task row. With a + composes→side_effects ordering the transition would run first and the + trailing create_pr would crash — so the pre-side-effect must run first. """ calls: list[str] = [] @@ -98,11 +98,11 @@ async def test_submit_up_creates_pr_before_transition() -> None: 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") + def _submit_for_review(*_args: object, **_kwargs: object) -> MagicMock: + calls.append("submit_for_review") + return MagicMock(status="awaiting_pr_review") - task_svc.submit_pm_review = AsyncMock(side_effect=_submit_pm_review) + task_svc.submit_for_review = AsyncMock(side_effect=_submit_for_review) git_svc = AsyncMock() @@ -123,8 +123,8 @@ async def test_submit_up_creates_pr_before_transition() -> None: 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}" + assert calls == ["create_pr", "submit_for_review"], ( + f"create_pr must precede submit_for_review for submit_up; got {calls}" ) diff --git a/tests/unit/runtime/test_idle_vs_respawn.py b/tests/unit/runtime/test_idle_vs_respawn.py index 5730e0f7..2780822d 100644 --- a/tests/unit/runtime/test_idle_vs_respawn.py +++ b/tests/unit/runtime/test_idle_vs_respawn.py @@ -16,9 +16,12 @@ container for that PM here would race the in-flight session that just called i_am_idle. The gate: skip closure spawn when the task is paused AND -``last_heartbeat_at`` is newer than ``settings.claim_stale_seconds``. -A genuinely-stale paused task (heartbeat older than the cutoff) still -gets the closure spawn — the gate is about *recency*, not paused-ness. +``last_heartbeat_at`` is newer than the SHORT closure debounce +(``settings.pm_closure_recently_paused_seconds``) — NOT the much longer +reaper window (``stale_claim_reap_seconds``), which would strand closures +for 10-30 minutes. A genuinely-stale paused task (heartbeat older than the +debounce) still gets the closure spawn — the gate is about *recency*, not +paused-ness. """ from __future__ import annotations @@ -34,10 +37,18 @@ from roboco.runtime.orchestrator import AgentOrchestrator def _make_orch() -> AgentOrchestrator: - """Bypass __init__ — tests don't need a full DI graph.""" + """Bypass __init__ — tests don't need a full DI graph. + + Mirror production wiring: the reaper window (``_claim_heartbeat_ttl``) is + the long ``stale_claim_reap_seconds``, while the closure debounce is the + short, dedicated ``pm_closure_recently_paused_seconds`` — they are NOT the + same constant (binding closure to the reaper window stranded closures for + up to 10-30 minutes). + """ orch = AgentOrchestrator.__new__(AgentOrchestrator) orch._instances = {} - orch._claim_heartbeat_ttl = settings.claim_stale_seconds + orch._claim_heartbeat_ttl = settings.stale_claim_reap_seconds + orch._closure_recently_paused_ttl = settings.pm_closure_recently_paused_seconds return orch @@ -55,8 +66,9 @@ def _paused_parent(*, last_heartbeat_at: datetime | str | None) -> dict[str, Any async def test_skips_spawn_when_paused_and_recently_touched_datetime() -> None: """Recent heartbeat + status=paused = agent just called i_am_idle. - Last heartbeat is 1 second ago, claim_stale_seconds default is 180s. - The task is fresh — closure spawn must not fire. + Last heartbeat is 1 second ago, well inside the closure debounce + (pm_closure_recently_paused_seconds). The task is fresh — closure spawn + must not fire (it would race the still-shutting-down session). """ orch = _make_orch() fresh = datetime.now(UTC) - timedelta(seconds=1) @@ -95,13 +107,15 @@ async def test_skips_spawn_when_paused_and_recently_touched_iso_string() -> None @pytest.mark.asyncio async def test_spawns_when_paused_but_heartbeat_is_stale() -> None: - """Heartbeat older than claim_stale_seconds = genuinely-stale agent. + """Heartbeat older than the closure debounce = genuinely-stale agent. The closure dispatcher should still spawn here — the i_am_idle race window has long since closed. """ orch = _make_orch() - stale = datetime.now(UTC) - timedelta(seconds=settings.claim_stale_seconds + 30) + stale = datetime.now(UTC) - timedelta( + seconds=settings.pm_closure_recently_paused_seconds + 30 + ) task = _paused_parent(last_heartbeat_at=stale) descendant = {"id": str(uuid4()), "status": "completed"} @@ -128,6 +142,50 @@ async def test_spawns_when_paused_but_heartbeat_is_stale() -> None: spawn.assert_awaited_once() +@pytest.mark.asyncio +async def test_spawns_when_paused_past_closure_debounce_but_within_reaper_window() -> ( + None +): + """Closure must NOT wait the reaper window (600s / 1800s on the NAS). + + A parent paused ~2 min ago — older than the short closure debounce + (``pm_closure_recently_paused_seconds``, 45s) but far younger than the + reaper window (``stale_claim_reap_seconds``) — must be respawned for + closure now. Binding the debounce to the reaper window stranded the + whole chain for up to 10-30 minutes after the PM idled. + """ + orch = _make_orch() + # Guard against regression: the debounce must be much shorter than the + # reaper window, else this scenario can't exist. + assert ( + settings.pm_closure_recently_paused_seconds < settings.stale_claim_reap_seconds + ) + paused_2min = datetime.now(UTC) - timedelta( + seconds=settings.pm_closure_recently_paused_seconds + 75 + ) + task = _paused_parent(last_heartbeat_at=paused_2min) + descendant = {"id": str(uuid4()), "status": "completed"} + + client = AsyncMock() + + with ( + patch.object( + orch, + "_fetch_all_descendants", + new=AsyncMock(return_value=[descendant]), + ), + patch.object(orch, "_already_promoted_for_closure", return_value=False), + patch.object(orch, "_is_agent_active", return_value=False), + patch.object(orch, "_auto_resume_paused_parent", new=AsyncMock()), + patch.object(orch, "_build_pm_closure_prompt", return_value="prompt"), + patch.object(orch, "_task_git_context", return_value=MagicMock()), + patch.object(orch, "spawn_agent", new=AsyncMock()) as spawn, + ): + await orch._maybe_spawn_pm_closure(client, task) + + spawn.assert_awaited_once() + + @pytest.mark.asyncio async def test_spawns_when_paused_but_heartbeat_missing() -> None: """No heartbeat at all means the freshness gate cannot trigger. diff --git a/tests/unit/test_enum_migration_parity.py b/tests/unit/test_enum_migration_parity.py index 9ba2121e..3bff6e10 100644 --- a/tests/unit/test_enum_migration_parity.py +++ b/tests/unit/test_enum_migration_parity.py @@ -19,6 +19,9 @@ from roboco.db.tables import Base # registers every ORM enum _ROOT = pathlib.Path(__file__).resolve().parents[2] +# alembic's ``alembic_version.version_num`` column is VARCHAR(32). +_ALEMBIC_VERSION_NUM_MAX = 32 + def _orm_enum_values() -> dict[str, set[str]]: orm: dict[str, set[str]] = {} @@ -68,3 +71,24 @@ def test_every_orm_enum_value_is_created_by_the_migration_chain() -> None: "autogenerate does NOT detect added enum labels):\n" + "\n".join(f" {name}: {vals}" for name, vals in sorted(drift.items())) ) + + +def test_every_migration_revision_id_fits_the_alembic_version_column() -> None: + """Revision ids must fit alembic's ``alembic_version.version_num`` VARCHAR(32). + + A longer id raises ``value too long for type character varying(32)`` when + alembic records the migration on a real ``upgrade head`` — but NOT in the + test suite, whose DB is built via ``Base.metadata.create_all`` and whose + parity test only renders SQL offline. So nothing else catches it; this guard + does, statically. + """ + versions = _ROOT / "alembic" / "versions" + too_long: dict[str, int] = {} + for path in versions.glob("*.py"): + match = re.search(r'^revision\s*=\s*"([^"]+)"', path.read_text(), re.M) + if match is not None and len(match.group(1)) > _ALEMBIC_VERSION_NUM_MAX: + too_long[match.group(1)] = len(match.group(1)) + assert not too_long, ( + "migration revision ids exceeding the alembic_version VARCHAR(32) limit " + f"(shorten them): {too_long}" + )