chore: standardize mise quality task contract - #716
Conversation
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
WalkthroughThe PR replaces legacy Mise quality tasks with dedicated read-only checks, adds a lock update task, updates CI and pre-commit integrations, removes deprecated Make targets, and revises contributor guidance and task validation tests. ChangesMise quality task migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to This PR standardizes development and CI quality tasks while updating related documentation and hooks; no actionable merge-blocking risk remains after normal checks and review. Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThe PR standardizes repository quality automation around read-only
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
|
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 384a4f8c-d9cb-4d3e-8e92-4980b478c435
📒 Files selected for processing (16)
.agents/skills/github-cli/SKILL.md.agents/skills/uv-build/SKILL.md.claude/commands/lint.md.github/PULL_REQUEST_TEMPLATE.md.github/workflows/README.md.github/workflows/ci-checks.yml.github/workflows/dependabot-sync-lock.yml.mise/tasks/quality.toml.pre-commit-config.yamlAGENTS.mdCONTRIBUTING.mdMakefileREADME.mdtests/tools/test_mise_tasks.pytools/codestyle/format.shtools/codestyle/ruff_check.sh
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: Unit Tests (3.13)
- GitHub Check: Unit Tests (3.12)
- GitHub Check: Smoke Tests
- GitHub Check: End-user Wheel Install
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (19)
**/*.sh
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Use
#!/usr/bin/env bash,set -euo pipefailunless incompatible, quote variables, use${VAR:-default}, use snake_case functions with_for private helpers, and lint with ShellCheck.Shell scripting: never use
~inside double-quoted strings -- it does not expand. Use$HOMEor an absolute path instead.
Files:
tools/codestyle/ruff_check.shtools/codestyle/format.sh
⚙️ CodeRabbit configuration file
Review shell scripts for #!/usr/bin/env bash, set -euo pipefail where appropriate, quoting, repo root detection, and shellcheck compliance.
Files:
tools/codestyle/ruff_check.shtools/codestyle/format.sh
**/*.{py,sh,yaml,yml,md}
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Include the required SPDX copyright and Apache-2.0 license headers, using comment syntax appropriate to the file format.
**/*.{py,sh,yaml,yml,md}: All source files (.py,.sh,.yaml,.yml,.md) require SPDX copyright headers.
Before contributing, runmise run format, review its changes, then runmise run check ::: test.
Files:
tools/codestyle/ruff_check.shREADME.mdtools/codestyle/format.shAGENTS.mdtests/tools/test_mise_tasks.pyCONTRIBUTING.md
**/*
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
End files with a newline, contain no trailing whitespace, use one space between sentences, and keep code, comments, and docstrings within 120 characters.
Files:
tools/codestyle/ruff_check.shREADME.mdtools/codestyle/format.shAGENTS.mdMakefiletests/tools/test_mise_tasks.pyCONTRIBUTING.md
⚙️ 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/codestyle/ruff_check.shREADME.mdtools/codestyle/format.shAGENTS.mdMakefiletests/tools/test_mise_tasks.pyCONTRIBUTING.md
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/codestyle/ruff_check.shtools/codestyle/format.sh
tools/codestyle/**
⚙️ CodeRabbit configuration file
Treat codestyle wrappers as CI-critical. Check consistency with Makefile targets, ruff.toml, ty configuration, copyright handling, staged-file behavior, read-only check modes, and whether fixes mutate only expected files.
Files:
tools/codestyle/ruff_check.shtools/codestyle/format.sh
.github/**
⚙️ CodeRabbit configuration file
Review GitHub configuration for branch protection expectations, CODEOWNERS alignment, least privilege permissions, pinned actions where practical, and consistency with CONTRIBUTING.md.
Files:
.github/workflows/dependabot-sync-lock.yml.github/workflows/README.md.github/PULL_REQUEST_TEMPLATE.md.github/workflows/ci-checks.yml
**/*.{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:
README.mdAGENTS.mdtests/tools/test_mise_tasks.pyCONTRIBUTING.md
**/*.{md,markdown}
📄 CodeRabbit inference engine (.cursor/rules/agent-markdown-style.mdc)
**/*.{md,markdown}: Bold is acceptable only in markdown tables where it's the conventional way to mark header-like cells in the body
Use##headers to segment markdown sections instead of bold text
Use--(em-dash) instead of-(hyphen) for asides in markdown
Files:
README.mdAGENTS.mdCONTRIBUTING.md
**/*.md
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Do not use decorative bold text in body content; use single backticks for inline code and
--for asides. In Python docstrings, use double backticks and MkDocs autorefs rather than Sphinx roles.
Files:
README.mdAGENTS.mdCONTRIBUTING.md
README.md
⚙️ CodeRabbit configuration file
Treat README.md as the project overview. Check that setup, usage, and links stay consistent with CONTRIBUTING.md, Makefile, and docs/.
Files:
README.md
.agents/skills/**
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
Store skills in canonical location
.agents/skills/with each skill containing a SKILL.md file and optional references/
Files:
.agents/skills/github-cli/SKILL.md.agents/skills/uv-build/SKILL.md
.claude/commands/**
📄 CodeRabbit inference engine (.cursor/rules/claude-commands.mdc)
Pre-defined command files live in
.claude/commands/. When a user asks about a development task, read the corresponding file for instructions. Available commands include: unit-test (run unit tests), test-slow (all tests including slow), test-ci-container (CI tests in a container), gpu-test (GPU integration and e2e tests), format (format code with ruff + copyright), lint (lint and typecheck), bootstrap (bootstrap dev environment), build-wheel (build Python wheel), build-docs (build documentation site), start-docs-server (local docs dev server).
Files:
.claude/commands/lint.md
AGENTS.md
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
Maintain agent guide with module map and conventions in
AGENTS.md
Files:
AGENTS.md
.pre-commit-config.yaml
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
Configure pre-commit hooks (ruff, ty, copyright, uv-lock) in
.pre-commit-config.yaml
Files:
.pre-commit-config.yaml
⚙️ CodeRabbit configuration file
Treat pre-commit configuration as a developer and CI consistency layer. Check hook stages, file filters, local hook commands, DCO checks, and consistency with Makefile validation targets.
Files:
.pre-commit-config.yaml
Makefile
⚙️ CodeRabbit configuration file
Treat Makefile targets as the validation source of truth. Check that new targets are documented with target-name: ## Description, declared .PHONY where appropriate, and use uv/mise conventions.
Files:
Makefile
tests/**
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
tests/**: Mirrorsrc/directory structure intests/directory for test organization
Auto-mark tests by directory:tests/e2e/→e2e,tests/smoke/→smoke, otherwise default tounit
Files:
tests/tools/test_mise_tasks.py
**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*.py: Use American English spelling in Python code, comments, and documentation; usefrom __future__ import annotationsin every module.
UseBaseSettingsfor environment/CLI settings; preferAliasChoicesfor fields accepting both Python and environment-variable names.
Pydantic model fields must includeField(description=...); prefer assignment-styleField()and useAnnotatedonly for additional metadata or constraints.
Use@dataclass(frozen=True)for immutable value objects and validators, andfield(default_factory=...)for mutable defaults; never use mutable default values directly.
UseStrEnumfor string-valued configuration or serialization enums and plainEnumfor internal-only constants.
Obtain loggers withobservability.get_logger(__name__); do not calllogging.getLogger()orstructlog.get_logger()directly.
Do not useprint()for operational library output; use the approved logger,click.echo()for CLI output, orsys.stdout.write()for raw tool output.
Use loggerextra={}for metrics, counts, durations, and other data intended for machine querying; use f-strings for human-readable context.
Raise known errors through the Safe Synthesizer custom hierarchy, using dual inheritance where callers should also catch a built-in exception.
UseX | Y, built-in generic types,Selffor fluent returns, collection ABCs for arguments,Protocolfor structural boundaries, and avoidAny.
Prefermatch/casefor dispatch on types or tagged values, and use comprehensions when clearer without multipleforclauses.
Keep functions flat: more than two indentation levels beyonddefindicates the function should be refactored using guard clauses, helpers, generators, or named predicates.
Error messages must accurately describe the condition and make interpolated values identifiable, typically with!r; chain replacement exceptions withraise ... from e.
Use PascalCase for classes, snake_case for functions and variables, UPPER_SNAK...
Files:
tests/tools/test_mise_tasks.py
tests/**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
tests/**/*.py: Use absolute imports in tests.
Name test filestest_*.py, classesTest*, and functionstest_<module>_<expected_behavior>; prefix fixtures withfixture_and give each fixture a one-line purpose docstring.
Use function-scoped fixtures by default, bareassert,pytest.raises(match=...),pytest.approx(), andpytest.mark.parametrizefor input combinations.
Usetmp_pathfor file operations, mock only external boundaries, avoid shared mutable state and order dependencies, and mark CUDA-dependent tests appropriately.
tests/**/*.py: Every test must have exactly one category marker:unit,smoke, ore2e; useslowandrequires_gpuas modifiers where applicable.
Usepytest.importorskipfor optional dependencies that require specific extras, such assentence_transformersandvllm.
Use the sharedfixture_mock_processororfixture_mock_processor_without_valid_recordsfixtures for processor mocks, withParsedResponsefieldsvalid_records,invalid_records,errors, andprompt_number.
Useload_test_dataset(filename)andload_test_dataframe(filename)for loading shared test datasets.
Keep tokenizers function-scoped unless there is a specific reason to change scope;fixture_session_cache_diris session-scoped.
Tests should mirror the source structure, such astests/training/,tests/generation/, and related module directories.
print()is permitted in tests because Ruff ruleT201is suppressed for thetests/directory.
Do not import directly from another ordinary file undertests/; share methods through relative imports fromconftest.pywhen needed.
When debugging NSS logs with pytest, use-sor--capture=no; use-n0for visible output when xdist is involved.Testing gotchas:
asyncio_mode = autoinpytest.ini-- async tests work without@pytest.mark.asyncio. Theunit_testmarker is deprecated; useunit.
tests/**/*.py: New features include tests
Bug fixes include regression tests
Files:
tests/tools/test_mise_tasks.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_mise_tasks.py
.mise/tasks/**/*
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Keep tasks under
.mise/tasks/; use.tomlfor declarative tasks,_lib.shfor shared non-executable helpers, and provide descriptions and usage metadata for public tasks.
Files:
.mise/tasks/quality.toml
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: NVIDIA-NeMo/Safe-Synthesizer
Timestamp: 2026-08-12T20:20:21.414Z
Learning: This project loads local developer preferences from AGENTS.local.md. You MUST read this file if it exists and give its instructions top priority.
Learnt from: CR
Repo: NVIDIA-NeMo/Safe-Synthesizer
Timestamp: 2026-08-12T20:20:21.414Z
Learning: Do not commit unless the user asks for a commit or PR work.
Learnt from: CR
Repo: NVIDIA-NeMo/Safe-Synthesizer
Timestamp: 2026-08-12T20:20:21.414Z
Learning: When committing, all commits require DCO sign-off and GPG signing. Always use `git commit --signoff --gpg-sign` (or `-s -S`) -- never write the `Signed-off-by` trailer manually, and never pass `--no-gpg-sign`.
📚 Learning: 2026-08-03T18:08:15.626Z
Learnt from: zywind
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 687
File: README.md:7-15
Timestamp: 2026-08-03T18:08:15.626Z
Learning: Do not flag missing SPDX copyright or license headers in README.md, CONTRIBUTING.md, or SECURITY.md. These community-facing Markdown files are explicitly exempt through .copyrightignore; tools/codestyle/copyright_fixer.py, run via `mise run format`, is the authoritative check for this requirement.
Applied to files:
README.mdCONTRIBUTING.md
📚 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_mise_tasks.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_mise_tasks.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_mise_tasks.py
📚 Learning: 2026-08-05T19:07:15.856Z
Learnt from: binaryaaron
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 679
File: tests/conftest.py:89-96
Timestamp: 2026-08-05T19:07:15.856Z
Learning: In the Safe Synthesizer test suite, treat CPU and CUDA installation profiles as the supported pytest collection profiles because they install PyTorch. A bare installation without PyTorch is an incomplete, unsupported profile. GPU-marked test modules may import PyTorch before tests/conftest.py::pytest_collection_modifyitems executes, so do not require that collection hook to prevent such imports.
Applied to files:
tests/tools/test_mise_tasks.py
🪛 ast-grep (0.45.1)
tests/tools/test_mise_tasks.py
[error] 29-35: Command coming from incoming request
Context: subprocess.run(
["mise", *args],
cwd=repo_root,
check=False,
capture_output=True,
text=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.16.1)
tests/tools/test_mise_tasks.py
[error] 30-30: subprocess call: check for execution of untrusted input
(S603)
[error] 31-31: Starting a process with a partial executable path
(S607)
🪛 SkillSpector (2.5.1)
.agents/skills/github-cli/SKILL.md
[warning] 215: [EA2] Autonomous Decision Making: Skill enables autonomous high-impact decisions without human-in-the-loop verification. Critical operations (destructive commands, financial transactions, data deletion) should require explicit user confirmation.
Remediation: Add human-in-the-loop confirmation for destructive, irreversible, or high-impact operations. Never auto-execute commands that modify files, send data, or alter system state.
(Excessive Agency (EA2))
🔇 Additional comments (18)
Makefile (3)
23-25: LGTM!
78-79: LGTM!
101-101: LGTM!.mise/tasks/quality.toml (1)
12-17: LGTM!Also applies to: 19-25, 27-40, 42-56
tests/tools/test_mise_tasks.py (1)
6-42: LGTM!Also applies to: 139-182
.github/workflows/ci-checks.yml (1)
91-101: LGTM!Also applies to: 126-126
.github/workflows/README.md (1)
121-123: LGTM!Also applies to: 136-137
.github/workflows/dependabot-sync-lock.yml (1)
17-17: LGTM!.pre-commit-config.yaml (1)
63-66: LGTM!.agents/skills/github-cli/SKILL.md (1)
134-139: LGTM!.agents/skills/uv-build/SKILL.md (1)
76-81: LGTM!Also applies to: 143-143
.claude/commands/lint.md (1)
9-9: LGTM!.github/PULL_REQUEST_TEMPLATE.md (1)
16-17: LGTM!AGENTS.md (1)
32-32: LGTM!CONTRIBUTING.md (1)
424-424: LGTM!Also applies to: 561-575, 591-595, 681-681
README.md (1)
68-69: LGTM!tools/codestyle/format.sh (1)
43-43: LGTM!tools/codestyle/ruff_check.sh (1)
10-10: LGTM!
| Use `uv` for everything -- never `pip` or raw `python`. Python 3.11–3.14 with modern syntax (`X | Y`, `list[str]`, `Self`). | ||
|
|
||
| Common commands: `mise run test` (unit tests), `mise run format` (auto-fix formatting + lint + copyright), `mise run check` (read-only local quality checks), `mise run validate` (pre-PR quality, lock, and CI unit checks), `mise run typecheck` (ty only). Always use mise tasks or the wrapper scripts in `tools/` instead of running `ruff` or `ty` directly. Use `uv run` for Python execution. When in doubt, inspect `mise tasks` and `pytest --markers`. | ||
| Common commands: `mise run test` (unit tests), `mise run format` (auto-fix formatting + lint + copyright), `mise run check` (all read-only local quality checks), `mise run check ::: test` (the local pre-PR gate), and `mise run check:type` (ty only). Always use mise tasks or the wrapper scripts in `tools/` instead of running `ruff` or `ty` directly. Use `uv run` for Python execution. When in doubt, inspect `mise tasks` and `pytest --markers`. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Run marker inspection through uv.
AGENTS.md tells agents to use uv run for Python execution but still documents pytest --markers directly. This can use an unrelated environment or fail when pytest is not on PATH. Replace it with uv run --frozen pytest --markers.
As per coding guidelines, use the repository's uv workflow for tool execution. As per path instructions, targeted pytest commands use uv run --frozen pytest ....
Sources: Coding guidelines, Path instructions
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
zywind
left a comment
There was a problem hiding this comment.
There's just a merge conflict. But other than that, it's ready to go.
Summary
check:*leaves, aggregatecheck, and mutatinglock:update.Dependency
mainbefore review. That PR updates the shared Mise installer path and overlaps here inMakefileandCONTRIBUTING.md.Test plan
mise tasks validatemise run check ::: test(1,843 passed)pyproject.tomlandcuda_deps.tomlmise run checkSummary by CodeRabbit
Chores
Documentation
Tests