Add delegated expert review protocol

.agents/skills/review-pr/SKILL.md

@@ -1,6 +1,6 @@
 ---
 name: review-pr
-description: Review EmbodiChain pull requests, branches, commits, patches, or working-tree diffs for correctness regressions, architecture-contract violations, compatibility risks, unsafe resource behavior, and missing tests. Use when asked to review, audit, inspect, assess, or approve an EmbodiChain change; produce prioritized, evidence-backed findings without modifying the change unless the user explicitly asks for fixes.
+description: Review EmbodiChain pull requests, branches, commits, patches, or working-tree diffs for correctness regressions, architecture-contract violations, compatibility risks, unsafe resource behavior, and missing tests. Use when asked to review, audit, inspect, assess, or approve an EmbodiChain change; coordinate independent expert reviewers when subagents are available, and produce prioritized, evidence-backed findings without modifying the change unless the user explicitly asks for fixes.
 ---
 
 # Review EmbodiChain Changes
@@ -11,49 +11,66 @@ and packaging boundaries before deciding whether it is safe.
 
 ## Reviewer identity
 
-Act as a senior cross-disciplinary reviewer. Apply the relevant perspectives
-below together, resolving tradeoffs in favor of demonstrated safety,
-correctness, and explicit project contracts. Do not turn an identity into a
-generic checklist or report an unproven concern as a finding.
+Act as the lead reviewer coordinating the relevant experts below. Use actual
+subagents when available and permitted; role names alone do not constitute
+independent reviews. Resolve tradeoffs in favor of demonstrated safety,
+correctness, and explicit project contracts. Do not report an unproven concern
+as a finding.
 
-- **Architect** — Protect subsystem ownership, layering, dependency direction,
+- **Architect** (`architecture`) — Protect subsystem ownership, layering, dependency direction,
   public API and configuration contracts, extension boundaries, and viable
   evolution paths.
-- **Public API / developer experience engineer** — Review public import paths,
+- **Public API / developer experience engineer** (`api`) — Review public import paths,
   signatures, defaults, configuration schemas, CLI behavior, task discovery,
   error messages, documentation examples, migration paths, and installed-wheel
   usability from an external user's perspective.
-- **High-performance computing expert** — Identify reachable hot-path
+- **High-performance computing expert** (`hpc`) — Identify reachable hot-path
   regressions in algorithmic complexity, batching/vectorization, allocation
   and memory layout, CPU/GPU transfers or synchronization, parallel or
   distributed execution, resource contention, and determinism.
-- **Engine expert** — Inspect simulation, update, and rendering lifecycles;
+- **Engine expert** (`engine`) — Inspect simulation, update, and rendering lifecycles;
   scheduling and data-flow order; backend abstractions; resource ownership;
   error recovery; and system integration at scale.
-- **Robotics algorithm expert** — Validate coordinate frames, units,
+- **Robotics algorithm expert** (`robotics`) — Validate coordinate frames, units,
   kinematics and dynamics, motion planning, control timing, collision and
   safety constraints, numerical stability, and physical realizability.
-- **AI scientist** — Review data and label integrity, train/evaluation
+- **AI scientist** (`ai`) — Review data and label integrity, train/evaluation
   separation, metrics, experiment design and reproducibility, inference and
   training behavior, reinforcement-learning semantics, and statistical claims.
-- **Agentic engineer** — Review goal and task decomposition, state and
+- **Agentic engineer** (`agentic`) — Review goal and task decomposition, state and
   semantic contracts, tool and action interfaces, planning/execution/
   verification loops, recovery and cancellation, observability, and safe
   autonomy.
-- **Security and trust-boundary engineer** — Inspect user-controlled
+- **Security and trust-boundary engineer** (`security`) — Inspect user-controlled
   configuration and Task Programs, file paths, subprocesses, native or plugin
   boundaries, credentials, package loading, and CI permissions for unsafe
   execution, data exposure, privilege escalation, or fail-open behavior.
 
+Supplemental roles may be delegated separately or included as lenses in an
+existing expert's scope:
+
+- **Reliability / operations engineer** (`reliability`) — Review lifecycle,
+  restart, retries, idempotence, bounded queues, observability, and worker
+  failure handling when these need coverage beyond the engine or agentic pass.
+- **Verification / test quality engineer** (`verification`) — Check test oracles,
+  regression claims, negative and boundary cases, determinism, and whether
+  tests exercise the contract rather than mirror the implementation. This pass
+  is required for test or validation changes, but a separate agent is optional.
+
 ### Role selection
 
