fix: add max_records_per_sequence when generating#663
Conversation
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
WalkthroughChangesGrouped record bounds
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
coderabbitai
Bot
added
feature
Greptile SummaryThis PR adds an optional
Confidence Score: 5/5Safe to merge; the core grammar-construction logic is well-tested and the train/val stat separation is correct. The bounded-group grammar path is covered by regex round-trip tests and structural-tag acceptance/rejection tests. The training stat separation is verified by asserting records_per_group.count == num_groups_train. The pre-existing is_val detection pattern is already relied on elsewhere in the assembler for identical stats. The four if max_records: truthiness nits were already called out in prior review threads. regex_manager.py — the four if max_records: truthiness checks (already flagged in prior threads) are the only open concern. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| record_repeat = rf"({record_regex}\n){{1,{max_records}}}" if max_records else rf"({record_regex}\n)+" | ||
| sequence_regex = rf"{re.escape(bos_token)}{record_repeat}{re.escape(eos_token)}" | ||
| else: | ||
| # Without grouping, the "sequence" is a single record. | ||
| sequence_regex = record_regex | ||
|
|
||
| if config.generation.structured_generation.use_single_sequence and config.data.max_sequences_per_example == 1: | ||
| if config.data.group_training_examples_by is not None and max_records: |
There was a problem hiding this comment.
if max_records: truthiness checks conflate None (no bound) with 0 (also treated as no bound). The returned type from _resolve_max_records_per_sequence is int | None, so the intended sentinel is None. Using is not None makes the intent explicit and avoids any future confusion if a caller somehow passes 0 — e.g., _repeat_format(record_line_format, 1, 0) would produce an invalid min=1, max=0 grammar node, whereas if max_records is not None: would treat it as a (malformed) bound and surface the error clearly.
| record_repeat = rf"({record_regex}\n){{1,{max_records}}}" if max_records else rf"({record_regex}\n)+" | |
| sequence_regex = rf"{re.escape(bos_token)}{record_repeat}{re.escape(eos_token)}" | |
| else: | |
| # Without grouping, the "sequence" is a single record. | |
| sequence_regex = record_regex | |
| if config.generation.structured_generation.use_single_sequence and config.data.max_sequences_per_example == 1: | |
| if config.data.group_training_examples_by is not None and max_records: | |
| record_repeat = rf"({record_regex}\n){{1,{max_records}}}" if max_records is not None else rf"({record_regex}\n)+" | |
| sequence_regex = rf"{re.escape(bos_token)}{record_repeat}{re.escape(eos_token)}" | |
| else: | |
| # Without grouping, the "sequence" is a single record. | |
| sequence_regex = record_regex | |
| if config.data.group_training_examples_by is not None and max_records is not None: |
| record_repetition = ( | ||
| _repeat_format(record_line_format, 1, max_records) if max_records else _plus_format(record_line_format) | ||
| ) | ||
| sequence_format = _sequence_format( | ||
| [ | ||
| _const_string_format(bos_token), | ||
| _plus_format(record_line_format), | ||
| record_repetition, | ||
| _const_string_format(eos_token), | ||
| ] | ||
| ) | ||
| else: | ||
| sequence_format = record_format | ||
|
|
||
| if config.generation.structured_generation.use_single_sequence and config.data.max_sequences_per_example == 1: | ||
| if config.data.group_training_examples_by is not None and max_records: |
There was a problem hiding this comment.
if max_records: truthiness pattern in build_json_structural_tag. Both conditions should use is not None to match the int | None contract of _resolve_max_records_per_sequence.
| record_repetition = ( | |
| _repeat_format(record_line_format, 1, max_records) if max_records else _plus_format(record_line_format) | |
| ) | |
| sequence_format = _sequence_format( | |
| [ | |
| _const_string_format(bos_token), | |
| _plus_format(record_line_format), | |
| record_repetition, | |
| _const_string_format(eos_token), | |
| ] | |
| ) | |
| else: | |
| sequence_format = record_format | |
| if config.generation.structured_generation.use_single_sequence and config.data.max_sequences_per_example == 1: | |
| if config.data.group_training_examples_by is not None and max_records: | |
| record_repetition = ( | |
| _repeat_format(record_line_format, 1, max_records) if max_records is not None else _plus_format(record_line_format) | |
| ) | |
| sequence_format = _sequence_format( | |
| [ | |
| _const_string_format(bos_token), | |
| record_repetition, | |
| _const_string_format(eos_token), | |
| ] | |
| ) | |
| else: | |
| sequence_format = record_format | |
| if config.data.group_training_examples_by is not None and max_records is not None: |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 365b1892-4127-47cf-8c95-a47f6ec2d36f
📒 Files selected for processing (8)
src/nemo_safe_synthesizer/config/generate.pysrc/nemo_safe_synthesizer/data_processing/assembler.pysrc/nemo_safe_synthesizer/generation/regex_manager.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pysrc/nemo_safe_synthesizer/llm/metadata.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pytests/generation/test_regex_manager.pytests/generation/test_vllm_backend.py
📜 Review details
⏰ Context from checks skipped due to timeout. (9)
- GitHub Check: Unit Tests (3.13)
- GitHub Check: Unit Tests (3.11)
- GitHub Check: Unit Tests (3.12)
- GitHub Check: Smoke Tests
- GitHub Check: End-user Wheel Install
- GitHub Check: Greptile Review
- GitHub Check: Analyze (Python)
- GitHub Check: Typecheck
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (16)
**/*.{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:
src/nemo_safe_synthesizer/data_processing/assembler.pysrc/nemo_safe_synthesizer/llm/metadata.pysrc/nemo_safe_synthesizer/config/generate.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pytests/generation/test_vllm_backend.pytests/generation/test_regex_manager.pysrc/nemo_safe_synthesizer/generation/regex_manager.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.
UseField(description=...)for every Pydantic model field.
Use assignment-styleField()by default; useAnnotatedonly 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.
Usefield(default_factory=list)instead of mutable list defaults.
UseStrEnumfor string-valued configuration or serialization enums and plainEnumfor internal constants.
Obtain loggers withobservability.get_logger(__name__); do not calllogging.getLogger()orstructlog.get_logger()directly.
Use.runtime,.user, and.systemcategory loggers appropriately.
Do not useprint()for operational library output; use the approved logger,click.echo(), orsys.stdout.write()where appropriate.
Useextra={}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.
PreferX | Y, built-in collection generics, andSelfoverOptional,Union, and legacy typing collections.
Use collection ABCs for function arguments and concrete collection types for return values.
UseProtocolfor structural subtyping and avoidAnywhenobject, generics, or protocols are suitable.
UseTYPE_CHECKINGguards for heavy imports such as pandas, torch, and transformers.
...
Files:
src/nemo_safe_synthesizer/data_processing/assembler.pysrc/nemo_safe_synthesizer/llm/metadata.pysrc/nemo_safe_synthesizer/config/generate.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pytests/generation/test_vllm_backend.pytests/generation/test_regex_manager.pysrc/nemo_safe_synthesizer/generation/regex_manager.py
src/**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
src/**/*.py: Use relative imports for package code undersrc/.
Do not useassertfor validation in library code; raise an appropriate exception instead.Write Google-style docstrings for public Python APIs because API reference pages are generated from source docstrings.
Files:
src/nemo_safe_synthesizer/data_processing/assembler.pysrc/nemo_safe_synthesizer/llm/metadata.pysrc/nemo_safe_synthesizer/config/generate.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pysrc/nemo_safe_synthesizer/generation/regex_manager.py
⚙️ CodeRabbit configuration file
Review library code against STYLE_GUIDE.md. Focus on behavior, API contracts, error handling, resource cleanup, typing, logging, and user-facing failures. Public APIs and nontrivial functions need Google-style docstrings.
Files:
src/nemo_safe_synthesizer/data_processing/assembler.pysrc/nemo_safe_synthesizer/llm/metadata.pysrc/nemo_safe_synthesizer/config/generate.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pysrc/nemo_safe_synthesizer/generation/regex_manager.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 contributions must use verified Git commits and DCO sign-off; unsigned or unsigned-off commits cannot be merged.
Branches other thanmainmust follow<author>/<description>, optionally including an issue ID or type; branch names must use lowercase alphanumeric characters and hyphens.
Commits merged tomainmust follow Conventional Commits, using a valid lowercase type and a description of at most 100 characters.
Files:
src/nemo_safe_synthesizer/data_processing/assembler.pysrc/nemo_safe_synthesizer/llm/metadata.pysrc/nemo_safe_synthesizer/config/generate.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pytests/generation/test_vllm_backend.pytests/generation/test_regex_manager.pysrc/nemo_safe_synthesizer/generation/regex_manager.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:
src/nemo_safe_synthesizer/data_processing/assembler.pysrc/nemo_safe_synthesizer/llm/metadata.pysrc/nemo_safe_synthesizer/config/generate.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pytests/generation/test_vllm_backend.pytests/generation/test_regex_manager.pysrc/nemo_safe_synthesizer/generation/regex_manager.py
**/*.{py,sh,yaml,yml,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
All Python, shell, YAML, YML, and Markdown source files require SPDX copyright headers.
Files:
src/nemo_safe_synthesizer/data_processing/assembler.pysrc/nemo_safe_synthesizer/llm/metadata.pysrc/nemo_safe_synthesizer/config/generate.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pytests/generation/test_vllm_backend.pytests/generation/test_regex_manager.pysrc/nemo_safe_synthesizer/generation/regex_manager.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Shared Python package code must remain compatible with Python 3.11 syntax; do not use Python 3.12-only syntax such as PEP 695
typestatements or bracketed generic class/function parameters.
Files:
src/nemo_safe_synthesizer/data_processing/assembler.pysrc/nemo_safe_synthesizer/llm/metadata.pysrc/nemo_safe_synthesizer/config/generate.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pytests/generation/test_vllm_backend.pytests/generation/test_regex_manager.pysrc/nemo_safe_synthesizer/generation/regex_manager.py
**/*.{py,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use the repository's documented Python and Markdown style conventions and validate changes with the pinned
miseformatting and checking tasks.
Files:
src/nemo_safe_synthesizer/data_processing/assembler.pysrc/nemo_safe_synthesizer/llm/metadata.pysrc/nemo_safe_synthesizer/config/generate.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pytests/generation/test_vllm_backend.pytests/generation/test_regex_manager.pysrc/nemo_safe_synthesizer/generation/regex_manager.py
**/*.{py,sh}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use the repository's pinned
misetasks for formatting, linting, type checking, and testing rather than invokingruffortydirectly for project-wide checks.
Files:
src/nemo_safe_synthesizer/data_processing/assembler.pysrc/nemo_safe_synthesizer/llm/metadata.pysrc/nemo_safe_synthesizer/config/generate.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pytests/generation/test_vllm_backend.pytests/generation/test_regex_manager.pysrc/nemo_safe_synthesizer/generation/regex_manager.py
src/nemo_safe_synthesizer/data_processing/**/*.py
⚙️ CodeRabbit configuration file
Review for data-contract regressions. Check input/training/test/synthetic naming, group boundaries, token-budget math, record ordering, schema and column validation, nullable dtypes, and deterministic behavior.
Files:
src/nemo_safe_synthesizer/data_processing/assembler.py
src/**/config/**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Use
NSSBaseModelfor user-facing configuration and parameter models.
Files:
src/nemo_safe_synthesizer/config/generate.py
src/nemo_safe_synthesizer/config/**/*.py
⚙️ CodeRabbit configuration file
Treat config changes as user-facing API changes. Check Pydantic field descriptions, defaults, validators, aliases, override behavior, CLI help text impact, YAML compatibility, and documented parameter semantics.
Files:
src/nemo_safe_synthesizer/config/generate.py
src/nemo_safe_synthesizer/training/**/*.py
⚙️ CodeRabbit configuration file
Review training changes for dataset preprocessing, model path handling, artifact writes, LoRA/DP behavior, GPU memory usage, reproducibility, and cleanup on failure.
Files:
src/nemo_safe_synthesizer/training/huggingface_backend.py
src/nemo_safe_synthesizer/generation/**/*.py
⚙️ CodeRabbit configuration file
Review generation changes for retry loops, stopping conditions, invalid record handling, regex/structured output contracts, backend teardown, memory cleanup, and vLLM assumptions.
Files:
src/nemo_safe_synthesizer/generation/vllm_backend.pysrc/nemo_safe_synthesizer/generation/regex_manager.py
**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use the
unitmarker instead of the deprecatedunit_testmarker for test identification
Files:
tests/generation/test_vllm_backend.pytests/generation/test_regex_manager.py
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 tounitMirror source code directory structure in tests directory (e.g.,
tests/training/,tests/generation/parallel to source structure)
Files:
tests/generation/test_vllm_backend.pytests/generation/test_regex_manager.py
tests/**/*.py
📄 CodeRabbit inference engine (tests/TESTING.md)
tests/**/*.py: Auto-mark tests based on file path: tests under/e2e/gete2emarker, tests under/smoke/getsmokemarker, all others getunitmarker (only if no category marker already present)
Every test should have exactly one category marker:unit,smoke, ore2e
Usepytest.mark.requires_gpumodifier on tests that need CUDA hardware
Usepytest.mark.vllmon tests using vLLM generation backend and ensure each vLLM test file runs in its own process for GPU memory isolation
Usepytest.mark.slowon long-running tests
Usepytest.mark.smollm2for SmolLM2 Hub download tests to enable process isolation
Usepytest.mark.noautouseto skip autouse fixtures for specific tests
Useload_test_dataset(filename)helper to load test datasets fromtests/stub_datasets/as HuggingFaceDatasetobjects
Useload_test_dataframe(filename)helper to load test data files fromtests/stub_datasets/as pandas DataFrames
Convert pandas columns to nullable dtypes (pd.Int64Dtype(),pd.BooleanDtype()) before assigningnp.nanvalues
Usefake.seed_instance(seed)andrandom.seed(seed)together for Faker-based test data reproducibility
When sharing methods across multiple test files, define them inconftest.pyand import them using relative imports (e.g.,from .conftest import train_with_sdk); note that importing from other test files liketests/cli/helpers.pydoes not work
Usefixture_mock_processororfixture_mock_processor_without_valid_recordsfor mocking ParsedResponse objects withvalid_records,invalid_records,errors, andprompt_numberfields
Usepytest.importorskipto 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 (ruffT201is suppressed fortests/directory) and should...
Files:
tests/generation/test_vllm_backend.pytests/generation/test_regex_manager.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/generation/test_vllm_backend.pytests/generation/test_regex_manager.py
🧠 Learnings (2)
📚 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/generation/test_vllm_backend.pytests/generation/test_regex_manager.py
📚 Learning: 2026-06-04T16:14:09.868Z
Learnt from: binaryaaron
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 526
File: tests/generation/test_vllm_backend.py:399-509
Timestamp: 2026-06-04T16:14:09.868Z
Learning: In this repo, only apply `pytest.mark.vllm` to smoke tests under `tests/smoke/` that actually run real vLLM GPU generation and therefore require per-file process isolation (e.g., `test-smoke-gpu-*` Makefile targets). Do not apply `pytest.mark.vllm` to unit-style tests under `tests/generation/` that merely import `vllm_backend` but never instantiate a real vLLM engine and never call `.generate()` (GPU not required). Note that `tests/conftest.py` auto-marks these as `unit` via `pytest_collection_modifyitems`, and `vllm` is not among the auto-mark categories—so if a test in `tests/generation/` has `vllm`, it should be treated as a review issue unless it meets the real GPU generation criteria above.
Applied to files:
tests/generation/test_vllm_backend.pytests/generation/test_regex_manager.py
🪛 ast-grep (0.44.1)
tests/generation/test_regex_manager.py
[warning] 463-463: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.fullmatch(regex, one_record)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
[warning] 466-466: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.fullmatch(regex, two_records)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
[warning] 470-470: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.fullmatch(regex, three_records)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
[warning] 474-474: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.fullmatch(regex, two_groups)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
🪛 Ruff (0.15.21)
src/nemo_safe_synthesizer/training/huggingface_backend.py
[warning] 792-792: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
🔇 Additional comments (3)
src/nemo_safe_synthesizer/config/generate.py (1)
168-188: LGTM!src/nemo_safe_synthesizer/generation/vllm_backend.py (1)
396-402: LGTM!Also applies to: 412-418
tests/generation/test_vllm_backend.py (1)
50-50: LGTM!Also applies to: 204-210, 254-260, 287-293, 320-326
| if config.data.group_training_examples_by is not None and max_records: | ||
| # Grouped generation produces one group per completion; a bounded single sequence | ||
| # (no outer repetition) guarantees a terminable, parseable group. | ||
| regex = sequence_regex | ||
| elif config.generation.structured_generation.use_single_sequence and config.data.max_sequences_per_example == 1: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep record-count and sequence-count constraints independent.
A non-null per-group record bound currently forces exactly one group for both output formats, bypassing use_single_sequence and the previous multi-group behavior.
src/nemo_safe_synthesizer/generation/regex_manager.py#L386-L390: retain or separately bound the outer sequence repetition unless single-sequence generation is selected.src/nemo_safe_synthesizer/generation/regex_manager.py#L472-L476: apply the same independent outer repetition to the structural-tag format.
📍 Affects 1 file
src/nemo_safe_synthesizer/generation/regex_manager.py#L386-L390(this comment)src/nemo_safe_synthesizer/generation/regex_manager.py#L472-L476
Source: Path instructions
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
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: 8da42f22-1222-4e14-a6dc-1301b6bdc06a
📒 Files selected for processing (1)
src/nemo_safe_synthesizer/config/generate.py
📜 Review details
⏰ Context from checks skipped due to timeout. (10)
- GitHub Check: Unit Tests (3.12)
- GitHub Check: Smoke Tests
- GitHub Check: Unit Tests (3.13)
- GitHub Check: Unit Tests (3.11)
- GitHub Check: End-user Wheel Install
- GitHub Check: Greptile Review
- GitHub Check: Typecheck
- GitHub Check: Analyze (Python)
- GitHub Check: Analyze (javascript-typescript)
- 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:
src/nemo_safe_synthesizer/config/generate.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.
UseField(description=...)for every Pydantic model field.
Use assignment-styleField()by default; useAnnotatedonly 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.
Usefield(default_factory=list)instead of mutable list defaults.
UseStrEnumfor string-valued configuration or serialization enums and plainEnumfor internal constants.
Obtain loggers withobservability.get_logger(__name__); do not calllogging.getLogger()orstructlog.get_logger()directly.
Use.runtime,.user, and.systemcategory loggers appropriately.
Do not useprint()for operational library output; use the approved logger,click.echo(), orsys.stdout.write()where appropriate.
Useextra={}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.
PreferX | Y, built-in collection generics, andSelfoverOptional,Union, and legacy typing collections.
Use collection ABCs for function arguments and concrete collection types for return values.
UseProtocolfor structural subtyping and avoidAnywhenobject, generics, or protocols are suitable.
UseTYPE_CHECKINGguards for heavy imports such as pandas, torch, and transformers.
...
Files:
src/nemo_safe_synthesizer/config/generate.py
src/**/config/**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Use
NSSBaseModelfor user-facing configuration and parameter models.
Files:
src/nemo_safe_synthesizer/config/generate.py
src/**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
src/**/*.py: Use relative imports for package code undersrc/.
Do not useassertfor validation in library code; raise an appropriate exception instead.Write Google-style docstrings for public Python APIs because API reference pages are generated from source docstrings.
Files:
src/nemo_safe_synthesizer/config/generate.py
⚙️ CodeRabbit configuration file
Review library code against STYLE_GUIDE.md. Focus on behavior, API contracts, error handling, resource cleanup, typing, logging, and user-facing failures. Public APIs and nontrivial functions need Google-style docstrings.
Files:
src/nemo_safe_synthesizer/config/generate.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 contributions must use verified Git commits and DCO sign-off; unsigned or unsigned-off commits cannot be merged.
Branches other thanmainmust follow<author>/<description>, optionally including an issue ID or type; branch names must use lowercase alphanumeric characters and hyphens.
Commits merged tomainmust follow Conventional Commits, using a valid lowercase type and a description of at most 100 characters.
Files:
src/nemo_safe_synthesizer/config/generate.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:
src/nemo_safe_synthesizer/config/generate.py
**/*.{py,sh,yaml,yml,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
All Python, shell, YAML, YML, and Markdown source files require SPDX copyright headers.
Files:
src/nemo_safe_synthesizer/config/generate.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Shared Python package code must remain compatible with Python 3.11 syntax; do not use Python 3.12-only syntax such as PEP 695
typestatements or bracketed generic class/function parameters.
Files:
src/nemo_safe_synthesizer/config/generate.py
**/*.{py,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use the repository's documented Python and Markdown style conventions and validate changes with the pinned
miseformatting and checking tasks.
Files:
src/nemo_safe_synthesizer/config/generate.py
**/*.{py,sh}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use the repository's pinned
misetasks for formatting, linting, type checking, and testing rather than invokingruffortydirectly for project-wide checks.
Files:
src/nemo_safe_synthesizer/config/generate.py
src/nemo_safe_synthesizer/config/**/*.py
⚙️ CodeRabbit configuration file
Treat config changes as user-facing API changes. Check Pydantic field descriptions, defaults, validators, aliases, override behavior, CLI help text impact, YAML compatibility, and documented parameter semantics.
Files:
src/nemo_safe_synthesizer/config/generate.py
| "Max records per grouped sequence under structured generation. " | ||
| "None uses the largest training group size. Must be None or >= 1." |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document the None fallback accurately.
None uses the largest training group size only when a training-derived default is provided. If that default is also None, the downstream regex and structural-tag builders use unbounded repetition. Update the description so CLI/config help does not promise a bound that may not exist.
Proposed fix
- "None uses the largest training group size. Must be None or >= 1."
+ "When unset, uses the largest training group size when available; "
+ "otherwise, generation is unbounded. Must be None or >= 1."As per path instructions, config changes are user-facing API changes and documented parameter semantics must match runtime behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "Max records per grouped sequence under structured generation. " | |
| "None uses the largest training group size. Must be None or >= 1." | |
| "Max records per grouped sequence under structured generation. " | |
| "When unset, uses the largest training group size when available; " | |
| "otherwise, generation is unbounded. Must be None or >= 1." |
Source: Path instructions
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7cbb23ab-dc46-4af4-9708-0c558cb04318
📒 Files selected for processing (4)
src/nemo_safe_synthesizer/data_processing/assembler.pysrc/nemo_safe_synthesizer/llm/metadata.pytests/data_processing/test_assembler.pytests/llm/test_metadata.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/nemo_safe_synthesizer/data_processing/assembler.py
- src/nemo_safe_synthesizer/llm/metadata.py
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Greptile Review
🧰 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/llm/test_metadata.pytests/data_processing/test_assembler.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.
UseField(description=...)for every Pydantic model field.
Use assignment-styleField()by default; useAnnotatedonly 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.
Usefield(default_factory=list)instead of mutable list defaults.
UseStrEnumfor string-valued configuration or serialization enums and plainEnumfor internal constants.
Obtain loggers withobservability.get_logger(__name__); do not calllogging.getLogger()orstructlog.get_logger()directly.
Use.runtime,.user, and.systemcategory loggers appropriately.
Do not useprint()for operational library output; use the approved logger,click.echo(), orsys.stdout.write()where appropriate.
Useextra={}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.
PreferX | Y, built-in collection generics, andSelfoverOptional,Union, and legacy typing collections.
Use collection ABCs for function arguments and concrete collection types for return values.
UseProtocolfor structural subtyping and avoidAnywhenobject, generics, or protocols are suitable.
UseTYPE_CHECKINGguards for heavy imports such as pandas, torch, and transformers.
...
Files:
tests/llm/test_metadata.pytests/data_processing/test_assembler.py
**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use the
unitmarker instead of the deprecatedunit_testmarker for test identification
Files:
tests/llm/test_metadata.pytests/data_processing/test_assembler.py
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 tounitMirror source code directory structure in tests directory (e.g.,
tests/training/,tests/generation/parallel to source structure)
Files:
tests/llm/test_metadata.pytests/data_processing/test_assembler.py
tests/**/*.py
📄 CodeRabbit inference engine (tests/TESTING.md)
tests/**/*.py: Auto-mark tests based on file path: tests under/e2e/gete2emarker, tests under/smoke/getsmokemarker, all others getunitmarker (only if no category marker already present)
Every test should have exactly one category marker:unit,smoke, ore2e
Usepytest.mark.requires_gpumodifier on tests that need CUDA hardware
Usepytest.mark.vllmon tests using vLLM generation backend and ensure each vLLM test file runs in its own process for GPU memory isolation
Usepytest.mark.slowon long-running tests
Usepytest.mark.smollm2for SmolLM2 Hub download tests to enable process isolation
Usepytest.mark.noautouseto skip autouse fixtures for specific tests
Useload_test_dataset(filename)helper to load test datasets fromtests/stub_datasets/as HuggingFaceDatasetobjects
Useload_test_dataframe(filename)helper to load test data files fromtests/stub_datasets/as pandas DataFrames
Convert pandas columns to nullable dtypes (pd.Int64Dtype(),pd.BooleanDtype()) before assigningnp.nanvalues
Usefake.seed_instance(seed)andrandom.seed(seed)together for Faker-based test data reproducibility
When sharing methods across multiple test files, define them inconftest.pyand import them using relative imports (e.g.,from .conftest import train_with_sdk); note that importing from other test files liketests/cli/helpers.pydoes not work
Usefixture_mock_processororfixture_mock_processor_without_valid_recordsfor mocking ParsedResponse objects withvalid_records,invalid_records,errors, andprompt_numberfields
Usepytest.importorskipto 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 (ruffT201is suppressed fortests/directory) and should...
Files:
tests/llm/test_metadata.pytests/data_processing/test_assembler.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/llm/test_metadata.pytests/data_processing/test_assembler.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 contributions must use verified Git commits and DCO sign-off; unsigned or unsigned-off commits cannot be merged.
Branches other thanmainmust follow<author>/<description>, optionally including an issue ID or type; branch names must use lowercase alphanumeric characters and hyphens.
Commits merged tomainmust follow Conventional Commits, using a valid lowercase type and a description of at most 100 characters.
Files:
tests/llm/test_metadata.pytests/data_processing/test_assembler.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/llm/test_metadata.pytests/data_processing/test_assembler.py
**/*.{py,sh,yaml,yml,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
All Python, shell, YAML, YML, and Markdown source files require SPDX copyright headers.
Files:
tests/llm/test_metadata.pytests/data_processing/test_assembler.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Shared Python package code must remain compatible with Python 3.11 syntax; do not use Python 3.12-only syntax such as PEP 695
typestatements or bracketed generic class/function parameters.
Files:
tests/llm/test_metadata.pytests/data_processing/test_assembler.py
**/*.{py,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use the repository's documented Python and Markdown style conventions and validate changes with the pinned
miseformatting and checking tasks.
Files:
tests/llm/test_metadata.pytests/data_processing/test_assembler.py
**/*.{py,sh}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use the repository's pinned
misetasks for formatting, linting, type checking, and testing rather than invokingruffortydirectly for project-wide checks.
Files:
tests/llm/test_metadata.pytests/data_processing/test_assembler.py
🧠 Learnings (1)
📚 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/llm/test_metadata.pytests/data_processing/test_assembler.py
🔇 Additional comments (2)
tests/data_processing/test_assembler.py (1)
457-460: LGTM!tests/llm/test_metadata.py (1)
729-742: LGTM!
| # Holdout groups must not inflate the training-derived generation bound. | ||
| assert examples.stats["records_per_group"].count == assembler.num_groups_train | ||
| assert assembler.stats_val["records_per_group"].count == assembler.num_groups_validation |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the training maximum, not only the count.
These assertions do not prove the stated holdout invariant: validation data could still affect examples.stats["records_per_group"].max while the count remains equal to num_groups_train. Assert the recorded training maximum against an independently known training-only value (and, if practical, the validation maximum separately).
As per path instructions, tests should prove the affected invariant rather than only exercising the code path.
Source: Path instructions
| def test_metadata_max_records_per_group_accepts_none_or_positive( | ||
| self, sample_prompt_config, mock_autoconfig_obj, sample_workdir | ||
| ): | ||
| """``None`` and values ``>= 1`` are valid persisted bounds.""" | ||
| none_meta = ModelMetadata( | ||
| model_name_or_path="test-model", | ||
| prompt_config=sample_prompt_config, | ||
| autoconfig=mock_autoconfig_obj, | ||
| workdir=sample_workdir, | ||
| max_records_per_group=None, | ||
| ) | ||
| assert none_meta.max_records_per_group is None | ||
| positive_meta = ModelMetadata( | ||
| model_name_or_path="test-model", | ||
| prompt_config=sample_prompt_config, | ||
| autoconfig=mock_autoconfig_obj, | ||
| workdir=sample_workdir, | ||
| max_records_per_group=3, | ||
| ) | ||
| assert positive_meta.max_records_per_group == 3 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover the inclusive lower bound with 1.
The contract says values >= 1 are valid, but the only positive case uses 3. Add 1 to the accepted cases so a validator that accidentally rejects the boundary cannot pass this test.
As per path instructions, tests should cover focused boundary behavior for changed validation logic.
Source: Path instructions
1 participant
Summary
Pre-Review Checklist
Ensure that the following pass:
mise run format && mise run checkor via prek validation.mise run testpasses locallymise run test:e2epasses locallymise run test:ci-containerpasses locally (recommended)/syncon this PR to trigger a run (auto-triggers on ready-for-review)Pre-Merge Checklist
Other Notes
Summary by CodeRabbit
New Features
max_records_per_sequenceto cap how many records are generated per grouped sequence.Bug Fixes
Tests