Nvidia Physicsnemo Curator Physicsnemo Curator

Curator PR Reviewer

This skill provides a structured, opinionated code review workflow for PhysicsNeMo Curator pull requests. It checks changed code against the project's API conventions, quality gates, and style patterns, then produces a prioritized findings report.

Report-only — no automatic fixes are applied. The reviewer presents findings and the user decides what to act on.

When to Use

• Reviewing any PR to NVIDIA/physicsnemo-curator
• Self-reviewing your own work before requesting external review
• After Greptile review to catch project-specific issues it may miss

Priority Levels

Workflow

Follow these steps in order. Use the TodoWrite tool to track each pass.

Step 0: Fetch the PR

Identify the PR to review. Accept a PR number, URL, or auto-detect from the current branch.

# Fetch PR metadata and diff
gh pr view <number> --repo NVIDIA/physicsnemo-curator
gh pr diff <number> --repo NVIDIA/physicsnemo-curator

Record:

• PR number, title, author, branch
• List of changed files (additions, modifications, deletions)
• Total lines added / removed

Read each changed file in full (not just the diff) — context matters. For modified files, also read the base version to understand what changed.

Step 1: API Conformance (P0)

Check every new or modified Source, Filter, or Sink against the ABCs in src/physicsnemo_curator/core/base.py.

Source[T] checklist:

• name: ClassVar[str] declared
• description: ClassVar[str] declared
• params() returns list[Param] (classmethod)
• __len__() returns int
• __getitem__(index: int) returns Generator[T]
• __getitem__ raises IndexError for out-of-range
• Generator semantics: yields items lazily, no eager loading of all data
• File discovery happens in __init__, not __getitem__

Filter[T] checklist:

• name, description, params() present
• __call__(items: Generator[T]) -> Generator[T]
• Generator contract: iterates input lazily, yields results
• If stateful: flush() -> str | None implemented
• Cardinality documented (1:1, 1:N, N:fewer)

Sink[T] checklist:

• name, description, params() present
• __call__(items: Iterator[T], index: int) -> list[str]
• Returns list of written file paths
• Consumes iterator (does not convert to list unless necessary)

Pipeline checklist:

• Components are registered in the domain __init__.py
• Pipeline builder chain works: source.filter(...).write(...)

Flag any deviation as P0.

Step 2: Correctness & Safety (P0)

Check for:

• Missing error handling on I/O operations (file open, network calls)
• Broken generator contracts (returning instead of yielding, not iterating the input generator in a filter)
• Incorrect indexing (off-by-one, missing IndexError on sources)
• Resource leaks (unclosed files, sockets, database connections)
• Race conditions in parallel-safe code
• Mutable default arguments
• Bare except: clauses (should catch specific exceptions)
• Silent data loss (swallowing exceptions, skipping items without logging)

Flag any issue as P0.

Step 3: License & Headers (P0)

Every new file must start with:

# SPDX-FileCopyrightText: Copyright (c) 2025 - 2026 NVIDIA CORPORATION & AFFILIATES.
# SPDX-FileCopyrightText: All rights reserved.
# SPDX-License-Identifier: Apache-2.0
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
#     http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

For Rust files, use // comment syntax with the same text.

Check:

• All new files have the SPDX header
• Header matches the exact format above (not a paraphrase)
• Commit messages follow Conventional Commits format

Flag missing headers as P0.

Step 4: Quality Gates (P0)

Run the automated quality checks against changed files:

# Lint changed Python files
uv run ruff check <changed_files>

# Type-check (full project — ty doesn't support file-level checking)
uv run ty check --exclude 'examples-old/**' --exclude 'benchmarks/**'

# Docstring coverage
uv run interrogate

Check:

• No ruff errors or warnings on changed files
• No new ty errors introduced by the PR
• Interrogate still at 99%+ coverage
• For Rust changes: cargo clippy and cargo fmt --check clean

Any quality gate failure is P0.

Step 5: Test Coverage (P1)

Evaluate testing adequacy:

• Every new public class/function has at least one test
• Test file exists in the correct location (test/<domain>/)
• Correct markers applied:
  • @pytest.mark.requires("mesh") or @pytest.mark.requires("da")
  • @pytest.mark.e2e for end-to-end tests
  • @pytest.mark.slow for tests > 10s
• Edge cases covered (empty input, single item, error conditions)
• Stateful filters: flush tested, multi-batch accumulation tested
• Sinks: write, roundtrip, append (if applicable) tested
• Sources: __len__, __getitem__, IndexError, generator semantics tested

Run coverage on affected test files if feasible:

uv run pytest test/<domain>/<test_file> --cov=src/physicsnemo_curator/<module> --cov-report=term-missing

Missing tests for public API surfaces are P1. Missing edge case tests are P2.

Step 6: Performance & Profiling (P1)

Look for performance anti-patterns:

• Eager loading: list(generator) or [x for x in gen] where lazy iteration would work — Sources should yield lazily, filters should not materialize the full stream
• Unbounded memory accumulation: Growing lists/dicts without bounds in stateful filters (acceptable if flush() clears them)
• Unnecessary copies: .clone(), .copy(), deepcopy() where in-place mutation is safe
• Repeated I/O: Reading the same file multiple times when once would suffice
• Missing streaming: Loading entire datasets into memory instead of streaming through the pipeline
• Quadratic patterns: Nested loops over data that could be O(n) with better data structures
• Process isolation awareness: Stateful filter state is NOT merged back when using run_pipeline(n_jobs>1). The merge() pattern must be used post-hoc. Check that new stateful filters document this.

Flag performance issues as P1. Profiling-specific concerns (missing ProfiledPipeline support, metric collection gaps) are P2.

Step 7: Code Quality (P2)

Review for clean, concise implementation:

• Over-commenting: Comments that restate what the code does. Code should be self-documenting. Comments should explain why, not what. Bad: # Increment the counter above counter += 1. Good: # Reset after flush to avoid double-counting.
• Verbose implementations: Could the same logic be expressed more concisely without sacrificing readability?
• Dead code: Unused imports, unreachable branches, commented-out code blocks
• Naming: Do names follow existing patterns in the codebase?
  • Sources: <Name>Source (e.g. VTKSource, ERA5Source)
  • Filters: <Name>Filter (e.g. MeanFilter, MomentsFilter)
  • Sinks: <Name>Sink (e.g. MeshSink, ZarrSink)
  • Test classes: Test<ClassName> (e.g. TestMeanFilter)
  • Test files: test_<module>.py
• Docstring quality: NumPy-style, with Parameters / Returns / Examples sections. Not overly verbose — each param gets one line of description.
• Function length: Functions over ~50 lines should be considered for decomposition. Not a hard rule, but flag for discussion.

Step 8: Style Consistency (NIT)

Compare against existing patterns in the package. These are not correctness issues — they're about matching what's already there.

• Import ordering: stdlib → third-party → local, with TYPE_CHECKING block for type-only imports
• from __future__ import annotations: Present in all source files (required for ClassVar[str] without quotes)
• Docstring phrasing: Existing codebase uses imperative mood for method docstrings ("Return the ...", "Compute the ...")
• Type annotations: All public function signatures have type hints. Use Generator[T], Iterator[T], not bare generator.
• Test fixture patterns: tmp_path for temporary directories, helper functions for creating test data, domain markers
• Parameter style: Param(name=..., description=..., type=..., default=...) — check that new params match existing conventions

Output Format

After completing all passes, compile findings into a structured report.

## PR Review: #<number> — <title>

**Author:** <author> | **Branch:** <branch> | **Files changed:** <count>

### Summary
<1-3 sentence overall assessment. Is this ready to merge, close to
ready, or needs significant work?>

### Findings

#### P0 — Must Fix
| # | File:Line | Pass | Finding | Suggestion |
|---|-----------|------|---------|------------|
| 1 | `src/.../foo.py:42` | API | Missing `name` ClassVar | Add `name: ClassVar[str] = "..."` |

