Skip to content

chore(typecheck): include top-level tools - #612

Merged
binaryaaron merged 3 commits into
mainfrom
binaryaaron/pr596-05-typecheck-tools
Aug 7, 2026
Merged

chore(typecheck): include top-level tools#612
binaryaaron merged 3 commits into
mainfrom
binaryaaron/pr596-05-typecheck-tools

Conversation

@binaryaaron

@binaryaaron binaryaaron commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Removes the top-level tools exclusion from ty checks.
  • Adds typed TOML and proxy helper boundaries for checked tools.

Test plan

  • mise run typecheck
  • mise run format-check

Related issue: #614

Summary by CodeRabbit

  • Bug Fixes

    • Improved lockfile comparisons across registry, Git, path, and other package sources.
    • Preserved separate entries when the same package has multiple versions.
    • Improved dependency update handling and validation for missing or invalid configuration sections.
  • Maintenance

    • Improved reliability and type safety across dependency, proxy, and release tooling.
    • Expanded automated validation for package metadata and lockfile processing.
    • Updated tooling configuration to include relevant validation targets.

@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 7f0bfb9e-ed24-46af-9347-10fedbb84314

📥 Commits

Reviewing files that changed from the base of the PR and between dc3e464 and a9c3e46.

📒 Files selected for processing (2)
  • tests/tools/test_diff_lockfile.py
  • tools/diff-lockfile.py
📜 Recent review details
⏰ Context from checks skipped due to timeout. (1)
  • GitHub Check: Unit Tests (3.14)
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{md,markdown,py}

📄 CodeRabbit inference engine (.cursor/rules/agent-markdown-style.mdc)

**/*.{md,markdown,py}: Avoid decorative bold (**text**) in list items, body text, and docstrings; use structural cues (headers, list markers, colons, backticks) for emphasis instead
Use backticks for code identifiers, paths, and CLI commands in markdown and docstrings

Files:

  • tools/diff-lockfile.py
  • tests/tools/test_diff_lockfile.py
**/*

📄 CodeRabbit inference engine (AGENTS.md)

**/*: If AGENTS.local.md exists, read it and give its instructions top priority.
Read and follow repository-specific skills in .agents/skills/ when a task matches their scope.
Follow the detailed coding conventions in STYLE_GUIDE.md.
Use uv for project operations, never pip or raw python; use uv run for Python execution.
Use mise tasks or wrapper scripts in tools/ instead of invoking ruff or ty directly.
For a full GPU/development environment, use uv sync --frozen --extra cu129 --extra engine --group dev; bare uv sync --frozen is incomplete.
Do not commit unless the user asks for a commit or PR work. When committing, require DCO sign-off and GPG signing via git commit --signoff --gpg-sign (or -s -S); never manually add Signed-off-by or use --no-gpg-sign.
Use feature branches based on main; branch names commonly include an issue-number prefix such as <author>/123-short-name.
Use the documented mise tasks and matching skills for testing, building, syncing, bootstrapping, worktrees, GitHub, and recurring workflows.

End files with a newline, avoid trailing whitespace, use one space between sentences, and keep code, comments, and docstrings within 120 characters.

**/*: Do not move a published release tag; if release code changes, create and validate the next release candidate instead.
The stable release tag must point to the same tested commit SHA as the validated release candidate.

Files:

  • tools/diff-lockfile.py
  • tests/tools/test_diff_lockfile.py

⚙️ CodeRabbit configuration file

**/*: Review as a senior maintainer for NeMo Safe Synthesizer. Prioritize issues that can change behavior, break user workflows, weaken privacy guarantees, hide failures, make tests unreliable, or create maintenance risk. Avoid generic style commentary unless it points to a concrete project convention that automated tools will not catch.
Comment only when the finding is actionable and tied to changed code. For each finding, state the impact, the condition that triggers it, and the smallest practical fix. Prefer one precise comment over broad advice. Do not ask for refactors outside the PR scope unless the changed code creates the problem.
Review type guidance: - Potential issue: use for correctness bugs, data loss, privacy leaks,
security risks, broken public APIs, invalid config behavior, missing
validation, hidden failures, nondeterministic tests, or CI breakage.

  • Refactor suggestion: use for local maintainability problems introduced
    by the diff when they have clear future cost, such as duplicated setup,
    unclear boundaries, over-mocking, avoidable complexity, or opaque test
    helpers.
  • Nitpick: avoid in chill mode. Do not emit formatting, import-order,
    wording, or style-only comments unless automated tools cannot catch the
    issue and it affects maintainability.

Severity guidance: - Critical: security/privacy leaks, data loss, training/test/holdout
contamination, or broken release/package/core pipeline execution.

  • Major: incorrect generation/training/evaluation behavior, broken
    CLI/SDK public API, invalid config defaults or validators, or GPU/vLLM
    cleanup and process-isolation bugs likely to fail CI or production
    runs.
  • Minor: localized bugs, missing focused tests for changed behavior, or
    bad test patterns that weaken regression coverage.
  • Trivial: small cleanup with no behavior impact. Usually suppress in
    chill mode.
  • Info: context only. Avoid unless it helps reviewers understand risk.
    Safe-Synthesizer-specific review focus: - Data ...

Files:

  • tools/diff-lockfile.py
  • tests/tools/test_diff_lockfile.py
**/*.{py,md,sh,Dockerfile}

📄 CodeRabbit inference engine (AGENTS.md)

Keep durable implementation guidance in public function/class docstrings or local source comments; keep test-suite guidance in tests/TESTING.md.