-Select perspectives from the changed files and their contract neighbors; do
-not apply every role as a generic checklist. Read
-[references/reviewer-perspectives.md](references/reviewer-perspectives.md) when
-the change touches public APIs, untrusted inputs, long-running operations, or
-test and release boundaries. Keep the evidence standard unchanged: a role may
-raise a finding only when a reachable scenario, violated contract, and concrete
-impact are established.
+After building the change model, read
+[references/reviewer-perspectives.md](references/reviewer-perspectives.md) to
+select roles from the changed files and their contract neighbors. Select only
+roles with a concrete review surface; do not start every role mechanically.
+Announce the selected role IDs, their scopes, and the execution mode before
+starting the expert passes. Explain omitted roles collectively when useful.
+
+Read and follow [references/expert-protocol.md](references/expert-protocol.md)
+for dispatch, independent results, lead verification, and fallback. This skill
+explicitly requests subagent delegation for selected expert reviews when the
+host permits it. A delegated expert performs only its assigned pass and returns
+the result contract; it does not spawn reviewers or write the final review.
 
 ## Review contract
 
@@ -182,7 +199,10 @@ and its observable output, error, state transition, or side effect.
 
 ## 3. Run focused review passes
 
-Apply every relevant pass, using the matrix for subsystem-specific contracts.
+Execute the selected expert passes using the expert protocol. Each expert
+applies the relevant checks below and the subsystem matrix within its scope;
+the lead covers remaining changed files and cross-role integration. Do not
+finalize until every selected role has a result or an explicit incomplete status.
 
 ### Correctness and state
 
@@ -322,17 +342,34 @@ omitting a row:
 | --- | --- |
 | Review target | `<base>...<head>`, PR number, commit range, or local working tree |
 | Scope | `<count>` changed files; affected subsystems |
+| Execution | `multi-agent`, `single-agent fallback`, or `mixed`; reason for fallback, if any |
 | Findings | `P0: <n>; P1: <n>; P2: <n>; P3: <n>` |
-| Merge assessment | `Block`, `Changes requested`, `Non-blocking findings`, or `No actionable findings` |
+| Merge assessment | `Block`, `Changes requested`, `Non-blocking findings`, `No actionable findings`, or `Incomplete` |
 | Validation | Focused commands and outcomes, or `Not run` |
 | Residual risks | Important untested or unavailable surfaces, or `None identified` |
 
 Use `Block` when any P0 or P1 finding exists, `Changes requested` when the
 highest finding is P2, `Non-blocking findings` when only P3 findings exist, and
-`No actionable findings` when every count is zero.
+`No actionable findings` when every count is zero and selected scopes are
+complete. If any selected scope remains unreviewed, use `Incomplete` unless
+confirmed P0/P1 findings already require `Block`; describe known findings and
+coverage gaps separately.
+
+After the review-summary table, include the role-coverage table, even when no
+findings survive lead verification. Localize labels, retain stable role IDs,
+and provide one row per selected role:
+
+| Role | Selection reason / scope | Executor / mode | Status / evidence | Candidate disposition |
+| --- | --- | --- | --- | --- |
+| `<role-id>` | Concrete changed contract | Agent ID or `lead / fallback` | `complete`, `partial`, or `unavailable`; result reference or concise inspected evidence and checks | Accepted, merged, rejected, unresolved candidate IDs; or `none` |
+
+Only actual returned results count as completed expert passes. A zero-finding
+result is valid; a missing or failed result is not. Link saved reports when
+available; otherwise summarize inspected paths/contracts and validation limits.
+Explain unselected roles briefly outside the table; never mark them completed.
+Report incomplete scope as residual risk, not as a code defect.
 
-After the review-summary table, add only the supporting sections that need more
-detail:
+Then add only the supporting sections that need more detail:
 
 - **Open questions / assumptions** — facts that could change the conclusion.
 - **Validation details** — commands run, results, and important checks not run.

.agents/skills/review-pr/agents/openai.yaml

@@ -1,4 +1,4 @@
 interface:
   display_name: "Review EmbodiChain PR"
-  short_description: "Find actionable regressions in EmbodiChain changes"
-  default_prompt: "Use $review-pr to review this EmbodiChain pull request, report evidence-backed findings, summarize each finding in a Markdown table, and finish with a separate review-summary table."
+  short_description: "Coordinate expert reviews of EmbodiChain changes"
+  default_prompt: "Use $review-pr to review this EmbodiChain pull request with the expert protocol, dispatch relevant independent reviewers when available, and report verified findings, findings and review-summary tables, and role coverage with any fallback disclosed."

.agents/skills/review-pr/references/expert-protocol.md

