diff --git a/.claude/skills/openmc-code-review/SKILL.md b/.claude/skills/openmc-code-review/SKILL.md new file mode 100644 index 00000000000..8abb8cbf3e8 --- /dev/null +++ b/.claude/skills/openmc-code-review/SKILL.md @@ -0,0 +1,84 @@ +--- +name: openmc-code-review +description: Reviews code changes in the OpenMC codebase against OpenMC's contribution criteria (correctness, testing, physics soundness, style, design, performance, docs, dependencies). Use when asked to review a PR, branch, patch, or set of code changes in OpenMC, or when an author wants to self-review changes before submitting a PR. +--- + +Apply repository-wide guidance from `AGENTS.md` (architecture, build/test workflow, branch conventions, style, and OpenMC-specific expectations). This skill adds a structured review process and rubric on top of that baseline knowledge. + +## Determine Review Context + +1. **Identify what to review.** Determine the review context based on what the user provides or what can be inferred: + - **Default (no explicit context):** Compare the current branch against `develop` using `git diff develop...HEAD` to see only commits unique to the current branch. + - **User specifies a base branch or commit range:** Use that instead. + - **PR context provided by tooling:** Use the base/head refs supplied by the tool. +2. **Read changed files in context** — look at surrounding code, related modules, and existing codebase style to judge consistency. +3. **Determine review mode:** + - **Self-review mode** (author preparing changes before submission): Emphasize likely reviewer objections, missing tests/docs, API naming consistency, and quick fixes that would speed up review. + - **Reviewer mode** (default): Provide a standard structured review suitable for a code review or PR review. + + If the user explicitly requests self-review, use self-review mode. Otherwise, default to reviewer mode. + +## Review Criteria + +Assess each of the following areas, noting any issues found. If an area looks good, briefly confirm it passes. + +### Purpose and Scope +- Do the changes have a clear, well-defined purpose? +- Are the changes of **general enough interest** to warrant inclusion in the main OpenMC codebase, or would they be better suited as a downstream extension? + +### Correctness and Testing +- Do the changes compile and appear functionally correct? +- Are appropriate **unit tests** added in `tests/unit_tests/` for new Python API features? +- Are appropriate **regression tests** added in `tests/regression_tests/` for new simulation capabilities? +- Are edge cases and error conditions handled and tested? + +### Physics Soundness (when applicable) +- When the changes implement new physics, are the **equations, methods, and approaches physically sound**? +- Are the algorithms consistent with established references? Are those references cited in comments or documentation? +- Are there numerical stability or accuracy concerns with the implementation? + +### Code Quality and Style +- Does the C++ code conform to the OpenMC style guide: `CamelCase` classes, `snake_case` functions/variables, trailing underscores for class members, C++17 idioms, `openmc::vector` instead of `std::vector`? +- Does the Python code conform to PEP 8, use numpydoc docstrings, `pathlib.Path` for filesystem operations, and `openmc.checkvalue` for input validation? +- Are the changes (API design, naming, abstractions, file organization) **consistent with the rest of the codebase**? + +### Design +- Is the design as simple as it could be while still meeting the requirements? +- Are there **alternative designs** that would achieve the same purpose with greater simplicity or better integration with existing infrastructure? +- Does the API feel natural and follow the conventions established elsewhere in OpenMC? + +### Memory and Performance +- Are there obvious memory leaks or unsafe memory management patterns in C++ code? +- Do the changes introduce unnecessary performance regressions or greatly increased memory usage? +- Do the changes introduce dynamic memory allocation (e.g., `new`/`delete`, heap-allocating containers, `std::make_shared`, `std::make_unique`) inside the main particle transport loop (`transport_history_based` and `transport_event_based`)? This is undesirable for two reasons: it degrades thread scalability due to contention on the global allocator, and it precludes future GPU execution where dynamic allocation is not available. + +### Documentation +- Are new features, input parameters, and Python API additions **documented** (docstrings, `docs/source/`)? +- Are new XML input attributes described in the input reference? +- Are any deprecations or breaking changes clearly noted? + +### Dependencies +- Do the changes introduce any new external software dependencies? +- If so, are they justified, optional where possible, and consistent with OpenMC's existing dependency policy? + +## Output Format + +Produce your review as a structured report with the following sections: + +**Context**: State the review mode (self-review or reviewer) and what is being compared (e.g., "current branch vs. `develop`", or the specific commit range/PR). + +**Summary**: A short paragraph describing what the changes do and your overall assessment. + +**Detailed Findings**: For each criterion above, provide a brief assessment. Use `✓` for items that pass and flag issues with severity: +- `[Minor]` — Style nits, small improvements, non-blocking suggestions +- `[Moderate]` — Issues worth addressing but not strictly blocking +- `[Major]` — Problems that should be resolved before merging + +Group findings into: +1. **Blocking issues** — Would justify requesting changes before merge +2. **Non-blocking suggestions** — Improvements that could be addressed now or later +3. **Questions for the author** — Ambiguities or design choices worth clarifying + +**Recommended Action**: +- *Reviewer mode*: One of — *Approve*, *Approve with minor suggestions*, *Request changes*, or *Reject* — with a brief justification. +- *Self-review mode*: A prioritized list of pre-submission fixes — missing tests, undocumented parameters, naming inconsistencies, or anything else likely to slow down review. diff --git a/.github/agents/Review.agent.md b/.github/agents/Review.agent.md new file mode 100644 index 00000000000..7e4a422d166 --- /dev/null +++ b/.github/agents/Review.agent.md @@ -0,0 +1,8 @@ +--- +name: Review +description: Reviews code changes on the current branch, evaluating them against OpenMC's contribution criteria and providing structured feedback. Useful for PR reviews and pre-submission self-review. +argument-hint: Optionally provide a focus area (e.g., "focus on physics correctness", "check Python API design", "self-review before PR"). If omitted, a full review is performed. +--- +You are an expert code reviewer for OpenMC. Use the `openmc-code-review` skill to perform a structured review of the code changes on the current branch. + +If the user provides a focus area, prioritize that section of the review. If the user requests a self-review, use the skill's self-review mode. diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 00000000000..231b9d89d95 --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1 @@ +When reviewing code changes in this repository, use the `openmc-code-review` skill. diff --git a/AGENTS.md b/AGENTS.md index d24eb68da67..a782b1e397f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -40,7 +40,7 @@ OpenMC uses a git flow branching model with two primary branches: ### Instructions for Code Review -When analyzing code changes on a feature or bugfix branch (e.g., when a user asks "what do you think of these changes?"), **compare the branch changes against `develop`, not `master`**. Pull requests are submitted to merge into `develop`, so differences relative to `develop` represent the actual proposed changes. Comparing against `master` will include unrelated changes from other features that have already been merged to `develop`. +When reviewing code changes in this repository, use the `openmc-code-review` skill. ### Workflow for contributors