refactor(records): add JSON record boundaries

[binaryaaron]

Summary

• Adds shared recursive JSON aliases and runtime guards for record handling.
• Narrows record key and generation result boundaries.

Test plan

• mise run test -- tests/data_processing/test_records.py tests/generation/test_generation.py
• mise run typecheck

Related issue: #614

Summary by CodeRabbit

• New Features
  • Added shared JSON type aliases and runtime type guards to improve schema validation safety.
  • Record keys are now consistently normalized for more predictable generated outputs.
• Bug Fixes
  • Non-object JSON inputs are now rejected earlier with clearer “invalid type” errors.
  • JSON-schema validation failures now report more informative details.
  • Elapsed-seconds timestamps use more consistent numeric coercion, and record key coercion is enforced.
• Refactor
  • Streamlined record flattening and type classification logic with stricter handling.
• Tests
  • Added coverage for top-level array flattening, JSON guards (including non-finite numbers), and non-object schema classification.

[coderabbitai]

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: ff8ed539-5591-4df1-bdab-e40bd44da611

📥 Commits

Reviewing files that changed from the base of the PR and between a7f61aa and 756d95c.

📒 Files selected for processing (8)

• src/nemo_safe_synthesizer/data_processing/record_utils.py
• src/nemo_safe_synthesizer/data_processing/records/base.py
• src/nemo_safe_synthesizer/data_processing/records/json_record.py
• src/nemo_safe_synthesizer/data_processing/records/json_types.py
• src/nemo_safe_synthesizer/data_processing/records/value_path.py
• src/nemo_safe_synthesizer/generation/results.py
• tests/data_processing/test_records.py
• tests/generation/test_generation.py

🚧 Files skipped from review as they are similar to previous changes (8)

• src/nemo_safe_synthesizer/data_processing/records/json_types.py
• tests/generation/test_generation.py
• src/nemo_safe_synthesizer/data_processing/records/base.py
• src/nemo_safe_synthesizer/generation/results.py
• src/nemo_safe_synthesizer/data_processing/records/value_path.py
• tests/data_processing/test_records.py
• src/nemo_safe_synthesizer/data_processing/records/json_record.py
• src/nemo_safe_synthesizer/data_processing/record_utils.py

📜 Recent review details

⏰ Context from checks skipped due to timeout. (9)

• GitHub Check: CodeRabbit
• GitHub Check: Unit Tests (3.13)
• GitHub Check: Unit Tests (3.11)
• GitHub Check: Unit Tests (3.12)
• GitHub Check: Smoke Tests
• GitHub Check: Greptile Review
• GitHub Check: Analyze (Python)
• GitHub Check: Typecheck
• GitHub Check: Analyze (python)

---

Walkthrough

This PR adds shared JSON type aliases and runtime guards, tightens record parsing and typing, refactors JSON record helpers and unflatten typing, normalizes record keys in generation, and updates tests for the new JSON and record behavior.

Changes

JSON typing and record utility refactor

Layer / File(s)	Summary
New JSON type aliases and runtime guards src/nemo_safe_synthesizer/data_processing/records/json_types.py	Defines recursive JSON aliases and adds runtime validation for JSON values and objects, including finite-number and string-key checks.
Record typing and validation updates src/nemo_safe_synthesizer/data_processing/record_utils.py	Adds record mapping aliases and key normalization, updates parsed-record and helper annotations, validates decoded JSON objects, stringifies schema validators, normalizes elapsed-seconds parsing, and updates JSONL input dispatch.
Record helper refactors src/nemo_safe_synthesizer/data_processing/records/base.py, src/nemo_safe_synthesizer/data_processing/records/json_record.py, src/nemo_safe_synthesizer/data_processing/records/value_path.py	Refactors type classification and JSON flattening, expands JSON record exports and annotations, and tightens unflatten helper types.
Record normalization wiring and tests src/nemo_safe_synthesizer/generation/results.py, tests/data_processing/test_records.py, tests/generation/test_generation.py	Normalizes transformed records before storing them in generation results and adds coverage for JSON guards, flattening, object parsing, and normalized fixtures.

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

