diff --git a/.agents/skills/review-pr/SKILL.md b/.agents/skills/review-pr/SKILL.md index 049d9fce4..93e5198f6 100644 --- a/.agents/skills/review-pr/SKILL.md +++ b/.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 | `...`, PR number, commit range, or local working tree | | Scope | `` changed files; affected subsystems | +| Execution | `multi-agent`, `single-agent fallback`, or `mixed`; reason for fallback, if any | | Findings | `P0: ; P1: ; P2: ; P3: ` | -| 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 | +| --- | --- | --- | --- | --- | +| `` | 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. diff --git a/.agents/skills/review-pr/agents/openai.yaml b/.agents/skills/review-pr/agents/openai.yaml index eb86ad445..d05ccc4be 100644 --- a/.agents/skills/review-pr/agents/openai.yaml +++ b/.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." diff --git a/.agents/skills/review-pr/references/expert-protocol.md b/.agents/skills/review-pr/references/expert-protocol.md new file mode 100644 index 000000000..a6b0e49de --- /dev/null +++ b/.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: +Target: +Scope: +Context: +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. diff --git a/.agents/skills/review-pr/references/reviewer-perspectives.md b/.agents/skills/review-pr/references/reviewer-perspectives.md index 8da78d23a..ce4943493 100644 --- a/.agents/skills/review-pr/references/reviewer-perspectives.md +++ b/.agents/skills/review-pr/references/reviewer-perspectives.md @@ -1,18 +1,25 @@ # Reviewer Perspectives -Use this reference to select review lenses after the changed files and their -contract neighbors are identified. It complements `review-matrix.md`: the +Use this reference to select expert roles and supplemental review lenses after +the changed files and their contract neighbors are identified. It complements +`review-matrix.md`: the matrix describes subsystem contracts, while this file describes the questions to ask from a user's or operator's point of view. ## Perspective selection -| Change signal | Select this perspective | Evidence to inspect | +| Change signal | Role ID / lens | Evidence to inspect | | --- | --- | --- | -| Public Python modules, `__all__`, config classes, YAML/JSON schemas, CLI, task IDs, package data, examples, or migration behavior | Public API / developer experience | Public facades, strict loaders, callers, registration, docs checker, installed-package paths, and round-trip tests | -| User-controlled files or programs, path resolution, dynamic imports, subprocesses, native extensions, plugins, secrets, or workflows | Security and trust boundary | Loader and validation boundaries, path joins, execution APIs, dependency loading, workflow permissions, and failure paths | -| Reset/close/cancel, retries, checkpoint/resume, asynchronous workers, distributed execution, or long-running services | Reliability / operations (conditional lens) | Lifecycle owners, cleanup, idempotence, restart state, bounded queues, observability, and worker-failure handling | -| Tests, fixtures, mocks, validation logic, or regression claims | Verification / test quality (mandatory pass, not a separate identity) | Test oracle, old-versus-new behavior, negative and boundary cases, partial batches, cleanup, determinism, and installation-level coverage | +| Ownership, dependency direction, cross-subsystem contracts, new extension points | `architecture` | Producers and consumers, layering, registries, ownership, and contract migration | +| Hot paths, tensor batching, allocations, CPU/GPU transfers, synchronization, parallel execution | `hpc` | Call frequency, shapes, complexity, transfer and allocation sites, and contention | +| Simulation/render lifecycle, backend state, scheduling, reset/close or resources | `engine` | Lifecycle owners, backend interfaces, update order, cleanup, and recovery | +| Frames, units, kinematics, dynamics, planning, collision, joint limits, control timing | `robotics` | Transform conventions, numerical assumptions, motion constraints, and physical feasibility | +| Datasets, policies, training/evaluation, rewards, metrics or reproducibility | `ai` | Data provenance, splits, objectives, rollout semantics, and evaluation claims | +| Task Programs, semantic lowering, tools/actions, planning/execution/verification loops | `agentic` | Goal/state contracts, action outcomes, recovery, cancellation, and observability | +| Public Python modules, `__all__`, config classes, YAML/JSON schemas, CLI, task IDs, package data, examples, or migration behavior | `api` | Public facades, strict loaders, callers, registration, docs checker, installed-package paths, and round-trip tests | +| User-controlled files or programs, path resolution, dynamic imports, subprocesses, native extensions, plugins, secrets, or workflows | `security` | Loader and validation boundaries, path joins, execution APIs, dependency loading, workflow permissions, and failure paths | +| Reset/close/cancel, retries, checkpoint/resume, asynchronous workers, distributed execution, or long-running services | `reliability` supplemental lens | Lifecycle owners, cleanup, idempotence, restart state, bounded queues, observability, and worker-failure handling | +| Tests, fixtures, mocks, validation logic, or regression claims | `verification` mandatory pass | Test oracle, old-versus-new behavior, negative and boundary cases, partial batches, cleanup, determinism, and installation-level coverage | ## Public API / developer experience @@ -43,11 +50,11 @@ dynamic execution, path escape, dependency confusion, credential exposure, and workflow permission expansion as findings only when the changed code makes the scenario reachable and the impact concrete. -## Conditional perspectives +## Supplemental lenses -Reliability and verification are review lenses rather than mandatory reviewer -identities. Use reliability for lifecycle and restart behavior that is not -fully explained by the engine or agentic passes. Use verification for every -test or validation change, checking that tests observe the contract and would -fail on the relevant old behavior rather than merely mirroring the -implementation. +`reliability` and `verification` have stable role IDs and responsibilities in +`SKILL.md` under Reviewer identity. Use `reliability` when lifecycle or restart +coverage needs a supplemental pass; `verification` remains required for test +or validation changes. Separate agents are optional. Follow the +[expert protocol](expert-protocol.md) to record either a dedicated supplemental +role or a lens within an existing role's scope consistently.