Reject scalar acceptance input rows - #4
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesAcceptance input validation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_kernel.py (1)
248-266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover
contradictionsand byte-string rows too.This regression test exercises only
gate_resultswithstrvalues. Since the implementation changes the shared normalizer for bothgate_resultsandcontradictions, add direct andfrom_dictcases for contradictions, plusbytescases, 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
📒 Files selected for processing (2)
src/evalopt_graph/kernel.pytests/test_kernel.py
|
Addressed the CodeRabbit notes in What changed:
Local validation on the pushed branch:
|
This keeps
AcceptanceInputfrom accepting a bare string row such asgate_results=("ok",)or{ "gate_results": ["ok"] }as("o", "k").Why it matters:
gate_resultsandcontradictionsare 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:
AcceptanceInputoutput shape is unchanged.Checklist:
Evidence:
uv run --frozen --extra dev python -m pytest tests/test_kernel.py -quv run --frozen --extra dev python -m pytest -quv run --frozen --extra dev python -m ruff check src tests scriptsuv run --frozen --extra dev python -m ruff format --check src tests scriptspython3 -m compileall -q src tests scriptsSummary by CodeRabbit
Bug Fixes
Tests