Suggested labels: refactor, test

Suggested reviewers: nina-xu, kendrickb-nvidia

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 58.97% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is concise and accurately reflects the main change: tightening JSON record boundaries in record handling.
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-03-record-boundaries

---

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

[codecov]

Codecov Report

❌ Patch coverage is 85.48387% with 18 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...o_safe_synthesizer/data_processing/records/base.py	0.00%	10 Missing ⚠️
...synthesizer/data_processing/records/json_record.py	82.85%	6 Missing ⚠️
...o_safe_synthesizer/data_processing/record_utils.py	95.65%	1 Missing ⚠️
..._synthesizer/data_processing/records/value_path.py	83.33%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[greptile-apps]

Greptile Summary

This PR introduces a dedicated json_types.py module with shared recursive JSON type aliases (JsonValue, JsonObject, etc.) and runtime guards (is_json_value, is_json_object), then wires those guards into _parse_and_validate_json to reject non-object top-level JSON and non-finite floats (NaN/Infinity) before schema validation. Supporting changes tighten type annotations throughout record_utils.py, json_record.py, and value_path.py, and add normalize_record_keys to enforce string keys on generation output records.

• New json_types.py: Defines JsonScalar | JsonValue | JsonObject | JsonArray | JsonContainer | JsonSchema aliases plus is_json_value/is_json_object guards; the bool() arm in is_json_value is unreachable dead code (flagged in a previous thread) but the overall behavior is correct.
• _parse_and_validate_json guards: Two early-return checks now reject bare arrays/scalars with "Invalid JSON type" and dicts containing non-finite floats or non-string keys with "Invalid JSON value", both before jsonschema.validate() runs.
• normalize_record_keys + test updates: Records coming out of data-action merges in GenerationBatches are now explicitly normalized to string keys, and test fixtures are updated to match.

Confidence Score: 5/5

Safe to merge; the new JSON guards and type narrowing are well-tested and introduce no behavioral regressions on the record-processing hot path.

All changed code paths have explicit unit tests, including the new NaN/Infinity rejection, non-object top-level JSON, and top-level array flattening. The normalize_record_keys addition is a one-liner with clear intent. No logic regressions were found in the flatten refactor or the timestamp conversion change.

No files require special attention.

Important Files Changed

Filename	Overview
src/nemo_safe_synthesizer/data_processing/records/json_types.py	New file defining shared JSON type aliases and runtime guards is_json_value/is_json_object; logic is correct, though the bool() match arm is unreachable dead code (already noted in a previous review thread).
src/nemo_safe_synthesizer/data_processing/record_utils.py	Tightened type annotations (RecordDict/RecordMapping/RawRecordMapping), added early JSON object guard in _parse_and_validate_json, changed int(float(value)) to int(float(str(value))), and introduced normalize_record_keys; all changes are safe.
src/nemo_safe_synthesizer/data_processing/records/json_record.py	flatten() refactored to match/case, using str(key) for dict keys and adding an explicit TypeError for non-dict/non-list input; behavior is preserved and tested.
src/nemo_safe_synthesizer/data_processing/records/base.py	get_type_as_string refactored from isinstance chain to match/case; bool() is correctly placed before Number() to avoid subclass precedence issue, preserving existing behavior.
src/nemo_safe_synthesizer/generation/results.py	Adds normalize_record_keys() call after data-action record merge to ensure string keys; straightforward and correct.
src/nemo_safe_synthesizer/data_processing/records/value_path.py	Minor typing improvements: generic Any parameters added to list/dict helpers and void return types annotated; no behavioral changes.
tests/data_processing/test_records.py	New tests cover top-level array flattening, recursive JSON type guards, non-finite float rejection, and non-object JSON classification; good coverage of the new guards.
tests/generation/test_generation.py	Test fixtures updated to wrap records with normalize_record_keys(), keeping them aligned with the new string-key contract in GenerationBatches.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["json.loads(matched_json)"] --> B{isinstance dict?}
    B -- No --> C["Error: 'Invalid JSON type'"]
    B -- Yes --> D{is_json_object?<br/>checks string keys<br/>+ isfinite floats recursively}
    D -- No --> E["Error: 'Invalid JSON value'"]
    D -- Yes --> F["jsonschema.validate(matched_dict, schema)"]
    F -- ValidationError --> G["Error: err.message / str(err.validator)"]
    F -- OK --> H{check_record_for_large_numbers}
    H -- overflow --> I["Error: 'Float Conversion'"]
    H -- OK --> J["Return matched_dict ✓"]

    style C fill:#f88,stroke:#c00
    style E fill:#f88,stroke:#c00
    style G fill:#f88,stroke:#c00
    style I fill:#f88,stroke:#c00
    style J fill:#8f8,stroke:#080

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A["json.loads(matched_json)"] --> B{isinstance dict?}
    B -- No --> C["Error: 'Invalid JSON type'"]
    B -- Yes --> D{is_json_object?<br/>checks string keys<br/>+ isfinite floats recursively}
    D -- No --> E["Error: 'Invalid JSON value'"]
    D -- Yes --> F["jsonschema.validate(matched_dict, schema)"]
    F -- ValidationError --> G["Error: err.message / str(err.validator)"]
    F -- OK --> H{check_record_for_large_numbers}
    H -- overflow --> I["Error: 'Float Conversion'"]
    H -- OK --> J["Return matched_dict ✓"]

    style C fill:#f88,stroke:#c00
    style E fill:#f88,stroke:#c00
    style G fill:#f88,stroke:#c00
    style I fill:#f88,stroke:#c00
    style J fill:#8f8,stroke:#080

Loading

_{Reviews (6): Last reviewed commit: "fix(records): address review feedback" | Re-trigger Greptile}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

src/nemo_safe_synthesizer/data_processing/records/json_types.py (1)

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

Delegate is_json_object to is_json_value to avoid logic drift.

is_json_object's body duplicates the exact validation is_json_value already performs in its case dict() branch (Line 43). Two independent implementations of the same check can silently diverge if one is updated later.

♻️ Proposed refactor

 def is_json_object(value: object) -> TypeIs[JsonObject]:
     """Return whether ``value`` is a JSON object with string keys."""
-    return isinstance(value, dict) and all(isinstance(key, str) and is_json_value(item) for key, item in value.items())
+    return isinstance(value, dict) and is_json_value(value)

src/nemo_safe_synthesizer/data_processing/records/value_path.py (1)

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

Prefer X | None over Optional[X | Y].

These touched signatures mix Optional[...] with the newer X | Y union syntax (Optional[dict[Any, Any] | list[Any]]). As per coding guidelines, "Prefer native typing syntax: X | Y instead of Optional[X] or Union[X, Y]."

♻️ Proposed fix

-def unflatten(data: dict[ValuePath, Any]) -> Optional[dict[Any, Any] | list[Any]]:
+def unflatten(data: dict[ValuePath, Any]) -> dict[Any, Any] | list[Any] | None:

 def _unflatten_path(
-    result: Optional[dict[Any, Any] | list[Any]], path: ValuePath, value: Any
+    result: dict[Any, Any] | list[Any] | None, path: ValuePath, value: Any
 ) -> dict[Any, Any] | list[Any]:

Also applies to: 110-112

Source: Coding guidelines

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: f1e9a0a1-531e-424e-8802-76d39b7a4772

📥 Commits

Reviewing files that changed from the base of the PR and between 26aa563 and 1b1c2db.

📒 Files selected for processing (8)

• src/nemo_safe_synthesizer/data_processing/record_utils.py
• src/nemo_safe_synthesizer/data_processing/records/base.py
• src/nemo_safe_synthesizer/data_processing/records/json_record.py
• src/nemo_safe_synthesizer/data_processing/records/json_types.py
• src/nemo_safe_synthesizer/data_processing/records/value_path.py
• src/nemo_safe_synthesizer/generation/results.py
• tests/data_processing/test_records.py
• tests/generation/test_generation.py

