Skip to content

Reject scalar acceptance input rows - #4

Open
goktugozkanmd wants to merge 2 commits into
ajaysurya1221:mainfrom
goktugozkanmd:harden-acceptance-input-row-shapes
Open

Reject scalar acceptance input rows#4
goktugozkanmd wants to merge 2 commits into
ajaysurya1221:mainfrom
goktugozkanmd:harden-acceptance-input-row-shapes

Conversation

@goktugozkanmd

@goktugozkanmd goktugozkanmd commented Jul 26, 2026

Copy link
Copy Markdown

This keeps AcceptanceInput from accepting a bare string row such as gate_results=("ok",) or { "gate_results": ["ok"] } as ("o", "k").

Why it matters:

  • gate_results and contradictions are record-shaped fields, so scalar strings should fail at the boundary instead of being split into characters.
  • from_dict() now leaves row-shape validation to the constructor, keeping the direct and serialized paths consistent.

Boundary and compatibility:

  • Stable public API names are unchanged.
  • Serialized AcceptanceInput output shape is unchanged.
  • Well-formed list/tuple rows still round-trip as before.
  • The only behavior change is rejecting malformed scalar row inputs that were previously accepted by accidental character splitting.
  • Trust-boundary impact: malformed host observations fail at construction instead of reaching policy evaluation as synthetic two-character rows.

Checklist:

  • Problem and intended behavior stated.
  • Kernel boundary change only; no host/runtime adapter expansion.
  • Public API and serialized-record compatibility checked.
  • Failure mode covered by regression tests.
  • Local validation evidence listed below.

Evidence:

  • uv run --frozen --extra dev python -m pytest tests/test_kernel.py -q
  • uv run --frozen --extra dev python -m pytest -q
  • uv run --frozen --extra dev python -m ruff check src tests scripts
  • uv run --frozen --extra dev python -m ruff format --check src tests scripts
  • python3 -m compileall -q src tests scripts

Summary by CodeRabbit

  • Bug Fixes

    • Improved validation for acceptance data entries.
    • Invalid scalar, string, byte, and non-iterable row values now produce clearer validation errors.
    • Consistent validation is applied whether data is constructed directly or loaded from a dictionary.
  • Tests

    • Added regression coverage for invalid gate result and contradiction entries.

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 8b898ea1-1f07-49c0-846c-e5e30854542e

📥 Commits

Reviewing files that changed from the base of the PR and between 7f3eae4 and e575701.

📒 Files selected for processing (2)
  • src/evalopt_graph/kernel.py
  • tests/test_kernel.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/evalopt_graph/kernel.py

📝 Walkthrough

Walkthrough

AcceptanceInput now normalizes and validates row-like fields, rejects string and byte rows, and reports conversion or shape errors as ValueError. Dictionary construction delegates normalization to this logic, with regression tests covering direct and dictionary-based inputs.

Changes

Acceptance input validation

Layer / File(s) Summary
Row normalization and validation
src/evalopt_graph/kernel.py
The row normalizer rejects str and bytes, converts rows to tuples, and validates row widths and string elements.
Construction wiring and regression coverage
src/evalopt_graph/kernel.py, tests/test_kernel.py
from_dict passes raw row structures to the constructor, and tests verify scalar and non-iterable entries raise the expected ValueError through the supported construction paths.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: ajaysurya1221

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% 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
Title check ✅ Passed The title clearly summarizes the main change: rejecting scalar acceptance input rows.
Description check ✅ Passed The description covers the problem, boundary/compatibility impact, testing evidence, and a checklist, with only minor template formatting differences.
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 unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
tests/test_kernel.py (1)

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

Cover contradictions and byte-string rows too.

This regression test exercises only gate_results with str values. Since the implementation changes the shared normalizer for both gate_results and contradictions, add direct and from_dict cases for contradictions, plus bytes cases, preferably via parametrization.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_kernel.py` around lines 248 - 266, Add parametrized regression
coverage in
test_gate_result_rows_reject_bare_strings_instead_of_splitting_characters for
both gate_results and contradictions, covering direct AcceptanceInput
construction and AcceptanceInput.from_dict inputs with str and bytes rows.
Preserve the existing ValueError assertion and message match while reusing the
parametrized field/value cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/evalopt_graph/kernel.py`:
- Around line 160-172: Update the validation block that normalizes values so
top-level iteration failures from inputs such as None or integers are caught and
re-raised as the existing ValueError message. Wrap the loop over values,
including iterator acquisition, while preserving the current row-level
normalization and validation behavior.

---

Nitpick comments:
In `@tests/test_kernel.py`:
- Around line 248-266: Add parametrized regression coverage in
test_gate_result_rows_reject_bare_strings_instead_of_splitting_characters for
both gate_results and contradictions, covering direct AcceptanceInput
construction and AcceptanceInput.from_dict inputs with str and bytes rows.
Preserve the existing ValueError assertion and message match while reusing the
parametrized field/value cases.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: cbf25d7a-8fe1-4744-b793-cbdad78fd113

📥 Commits

Reviewing files that changed from the base of the PR and between cdf51d3 and 7f3eae4.

📒 Files selected for processing (2)
  • src/evalopt_graph/kernel.py
  • tests/test_kernel.py

Comment thread src/evalopt_graph/kernel.py
@goktugozkanmd

Copy link
Copy Markdown
Author

Addressed the CodeRabbit notes in e5757016ebf18423451fb12ac7868a91842fa66d.

What changed:

  • top-level non-iterable row containers such as None and integers now raise the existing ValueError shape message
  • regression coverage now includes gate_results and contradictions, with both str and bytes rows, through direct construction and from_dict

Local validation on the pushed branch:

  • uv run --frozen --extra dev python -m pytest -q
  • uv run --frozen --extra dev python -m ruff check src tests scripts
  • uv run --frozen --extra dev python -m ruff format --check src tests scripts
  • python3 -m compileall -q src tests scripts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant