From efbc505690f6a8dc098dc7dd7d490296b596d4dc Mon Sep 17 00:00:00 2001 From: yuecideng Date: Tue, 1 Sep 2026 21:54:42 +0800 Subject: [PATCH] feat(agent): add pull request review skill --- .agents/skills/review-pr/SKILL.md | 246 +++++++++++++++ .agents/skills/review-pr/agents/openai.yaml | 4 + .../review-pr/references/review-matrix.md | 286 ++++++++++++++++++ .claude/skills/review-pr/SKILL.md | 19 ++ .github/copilot/instructions.md | 1 + .github/copilot/review-pr.md | 7 + AGENTS.md | 6 + 7 files changed, 569 insertions(+) create mode 100644 .agents/skills/review-pr/SKILL.md create mode 100644 .agents/skills/review-pr/agents/openai.yaml create mode 100644 .agents/skills/review-pr/references/review-matrix.md create mode 100644 .claude/skills/review-pr/SKILL.md create mode 100644 .github/copilot/review-pr.md diff --git a/.agents/skills/review-pr/SKILL.md b/.agents/skills/review-pr/SKILL.md new file mode 100644 index 000000000..417449d3d --- /dev/null +++ b/.agents/skills/review-pr/SKILL.md @@ -0,0 +1,246 @@ +--- +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. +--- + +# Review EmbodiChain Changes + +Perform a read-only, defect-focused review. Trace each change through the +project's simulation, environment, task, planning, learning, configuration, +and packaging boundaries before deciding whether it is safe. + +## Review contract + +- Treat review and implementation as separate tasks. Do not edit files, + approve a remote PR, submit comments, or change Git state unless explicitly + requested. +- Report actionable defects introduced or exposed by the reviewed change. + Ignore cosmetic preferences and issues enforced mechanically by Black unless + they hide a correctness problem. +- Support every finding with a reachable scenario, the violated contract, and + a concrete impact. Do not promote speculation to a finding. +- Read enough surrounding code, callers, registries, configuration loaders, + tests, and documentation to disprove a candidate issue before reporting it. +- Review the tests as production code: confirm that they exercise the intended + behavior, fail on the old behavior when relevant, and do not merely mirror + the implementation. +- Continue through the full diff after finding an issue. For a large PR, keep a + file or subsystem coverage ledger and review dependency foundations before + their consumers. + +## 1. Resolve the review target + +Read the applicable `AGENTS.md` instructions first. Determine the exact delta +from the user's request and available metadata. + +For a local working tree, inspect all tracked, staged, and untracked changes: + +```bash +git status --short +git diff --stat +git diff +git diff --cached +git ls-files --others --exclude-standard +``` + +For a branch or commit range, determine the real base branch from PR metadata, +the upstream configuration, or the user's request. Record any assumed base, +then use its merge base: + +```bash +git merge-base HEAD +git diff --stat ...HEAD +git diff --find-renames ...HEAD +``` + +For a GitHub PR, read its title, body, base/head branches, commits, changed +files, checks, and diff with a read-only GitHub connector or `gh pr view` / +`gh pr diff`. Do not checkout, pull, rebase, or fetch merely to perform a +review. For a stacked PR, review only the layer's base-to-head delta; mention +dependency assumptions separately. + +If the target remains ambiguous and different choices would materially change +the findings, ask for the base or PR identifier instead of guessing. + +## 2. Build a change model + +1. State the intended behavior in one or two sentences from the request, PR + body, issue, tests, and diff. Treat the description as intent, not proof. +2. Inventory changed files by subsystem and identify changes to public APIs, + serialized configuration, registration, defaults, execution order, tensor + contracts, resource ownership, and package contents. +3. Read `agent_context/MAP.yaml`. Match changed symbols and paths to topic IDs, + then load only the matched topic files from each topic's `paths`. Verify + relevant facts against current `source_of_truth` files. Follow + `related_topics` only when the diff crosses that contract boundary. +4. If no topic matches, use `rg --files` and `rg -n` to locate callers, + implementations, registries, exports, tests, config loaders, and entry + points. Do not read `docs/source/` unless public documentation is part of the + review surface. +5. Read the common checks and only the affected subsystem sections in + [references/review-matrix.md](references/review-matrix.md). + +Do not review a changed function in isolation. Trace at least one complete +resolution path from external input or configuration to the changed behavior +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. + +### Correctness and state + +Check happy paths, boundary values, invalid input, partial batches, exception +paths, state transitions, mutation and aliasing, ordering, defaults, and stale +caches. For tensor code, verify shapes, indexing, dtype, device, broadcasting, +autograd behavior, and empty or singleton batches. For robotics code, verify +units, coordinate frames, joint/link identity, limits, and timing. + +### Integration and architecture + +Trace imports, `__all__`, registries, entry points, factory dispatch, +configuration composition, task discovery, semantic lowering, package data, +and CLI paths. Check both sides of every changed contract, especially when the +producer and consumer live in different packages or configuration files. + +### Compatibility and migration + +Look for unannounced breaks to public Python APIs, YAML/JSON schemas, saved +checkpoints or datasets, environment IDs, defaults, component ownership, task +package discovery, and third-party extension points. Accept a breaking change +only when it is intentional, consistently implemented, documented, and covered +by migration or clear failure behavior appropriate to the project. + +### Resources, concurrency, and performance + +Check simulator and renderer lifecycle, cleanup queues, GPU/VRAM ownership, +device initialization, asynchronous writers, process boundaries, cancellation, +safe stop behavior, locks, deterministic seeding, and error cleanup. Flag +performance only when the diff introduces a material algorithmic, allocation, +synchronization, or per-environment regression on a reachable hot path. + +### Validation and documentation + +Check whether focused tests cover the changed ownership boundary, including a +negative or failure path when validation logic changed. Select the smallest +read-only command that can confirm or refute a candidate issue; do not run the +full suite by default. Start with static evidence, and do not launch live +simulation, GPU, renderer, or distributed tests unless the user requests them +or static evidence cannot resolve a material candidate. Before any command +likely to take more than two minutes, explain its scope and why a narrower probe +is insufficient. Keep these responsibilities distinct: + +- Use `$review-pr` to analyze change safety and report defects. +- Use `$pre-commit-check` for comprehensive pre-commit gates and proportional + validation. +- Use `$pr` to draft, label, push, or create PRs. +- Use the matching `add-*` or `update-*` skill only after the user asks to fix + a finding. + +Require public docs or agent-context updates only when the change makes those +artifacts materially incomplete or incorrect. A behavior change covered by an +`agent_context/MAP.yaml` topic must update its mapped context according to the +project context update contract. + +## 4. Prove each candidate finding + +Before reporting a candidate: + +1. Locate the smallest changed line range that causes or exposes the problem. +2. Confirm the behavior differs from the chosen base and is not merely an + unrelated pre-existing issue. +3. Identify a valid input, configuration, runtime state, or caller that reaches + the line. +4. Check for guards, normalization, cleanup, retries, or downstream handling + that may invalidate the concern. +5. Explain the observable consequence: wrong result, crash, silent data + corruption, unsafe motion, compatibility break, leaked resource, or credible + test/CI escape. +6. Run a narrow reproduction or test when static evidence is not decisive and + the command is safe and proportionate. + +If one of these cannot be established, omit the finding or record it as an +explicit open question. Do not use vague language such as "might break" without +the conditions that make it break. + +## 5. Assign priority + +- **P0 — Critical:** Unconditional or broadly reachable catastrophic impact, + such as destructive data loss, unsafe robot behavior, or a repository-wide + outage. Stop and surface it immediately. +- **P1 — High:** A common path is broken, a release or core workflow is blocked, + or correctness/safety is seriously compromised. Fix before merge. +- **P2 — Medium:** A real defect affects a narrower but supported scenario, + architecture contract, or maintainability boundary. Normally fix before + merge. +- **P3 — Low:** A limited, non-cosmetic defect with minor impact. Fix when + practical. + +Do not inflate priority because a subsystem is important; rank the demonstrated +impact and reachability of this specific change. + +## 6. Write the review + +Put findings first, ordered by priority and then file order. Use one item per +root cause: + +```text +[P1] Short imperative title +path/to/file.py: + +Under , this code . . . +``` + +Keep the cited line range tight and prefer changed lines. Make the title +specific enough to understand without opening the body. Do not include a full +patch unless the user asks for one. + +Make each cited location clickable when the environment supports it. For a +remote PR, prefer an immutable head-commit blob link anchored to the smallest +changed line range. For a local target, use the environment's clickable +absolute or workspace-resolvable file link with a line number. + +After the detailed findings, always provide a distinct findings-summary table. +Localize the labels to the response language, preserve the same priority and +file order as the detailed findings, and keep one row per root cause: + +| Priority | Location | Defect | Impact | Recommended action | +| --- | --- | --- | --- | --- | +| `P0`-`P3` | Clickable `path:line` | Concise root cause | Observable consequence | Concise correction direction | + +This table summarizes rather than replaces the evidence-backed finding bodies. +If there are no actionable findings, still render one row with `N/A` for the +priority and location and `No actionable findings` for the defect. A separate +review-summary table does not satisfy a request to summarize findings in +table form. + +Then provide a compact review-summary table. Localize the labels to the +response language, keep cells concise, and use `None` or `N/A` instead of +omitting a row: + +| Review item | Result | +| --- | --- | +| Review target | `...`, PR number, commit range, or local working tree | +| Scope | `` changed files; affected subsystems | +| Findings | `P0: ; P1: ; P2: ; P3: ` | +| Merge assessment | `Block`, `Changes requested`, `Non-blocking findings`, or `No actionable findings` | +| 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. + +After the review-summary table, 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. +- **Residual-risk details** — untested GPU, renderer, hardware, distributed, + or live simulation paths. + +If there are no actionable findings, say so explicitly and still identify +material residual risks or validation gaps. Do not claim the change is proven +correct solely because tests pass. diff --git a/.agents/skills/review-pr/agents/openai.yaml b/.agents/skills/review-pr/agents/openai.yaml new file mode 100644 index 000000000..eb86ad445 --- /dev/null +++ b/.agents/skills/review-pr/agents/openai.yaml @@ -0,0 +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." diff --git a/.agents/skills/review-pr/references/review-matrix.md b/.agents/skills/review-pr/references/review-matrix.md new file mode 100644 index 000000000..3627b79ae --- /dev/null +++ b/.agents/skills/review-pr/references/review-matrix.md @@ -0,0 +1,286 @@ +# EmbodiChain Review Matrix + +Use the common checks for every review, then load only the sections matching +the changed files and their contract neighbors. Prefer `agent_context/MAP.yaml` +and current source code when this matrix and the repository diverge. + +## Contents + +- [Common checks](#common-checks) +- [Simulation core and scene objects](#simulation-core-and-scene-objects) +- [Gym environments, managers, functors, and randomization](#gym-environments-managers-functors-and-randomization) +- [Task Programs and configured deployments](#task-programs-and-configured-deployments) +- [Atomic actions, motion planning, and IK](#atomic-actions-motion-planning-and-ik) +- [Robots, sensors, and visualization](#robots-sensors-and-visualization) +- [Reinforcement learning and data pipelines](#reinforcement-learning-and-data-pipelines) +- [Official tasks and configuration packages](#official-tasks-and-configuration-packages) +- [Packaging, CLI, workflows, native code, and docs](#packaging-cli-workflows-native-code-and-docs) +- [Agent skills and project context](#agent-skills-and-project-context) + +## Common checks + +- Preserve the distribution name `embodichain`, the core import package + `embodichain`, and the bundled task import package `embodichain_tasks`. +- Check the complete call or configuration-resolution path, not only the + changed definition. Inspect its closest tests and at least one caller or + consumer. +- Check public Python modules for the Apache header, + `from __future__ import annotations`, typed public APIs, meaningful Google + docstrings, static `__all__`, and matching API documentation when the public + contract changes. +- Check configuration objects for `@configclass`, required fields expressed + with `dataclasses.MISSING`, safe nested defaults, and `to_dict` / `from_dict` + round trips where serialization is supported. +- Check file paths relative to the owning config or package, not the reviewer's + current working directory. Reject unchecked path escape when inputs are not + trusted. +- Check failure behavior as closely as success behavior: validation should fail + at the earliest owning boundary with a useful error and without partially + initialized state. +- Check that tests target observable contracts and include boundary, negative, + partial-batch, or cleanup cases appropriate to the change. +- Prefer focused tests. Treat GPU, renderer, real-simulation, and distributed + paths as separate resource classes and report them as residual risk when they + cannot be run. + +## Simulation core and scene objects + +**Paths:** `embodichain/lab/sim/**`, plus environment code that owns a +`SimulationManager`. + +Check: + +- Simulator creation, update, reset, destruction, and cleanup-queue ordering; + ensure exception paths release scenes, GPU resources, and renderer state. +- Vectorized state shape and `env_ids` semantics, including non-contiguous, + empty, singleton, and partial-reset selections. +- Tensor dtype/device consistency and avoidance of implicit CPU copies or + per-step allocations on hot paths. +- Physics-step versus render/update ordering, stale handles after resets, and + cache invalidation after topology or asset changes. +- World/local/link coordinate frames, meters/radians conventions, quaternion + order, joint/link indices, mimic joints, limits, and asset scale. +- Scene UID uniqueness, object lookup, articulation/rigid/soft/cloth ownership, + and behavior when an asset is absent or only partly initialized. + +Evidence surfaces: the matching object/config module, `sim_manager.py`, +`base_env.py` or `embodied_env.py`, and the nearest `tests/sim/**` or +simulation-marked test. + +## Gym environments, managers, functors, and randomization + +**Paths:** `embodichain/lab/gym/**`, especially `envs/managers/**`, +`action_bank/**`, and wrappers. + +Check: + +- Gymnasium reset/step contracts, observation and action spaces, reward shape, + `terminated` versus `truncated`, episode counters, and reset ordering. +- Manager execution order and data dependencies. Verify that observation, + action, reward, event, record, and dataset managers see the intended state. +- Function-style functors accept `(env, env_ids, ...)`; class-style functors + implement `__init__(cfg, env)` and `__call__(env, env_ids, ...)`. +- All functors respect the selected `env_ids` and do not overwrite unaffected + rows. Check broadcast parameters and device placement. +- Startup/reset/interval randomization modes, deterministic seeding, legal + physics/geometry ranges, and independence across environments. +- Wrappers preserve spaces, metadata, reset options, info dictionaries, and + the underlying completion semantics. +- Task discovery occurs through installed task packages and init hooks; do not + assume importing `embodichain.lab.gym.envs` registers official tasks. + +Evidence surfaces: manager base/config, production registration and component +loaders, the owning task config, and focused `tests/gym/**` tests. + +## Task Programs and configured deployments + +**Paths:** `embodichain/lab/task_program/**`, +`embodichain/lab/gym/envs/task_program/**`, and task `program.yaml`, +`integration.yaml`, `task.*.yaml`, or component files. + +Check: + +- Strict language decoding rejects unknown or malformed fields before compile + or runtime; AST validation, compiler lowering, and runtime execution agree on + node semantics. +- Registered Semantic Calls have coherent descriptors, schemas, lowerers, + catalog entries, runtime services, tests, and public exports when applicable. +- Physical and semantic ownership stays separated: reusable `env.yaml` and + scene components remain physical; integration-owned `scene_binding` contains + semantic roots and affordances. +- A deployment chooses exactly one of environment component or inline + environment/scene, and exactly one of embodiment component or inline + robot/sensor. Component files do not add compatibility `version` fields. +- An embodiment owns one robot, sensors, and optional `skill_profile`; programs + remain embodiment-independent and trusted integration binds concrete IDs. +- Scene-binding targets exist in the physical scene and preserve canonical + entity identity through composition, planning, effects, and evidence. +- Parallel branches claim disjoint resources or intentionally synchronize; + completion masks, cancellation, effect verification, and failure propagation + work per environment row. +- Relative component paths, execution policies, integration fingerprints, + package data, and dynamic environment registration survive installation. + +Evidence surfaces: language decoder/validation, compiler, semantic catalog, +configured composition/services, Gym registration/bridge, package-data tests, +and the `$add-task-program` read-only deployment inspector. + +## Atomic actions, motion planning, and IK + +**Paths:** `embodichain/lab/sim/atomic_actions/**`, `planners/**`, +`solvers/**`, and grasp/workspace utilities that feed plans. + +Check: + +- Goal, options, affordance, requirement, binding, plan, command, effect, and + evidence types remain coherent across registration, planning, compilation, + execution, tracking, and verification. +- Planning is side-effect free; execution does not silently re-plan or bypass + typed validation. Failures preserve useful causes and do not emit partial + unsafe commands. +- Resource claims, endpoint resolution, invocation revisions, per-row + eligibility, cancellation, safe stop/hold, and recovery policies remain + consistent under partial failure. +- Trajectory timing is explicit and consistent (`dt`, interpolation, velocity, + acceleration); concatenation does not duplicate or skip boundary samples. +- Planner collision worlds use canonical logical IDs, refresh dynamic + obstacles, handle empty geometry, and preserve batch-mode expectations. +- IK/FK respects base/tool frames, joint order and limits, batch shapes, + convergence/failure signaling, singularities, dtype/device, and backend + differences. +- New actions or solvers include registration, package exports, documentation, + focused unit tests, and benchmarks when performance claims are part of the + contract. + +Evidence surfaces: typed core contracts, engine/runner, simulation adapter, +planner/solver base class, registration/export modules, and the nearest +`tests/sim/atomic_actions/**`, planner, solver, or toolkit test. + +## Robots, sensors, and visualization + +**Paths:** `embodichain/lab/sim/robots/**`, `objects/robot.py`, +`sensors/**`, and `embodichain/lab/visualization/**`. + +Check: + +- `RobotCfg` defaults, URDF paths, serial-chain construction, control-part + definitions, joint-name/index mapping, drive properties, end effectors, and + supported variants stay aligned. +- Robot and embodiment configuration round trips do not lose nested solver, + drive, sensor, or skill-profile data. +- Sensor outputs match declared shape, dtype, device, frame, intrinsics, + extrinsics, clipping/range semantics, and update frequency for every backend. +- Camera/depth/point-cloud conversion handles batched environments, invalid + pixels, axis conventions, and headless or renderer-unavailable operation. +- Visualization is observational: starting, refreshing, or disconnecting the + browser must not change simulation state. Topology changes and resource + teardown must not leave stale nodes, callbacks, or background tasks. + +Evidence surfaces: base and concrete cfg/runtime classes, embodiment component +composition, asset-preview or visualization entry points, and focused robot, +sensor, or visualization tests. + +## Reinforcement learning and data pipelines + +**Paths:** `embodichain/learning/**`, `embodichain/data_pipeline/**`, and +environment managers that record datasets. + +Check: + +- Rollout time axes, environment axes, actions, rewards, values, log-probs, + dones, truncations, bootstrap values, masks, and advantage normalization + remain aligned. +- Differentiable paths avoid unintended `detach`, in-place autograd mutation, + device transfers, or non-differentiable environment adapters; ordinary PPO + paths do not accidentally retain graphs. +- Collector/trainer routing selects the intended algorithm and rollout kind; + checkpoint save/resume restores models, optimizers, counters, normalizers, + RNG state, and configuration needed for equivalent continuation. +- Evaluation does not mutate training state and handles vector completion, + deterministic policies, and task discovery consistently with training. +- Distributed workers initialize devices and seeds correctly, synchronize only + intended state, propagate failures, and clean up process groups. +- Dataset schemas, episode boundaries, timestamps, image encoding, buffering, + asynchronous finalization, error propagation, and partial writes preserve + data integrity and bounded memory. + +Evidence surfaces: algorithm, buffer, collector, trainer, RL environment, +evaluation, dataset manager/writer, and focused `tests/learning/**` or dataset +tests. Run GPU/distributed tests serially and only when justified. + +## Official tasks and configuration packages + +**Paths:** `embodichain_tasks/**` and reusable task components. + +Check: + +- Preserve the task-first layout under a task family and optional subdomain; + do not organize tasks by solution method. +- Keep `@register_env` in the task-named module when a Python entry point is + required. Do not add a same-named package or Python `scenario`/`mdp` layers + when configuration and manager functors own the behavior. +- Configuration-defined Task Programs may omit a Python task module only when + their runnable deployment supplies the required environment, embodiment, + program, integration, and execution policy composition. +- Environment IDs and registration are unique and discoverable after package + installation, not only from the source tree. +- JSON/YAML is validated through production strict loaders and component + composition, not only `yaml.safe_load`. Relative files and scene targets must + resolve after wheel installation. +- New or moved config/assets are present in wheel contents; deleted files are + not retained accidentally. Imports use `embodichain_tasks`, not a repository + folder assumption. + +Evidence surfaces: the task-named module, deployment/component files, +registration utilities, `pyproject.toml`/`setup.py`, task layout tests, package +data tests, and wheel-content validation. + +## Packaging, CLI, workflows, native code, and docs + +**Paths:** `pyproject.toml`, `setup.py`, `embodichain/__main__.py`, +`.github/**`, `scripts/**`, `docs/**`, and C++/CUDA extension sources. + +Check: + +- CLI commands discover task packages and initialize them before consuming + environment IDs; exit status, stderr/stdout, path handling, and optional + dependency failures remain useful. +- Python dependencies, optional extras, entry points, versions, build backend, + native extension flags, wheel membership, and import paths agree across + source and installed artifacts. +- Native interfaces preserve shape/dtype/device/contiguity contracts, lifetime + ownership, CPU/GPU fallback, error propagation, and build compatibility. +- Workflow changes preserve event filters, permissions, secrets isolation, + cache keys, artifacts, concurrency, and the intended separation of lint, + docs, non-simulation, simulation, distributed, GPU, and release jobs. +- Public API changes are reachable from documented import paths and pass the + read-only API docs checker. Documentation examples use current configuration + and package names. + +Evidence surfaces: CLI dispatch and tests, package metadata/build scripts, +workflow-specific tests or `actionlint`, API docs checker, Sphinx dummy build, +and a built wheel for packaging changes. + +## Agent skills and project context + +**Paths:** `.agents/skills/**`, `agent_context/**`, `.claude/skills/**`, +`.github/copilot/**`, and `AGENTS.md`. + +Check: + +- `.agents/skills//SKILL.md` remains the canonical implementation; + Claude and Copilot adapters stay thin and point to it instead of duplicating + instructions. +- Skill names are lowercase kebab-case, frontmatter contains only `name` and + `description`, trigger wording is specific, and `agents/openai.yaml` matches + the canonical behavior. +- References are linked directly from `SKILL.md`, scripts are deterministic and + tested, and no auxiliary README or process-history files are added. +- `agent_context/MAP.yaml` resolves topics by ID, aliases, then keywords; paths + and `source_of_truth` remain current. Behavior changes update the mapped topic + and routing adapters when required by the context update contract. +- New canonical skills pass `quick_validate.py`; adapters and the project skill + index expose them consistently. + +Evidence surfaces: the canonical skill, metadata, thin adapters, context map +and topic files, project instructions, and the Skill Creator validator. diff --git a/.claude/skills/review-pr/SKILL.md b/.claude/skills/review-pr/SKILL.md new file mode 100644 index 000000000..15a61894f --- /dev/null +++ b/.claude/skills/review-pr/SKILL.md @@ -0,0 +1,19 @@ +--- +name: review-pr +description: Claude adapter for the canonical EmbodiChain review-pr skill. +--- + +# Review PR - Claude Adapter + +Canonical source: `.agents/skills/review-pr/` + +## When to use + +- reviewing an EmbodiChain pull request, branch, commit, patch, or local diff +- finding correctness regressions and architecture-contract violations +- producing prioritized, evidence-backed review findings + +## Start here + +1. Use this adapter when the task asks for a change review. +2. Then follow `.agents/skills/review-pr/SKILL.md`. diff --git a/.github/copilot/instructions.md b/.github/copilot/instructions.md index 21a2561dd..7c496b1ca 100644 --- a/.github/copilot/instructions.md +++ b/.github/copilot/instructions.md @@ -19,5 +19,6 @@ follow the canonical routing rules in `.agents/skills/project-dev-context/`. - Add tests: `.github/copilot/add-test.md` - Update public API docs: `.github/copilot/update-api-docs.md` - Run pre-commit checks: `.github/copilot/pre-commit-check.md` +- Review pull requests and diffs: `.github/copilot/review-pr.md` - Draft or create pull requests: `.github/copilot/pr.md` - Write benchmarks: `.github/copilot/benchmark.md` diff --git a/.github/copilot/review-pr.md b/.github/copilot/review-pr.md new file mode 100644 index 000000000..a754ba5c2 --- /dev/null +++ b/.github/copilot/review-pr.md @@ -0,0 +1,7 @@ +# EmbodiChain PR Review for GitHub Copilot + +Canonical source: `.agents/skills/review-pr/` + +Use this adapter to review an EmbodiChain pull request, branch, commit, patch, +or local diff for actionable correctness and architecture findings. Then follow +`.agents/skills/review-pr/SKILL.md`. diff --git a/AGENTS.md b/AGENTS.md index 8225d6ee3..0e5a74cee 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -309,6 +309,11 @@ Include: 5. **Add tests** that prove your fix or feature works. 6. Use the `/pr` skill to create PRs following the project's template and label conventions. +Use the `/review-pr` skill for read-only, evidence-backed review of a PR, +branch, commit, patch, or working-tree diff. Keep review separate from +`/pre-commit-check`, which runs local readiness gates, and `/pr`, which drafts +or creates the pull request. + ### Adding a New Robot Refer to `docs/source/guides/add_robot.rst` for a detailed guide. The basic structure requires: @@ -373,5 +378,6 @@ Tool-specific adapter files should stay thin and point back to the canonical ski | Add Test | `/add-test` | Scaffold tests following project conventions | | Update API Docs | `/update-api-docs` | Document public exports reported by the read-only API checker | | Pre-Commit Check | `/pre-commit-check` | Run all local CI checks before committing | +| Review PR | `/review-pr` | Review changes for actionable regressions and architecture violations | | Create PR | `/pr` | Create a PR following the project template | | Benchmark | `/benchmark` | Write benchmark scripts for EmbodiChain modules |