📜 Review details

⏰ Context from checks skipped due to timeout. (6)

• GitHub Check: Unit Tests (3.13)
• GitHub Check: Unit Tests (3.11)
• GitHub Check: Unit Tests (3.12)
• GitHub Check: Smoke Tests
• GitHub Check: Greptile Review
• GitHub Check: Analyze (Python)

🧰 Additional context used

📓 Path-based instructions (12)

**/*.{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/generation/test_generation.py
• src/nemo_safe_synthesizer/data_processing/records/json_types.py
• src/nemo_safe_synthesizer/generation/results.py
• src/nemo_safe_synthesizer/data_processing/records/base.py
• src/nemo_safe_synthesizer/data_processing/records/value_path.py
• tests/data_processing/test_records.py
• src/nemo_safe_synthesizer/data_processing/records/json_record.py
• src/nemo_safe_synthesizer/data_processing/record_utils.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: Source code must remain Python 3.11 syntax-compatible; do not use Python 3.12-only syntax such as PEP 695 type statements or bracketed generic class/function parameters in shared package code
Use ruff for formatting and linting via mise run format and mise run check tasks; run formatting before committing
Use ty for type checking via mise run check task; ensure type hints are present and valid
New features must include tests; bug fixes must include regression tests

**/*.py: Use BaseSettings for env/CLI settings, prefer AliasChoices for per-field dual naming, and use env_prefix only for simple settings classes with a shared prefix.
Use Field(description=...) as the canonical field docstring for Pydantic models, and always include it.
Use assignment-style Field(default=..., description="...") as the default for model fields; prefer it over Annotated unless extra metadata is needed.
Use Annotated only when the field carries additional metadata beyond Field() (for example validators, reusable constrained aliases, nested-type constraints, or discriminated unions).
When Annotated is used, place defaults as bare assignments (= value), except default_factory, which should still use assignment-style Field(default_factory=...).
For immutable value objects and validators, prefer @dataclass(frozen=True); use mutable dataclasses only for builders, accumulators, and pipeline state.
Use field(default_factory=list) for mutable defaults; never use = [].
Use StrEnum for string-valued enums used in configs or serialization; use plain Enum for internal-only named constants.
Use observability.get_logger(__name__) for logging; do not call logging.getLogger() or structlog.get_logger() direc...

Files:

• tests/generation/test_generation.py
• src/nemo_safe_synthesizer/data_processing/records/json_types.py
• src/nemo_safe_synthesizer/generation/results.py
• src/nemo_safe_synthesizer/data_processing/records/base.py
• src/nemo_safe_synthesizer/data_processing/records/value_path.py
• tests/data_processing/test_records.py
• src/nemo_safe_synthesizer/data_processing/records/json_record.py
• src/nemo_safe_synthesizer/data_processing/record_utils.py

**/test_*.py

📄 CodeRabbit inference engine (AGENTS.md)

Use the unit marker instead of the deprecated unit_test marker for test identification

Files:

• tests/generation/test_generation.py
• tests/data_processing/test_records.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/generation/test_generation.py
• tests/data_processing/test_records.py

---

⚙️ CodeRabbit configuration file