Files:

  • tools/diff-lockfile.py
  • tests/tools/test_diff_lockfile.py
**/*.{py,pyi}

📄 CodeRabbit inference engine (AGENTS.md)

Use Python 3.11–3.13 and modern syntax such as X | Y, list[str], and Self; Python 3.14+ is unsupported.

Files:

  • tools/diff-lockfile.py
  • tests/tools/test_diff_lockfile.py
**/*.py

📄 CodeRabbit inference engine (STYLE_GUIDE.md)

**/*.py: Use American English spelling in Python code and documentation; identifiers with leading _ are private, and __all__ defines the public API.
Pydantic model fields must include Field(description=...); prefer assignment-style Field() and use Annotated only for additional metadata or constraints.
Use @dataclass(frozen=True) for immutable value objects and validators, and field(default_factory=list) rather than mutable list defaults.
Use StrEnum for string-valued configuration or serialization enums and plain Enum for internal constants.
Obtain loggers with observability.get_logger(__name__); do not call logging.getLogger() or structlog.get_logger() directly, and do not use print() for operational output.
Use .runtime, .user, and .system category loggers appropriately, and use extra={} for machine-queryable metrics, counts, durations, and similar data.
Raise errors from the Safe Synthesizer hierarchy, using dual inheritance where callers should also catch a built-in exception: DataError, ParameterError, GenerationError, and InternalError.
Maintain Python 3.11 compatibility: use native X | Y, built-in generic types, Self, collection ABCs for arguments, Protocol for structural boundaries, and avoid Python 3.12-only PEP 695 syntax and unnecessary Any.
Use TYPE_CHECKING guards for heavy imports such as pandas, torch, and transformers; add from __future__ import annotations to every module.
Prefer match/case for dispatch on types or tagged values, comprehensions when clearer, and avoid comprehensions with multiple for clauses.
Keep functions flat: avoid more than two indentation levels beyond def; use guard clauses, named helpers, generators, or named predicates to decompose complex logic.
Error messages must describe the actual condition precisely, and interpolated values must be clearly identifiable, typically with !r.
Use PascalCase for classes, snake_case for functions and variables, UPPER_SN...

Files:

  • tools/diff-lockfile.py
  • tests/tools/test_diff_lockfile.py
**/*.{py,sh,yaml,yml,md}

📄 CodeRabbit inference engine (STYLE_GUIDE.md)

Every source file requires the appropriate SPDX copyright and license header; Markdown files with YAML frontmatter place hash-comment headers inside the frontmatter.

**/*.{py,sh,yaml,yml,md}: All source files with .py, .sh, .yaml, .yml, or .md extensions require SPDX copyright headers, except files listed in .copyrightignore.
Run mise run format and mise run check before contributing so formatting, linting, type checking, and copyright checks pass.

Files:

  • tools/diff-lockfile.py
  • tests/tools/test_diff_lockfile.py
tools/**

⚙️ CodeRabbit configuration file

Review tools as developer and CI infrastructure. Check that scripts use uv or Makefile wrappers instead of ad hoc python/pip commands, preserve read-only behavior for check targets, fail with clear messages, avoid hidden network or filesystem side effects, and stay consistent with STYLE_GUIDE.md and CONTRIBUTING.md. Tooling may use print() when it is a standalone script or intentional CLI output.

Files:

  • tools/diff-lockfile.py
tests/**

📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)

tests/**: Mirror src/ directory structure in tests/ directory for test organization
Auto-mark tests by directory: tests/e2e/e2e, tests/smoke/smoke, otherwise default to unit

Mirror source code directory structure in tests directory (e.g., tests/training/, tests/generation/ parallel to source structure)

Files:

  • tests/tools/test_diff_lockfile.py
tests/**/*.py

📄 CodeRabbit inference engine (tests/TESTING.md)

tests/**/*.py: Auto-mark tests based on file path: tests under /e2e/ get e2e marker, tests under /smoke/ get smoke marker, all others get unit marker (only if no category marker already present)
Every test should have exactly one category marker: unit, smoke, or e2e
Use pytest.mark.requires_gpu modifier on tests that need CUDA hardware
Use pytest.mark.vllm on tests using vLLM generation backend and ensure each vLLM test file runs in its own process for GPU memory isolation
Use pytest.mark.slow on long-running tests
Use pytest.mark.smollm2 for SmolLM2 Hub download tests to enable process isolation
Use pytest.mark.noautouse to skip autouse fixtures for specific tests
Use load_test_dataset(filename) helper to load test datasets from tests/stub_datasets/ as HuggingFace Dataset objects
Use load_test_dataframe(filename) helper to load test data files from tests/stub_datasets/ as pandas DataFrames
Convert pandas columns to nullable dtypes (pd.Int64Dtype(), pd.BooleanDtype()) before assigning np.nan values
Use fake.seed_instance(seed) and random.seed(seed) together for Faker-based test data reproducibility
When sharing methods across multiple test files, define them in conftest.py and import them using relative imports (e.g., from .conftest import train_with_sdk); note that importing from other test files like tests/cli/helpers.py does not work
Use fixture_mock_processor or fixture_mock_processor_without_valid_records for mocking ParsedResponse objects with valid_records, invalid_records, errors, and prompt_number fields
Use pytest.importorskip to gate tests on optional dependencies that require specific extras (e.g., sentence_transformers, vllm)
Run vLLM tests with separate pytest invocations (one per file) using -n 0 (single process) for GPU memory isolation, or use staged mise tasks for CI visibility
Print statements are allowed in tests (ruff T201 is suppressed for tests/ directory) and should...