#### P1 — Should Fix
| # | File:Line | Pass | Finding | Suggestion |
|---|-----------|------|---------|------------|

#### P2 — Consider
| # | File:Line | Pass | Finding | Suggestion |
|---|-----------|------|---------|------------|

#### NIT — Stylistic
| # | File:Line | Pass | Finding | Suggestion |
|---|-----------|------|---------|------------|

### Test Coverage Assessment
<What's tested, what's missing, coverage % if measured>

### Performance Notes
<Any profiling concerns, streaming issues, or memory patterns>

If there are zero findings for a priority level, omit that section.

Step 9: Post to PR (Optional)

After presenting the report, ask the user:

"Would you like me to post these findings as review comments on PR #<number>?"

If the user says yes, post the findings using gh api as a single pull request review with line-level comments.

Comment Template

Each comment follows this format:

**[{PRIORITY}]** {TITLE}

{DESCRIPTION}

```suggestion
{SUGGESTED_CODE_FIX}
```​

• {PRIORITY}: One of P0, P1, P2, NIT
• {TITLE}: Short (< 10 words) summary of the issue
• {DESCRIPTION}: 1-3 sentences explaining the problem and why it matters. Reference existing code patterns where relevant.
• {SUGGESTED_CODE_FIX}: Optional. If the fix is clear, include it in a GitHub suggestion block so the author can apply it with one click. Omit the suggestion block if the fix requires broader refactoring.

Posting via gh api

Submit all comments as a single review (not individual comments):

gh api repos/NVIDIA/physicsnemo-curator/pulls/<number>/reviews \
  --method POST \
  --field event="COMMENT" \
  --field body="<overall summary>" \
  --field 'comments=[
    {
      "path": "<file>",
      "line": <line_number>,
      "body": "<formatted comment>"
    }
  ]'

Use event="COMMENT" (not REQUEST_CHANGES or APPROVE) — the reviewer reports findings but the merge decision is the user's.

For findings that apply to a file but not a specific line (e.g. missing SPDX header, missing test file), use line 1 of the relevant file, or include them only in the review body summary.

Review Body Template

The top-level review body summarizes the full review:

## Curator Review: <count> finding(s)

**P0:** <n> | **P1:** <n> | **P2:** <n> | **NIT:** <n>

<1-2 sentence summary of overall assessment>

Checklist

Use this checklist to verify the review is complete:

• PR diff fetched and all changed files read in full
• Pass 1: API conformance checked for all new/modified components
• Pass 2: Correctness & safety reviewed
• Pass 3: SPDX headers verified on all new files
• Pass 4: Quality gates run (ruff, ty, interrogate)
• Pass 5: Test coverage assessed
• Pass 6: Performance patterns reviewed
• Pass 7: Code quality evaluated (comments, verbosity, naming)
• Pass 8: Style consistency checked against existing patterns
• Findings compiled into structured report
• Priority levels assigned correctly (P0/P1/P2/NIT)
• User asked about posting to PR

Reference: Project Standards

Quick reference for the standards checked by this review.

Quality Thresholds

API Contracts (from core/base.py)

Source[T]:
  name: ClassVar[str]
  description: ClassVar[str]
  params() -> list[Param]
  __len__() -> int
  __getitem__(index: int) -> Generator[T]

Filter[T]:
  name: ClassVar[str]
  description: ClassVar[str]
  params() -> list[Param]
  __call__(items: Generator[T]) -> Generator[T]
  flush() -> str | None  (optional, for stateful filters)

Sink[T]:
  name: ClassVar[str]
  description: ClassVar[str]
  params() -> list[Param]
  __call__(items: Iterator[T], index: int) -> list[str]

Naming Conventions

Test Markers

@pytest.mark.requires("mesh")   # skip if mesh deps missing
@pytest.mark.requires("da")     # skip if da deps missing
@pytest.mark.e2e                # end-to-end pipeline test
@pytest.mark.slow               # excluded by default
@pytest.mark.benchmark          # performance benchmark

Source: NVIDIA/physicsnemo-curator — distributed by TomeVault.