tests/**:

Testing Guide

Comprehensive testing reference for Safe-Synthesizer developers. Covers commands, markers, test data, fixtures, and gotchas.

Read First

1. tests/conftest.py -- auto-marking, load_test_dataset/load_test_dataframe, fixture_mock_processor pattern
2. pytest.ini -- markers, asyncio, timeout
3. tests/evaluation/conftest.py -- most complex: Faker-based make_df, nullable dtype conversion
4. tests/generation/conftest.py -- JSONL/schema fixtures, fixture_valid_iris_dataset_jsonl_and_schema

Running Tests

All mise test tasks, grouped by scope:

mise run test                              # Unit (excludes slow, e2e, and smoke)
mise run test:unit-slow                    # Unit tests including slow (excludes e2e and smoke)
mise run test:smoke                        # CPU smoke tests (~few min, no GPU required)
mise run test:smoke:gpu                    # All staged GPU smoke tests (requires CUDA)
mise run test:smoke:gpu:train-only
mise run test:smoke:gpu:generation
mise run test:smoke:gpu:resume
mise run test:smoke:gpu:structured-generation
mise run test:smoke:gpu:timeseries
mise run test:smoke:gpu:smollm2
mise run test:e2e                          # All e2e (requires CUDA) -- runs default + dp
mise run test:e2e:default                  # e2e default (no-DP) tests only
mise run test:e2e:dp                       # e2e DP tests only
mise run test:ci                           # CI unit tests with coverage (excludes slow, e2e, gpu, smoke)
mise run test:ci-slow                      # CI slow tests with coverage
mise run test:ci-container                 # CI tests in a Linux container (Docker/Podman)

Run a single test:

uv run --frozen pytest tests/path/test_file.py::test_name -vvs -n0

Test runner: uv run --frozen pytest -n auto --dist loadscope -vv...

Files:

• tests/generation/test_generation.py
• tests/data_processing/test_records.py

**/*.{py,sh,yaml,yml,md}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

All source files (.py, .sh, .yaml, .yml, .md) require SPDX copyright headers; mise run format adds them automatically

Files:

• tests/generation/test_generation.py
• src/nemo_safe_synthesizer/data_processing/records/json_types.py
• src/nemo_safe_synthesizer/generation/results.py
• src/nemo_safe_synthesizer/data_processing/records/base.py
• src/nemo_safe_synthesizer/data_processing/records/value_path.py
• tests/data_processing/test_records.py
• src/nemo_safe_synthesizer/data_processing/records/json_record.py
• src/nemo_safe_synthesizer/data_processing/record_utils.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/generation/test_generation.py
• tests/data_processing/test_records.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_generation.py
• tests/data_processing/test_records.py

**/*

📄 CodeRabbit inference engine (STYLE_GUIDE.md)

**/*: Every source file must include the required SPDX copyright and license header; use HTML comments for Markdown, hash comments for .py, .sh, .yaml, and .yml, and hash-comment headers inside YAML frontmatter for Markdown files with frontmatter.
Ensure files end with a newline and have no trailing whitespace; use a single space between sentences.

Files:

• tests/generation/test_generation.py
• src/nemo_safe_synthesizer/data_processing/records/json_types.py
• src/nemo_safe_synthesizer/generation/results.py
• src/nemo_safe_synthesizer/data_processing/records/base.py
• src/nemo_safe_synthesizer/data_processing/records/value_path.py
• tests/data_processing/test_records.py
• src/nemo_safe_synthesizer/data_processing/records/json_record.py
• src/nemo_safe_synthesizer/data_processing/record_utils.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/generation/test_generation.py
• src/nemo_safe_synthesizer/data_processing/records/json_types.py
• src/nemo_safe_synthesizer/generation/results.py
• src/nemo_safe_synthesizer/data_processing/records/base.py
• src/nemo_safe_synthesizer/data_processing/records/value_path.py
• tests/data_processing/test_records.py
• src/nemo_safe_synthesizer/data_processing/records/json_record.py
• src/nemo_safe_synthesizer/data_processing/record_utils.py

**

⚙️ CodeRabbit configuration file

**:

AGENTS.md

Guide for AI agents (Cursor, Windsurf, Claude Code, etc.) working in the Safe-Synthesizer repo.

This project loads local developer preferences from @AGENTS.local.md. You MUST read this file if it exists and give its instructions top priority.

Skills

Repo-specific skills live in .agents/skills/; see .agents/README.md for the catalog. Read a skill when the task matches its scope instead of copying workflow details into this file.

Durable implementation guidance belongs with the code it describes: function and class docstrings for public contracts and source comments for local invariants. Test-suite guidance belongs in tests/TESTING.md.

Repo Conventions

See STYLE_GUIDE.md for detailed code style conventions (Python, markdown, Dockerfiles, shell scripts, testing, config files, docstrings).