Files:

  • tests/tools/test_diff_lockfile.py

⚙️ CodeRabbit configuration file

Review tests against tests/TESTING.md. Check marker usage, fixture naming, tmp_path usage, determinism, and GPU/vLLM process-isolation requirements. Flag slop tests that only check that code runs, assert result is not None when stronger invariants exist, over-mock internal implementation details, patch around the bug instead of reproducing it, or add broad snapshot/golden churn without a clear contract. Flag change detector tests that fail on harmless refactors, formatting, record ordering, incidental wording, or private implementation details without demonstrating a behavior regression. Prefer existing fixtures or focused new fixtures for repeated setup; keep tests DRY when reasonable without making the behavior under test opaque. print() is allowed in tests.

Files:

  • tests/tools/test_diff_lockfile.py
tests/**/*.{py,ini}

📄 CodeRabbit inference engine (AGENTS.md)

Because asyncio_mode = auto is configured in pytest.ini, asynchronous tests do not need @pytest.mark.asyncio; use the unit marker instead of deprecated unit_test.

Files:

  • tests/tools/test_diff_lockfile.py
🧠 Learnings (3)
📚 Learning: 2026-05-27T22:20:37.354Z
Learnt from: kendrickb-nvidia
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 520
File: tests/generation/test_vllm_backend.py:556-587
Timestamp: 2026-05-27T22:20:37.354Z
Learning: In NVIDIA-NeMo/Safe-Synthesizer, `tests/conftest.py`’s `pytest_collection_modifyitems` hook applies pytest category markers automatically based on each test file’s path: tests under `/e2e/` get `pytest.mark.e2e`, tests under `/smoke/` get `pytest.mark.smoke`, and all other tests get `pytest.mark.unit`. Therefore, when reviewing pytest tests outside `tests/e2e/` and `tests/smoke/`, do not flag missing explicit `pytest.mark.unit` decorators on test classes/functions as an issue (the hook will add them during collection). If a new test directory/category is introduced, ensure the hook is updated so it’s categorized correctly.

Applied to files:

  • tests/tools/test_diff_lockfile.py
📚 Learning: 2026-07-27T22:07:22.590Z
Learnt from: binaryaaron
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 673
File: tests/pii_replacer/test_edit.py:327-327
Timestamp: 2026-07-27T22:07:22.590Z
Learning: When tests read structured logging context from Python `logging.LogRecord` instances, don’t access `record.ctx` directly (it isn’t declared on `LogRecord` and will break static typing). Instead, use `getattr(record, "ctx", default)` (or an appropriate fallback) to safely handle cases where `ctx` may or may not be attached. This applies even if Ruff rule `B009` isn’t enabled in the repo.

Applied to files:

  • tests/tools/test_diff_lockfile.py
📚 Learning: 2026-07-29T17:12:32.642Z
Learnt from: zywind
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 654
File: tests/config/test_parameters.py:116-118
Timestamp: 2026-07-29T17:12:32.642Z
Learning: In Pydantic validation tests (e.g., models configured with `from_attributes`), when asserting failures from `model_validate(...)`, assert the structured error details (such as `ValidationError.errors()[0]["type"]`, e.g. `"model_attributes_type"`) rather than relying on the human-readable error message text. This keeps tests stable even if wording changes, while still verifying the correct validation rule is triggered.

Applied to files:

  • tests/tools/test_diff_lockfile.py
🔇 Additional comments (2)
tools/diff-lockfile.py (1)

38-38: LGTM!

Also applies to: 190-243

tests/tools/test_diff_lockfile.py (1)

12-42: LGTM!

Also applies to: 84-122


Walkthrough

The change enables type-checking for tool scripts and strengthens typing and structural data handling in lockfile, network guard, Dependabot, and release tooling. It also preserves distinct lockfile package versions and adds tests for duplicate-version comparisons.

Changes

Tooling typing and data handling

Layer / File(s) Summary
Enable tool typechecking
pyproject.toml
Removes the tools Python-file exclusion from ty configuration.
Normalize lockfile sources and variants
tools/diff-lockfile.py, tests/tools/test_diff_lockfile.py
Uses structural source matching, includes versions in package keys, compares same-source variants, and tests upgrade classification.
Type network guard lifecycle
tools/hf_network_guard_proxy.py
Updates context-manager annotations and adds the override decorator and typing dependency declaration.
Refactor Dependabot TOML processing
tools/patch_dependabot.py
Adds typed TOML accessors and structural matching for dependency, constraint, advisory, and pagination handling.
Clarify version bump typing
tools/release_version.py
Changes StableVersion.bump to return StableVersion explicitly and removes the unused Self import.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

Suggested labels: bug, chore

