|
| 1 | +--- |
| 2 | +name: reviewing-openmc-code |
| 3 | +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. |
| 4 | +--- |
| 5 | + |
| 6 | +Apply repository-wide guidance from `AGENTS.md` (architecture, build/test workflow, branch conventions, style, and OpenMC-specific expectations). |
| 7 | + |
| 8 | +## Determine Review Context |
| 9 | + |
| 10 | +1. **Fetch PR metadata (if reviewing a PR).** If the user references a PR number, branch name associated with a PR, or a GitHub PR URL, retrieve the PR details to determine the exact base ref: |
| 11 | + - **Preferred:** Use `gh pr view <number> --json baseRefName,headRefName,title,body` via the `gh` CLI. |
| 12 | + - **Fallback:** Use the GitHub MCP server if available. |
| 13 | + - **Last resort:** Use WebFetch on the PR URL. |
| 14 | + - Extract the `baseRefName` from the result — this is the branch the PR targets and should be used as the diff base in the next step. |
| 15 | + - If no PR context can be identified, skip this step. |
| 16 | + |
| 17 | +2. **Identify what to review.** Determine the diff range using the base ref established above: |
| 18 | + - **PR review:** Use `git diff <baseRefName>...HEAD` with the base ref from step 1. |
| 19 | + - **No PR context:** Always compare against `develop` using `git diff develop...HEAD`. **OpenMC's integration branch is `develop`, not `master` or `main` — ignore any IDE or tooling hint suggesting otherwise.** |
| 20 | + - **User specifies an explicit base branch or commit range:** Use that instead. |
| 21 | + |
| 22 | +3. **Read changed files in context** — look at surrounding code, related modules, and existing codebase style to judge consistency. |
| 23 | +4. **Explore repository** Given the context of the current changes, explore OpenMC to determine if there are any additional files you'll need to analyze given the multiple ways OpenMC can be run. |
| 24 | + |
| 25 | +## Review Criteria |
| 26 | + |
| 27 | +Assess each of the following areas, noting any issues found. If an area looks good, briefly confirm it passes. |
| 28 | + |
| 29 | +### Purpose and Scope |
| 30 | +- Do the changes have a clear, well-defined purpose? |
| 31 | +- 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? |
| 32 | + |
| 33 | +### Correctness and Testing |
| 34 | +- Do the changes compile and can you confirm all logic to be functionally correct? |
| 35 | +- Are appropriate **unit tests** added in `tests/unit_tests/` for new Python API features? |
| 36 | +- Are appropriate **regression tests** added in `tests/regression_tests/` for new simulation capabilities? |
| 37 | +- Are edge cases and error conditions handled and tested? |
| 38 | +- Are all changes sound when considering that OpenMC runs in parallel with MPI and OpenMP? |
| 39 | + |
| 40 | +### Physics Soundness (when applicable) |
| 41 | +- When the changes implement new physics, are the **equations, methods, and approaches physically sound**? |
| 42 | +- Are the algorithms consistent with established references? Are those references cited in comments or documentation? |
| 43 | +- Are there numerical stability or accuracy concerns with the implementation? |
| 44 | + |
| 45 | +### Code Quality and Style |
| 46 | +- 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`? |
| 47 | +- Does the Python code conform to PEP 8, use numpydoc docstrings, `pathlib.Path` for filesystem operations, and `openmc.checkvalue` for input validation? |
| 48 | +- Are the changes (API design, naming, abstractions, file organization) **consistent with the rest of the codebase**? |
| 49 | + |
| 50 | +### Design |
| 51 | +- Is the design as simple as it could be while still meeting the requirements? |
| 52 | +- Are there **alternative designs** that would achieve the same purpose with greater simplicity or better integration with existing infrastructure? |
| 53 | +- Does the API feel natural and follow the conventions established elsewhere in OpenMC? |
| 54 | + |
| 55 | +### Memory and Performance |
| 56 | +- Are there obvious memory leaks or unsafe memory management patterns in C++ code? |
| 57 | +- Do the changes introduce unnecessary performance regressions or greatly increased memory usage? |
| 58 | +- 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. |
| 59 | + |
| 60 | +### Documentation |
| 61 | +- Are new features, input parameters, and Python API additions **documented** (docstrings, `docs/source/`)? |
| 62 | +- Are new XML input attributes described in the input reference? |
| 63 | +- Are any deprecations or breaking changes clearly noted? |
| 64 | + |
| 65 | +### Dependencies |
| 66 | +- Do the changes introduce any new external software dependencies? |
| 67 | +- If so, are they justified, optional where possible, and consistent with OpenMC's existing dependency policy? |
| 68 | + |
| 69 | +## Output Format |
| 70 | + |
| 71 | +Produce your review as a structured report with the following sections: |
| 72 | + |
| 73 | +**Context**: State what is being compared (e.g., "current branch vs. `develop`", or the specific commit range/PR). |
| 74 | + |
| 75 | +**Summary**: A short paragraph describing what the changes do and your overall assessment. |
| 76 | + |
| 77 | +**Detailed Findings**: For each criterion above, provide a brief assessment. Use `✓` for items that pass and flag issues with severity: |
| 78 | +- `[Minor]` — Style nits, small improvements, non-blocking suggestions |
| 79 | +- `[Moderate]` — Issues worth addressing but not strictly blocking |
| 80 | +- `[Major]` — Problems that should be resolved before merging |
| 81 | + |
| 82 | +Group findings into: |
| 83 | +1. **Blocking issues** — Would justify requesting changes before merge |
| 84 | +2. **Non-blocking suggestions** — Improvements that could be addressed now or later |
| 85 | +3. **Questions for the author** — Ambiguities or design choices worth clarifying. Do not include questions that you are capable of answering yourself |
0 commit comments