Use uv for everything -- never pip or raw python. Python 3.11–3.13 with modern syntax (X | Y, list[str], Self). Python 3.14+ is not supported.

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.

The canonical uv sync command for a full GPU/dev environment is:

uv sync --frozen --extra cu129 --extra engine --group dev

Bare uv sync --frozen (without extras) installs an incomplete environment -- ty, import checks, and GPU tests will fail.

Feature branches off main. Branch names often include an issue number prefix (e.g., <author>/123-short-name).

Do ...

Files:

• tests/generation/test_generation.py
• src/nemo_safe_synthesizer/data_processing/records/json_types.py
• src/nemo_safe_synthesizer/generation/results.py
• src/nemo_safe_synthesizer/data_processing/records/base.py
• src/nemo_safe_synthesizer/data_processing/records/value_path.py
• tests/data_processing/test_records.py
• src/nemo_safe_synthesizer/data_processing/records/json_record.py
• src/nemo_safe_synthesizer/data_processing/record_utils.py

src/nemo_safe_synthesizer/**/*.py

📄 CodeRabbit inference engine (CONTRIBUTING.md)

API reference pages are auto-generated from Python docstrings using Google-style format; write docstrings in src/nemo_safe_synthesizer/ and they will appear in the reference/

Files:

• src/nemo_safe_synthesizer/data_processing/records/json_types.py
• src/nemo_safe_synthesizer/generation/results.py
• src/nemo_safe_synthesizer/data_processing/records/base.py
• src/nemo_safe_synthesizer/data_processing/records/value_path.py
• src/nemo_safe_synthesizer/data_processing/records/json_record.py
• src/nemo_safe_synthesizer/data_processing/record_utils.py

src/**/*.py

📄 CodeRabbit inference engine (STYLE_GUIDE.md)

src/**/*.py: Use relative imports inside src/, for example from ..observability import get_logger.
Every directory under src/ that contains Python files must include an __init__.py file.

Files:

• src/nemo_safe_synthesizer/data_processing/records/json_types.py
• src/nemo_safe_synthesizer/generation/results.py
• src/nemo_safe_synthesizer/data_processing/records/base.py
• src/nemo_safe_synthesizer/data_processing/records/value_path.py
• src/nemo_safe_synthesizer/data_processing/records/json_record.py
• src/nemo_safe_synthesizer/data_processing/record_utils.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/records/json_types.py
• src/nemo_safe_synthesizer/generation/results.py
• src/nemo_safe_synthesizer/data_processing/records/base.py
• src/nemo_safe_synthesizer/data_processing/records/value_path.py
• src/nemo_safe_synthesizer/data_processing/records/json_record.py
• src/nemo_safe_synthesizer/data_processing/record_utils.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/records/json_types.py
• src/nemo_safe_synthesizer/data_processing/records/base.py
• src/nemo_safe_synthesizer/data_processing/records/value_path.py
• src/nemo_safe_synthesizer/data_processing/records/json_record.py
• src/nemo_safe_synthesizer/data_processing/record_utils.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/results.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_generation.py
• tests/data_processing/test_records.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_generation.py

🪛 Ruff (0.15.20)

src/nemo_safe_synthesizer/data_processing/records/value_path.py

[warning] 101-101: Loop control variable i not used within loop body

Rename unused i to _i

(B007)

🔇 Additional comments (7)

src/nemo_safe_synthesizer/generation/results.py (1)

17-17: LGTM!

Also applies to: 298-298

tests/data_processing/test_records.py (1)

11-56: LGTM!

Also applies to: 181-187

tests/generation/test_generation.py (1)

16-16: LGTM!

Also applies to: 264-272

src/nemo_safe_synthesizer/data_processing/records/json_record.py (2)

55-62: Already discussed — the None-keyed wrapping for top-level lists.

This was raised in a prior review thread and confirmed intentional (the None key is later replaced with real values during flattening).

---

15-152: LGTM!

src/nemo_safe_synthesizer/data_processing/records/base.py (1)

92-104: LGTM!

src/nemo_safe_synthesizer/data_processing/records/value_path.py (1)

142-142: LGTM!