Suggested reviewers: mckornfield

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: including top-level tools in type checking.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch binaryaaron/pr596-05-typecheck-tools

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added area:dev-ex Affects build or dev experience area:build-dist labels Jun 24, 2026
@binaryaaron
binaryaaron force-pushed the binaryaaron/pr596-04-checked-typing-coverage branch from c7fbb20 to eceb66e Compare June 24, 2026 18:11
@binaryaaron
binaryaaron force-pushed the binaryaaron/pr596-05-typecheck-tools branch from 75ff152 to 1e74819 Compare June 24, 2026 18:11
@binaryaaron
binaryaaron force-pushed the binaryaaron/pr596-04-checked-typing-coverage branch from eceb66e to 46af38a Compare June 24, 2026 18:18
@binaryaaron
binaryaaron force-pushed the binaryaaron/pr596-05-typecheck-tools branch from 1e74819 to 670ffdd Compare June 24, 2026 18:18
@binaryaaron
binaryaaron force-pushed the binaryaaron/pr596-04-checked-typing-coverage branch from 46af38a to 0c08cb9 Compare June 24, 2026 18:23
@binaryaaron
binaryaaron force-pushed the binaryaaron/pr596-05-typecheck-tools branch 3 times, most recently from 9c96037 to 8d55e69 Compare June 24, 2026 21:46
@binaryaaron
binaryaaron force-pushed the binaryaaron/pr596-04-checked-typing-coverage branch from 62ba809 to 8d3f148 Compare June 24, 2026 21:46
@binaryaaron
binaryaaron force-pushed the binaryaaron/pr596-05-typecheck-tools branch from 8d55e69 to 97c3d20 Compare June 24, 2026 22:16
@binaryaaron
binaryaaron force-pushed the binaryaaron/pr596-04-checked-typing-coverage branch 2 times, most recently from 9af5f17 to cf5ea01 Compare June 26, 2026 21:02
@binaryaaron
binaryaaron force-pushed the binaryaaron/pr596-05-typecheck-tools branch from 97c3d20 to b0fc400 Compare June 26, 2026 21:04
@binaryaaron
binaryaaron force-pushed the binaryaaron/pr596-04-checked-typing-coverage branch from cf5ea01 to 3f2bc65 Compare June 30, 2026 21:40
@binaryaaron
binaryaaron force-pushed the binaryaaron/pr596-05-typecheck-tools branch from b0fc400 to 3ca326f Compare June 30, 2026 21:40
@binaryaaron
binaryaaron force-pushed the binaryaaron/pr596-04-checked-typing-coverage branch from 3f2bc65 to 967deb4 Compare July 6, 2026 18:07
@binaryaaron
binaryaaron force-pushed the binaryaaron/pr596-05-typecheck-tools branch from 3ca326f to ab75d47 Compare July 6, 2026 18:07
@binaryaaron
binaryaaron force-pushed the binaryaaron/pr596-04-checked-typing-coverage branch from 967deb4 to a9286fc Compare July 6, 2026 19:23
@binaryaaron
binaryaaron force-pushed the binaryaaron/pr596-05-typecheck-tools branch 2 times, most recently from ae3cfb7 to f99a9d9 Compare July 8, 2026 16:05
@binaryaaron
binaryaaron force-pushed the binaryaaron/pr596-04-checked-typing-coverage branch from 0e72757 to 3d5719d Compare July 9, 2026 17:44
@binaryaaron
binaryaaron force-pushed the binaryaaron/pr596-05-typecheck-tools branch from f99a9d9 to 148fa85 Compare July 9, 2026 17:44
@binaryaaron
binaryaaron force-pushed the binaryaaron/pr596-04-checked-typing-coverage branch 2 times, most recently from 756d95c to 3d5719d Compare July 10, 2026 21:25
@coderabbitai coderabbitai Bot added the chore Maintenance not tied to a user-visible change label Jul 15, 2026
@greptile-apps

greptile-apps Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR expands typechecking to top-level tools and adds typed boundaries around TOML, proxy, release, and lockfile helpers.

  • Preserves multiple versions of the same package while improving lockfile change classification.
  • Refactors Dependabot configuration traversal around typed TOML helpers.
  • Adds proxy context-manager and override annotations required by expanded typechecking.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
pyproject.toml Removes the blanket top-level tools exclusion so these scripts participate in typechecking.
tools/diff-lockfile.py Preserves duplicate package variants and compares remaining versions by package name and source.
tests/tools/test_diff_lockfile.py Adds regression coverage for duplicate same-source versions and changed duplicate variants.
tools/patch_dependabot.py Introduces typed TOML table and array helpers while retaining dependency and constraint update behavior.
tools/hf_network_guard_proxy.py Adds explicit context-manager typing and an override annotation without changing proxy lifecycle behavior.
tools/release_version.py Replaces a Self return annotation with the concrete StableVersion type.

Reviews (4): Last reviewed commit: "fix(typecheck): classify duplicate lockf..." | Re-trigger Greptile

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
tools/diff-lockfile.py (1)

39-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use a read-only typed mapping instead of Any.

_extract_source only reads raw, so Mapping[str, object] models the contract more accurately, avoids unchecked Any propagation, and accepts the concrete TOML mapping passed by callers.

Suggested typing adjustment
+from collections.abc import Mapping
-from typing import Annotated, Any, Optional
+from typing import Annotated, Optional
-def _extract_source(raw: dict[str, Any]) -> str:
+def _extract_source(raw: Mapping[str, object]) -> str:

As per coding guidelines: use collection ABCs for function arguments and avoid Any when object is suitable.

Also applies to: 119-119

Source: Coding guidelines


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 678d2ec6-1796-43a4-a17b-7bbc45f64f3b

📥 Commits

Reviewing files that changed from the base of the PR and between a321768 and b5549c2.

📒 Files selected for processing (4)
  • pyproject.toml
  • tools/diff-lockfile.py
  • tools/hf_network_guard_proxy.py
  • tools/patch_dependabot.py
💤 Files with no reviewable changes (1)
  • pyproject.toml
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
  • GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{md,markdown,py}

📄 CodeRabbit inference engine (.cursor/rules/agent-markdown-style.mdc)

