Files
roboco/docs/map/choreographer.md
T
7901ea419e Retire channels/sessions/messages; A2A becomes primary agent comms (#306)
* feat(a2a): deliver latest incoming message preview into the claim briefing

list_unread_a2a now carries last_message_preview (the latest message from the
OTHER agent, never the agent's own reply), fetched via a correlated subquery in
the same query — no N+1 on the per-verb briefing path.

* feat(a2a): read_a2a verb delivers unread message bodies to the agent

A2AService.get_unread_messages returns the caller's unread INCOMING messages
(never its own sends), marking exactly those rows read atomically so a message
arriving mid-call is preserved. Wired as the read_a2a content verb (route +
do_server tool + granted to every delivery role) — the content-bearing read the
A2A inbox lacked (read_messages only zeroed the counter).

* docs(rag): document read_a2a as the A2A content-read path

* fix(task): backlog activation no longer requires a discussion session

Removes the SessionTaskTable gate in activate() (and its dangling log field),
deletes _inherit_parent_session + its create() call, and drops the now-unused
SessionTaskTable import. Coordination rides task state; the session subsystem is
being retired. Tests updated to the new (no-session) behavior.

* fix(orchestrator): drop session sweep from _run_sweep

Removes the messaging import + sweep_timed_out_sessions call. That import sat
outside the try/except, so once messaging.py is deleted it would have killed the
entire sweep cascade (budget kill-switch, token rollups, retention, image prune,
superseded-PR reconcile). Notification sweep + all maintenance sweeps unchanged.

* release-manager --no-tags read-clone fix

* test: update evidence_repo unit test for a2a last_message_preview

* refactor(gateway): drop session propagation on delegate

Removes propagate_sessions_to_subtask from delegate(), the ChoreographerDeps
messaging field + property, and the ChoreographerDeps messaging arg in deps.py
(ContentActions messaging + import stay until the verbs are removed). Deletes the
propagation test; strips the now-invalid messaging kwarg from ChoreographerDeps
test builders.

* refactor(gateway): remove say/open_session/link_session/channels verbs

Removes the four channel/session verbs across content_actions (impls +
ContentActionsDeps.messaging), do_server (tools + registry), role_config (grants
+ _CHANNEL_DISCOVERY), do.py (routes), schemas/v1/do.py (request models), and
deps.py (MessagingService import + construction). Regenerates the prompt verb
tables. dm/notify/read_messages/read_a2a stay. Tests deleted/updated accordingly.

* uv.lock Upgrade

* refactor: remove conversation RAG indexing; Secretary announces via notification

Drops the CONVERSATIONS index (index_conversation, ConversationsIndexPlugin,
IndexType.CONVERSATIONS enum, IndexConversationParams, mentor.py type-label, the
messaging index hook) and its chunk-table manifest entries. The Secretary's
ANNOUNCE/RELAY_MESSAGE now fan out a BROADCAST notification to every agent's
inbox (NotificationService.broadcast) instead of posting to a dead channel.

* fix(panel): label RAG health error lines by subsystem

A red llm_error (e.g. the glm-5.2:cloud weekly-limit 429) rendered under
the 'Embedding: ok' header with no label, reading as an embedding failure.
Prefix each error line with LLM / Embedding / Vector store.

* refactor: remove channel/message reads from metrics, dashboard, git, events

MetricsService drops get_communication_volume + the MessageTable
message-count in get_agent_metrics (and the now-dead messages_sent_week
field). DashboardService drops get_channel_feeds/_compute_channel_status
and the message read in get_recent_activity (task activity kept);
get_auditor_metrics no longer reports communication_volume.
GitService's two primary-session-id helpers always return None now
(callers already treat None as "no primary session"). events/handlers.py
drops the SESSION_CLOSED/SESSION_TIMEOUT subscriptions + the
handle_session_boundary handler.

Forced follow-on: api/routes/dashboard.py + api/schemas/dashboard.py
dropped the now-dangling live_feeds/ChannelFeed surface and the
/metrics/communication route, which wrapped the removed service calls
directly (mypy would otherwise fail on the missing attributes).

* refactor: delete MessagingService + channel seeding

Edited db/__init__.py and services/__init__.py first (drop the unconditional
Channel/Group/Message/Session table + MessagingService re-exports), then
deleted services/messaging.py, then trimmed db/seed.py to only create_agents
(create_channels/create_channel_memberships/create_initial_messages gone).

Forced expansion: api/routes/{channels,groups,sessions,messages}.py import
roboco.services.messaging directly (not through the package __init__), as
does api/routes/tasks.py (the session-links embed on GET /tasks/{id} and the
GET /{id}/sessions route). Deleting messaging.py without addressing these
breaks `import roboco.api.app` immediately, since app.py eagerly imports all
route modules at startup. Since the 4 CRUD route files are 100%
MessagingService-backed with zero independent logic (and are wholesale
deletes in the plan's later API-routes task anyway), deleted them now +
unmounted from app.py/routes/__init__.py; tasks.py got the same surgical
trim its later task already specified (drop session-links embed +
TaskSessionLinkResponse/TaskResponse.sessions). This pulls a slice of that
later work forward — the routes/schemas for channels/groups/sessions/messages
still need their own pass, but their messaging-coupled parts are gone.

Verified with a full-suite collection sweep (12010 tests collected, zero
import errors) beyond the directly touched test dirs, given the expanded
blast radius.

* refactor: remove channel/session/message models, tables, and channel policy

Models: deleted channel.py/group.py/session.py/messaging.py wholesale
(zero external consumers besides the models/__init__.py re-export).
message.py surgically trimmed: removed MessageCreate (dead) and MessageEdit
(never instantiated; ExtractedMessage.edit_history retyped to
list[dict[str, Any]] to match how it's actually persisted — confirmed
ExtractedMessage was never written to any DB table, so MessageTable's
removal carries no functional risk to the kept extraction pipeline).
base.py: removed SessionStatus + ChannelType, kept MessageType. Also
removed the confirmed-dead channels_read/channels_write fields from
models/agent.py:AgentPermissions and models/dashboard.py:ChannelFeedData.

db/tables.py: deleted ChannelTable/GroupTable/SessionTable/SessionTaskTable/
MessageTable, TaskTable.session_links, and JournalEntryTable.session_id —
cascaded through models/journal.py, services/journal.py, and
api/schemas+routes/journals.py (22 plumbing sites).

foundation/policy/communications.py: removed the ChannelSpec/CHANNELS
catalog + TEAM_SCOPED_ROLES/_CELL_*/_AUDITOR_ONLY helpers, kept the
notification policy (Priority/parse_priority/NOTIFY_SENDER_ROLES/
ACK_REQUIRED_BY_TYPE). enforcement/channel_access.py deleted (confirmed
fully dead in production). agents_config.py: removed CHANNEL_ACCESS
(kept A2A_ALLOWED_PAIRS). seeds/initial_data.py: removed
DEFAULT_CHANNELS/CHANNEL_MEMBERSHIPS/AUDITOR_SILENT_ACCESS + the
never-consumed INITIAL_MESSAGES. config.py: removed
session_idle_timeout_seconds (zero consumers). exceptions.py: removed
dead ChannelError/ChannelAccessDeniedError/SessionClosedError.

Forced expansion beyond the original file list — ChannelType cascaded
into a live, mounted surface the plan didn't trace: agents_config.
CHANNEL_ACCESS -> services/permissions.py's channel-RBAC methods (not
models/permissions.py, which turned out to have no channel code at all)
-> two real endpoints in api/routes/stream.py (GET /permissions,
GET /permissions/channel/{name}) and two dependency factories in
api/deps.py. Removed the channel methods + fields, deleted the
channel-specific stream.py endpoint, deleted require_channel_read/write.
Also deleted api/schemas/{channels,sessions}.py (hard dependency on the
removed enums; already fully dead after the Task 10 route deletions) and
api/schemas/messages.py (a TYPE_CHECKING-only import of the deleted
MessageTable; likewise already fully dead) + its dedicated test file.

Test updates: test_permissions.py -14 channel tests (matches the planned
count exactly), test_communications.py / test_communications_consumers.py
split to keep only notification-policy coverage, test_exceptions.py -9,
test_deps.py -4, plus the journal/stream/foundation-smoke fallout. Also
fixed a pre-existing (Task 7) broken assertion in
test_foundation_phase3_smoke.py that inspected a `say()` method already
removed from ContentActions.

Verified: full-suite collection (11961 tests, zero import errors) and a
complete test run (11567 passed, 394 skipped, 0 failed) in addition to
the targeted suites.

* migration: drop channels/groups/sessions/session_tasks/messages + enum types

alembic/versions/060_drop_messaging.py: drop_column journal_entries.
session_id (sidesteps hardcoding the FK constraint name — verified
empirically against a live migrated DB that it's actually
fk_journal_entries_session_id_sessions, but drop_column doesn't care
either way); drop_table in FK order (messages -> session_tasks ->
sessions -> groups -> channels); DROP TABLE IF EXISTS chunks_conversations
(runtime-provisioned, not alembic-managed, would otherwise orphan); DROP
TYPE IF EXISTS for messagetype/sessionstatus/sessionscope/channeltype
(messagetype's Python enum stays for ExtractedMessage, but the DB type
had zero live columns left once MessageTable was dropped in the prior
commit). downgrade() raises NotImplementedError — one-way removal.

Pruned scripts/reset_runtime_state.sql + .sh: removed the DELETE/COUNT
lines for messages/session_tasks/sessions/groups/channels and the
groups.active_session_id reset block.

Verified end-to-end against a scratch Postgres DB: full migration chain
001->060 applies cleanly, alembic heads shows a single head, all 6 dropped
tables + 4 enum types + the journal_entries.session_id column are
confirmed gone, journal_entries keeps only its journal_id/task_id FKs,
downgrade correctly raises NotImplementedError without corrupting DB
state, and the pruned reset_runtime_state.sql runs clean (no errors)
against a fully-migrated DB.

* refactor(api): remove channel/session/message routes + WS streams

Most of this task's file list was already forced through in earlier
commits (routes/{channels,groups,sessions,messages}.py + app.py/__init__.py
unmounting in the MessagingService-deletion commit; tasks.py's
session-links embed + GET /{id}/sessions + schemas/tasks.py's
TaskResponse.sessions in that same commit; deps.py's require_channel_read/
write + schemas/{channels,sessions}.py in the models/tables commit). This
closes out what was left:

- api/websocket.py: deleted the channel_stream + session_stream routes,
  ConnectionManager's channel_connections/session_connections dicts,
  connect_channel/connect_session, broadcast_to_channel/broadcast_to_session,
  get_channel_subscriber_count, and their cleanup lines in disconnect().
  Agent streams, notification streams, and the operator system stream are
  untouched.
- api/websocket_bridge.py: deleted _handle_session_event +
  _handle_message_event and their SESSION_CREATED/SESSION_CLOSED/
  SESSION_TIMEOUT/MESSAGE_SENT subscriptions. The A2A live-view, rate-limit,
  usage, agent-lifecycle, and notification bridges are untouched.
- api/schemas/websocket.py: removed NewMessageBroadcast, WSMessageNew,
  WSMessageEdit, WSMessageDelete, WSSessionClosed — kept the WSMessage base
  class (still subclassed by the kept WSAgentStream/WSNotification) plus
  those two.
- api/schemas/groups.py: deleted (already fully orphaned since routes/
  groups.py was removed; its GroupResponse/GroupDetailResponse had zero
  consumers).

Updated the 5 websocket test files accordingly (removed the channel/
session-specific tests + fixed imports); test_websocket_bridge.py's
registration-coverage test dropped the SESSION_*/MESSAGE_SENT assertions.

Verified: full-suite collection (11943 tests, zero import errors) and a
complete test run (11549 passed, 394 skipped, 0 failed).

* docs: retire channels/sessions/messages from agent-facing docs + CLAUDE.md

Rewrites docs/rag (RAG-indexed) + docs/map + CLAUDE.md to reflect A2A (dm +
read_a2a) as primary agent comms; deletes the channel docs, splits messaging-tools
+ messaging-notification (renamed notification.md), swaps the WS worked example to
A2A_MESSAGE_SENT. _complete_map.md still needs regeneration (generated file).

* refactor(panel): remove Communications surface (channels/sessions)

Deletes the /communications routes, message components, task-detail Sessions tab,
use-channels + channel/session WS hooks, and the channels/sessions/messages/groups
api clients; prunes the Channel/Session/Message/Group types + mock data. (Auditor
live-feeds + dashboard.ts dead-route cleanup is a follow-up.)

* refactor(panel): drop auditor channel-feed + dead communication-metric route

* docs(map): regenerate _complete_map from updated slices

* fix(a2a): reduce get_unread_messages complexity below xenon C + stale comments

Extract the per-conversation unread-counter recompute into _reset_unread_counter
(the CI quality gate flagged get_unread_messages as rank C). Also drop the deleted
open_session from a content_actions comment and reword an evidence_repo docstring
that cited the removed messaging._notify_mentions.

---------

Co-authored-by: Renn F <rennf93@users.noreply.github.com>
2026-07-04 03:10:33 +02:00

20 KiB
Raw Blame History

Choreographer Slice Map

Purpose

The Choreographer is the server-side composition layer that turns agent intent-verbs (give_me_work, i_will_work_on, i_am_done, delegate, submit_up, submit_root, complete, …) into ordered sequences of atomic TaskService / GitService actions. It owns the precondition gates the lifecycle spec does not model (concurrency invariants, tracing/progress gates, free-text soup, conventions, behind-base, unchanged-PR loop-stoppers) and wraps every composed mutation in a SAVEPOINT via VerbRunner. Every verb returns a standardized Envelope (ok / error + next + remediate + context_briefing).

Files

Path Role
roboco/services/gateway/choreographer/_impl.py The _LegacyChoreographer / Choreographer class — all verb bodies + guard helpers (~6.9k lines).
roboco/services/gateway/choreographer/_protocol.py ChoreographerHelpers — TYPE_CHECKING-only stub of helpers role mixins call on self, so mypy sees typed signatures (runtime object).
roboco/services/gateway/choreographer/_verb_runner.py VerbRunner — composed-actions runner; wraps composes in session.begin_nested() SAVEPOINT, runs pre_side_effects / side_effects outside.

Key Symbols (landmarks only)

Name Kind File:Line Responsibility
Choreographer class _impl.py:339 Composed entry point; deps-injected; exposes verbs + helpers.
ChoreographerDeps dataclass _impl.py:207 Dependency injection container (task, work_session, git, a2a, journal, audit, evidence_repo, messaging, product, orchestrator, stream_bus).
_COORDINATOR_ROLES ClassVar _impl.py:914 {main_pm, cell_pm} — exempt from already_active/paused claim guards + advisory lock.
give_me_work async verb _impl.py:766 Picks next task for agent + builds briefing (institutional memory injected here).
_briefing_for async helper _impl.py:820 Builds context_briefing. Claim-scoped: full=True (context-acquisition verbs only — give_me_work/claims/plan/resume/triage) carries the heavy sections via _heavy_briefing_sections (company_goals, recent_team_activity, blockers, task_handoff, institutional_memory); every other verb gets slim signals-only (unread a2a/mentions/notifications + metadata gaps). AC coverage stays independent of full.
_run_claim_guards async helper _impl.py:916 already_active / paused / unmet_dependency (with re-check race narrowing) + _lane_claim_guard; skip_dev_guards=False param skips dev-only guards for pr_reviewer gate claims (claim_gate_review).
_lane_claim_guard async helper _impl.py:977 Out-of-order-start barrier: refuse code leaf behind an earlier open same-assignee sibling. Fail-closed on lookup error.
_claim_plan_start_gate async helper _impl.py:1179 spec gate → advisory claim lock (non-PM) → behavioral guards.
_claim_plan_start_run async helper _impl.py:1251 runner.run_intent(verb) + ensure_work_session + _touch.
i_will_work_on async verb _impl.py:1326 Dev claim+plan+start path; routes re-entry vs fresh claim.
open_pr async verb _impl.py:1571 Pre-flight + run_intent("open_pr") (push_branch + create_pr side effects).
i_am_done async verb _impl.py:1771 Dev pre-submit; runs _i_am_done_gate then run_intent("i_am_done").
_i_am_done_gate async helper _impl.py:1885 Ordered gate chain: tracing → submit_qa fields → push → behind_base → quality → toolchain → conventions; then write AC status.
_behind_base_gate async helper _impl.py:2154 Refuse submit when branch behind its base (sibling PR merged); fail-open on git error.
_toolchain_broken_guard async helper _impl.py:1939 Block delivery gate when agent workspace can't run suite; reviewer=True for pr_pass.
_conventions_guard async helper _impl.py:2043 Run architectural-conventions validator; block finding refuses gate.
_pm_task_type_error static method _impl.py:4752 Reject a code/non-planning task_type delegated to a PM (cell or main); extracted from _validate_assignee_task_type to keep that dispatcher under complexity budget.
i_am_blocked async verb _impl.py:3086 Rate-limit parking vs generic block; run_intent("i_am_blocked").
_handle_rate_limited_parking async helper _impl.py:2983 Park provider on 429/overload/session-limit.
unclaim async verb _impl.py:3184 Release claimed task → pending (optional reassign).
reassign async verb _impl.py:3343 Reassign task with _validate_reassign.
resume async verb _impl.py:3422 Resume paused/blocked task.
sync_branch async verb _impl.py:3528 Gate-level rebase verb (new since baseline).
i_am_idle async verb _impl.py:3678 Idle signal; auto-pause in_progress tasks; pending-assignment / PM review / auditor guards.
i_will_plan async verb _impl.py:4071 PM plan verb; _pm_sub_tasks_gate enforces substantive approach/sub_tasks.
delegate async verb _impl.py:4173 PM creates subtask; sizing, sibling-dedup, spine-cap, lifecycle guards; _create_subtask_from_inputs.
submit_up async verb _impl.py:5348 Cell PM opens cell→root PR + enters awaiting_pr_review; unchanged-PR guard.
submit_root async verb _impl.py:6260 Main PM opens root→master PR + enters gate; umbrella hard-reject + unchanged-PR guard.
_submit_root_unchanged_pr_guard async helper _impl.py:6149 Loop-stopper: refuse re-submit when PR head SHA == last pr_fail SHA. Fail-open on ambiguity.
_current_pr_head_sha async helper _impl.py:6196 Best-effort current PR head SHA via _project_slug_for + git.get_pr_head_sha.
complete async verb _impl.py:6599 Role-dispatch to cell_pm_complete / main_pm_complete; umbrella-in-progress bypasses spec gate.
main_pm_complete async verb _impl.py:6496 Main PM merge + escalate to CEO (never merges master itself).
escalate_to_ceo async verb _impl.py:6844 Escalate to awaiting_ceo_approval.
VerbRunner.run_intent async method _verb_runner.py:37 pre_side_effects → SAVEPOINT(composes) → side_effects; intermediate-None raises INVALID_STATE.
VerbRunner._do_pr_merge async handler _verb_runner.py:257 pr_merge with project_id scoping (cross-repo collision fix) + resolve_parent_branch.
ChoreographerHelpers stub class _protocol.py:31 TYPE_CHECKING-only typed view of self helpers for role mixins.

Data Flow

An MCP flow/* call hits the orchestrator → the role-specific gateway verb → Choreographer.<verb>(agent_id, task_id, ...). The verb fetches the task (self.task.get), builds a briefing (_briefing_for), runs the spec gate (spec.can_invoke_intent(role, verb, t, ctx)) and any verb-specific preflight guards (free-text soup, claim guards, conventions, behind-base, unchanged-PR). On rejection it emits via _emit_rejection with next/remediate. On allow it calls VerbRunner.run_intent(verb, t, agent, ctx): pre_side_effects (e.g. create_root_pr for submit_root) run OUTSIDE the SAVEPOINT; then session.begin_nested() wraps the composed atomic actions (claim/set_plan/start/submit_qa/…) re-fetching the task after each; then side_effects (push_branch / create_pr / pr_merge) run after the savepoint commits. An intermediate None from a composed action raises INVALID_STATE (concurrent transition); a trailing None flows out as the verb result. The verb wraps the final task in Envelope.ok(status, next_hint, briefing) with .with_introspection(task, role).

Mermaid

sequenceDiagram
    participant Agent
    participant Gateway as MCP flow verb
    participant Ch as Choreographer
    participant Spec as lifecycle spec
    participant VR as VerbRunner
    participant TS as TaskService
    participant Git as GitService
    Agent->>Gateway: verb(agent_id, task_id, notes)
    Gateway->>Ch: verb(agent_id, task_id, notes)
    Ch->>TS: task.get(task_id)
    Ch->>Ch: _briefing_for(...)
    Ch->>Spec: can_invoke_intent(role, verb, t, ctx)
    alt not allowed
        Ch-->>Agent: Envelope error (from_decision) + remediate
    else allowed
        Ch->>Ch: preflight guards (soup / claim / conventions / behind_base / unchanged_pr)
        alt guard rejects
            Ch-->>Agent: Envelope invalid_state + remediate
        else pass
            Ch->>VR: run_intent(verb, t, agent, ctx)
            VR->>Git: pre_side_effects (create_root_pr)
            VR->>TS: session.begin_nested() (SAVEPOINT)
            loop composes
                VR->>TS: atomic action (claim / set_plan / start / submit_qa / ...)
                TS-->>VR: updated task (None on concurrent source-state mismatch)
                alt intermediate None
                    VR-->>Ch: raise INVALID_STATE
                end
            end
            VR->>TS: commit savepoint
            VR->>Git: side_effects (push_branch / create_pr / pr_merge)
            VR-->>Ch: final task
            Ch->>TS: ensure_work_session / _touch
            Ch-->>Agent: Envelope ok(status, next, briefing)
        end
    end

Logical Tree

Choreographer (composed class, _impl.py)
├── Deps: task / work_session / git / a2a / journal / audit / evidence_repo / messaging / product / orchestrator / stream_bus
├── Verb bodies
│   ├── Dev: give_me_work, i_will_work_on, open_pr, i_am_done, i_am_blocked, unclaim, resume, sync_branch, i_am_idle
│   ├── PM:  i_will_plan, delegate, submit_up, submit_root, complete→{cell_pm_complete, main_pm_complete}, escalate_up, triage, triage_all, unblock, reassign, pm_give_me_work
│   └── Board/CEO: escalate_to_ceo
├── Guard helpers
│   ├── _run_claim_guards → already_active / paused / unmet_dependency / _lane_claim_guard
│   ├── _i_am_done_gate chain → tracing / submit_qa_fields / push / behind_base / quality / toolchain / conventions
│   ├── _submit_up_guard / _submit_up_unchanged_pr_guard / _submit_root_unchanged_pr_guard
│   ├── _guard_free_text / _free_text_soup / _soup_or_decision_env
│   └── _conventions_guard / _toolchain_broken_guard
└── VerbRunner (_verb_runner.py)
    ├── pre_side_effects → {create_root_pr}
    ├── composes (SAVEPOINT) → {claim, set_plan, start, submit_qa, qa_pass, qa_fail, docs_complete, complete, submit_pm_review, submit_for_review, pr_pass, pr_fail, escalate_to_ceo, block, unblock, resume, pr_review_done}
    └── side_effects → {push_branch, create_pr, create_root_pr, pr_merge}

Dependencies

  • Internal services: TaskService, WorkSessionService, GitService, EvidenceRepo/evidence_builder, a2a, JournalService, AuditService, ProjectService/product, orchestrator handle, StreamEventBus.
  • Policy (pure): roboco.foundation.policy.lifecycle (can_invoke_intent, Context, Role, _INTENT_VERBS), foundation.policy.batch (is_batch_umbrella), foundation.policy.content markers + reject_trivial.
  • Gateway helpers: claim_guards (already_active_guard, paused_tasks_guard, unmet_dependency_guard), merge_chain.resolve_parent_branch, envelope.Envelope, remediation hints.
  • External: structlog, asyncpg (via task_service session / SAVEPOINT), redis (rate-limit parking), git/gh CLI (via git_service).

Entry Points

  • MCP roboco-flow server → per-role verb methods (manifest-driven allowlist from role_config.py).
  • Orchestrator /api/v1/flow/* REST endpoints → same Choreographer methods.
  • Internal cross-verb calls (e.g. completecell_pm_complete / main_pm_complete; _claim_plan_start_run shared by i_will_work_on + i_will_plan).

Config Flags

  • ROBOCO_TOOLCHAIN_MATCH_ENABLED — gates _toolchain_broken_guard (default-off).
  • ROBOCO_CONVENTIONS_ENABLED — gates _conventions_guard (default-off).
  • ROBOCO_OVERLOAD_BREAK_ENABLED — rate-limit/overload parking path (default-on).
  • ROBOCO_ORG_MEMORY_ENABLED — institutional-memory injection in _briefing_for (default-off).
  • ROBOCO_GATEWAY_HEALTH_ENABLED — reaper gateway-health probe (default-on; not directly in choreographer but feeds the parking path).

Gotchas

  • The SAVEPOINT wraps only DB atomic actions; pre_side_effects (e.g. create_root_pr) run BEFORE the savepoint and are NOT rolled back if a later composed action raises — they are idempotent by contract.
  • side_effects (push/create_pr/pr_merge) run AFTER savepoint commit and are idempotent + retryable; a crash between commit and side-effect leaves the PR uncreated (recovered by re-issue / open_pr parity).
  • VerbRunner only raises on an INTERMEDIATE None composed action; a trailing None is the verb's own "no transition" result and each verb body must handle it (submit_root_finalize, _claim_plan_start_run do; a verb that forgets will None-deref).
  • _lane_claim_guard fail-closed on lookup error — a DB hiccup rejects the claim (calls release_dependency_blocked_claim); acceptable but can briefly bounce a dev.
  • _submit_*_unchanged_pr_guard FAILS OPEN on every ambiguous case (no recorded sha, no project slug, git error, closed PR) — only exact-unchanged is hard-blocked; a regression in _current_pr_head_sha resolver silently re-opens the loop.
  • complete bypasses the spec gate for an in-progress batch umbrella (_is_umbrella_in_progress) and relies on main_pm_complete's own guards — a mis-classified umbrella could skip the AWAITING_PM_REVIEW status constraint.

Drift from CLAUDE.md

  • CLAUDE.md verb table lists submit_root for main_pm and submit_up for cell_pm — matches code (_impl.py:5348, 6260). No drift.
  • CLAUDE.md: "PM coordinator concurrency … claim-time concurrency guards skipped for _COORDINATOR_ROLES" — matches (_impl.py:939, 1234). No drift.
  • CLAUDE.md verb surface omits sync_branch from the developer list — code has sync_branch at _impl.py:3528 (added since baseline; memory note project_sync_branch_tracing_gap.md flags it). Minor doc drift.
  • CLAUDE.md: "PR is created BEFORE QA review" — i_am_done gate chain pushes + creates PR context but the actual create_pr side-effect runs in open_pr/submit_up/submit_root, not i_am_done; consistent with the described flow. No drift.
  • CLAUDE.md: "only the CEO merges master; Main PM ready root PR → awaiting_ceo_approval (does NOT merge)" — main_pm_complete escalates; VerbRunner._do_pr_merge exists for cell-level merges and create_root_pr opens but the root merge is CEO-gated. Consistent. No drift.

Changes Since Baseline

git log --oneline fd10cc862c2020b3f639cdb686d427b0198a2441..HEAD -- roboco/services/gateway/choreographer/ → 2 commits touching these files (+814/89):

  1. 15effce0 — 141 Gaps fill-in (#283): added out-of-order-start guards (_lane_claim_guard, _behind_base_gate, sync_branch), unchanged-PR loop-stoppers (_submit_root_unchanged_pr_guard, _submit_up_unchanged_pr_guard, _current_pr_head_sha), project_id scoping on pr_merge (cross-repo collision fix), umbrella-in-progress bypass in complete, _submit_root_finalize None-guard, reviewer flag on _toolchain_broken_guard.
  2. 3aff6e04 — Close gaps (#285): follow-on touch-ups in the same areas (per-cell project map root-subtask support umbrella handling).

Post-snapshot updates (since 2026-06-29):

  • 536bbb64 — logical-gaps sweep: _run_claim_guards gains skip_dev_guards: bool = False param (pr_reviewer claim_gate_review calls skip dev-only already_active/paused/lane guards; dependency guard still runs). _pm_task_type_error extracted as new @staticmethod on Choreographer (_impl.py:4752) from _validate_assignee_task_type — fixes Main PM omission from the PM-cannot-own-code delegate gate. _submit_*_unchanged_pr_guard now logs a warning on head_sha resolver failure so fail-open behavior is observable. Matching skip_dev_guards stub added to _protocol.py. _impl.py +78/0 lines; _protocol.py +1 line.
  • 0e7674af — verb_runner trailing-None side-effect guard + actor_agent_id threading: run_intent now skips the side_effects loop when the trailing composed action returns None (prevents _do_push_branch(None) / _do_pr_merge(None) AttributeError crash, converting it to a clean caller-handled None). _do_push_branch, _do_create_pr, _do_create_root_pr, and _do_escalate_to_ceo all forward actor_agent_id=agent.id into git_service / task_service. main_pm_complete escalate path in _impl.py also forwards actor_agent_id. _verb_runner.py +52/8 lines; _impl.py +8/2 lines.

Regression Risks

Title File:Line Claim Severity
Intermediate-None contract depends on every verb body handling trailing None _verb_runner.py:89 + _impl.py:1277,6358 A verb that forgets the trailing-None guard None-derefs t.status; submit_root + claim_plan_start handle it, but any NEW verb using run_intent with a possibly-None last action inherits the trap. Side-effect crash path closed (0e7674af): run_intent now skips the side_effects loop on a trailing None, so _do_push_branch(None) no longer crashes. Verb bodies still need to handle None return for their own error messaging but will not AttributeError. Risk reduced: runner-level crash path fixed; verb-body None-deref in messaging remains a code-discipline risk. High → Medium
pr_merge project_id scoping assumes task.project_id is non-None _verb_runner.py:263 project_id=task.project_id — if a coordination/umbrella task ever reaches pr_merge with project_id=None, the cross-repo collision guard silently matches nothing or None-keys the scoping; could merge the wrong PR or no-op. High
_submit_*_unchanged_pr_guard fails open on resolver regressions _impl.py:6175,6240 Any future break in _current_pr_head_sha / _project_slug_for / git.get_pr_head_sha makes the loop-stopper a no-op, re-opening the 2026-06-27 pr_fail re-submit loop. Partially mitigated (536bbb64): _current_pr_head_sha now emits a structlog.warning on resolver failure so the fail-open path is observable in logs; the underlying fail-open behavior is intentional and unchanged. High
_lane_claim_guard calls release_dependency_blocked_claim on lookup error _impl.py:988,996 Fail-closed path releases the claim before returning the error envelope; if the lookup error is transient the dev is bounced + work-session abandoned even though the lane was actually free. Medium
complete umbrella-in-progress bypass skips the spec AWAITING_PM_REVIEW status check _impl.py:6657 _is_umbrella_in_progress mis-classification (e.g. a non-batch branchless task with matching predicates) lets a non-awaiting_pm_review task reach main_pm_complete/cell_pm_complete. Medium
_run_claim_guards dependency re-check narrows but does not close the race _impl.py:965-967 The re-check returns None (skip release) when fresh read sees deps met, but the window between re-check and the caller's claim is still unlocked; a concurrent terminal transition there is benign (monotonic), but a non-monotonic future status could re-open. Low
submit_root runs create_root_pr as a pre_side_effect OUTSIDE the savepoint _verb_runner.py:67 + _impl.py:6339 If a later composed submit_for_review raises, the root→master PR is already opened and NOT rolled back; re-issue is idempotent-by-contract but a non-idempotent future pre_side_effect would leak. Medium
_i_am_done_gate writes AC criteria status AFTER all gates pass but BEFORE run_intent _impl.py:1910 _write_criteria_status runs in the gate phase; if run_intent("i_am_done") then raises, the AC status rows persist for a task that did not transition — a stale write the next attempt must overwrite. Low

Health

The slice is structurally sound: the SAVEPOINT boundary, intermediate-None INVALID_STATE guard, and role-exempt coordinator concurrency model are coherent and well-documented. The highest-temperature areas are the new (since baseline) fail-open loop-stoppers and fail-closed lane guard — both correct by design but tightly coupled to resolvers (_current_pr_head_sha, has_earlier_incomplete_code_sibling) whose regressions silently revert the protection. The 814-line delta is concentrated in guard additions rather than control-flow rewrites, so baseline behavior is largely preserved; the main residual risk is verb-body discipline around the trailing-None contract for any future verb.