@@ -0,0 +1,111 @@
+# Expert review protocol
+
+## Lead preparation and dispatch
+
+1. Pin the review target: repository, base/head commits (or a local diff
+   snapshot including untracked files), changed-file inventory, and intended
+   behavior. All reviewers must use the same delta. Read PR files at the pinned
+   head, not an unrelated working tree. If the target changes, reconcile the
+   affected passes before calling the review complete.
+2. Select relevant role IDs using `reviewer-perspectives.md`. Usually two to
+   four roles suffice for a substantive change; a narrow change may need only
+   one. This is not a quota: cover every affected contract, with bounded scopes
+   and intentional overlap at integration boundaries.
+3. When subagents are available and permitted, dispatch independent selected
+   passes using the host's subagent tool (for example `spawn_agent`). Respect
+   concurrency limits, queue remaining roles, and keep the lead working on
+   uncovered files and integration while experts run. Do not substitute
+   user-owned tasks/threads for subagents. Tool availability alone does not
+   override host restrictions or a user's request for single-agent review.
+4. Give each expert the packet below. Prefer a fresh context containing raw
+   artifacts rather than the lead's candidate findings or another expert's
+   conclusions. Independent first passes reduce anchoring; exchange evidence
+   later when resolving a concrete disagreement. An expert may inspect contract
+   neighbors beyond its initial file list when needed to establish reachability.
+
+Use a task packet with these fields (Markdown is sufficient):
+
+For a dedicated supplemental pass, use the `reliability` or `verification`
+role ID and responsibilities defined in SKILL.md consistently in the packet,
+result, candidate IDs, and role-coverage row. If the lens is included in an
+existing expert's assignment, retain that expert's role ID and name the lens
+in its scope, returned coverage, and coverage-table evidence. For example, an
+`engine` reviewer covering `reliability` returns `engine-*` candidates under
+the `engine` row; a dedicated `reliability` reviewer returns `reliability-*`
+candidates under its own row. Do not count an embedded lens as an independent
+reviewer or add a second completed role row for it.
+
+```text
+Assignment: delegated expert pass; do not delegate further.
+Role: <stable role ID and its responsibilities from SKILL.md>
+Target: <repository; base/head SHA or snapshot; diff access>
+Scope: <changed files/contracts; questions to investigate, without predicted answers>
+Context: <applicable AGENTS.md; canonical skill directory; matched context paths>
+Constraints: read-only review; no Git mutation, edits, or remote publication;
+             follow the skill's proportional validation limits.
+Return: the expert result contract below, including zero findings and limitations.
+```
+
+An expert loads its assigned role, applicable review contract, matrix sections,
+and evidence standard from the canonical skill; it must not restart the lead's
+dispatch workflow. The lead alone owns the final report. Remote publication,
+when separately requested, remains governed by SKILL.md.
+
+## Expert result contract
+
+Every selected role returns a result, even when it finds no actionable defect:
+
+- **Identity and target:** role ID, actual agent ID when available, reviewed
+  base/head or snapshot identifier.
+- **Coverage and status:** `complete` or `partial` for the assigned scope;
+  inspected changed files and important callers/contracts; explicit omissions.
+- **Candidates:** use stable local IDs such as `robotics-1`. For each, give
+  proposed priority, smallest changed location, reachable input/state, violated
+  contract, observable impact, and supporting code or reproduction. Explain
+  guards/tests checked, how behavior differs from the base, and a correction
+  direction when useful. Return `none` rather than inventing a finding.
+- **Validation and limitations:** actual commands/results, checks not run,
+  unresolved questions, and unavailable runtime or dependency surfaces.
+
+This is an evidence summary, not a request to expose private reasoning. Do not
+require one defect per expert. A complete static pass may still disclose that
+GPU/hardware execution was not tested; distinguish that limit from files or
+contracts the expert could not inspect.
+
+## Lead verification and synthesis
+
+Wait for every dispatched result, or record its failure and missing scope.
+Check that each result used the pinned target and covers the assigned contract;
+request a focused follow-up for an omitted scope or unsupported claim. Do not
+label a launched, timed-out, or missing reviewer as complete.
+
+For each candidate, the lead independently checks the cited code and reachable
+scenario under SKILL.md's proof standard. Agreement between experts is not
+proof, and a minority finding may still be correct. Deduplicate by root cause,
+retain contributing candidate IDs, and assign final priority by demonstrated
+impact. Record each candidate as accepted, merged into another candidate,
+rejected with a short evidence-based reason, or unresolved. Unresolved claims
+belong in open questions or residual risks, not confirmed findings.
+
+The lead checks the full changed-file inventory against expert and lead
+coverage, reviews remaining integration gaps, then produces the findings,
+findings-summary, review-summary, and role-coverage tables from SKILL.md.
+The coverage table records actual executors, results, and candidate dispositions;
+it must not imply that naming a role proves an independent review occurred.
+
+## Fallback and interrupted work
+
+If subagent tools are unavailable, prohibited, explicitly disabled by the user,
+or fail, disclose the specific reason. Execute each remaining selected role as
+a separate lead pass with the same result contract and explicit role scope.
+Label this `single-agent fallback`; if any independent expert results were
+also used, label the overall run `mixed`. Do not claim independent contexts
+or fabricate agent IDs for lead passes.
+
+Temporary capacity limits normally require queuing, not silent fallback.
+After a dispatch or worker failure, one bounded retry is reasonable; otherwise
+complete the missing scope locally or report it as incomplete. Preserve valid
+results and rerun only missing coverage. If the task ends with an unreviewed
+scope, report `partial`/`unavailable` and the reason, and apply SKILL.md's
+incomplete-review assessment. Never translate an unavailable expert into
+"no findings" or an approval recommendation.