**/*.{md,markdown,py}: Avoid decorative bold (**text**) in list items, body text, and docstrings; use structural cues (headers, list markers, colons, backticks) for emphasis instead
Use backticks for code identifiers, paths, and CLI commands in markdown and docstrings

Files:

  • tools/diff-lockfile.py
  • tools/hf_network_guard_proxy.py
  • tools/patch_dependabot.py
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Place durable implementation guidance in function and class docstrings for public contracts and source comments for local invariants
Target Python 3.11–3.13 with modern syntax (X | Y, list[str], Self). Python 3.14+ is not supported

**/*.py: Use American English spelling in Python code, documentation, and messages.
Use Field(description=...) for every Pydantic model field.
Use assignment-style Field() by default; use Annotated only for additional metadata such as validators, constrained aliases, or discriminated unions.
Use @dataclass(frozen=True) for immutable value objects and validators; use mutable dataclasses only for builders, accumulators, and pipeline state.
Use field(default_factory=list) instead of mutable list defaults.
Use StrEnum for string-valued configuration or serialization enums and plain Enum for internal constants.
Obtain loggers with observability.get_logger(__name__); do not call logging.getLogger() or structlog.get_logger() directly.
Use .runtime, .user, and .system category loggers appropriately.
Do not use print() for operational library output; use the approved logger, click.echo(), or sys.stdout.write() where appropriate.
Use extra={} for machine-queryable logging data and f-strings only for human-readable context.
Raise errors from the custom Safe Synthesizer error hierarchy, using the documented dual inheritance for user and internal errors.
Keep shared package code compatible with Python 3.11; do not use Python 3.12-only syntax such as PEP 695 type statements or bracketed generic parameters.
Prefer X | Y, built-in collection generics, and Self over Optional, Union, and legacy typing collections.
Use collection ABCs for function arguments and concrete collection types for return values.
Use Protocol for structural subtyping and avoid Any when object, generics, or protocols are suitable.
Use TYPE_CHECKING guards for heavy imports such as pandas, torch, and transformers.
...

Files:

  • tools/diff-lockfile.py
  • tools/hf_network_guard_proxy.py
  • tools/patch_dependabot.py
**/*

📄 CodeRabbit inference engine (STYLE_GUIDE.md)

**/*: Every source file requires the SPDX copyright and license header appropriate to its file format.
End files with a newline, remove trailing whitespace, use one space between sentences, and keep code, comments, and docstrings within 120 characters.

**/*: All merged commits must use Conventional Commits format: <type>(<scope>): <description> or <type>: <description>, with a lowercase valid type and a description no longer than 100 characters.
All contributions require a DCO Signed-off-by trailer and a cryptographic commit signature.
Before submitting a pull request, run formatting, checks, and tests using mise run format, mise run check, and mise run test.
Branches other than main must use lowercase author-prefixed names in one of the documented forms, optionally including an issue ID and category.
Release tags must use a v prefix and PEP 440 stable or release-candidate versions, such as v1.0.0 or v0.1.0rc0; alpha versions and dashed -rc suffixes are not used.
Do not move a published release tag; create and validate a new release candidate when code changes.
Use mise run <task> for project tasks; the Makefile only bootstraps mise and provides deprecated compatibility messages.

Files:

  • tools/diff-lockfile.py
  • tools/hf_network_guard_proxy.py
  • tools/patch_dependabot.py

⚙️ CodeRabbit configuration file

**/*: Review as a senior maintainer for NeMo Safe Synthesizer. Prioritize issues that can change behavior, break user workflows, weaken privacy guarantees, hide failures, make tests unreliable, or create maintenance risk. Avoid generic style commentary unless it points to a concrete project convention that automated tools will not catch.
Comment only when the finding is actionable and tied to changed code. For each finding, state the impact, the condition that triggers it, and the smallest practical fix. Prefer one precise comment over broad advice. Do not ask for refactors outside the PR scope unless the changed code creates the problem.
Review type guidance: - Potential issue: use for correctness bugs, data loss, privacy leaks,
security risks, broken public APIs, invalid config behavior, missing
validation, hidden failures, nondeterministic tests, or CI breakage.

  • Refactor suggestion: use for local maintainability problems introduced
    by the diff when they have clear future cost, such as duplicated setup,
    unclear boundaries, over-mocking, avoidable complexity, or opaque test
    helpers.
  • Nitpick: avoid in chill mode. Do not emit formatting, import-order,
    wording, or style-only comments unless automated tools cannot catch the
    issue and it affects maintainability.

Severity guidance: - Critical: security/privacy leaks, data loss, training/test/holdout
contamination, or broken release/package/core pipeline execution.

  • Major: incorrect generation/training/evaluation behavior, broken
    CLI/SDK public API, invalid config defaults or validators, or GPU/vLLM
    cleanup and process-isolation bugs likely to fail CI or production
    runs.
  • Minor: localized bugs, missing focused tests for changed behavior, or
    bad test patterns that weaken regression coverage.
  • Trivial: small cleanup with no behavior impact. Usually suppress in
    chill mode.
  • Info: context only. Avoid unless it helps reviewers understand risk.
    Safe-Synthesizer-specific review focus: - Data ...

Files:

  • tools/diff-lockfile.py
  • tools/hf_network_guard_proxy.py
  • tools/patch_dependabot.py
**/*.{py,sh,yaml,yml,md}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

All source files with .py, .sh, .yaml, .yml, or .md extensions must include SPDX copyright headers, except files listed in .copyrightignore.

Files:

  • tools/diff-lockfile.py
  • tools/hf_network_guard_proxy.py
  • tools/patch_dependabot.py
tools/**

⚙️ CodeRabbit configuration file

Review tools as developer and CI infrastructure. Check that scripts use uv or Makefile wrappers instead of ad hoc python/pip commands, preserve read-only behavior for check targets, fail with clear messages, avoid hidden network or filesystem side effects, and stay consistent with STYLE_GUIDE.md and CONTRIBUTING.md. Tooling may use print() when it is a standalone script or intentional CLI output.

Files:

  • tools/diff-lockfile.py
  • tools/hf_network_guard_proxy.py
  • tools/patch_dependabot.py
🔇 Additional comments (15)
tools/patch_dependabot.py (8)

10-10: LGTM!


34-58: LGTM!


130-195: LGTM!


236-256: LGTM!


274-274: LGTM!


291-302: LGTM!


322-324: LGTM!

Also applies to: 336-336


366-382: LGTM!

tools/hf_network_guard_proxy.py (5)

11-11: LGTM!


78-78: LGTM!


94-94: LGTM!


175-175: LGTM!


206-207: LGTM!

tools/diff-lockfile.py (2)

121-123: LGTM!


126-129: LGTM!

Comment thread tools/diff-lockfile.py
@binaryaaron
binaryaaron added this pull request to the merge queue Jul 16, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 16, 2026
@binaryaaron
binaryaaron added this pull request to the merge queue Jul 17, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 17, 2026
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

@coderabbitai coderabbitai Bot added the refactor Internal restructuring with no behavior change label Aug 4, 2026
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
@binaryaaron
binaryaaron requested a review from a team as a code owner August 4, 2026 22:43

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 41452f39-153e-45ad-b21b-db2abd627a09

📥 Commits

Reviewing files that changed from the base of the PR and between 5f41920 and dc3e464.

📒 Files selected for processing (3)
  • tests/tools/test_diff_lockfile.py
  • tools/diff-lockfile.py
  • tools/release_version.py
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
  • GitHub Check: Greptile Review
  • GitHub Check: Analyze (Python)
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{md,markdown,py}

📄 CodeRabbit inference engine (.cursor/rules/agent-markdown-style.mdc)

**/*.{md,markdown,py}: Avoid decorative bold (**text**) in list items, body text, and docstrings; use structural cues (headers, list markers, colons, backticks) for emphasis instead
Use backticks for code identifiers, paths, and CLI commands in markdown and docstrings

Files:

  • tests/tools/test_diff_lockfile.py
  • tools/release_version.py
  • tools/diff-lockfile.py
tests/**

📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)

tests/**: Mirror src/ directory structure in tests/ directory for test organization
Auto-mark tests by directory: tests/e2e/e2e, tests/smoke/smoke, otherwise default to unit

Mirror source code directory structure in tests directory (e.g., tests/training/, tests/generation/ parallel to source structure)

Files:

  • tests/tools/test_diff_lockfile.py
tests/**/*.py

📄 CodeRabbit inference engine (tests/TESTING.md)

tests/**/*.py: Auto-mark tests based on file path: tests under /e2e/ get e2e marker, tests under /smoke/ get smoke marker, all others get unit marker (only if no category marker already present)
Every test should have exactly one category marker: unit, smoke, or e2e
Use pytest.mark.requires_gpu modifier on tests that need CUDA hardware
Use pytest.mark.vllm on tests using vLLM generation backend and ensure each vLLM test file runs in its own process for GPU memory isolation
Use pytest.mark.slow on long-running tests
Use pytest.mark.smollm2 for SmolLM2 Hub download tests to enable process isolation
Use pytest.mark.noautouse to skip autouse fixtures for specific tests
Use load_test_dataset(filename) helper to load test datasets from tests/stub_datasets/ as HuggingFace Dataset objects
Use load_test_dataframe(filename) helper to load test data files from tests/stub_datasets/ as pandas DataFrames
Convert pandas columns to nullable dtypes (pd.Int64Dtype(), pd.BooleanDtype()) before assigning np.nan values
Use fake.seed_instance(seed) and random.seed(seed) together for Faker-based test data reproducibility
When sharing methods across multiple test files, define them in conftest.py and import them using relative imports (e.g., from .conftest import train_with_sdk); note that importing from other test files like tests/cli/helpers.py does not work
Use fixture_mock_processor or fixture_mock_processor_without_valid_records for mocking ParsedResponse objects with valid_records, invalid_records, errors, and prompt_number fields
Use pytest.importorskip to gate tests on optional dependencies that require specific extras (e.g., sentence_transformers, vllm)
Run vLLM tests with separate pytest invocations (one per file) using -n 0 (single process) for GPU memory isolation, or use staged mise tasks for CI visibility
Print statements are allowed in tests (ruff T201 is suppressed for tests/ directory) and should...

Files:

  • tests/tools/test_diff_lockfile.py

⚙️ CodeRabbit configuration file

Review tests against tests/TESTING.md. Check marker usage, fixture naming, tmp_path usage, determinism, and GPU/vLLM process-isolation requirements. Flag slop tests that only check that code runs, assert result is not None when stronger invariants exist, over-mock internal implementation details, patch around the bug instead of reproducing it, or add broad snapshot/golden churn without a clear contract. Flag change detector tests that fail on harmless refactors, formatting, record ordering, incidental wording, or private implementation details without demonstrating a behavior regression. Prefer existing fixtures or focused new fixtures for repeated setup; keep tests DRY when reasonable without making the behavior under test opaque. print() is allowed in tests.

Files:

  • tests/tools/test_diff_lockfile.py
**/*

📄 CodeRabbit inference engine (AGENTS.md)

**/*: If AGENTS.local.md exists, read it and give its instructions top priority.
Read and follow repository-specific skills in .agents/skills/ when a task matches their scope.
Follow the detailed coding conventions in STYLE_GUIDE.md.
Use uv for project operations, never pip or raw python; use uv run for Python execution.
Use mise tasks or wrapper scripts in tools/ instead of invoking ruff or ty directly.
For a full GPU/development environment, use uv sync --frozen --extra cu129 --extra engine --group dev; bare uv sync --frozen is incomplete.
Do not commit unless the user asks for a commit or PR work. When committing, require DCO sign-off and GPG signing via git commit --signoff --gpg-sign (or -s -S); never manually add Signed-off-by or use --no-gpg-sign.
Use feature branches based on main; branch names commonly include an issue-number prefix such as <author>/123-short-name.
Use the documented mise tasks and matching skills for testing, building, syncing, bootstrapping, worktrees, GitHub, and recurring workflows.

End files with a newline, avoid trailing whitespace, use one space between sentences, and keep code, comments, and docstrings within 120 characters.

**/*: Do not move a published release tag; if release code changes, create and validate the next release candidate instead.
The stable release tag must point to the same tested commit SHA as the validated release candidate.

Files:

  • tests/tools/test_diff_lockfile.py
  • tools/release_version.py
  • tools/diff-lockfile.py

⚙️ CodeRabbit configuration file

**/*: Review as a senior maintainer for NeMo Safe Synthesizer. Prioritize issues that can change behavior, break user workflows, weaken privacy guarantees, hide failures, make tests unreliable, or create maintenance risk. Avoid generic style commentary unless it points to a concrete project convention that automated tools will not catch.
Comment only when the finding is actionable and tied to changed code. For each finding, state the impact, the condition that triggers it, and the smallest practical fix. Prefer one precise comment over broad advice. Do not ask for refactors outside the PR scope unless the changed code creates the problem.
Review type guidance: - Potential issue: use for correctness bugs, data loss, privacy leaks,
security risks, broken public APIs, invalid config behavior, missing
validation, hidden failures, nondeterministic tests, or CI breakage.

  • Refactor suggestion: use for local maintainability problems introduced
    by the diff when they have clear future cost, such as duplicated setup,
    unclear boundaries, over-mocking, avoidable complexity, or opaque test
    helpers.
  • Nitpick: avoid in chill mode. Do not emit formatting, import-order,
    wording, or style-only comments unless automated tools cannot catch the
    issue and it affects maintainability.

Severity guidance: - Critical: security/privacy leaks, data loss, training/test/holdout
contamination, or broken release/package/core pipeline execution.

  • Major: incorrect generation/training/evaluation behavior, broken
    CLI/SDK public API, invalid config defaults or validators, or GPU/vLLM
    cleanup and process-isolation bugs likely to fail CI or production
    runs.
  • Minor: localized bugs, missing focused tests for changed behavior, or
    bad test patterns that weaken regression coverage.
  • Trivial: small cleanup with no behavior impact. Usually suppress in
    chill mode.
  • Info: context only. Avoid unless it helps reviewers understand risk.
    Safe-Synthesizer-specific review focus: - Data ...

Files:

  • tests/tools/test_diff_lockfile.py
  • tools/release_version.py
  • tools/diff-lockfile.py
**/*.{py,md,sh,Dockerfile}

📄 CodeRabbit inference engine (AGENTS.md)

Keep durable implementation guidance in public function/class docstrings or local source comments; keep test-suite guidance in tests/TESTING.md.

Files:

  • tests/tools/test_diff_lockfile.py
  • tools/release_version.py
  • tools/diff-lockfile.py
**/*.{py,pyi}

📄 CodeRabbit inference engine (AGENTS.md)

Use Python 3.11–3.13 and modern syntax such as X | Y, list[str], and Self; Python 3.14+ is unsupported.

Files:

  • tests/tools/test_diff_lockfile.py
  • tools/release_version.py
  • tools/diff-lockfile.py
tests/**/*.{py,ini}

📄 CodeRabbit inference engine (AGENTS.md)

Because asyncio_mode = auto is configured in pytest.ini, asynchronous tests do not need @pytest.mark.asyncio; use the unit marker instead of deprecated unit_test.

Files:

  • tests/tools/test_diff_lockfile.py
**/*.py

📄 CodeRabbit inference engine (STYLE_GUIDE.md)

**/*.py: Use American English spelling in Python code and documentation; identifiers with leading _ are private, and __all__ defines the public API.
Pydantic model fields must include Field(description=...); prefer assignment-style Field() and use Annotated only for additional metadata or constraints.
Use @dataclass(frozen=True) for immutable value objects and validators, and field(default_factory=list) rather than mutable list defaults.
Use StrEnum for string-valued configuration or serialization enums and plain Enum for internal constants.
Obtain loggers with observability.get_logger(__name__); do not call logging.getLogger() or structlog.get_logger() directly, and do not use print() for operational output.
Use .runtime, .user, and .system category loggers appropriately, and use extra={} for machine-queryable metrics, counts, durations, and similar data.
Raise errors from the Safe Synthesizer hierarchy, using dual inheritance where callers should also catch a built-in exception: DataError, ParameterError, GenerationError, and InternalError.
Maintain Python 3.11 compatibility: use native X | Y, built-in generic types, Self, collection ABCs for arguments, Protocol for structural boundaries, and avoid Python 3.12-only PEP 695 syntax and unnecessary Any.
Use TYPE_CHECKING guards for heavy imports such as pandas, torch, and transformers; add from __future__ import annotations to every module.
Prefer match/case for dispatch on types or tagged values, comprehensions when clearer, and avoid comprehensions with multiple for clauses.
Keep functions flat: avoid more than two indentation levels beyond def; use guard clauses, named helpers, generators, or named predicates to decompose complex logic.
Error messages must describe the actual condition precisely, and interpolated values must be clearly identifiable, typically with !r.
Use PascalCase for classes, snake_case for functions and variables, UPPER_SN...

Files:

  • tests/tools/test_diff_lockfile.py
  • tools/release_version.py
  • tools/diff-lockfile.py
**/*.{py,sh,yaml,yml,md}

📄 CodeRabbit inference engine (STYLE_GUIDE.md)

Every source file requires the appropriate SPDX copyright and license header; Markdown files with YAML frontmatter place hash-comment headers inside the frontmatter.

**/*.{py,sh,yaml,yml,md}: All source files with .py, .sh, .yaml, .yml, or .md extensions require SPDX copyright headers, except files listed in .copyrightignore.
Run mise run format and mise run check before contributing so formatting, linting, type checking, and copyright checks pass.

Files:

  • tests/tools/test_diff_lockfile.py
  • tools/release_version.py
  • tools/diff-lockfile.py
tools/**

⚙️ CodeRabbit configuration file

Review tools as developer and CI infrastructure. Check that scripts use uv or Makefile wrappers instead of ad hoc python/pip commands, preserve read-only behavior for check targets, fail with clear messages, avoid hidden network or filesystem side effects, and stay consistent with STYLE_GUIDE.md and CONTRIBUTING.md. Tooling may use print() when it is a standalone script or intentional CLI output.

Files:

  • tools/release_version.py
  • tools/diff-lockfile.py
🧠 Learnings (3)
📚 Learning: 2026-05-27T22:20:37.354Z
Learnt from: kendrickb-nvidia
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 520
File: tests/generation/test_vllm_backend.py:556-587
Timestamp: 2026-05-27T22:20:37.354Z
Learning: In NVIDIA-NeMo/Safe-Synthesizer, `tests/conftest.py`’s `pytest_collection_modifyitems` hook applies pytest category markers automatically based on each test file’s path: tests under `/e2e/` get `pytest.mark.e2e`, tests under `/smoke/` get `pytest.mark.smoke`, and all other tests get `pytest.mark.unit`. Therefore, when reviewing pytest tests outside `tests/e2e/` and `tests/smoke/`, do not flag missing explicit `pytest.mark.unit` decorators on test classes/functions as an issue (the hook will add them during collection). If a new test directory/category is introduced, ensure the hook is updated so it’s categorized correctly.

Applied to files:

  • tests/tools/test_diff_lockfile.py
📚 Learning: 2026-07-27T22:07:22.590Z
Learnt from: binaryaaron
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 673
File: tests/pii_replacer/test_edit.py:327-327
Timestamp: 2026-07-27T22:07:22.590Z
Learning: When tests read structured logging context from Python `logging.LogRecord` instances, don’t access `record.ctx` directly (it isn’t declared on `LogRecord` and will break static typing). Instead, use `getattr(record, "ctx", default)` (or an appropriate fallback) to safely handle cases where `ctx` may or may not be attached. This applies even if Ruff rule `B009` isn’t enabled in the repo.

Applied to files:

  • tests/tools/test_diff_lockfile.py
📚 Learning: 2026-07-29T17:12:32.642Z
Learnt from: zywind
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 654
File: tests/config/test_parameters.py:116-118
Timestamp: 2026-07-29T17:12:32.642Z
Learning: In Pydantic validation tests (e.g., models configured with `from_attributes`), when asserting failures from `model_validate(...)`, assert the structured error details (such as `ValidationError.errors()[0]["type"]`, e.g. `"model_attributes_type"`) rather than relying on the human-readable error message text. This keeps tests stable even if wording changes, while still verifying the correct validation rule is triggered.

Applied to files:

  • tests/tools/test_diff_lockfile.py
🔇 Additional comments (1)
tools/release_version.py (1)

22-22: 🎯 Functional Correctness

No change needed. tools/release_version.py declares from __future__ import annotations, and the shown code contains no remaining Self references.

			> Likely an incorrect or invalid review comment.

Comment thread tools/diff-lockfile.py
Comment thread tests/tools/test_diff_lockfile.py Fixed
Comment thread tools/release_version.py
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Comment thread tests/tools/test_diff_lockfile.py
Comment thread tests/tools/test_diff_lockfile.py
@coderabbitai coderabbitai Bot added bug Defects in shipped behavior and removed refactor Internal restructuring with no behavior change labels Aug 5, 2026
@binaryaaron
binaryaaron added this pull request to the merge queue Aug 7, 2026
Merged via the queue into main with commit 4ce290f Aug 7, 2026
26 checks passed
@binaryaaron
binaryaaron deleted the binaryaaron/pr596-05-typecheck-tools branch August 7, 2026 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:build-dist area:ci area:data-processing area:dev-ex Affects build or dev experience area:docs area:generation area:sdk-cli area:tests bug Defects in shipped behavior chore Maintenance not tied to a user-visible change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants