DEV-1606: converge in-task + cascade graders on "accept what ex_base accepts" - #63
Conversation
…accepts" Three grading-stack defects made in-task / cascade grading STRICTER than the benchmark's authoritative ex_base grader, causing valid-answer thrash and zeroing correct answers. Defect 1 — in-task best-of-variant. Under --use-audited-gold-sql the in-task grader scored the agent ONLY against the audited PRIMARY variant. apply_audited_gold_overlay now attaches the FULL variant set as task["audited_variants"] for any guard-passing grouped row (decoupled from the sol_sql-swap status gate, so a primary with audit_status= "original" still exposes its edited variants); new evaluate_best_of_audited_variants accepts the first matching variant; _dispatch_eval falls back to best-of on a primary miss and surfaces the matched variant id via a non-agent-visible diagnostic (phase1_matched_audited_variant_id) — never in the agent observation. Defect 2 — cascade tier composition. N6/N7/N8/N9 were evaluated independently, so an answer correct only under (column-reorder + epsilon) passed no single tier. New bipartite compare_relaxed (fixes a pre-existing greedy false-fail in compare_numeric_epsilon too) + compare_column_order_ relaxed; N8 = strict OR reorder∘epsilon; N9 = terminal full cross-product (reorder ∘ epsilon ∘ trailing-whitespace ∘ case-fold, plus a no-reorder path for mismatched column names). Monotonicity unchanged. Defect 3 — precision parity with ex_base. N2/N3 + cell tiers compared raw rows; ex_base rounds to 2dp and strips ROUND from SQL. New upstream_ex_base adapters clean_sqls_like_ex_base (remove_round) + preprocess_rows_like_ex_base (2dp / date / dict canonicalisation); grade_submission normalizes pred/orig/variant rows (the single normalized variables used by every tier, the judge, persistence and diagnostics); the postgres in-task grader (_pg_execute_submit_action) routes through a new _compare_pg_rows_2dp (+ remove_round), with _pg_hashable_row as the hashable-safe fallback. Tests: full TDD suite landed first (composition, precision cascade + adapter, postgres 2dp, in-task best-of). Full non-integration suite green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
DEV-1606 In-task grader stricter than final cascade: single-gold + non-composed tolerance tiers cause valid-answer thrash
In-task grader is harsher than the final cascadeTwo related grader defects make the agent thrash (120+ turns, many resubmits) Defect 1 — in-task submit grades against ONE gold, not best-of audited variants
Best-of-audited-variant matching exists ONLY at final offline cascade grading Consequence: on an ambiguous task ( Fix: teach the in-task submit grader to accept a best-of match against ANY Defect 2 — cascade relaxation tiers are applied independently, never composed
Consequence: Fix: compose the cell/column relaxation tiers (at minimum N8 ∘ N6 — Why one issueBoth live in the grading stack ( Tests
Out of scope
ContextSequel finding from DEV-1589 (claude_sdk OTF encoder) reverse_logistics eval. |
|
Warning Review limit reached
More reviews will be available in 6 minutes and 25 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe pull request adds upstream-compatible SQL and row normalization for grading, revises tolerant comparison tiers with relaxed matching and column-order handling, and adds audited-variant best-of fallback with matched-variant tracking in submission results. ChangesGrading precision and audited fallback
Sequence Diagram(s)Postgres precision pathsequenceDiagram
participant _pg_execute_submit_action
participant clean_sqls_like_ex_base
participant preprocess_rows_like_ex_base
participant _compare_pg_rows_2dp
_pg_execute_submit_action->>clean_sqls_like_ex_base: strip ROUND(...) from agent and gold SQL
_pg_execute_submit_action->>preprocess_rows_like_ex_base: normalize pred_rows and gold_rows
_pg_execute_submit_action->>_compare_pg_rows_2dp: compute p1 from normalized rows
_compare_pg_rows_2dp-->>_pg_execute_submit_action: p1 and reward
Audited best-of fallbacksequenceDiagram
participant submit_raw_sql
participant _dispatch_eval
participant evaluate_dual_gold
participant evaluate_best_of_audited_variants
participant execute_submit_action
participant state.result
submit_raw_sql->>_dispatch_eval: phase-1 evaluation
_dispatch_eval->>evaluate_dual_gold: evaluate audited primary and original
_dispatch_eval->>evaluate_best_of_audited_variants: fallback when audited primary misses
evaluate_best_of_audited_variants->>execute_submit_action: try each audited variant SQL
execute_submit_action-->>evaluate_best_of_audited_variants: p1 / observation
evaluate_best_of_audited_variants-->>_dispatch_eval: matched_variant_id
_dispatch_eval-->>submit_raw_sql: p1 / reward / observation
submit_raw_sql->>state.result: phase1_matched_audited_variant_id
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/bird_interact_agents/agents/_submit.py (1)
429-429: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate
_dispatch_eval’s tuple contract docstring.Line 429 adds a 10th return value, but the docstring still documents only nine elements. Please include
matched_variant_idso future callers don’t unpack the helper incorrectly.🤖 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 `@src/bird_interact_agents/agents/_submit.py` at line 429, The `_dispatch_eval` tuple contract docstring is out of sync with the helper’s return values and still lists only nine items. Update the docstring in `_submit.py` to document the new `matched_variant_id` element as the 10th return value, using the `_dispatch_eval` symbol so callers can unpack it correctly.tests/test_pg_submit_precision.py (1)
123-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the agent-side
ROUND(...)stripping too.This test records
rec["pred_sql"], but the submitted SQL already has noROUND(...), so line 216 in_pg_execute_submit_actioncan regress without failing. Make the predicted query useROUND(...)as well and assert both recorded streams were cleaned.Suggested tweak
- obs, reward, p1, p2, finished = harness._pg_execute_submit_action( - "SELECT x AS m FROM t", status, "/tmp/ignored", + _obs, reward, p1, _p2, _finished = harness._pg_execute_submit_action( + "SELECT ROUND(x, 2) AS m FROM t", status, "/tmp/ignored", ) + assert all("ROUND" not in s.upper() for s in rec["pred_sql"]) # remove_round stripped ROUND from the executed gold SQL. assert all("ROUND" not in s.upper() for s in rec["gold_sqls"])🤖 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_pg_submit_precision.py` around lines 123 - 147, The test only verifies ROUND stripping on the executed gold SQL, so regressions in the agent-side cleanup inside _pg_execute_submit_action can slip through. Update test_pg_execute_submit_action_strips_round_and_uses_2dp to submit a predicted query that also contains ROUND(...), then assert both rec["pred_sql"] and rec["gold_sqls"] have ROUND removed after _pg_execute_submit_action runs, while keeping the 2dp normalization assertion intact.
🤖 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/bird_interact_agents/agents/_submit.py`:
- Around line 600-603: The raw dry-run failure path in _submit.py is leaving a
stale phase1_matched_audited_variant_id in prior because it spreads prior
without resetting that diagnostic. Update the dry-run failure branch in the
submission flow so it explicitly clears phase1_matched_audited_variant_id
whenever phase1_passed is set to False, while keeping the evaluated-submission
path that writes matched_variant_id unchanged.
In `@src/bird_interact_agents/eval/tolerant_grader.py`:
- Around line 442-446: The relaxed string comparison in compare_relaxed should
use Unicode-aware case folding instead of lowercasing when casefold is enabled.
Update the string normalization branch in tolerant_grader.compare_relaxed so the
casefold path calls str.casefold() on both inputs, preserving the existing strip
behavior and keeping the rest of the matching logic unchanged.
In `@src/bird_interact_agents/harness.py`:
- Around line 896-900: The legacy single_file path in harness.py is attaching
all rows for an instance_id without checking whether they belong to the same
selected_database or benchmark, which can leak foreign SQL into
task["audited_variants"]. Update the logic around flat_rows_by_iid /
variants_by_iid so the list is filtered to the current primary row’s
database/benchmark before assigning audited_variants, and apply the same
safeguard in the later single_file handling block near the other referenced
section.
---
Nitpick comments:
In `@src/bird_interact_agents/agents/_submit.py`:
- Line 429: The `_dispatch_eval` tuple contract docstring is out of sync with
the helper’s return values and still lists only nine items. Update the docstring
in `_submit.py` to document the new `matched_variant_id` element as the 10th
return value, using the `_dispatch_eval` symbol so callers can unpack it
correctly.
In `@tests/test_pg_submit_precision.py`:
- Around line 123-147: The test only verifies ROUND stripping on the executed
gold SQL, so regressions in the agent-side cleanup inside
_pg_execute_submit_action can slip through. Update
test_pg_execute_submit_action_strips_round_and_uses_2dp to submit a predicted
query that also contains ROUND(...), then assert both rec["pred_sql"] and
rec["gold_sqls"] have ROUND removed after _pg_execute_submit_action runs, while
keeping the 2dp normalization assertion intact.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 45933ada-c73f-44e9-bb5e-a47d6458e114
📒 Files selected for processing (8)
src/bird_interact_agents/agents/_submit.pysrc/bird_interact_agents/eval/tolerant_grader.pysrc/bird_interact_agents/eval/upstream_ex_base.pysrc/bird_interact_agents/harness.pytests/eval/test_tolerant_grader_composition.pytests/eval/test_tolerant_grader_precision.pytests/test_in_task_best_of_variant.pytests/test_pg_submit_precision.py
- _submit.py: clear stale phase1_matched_audited_variant_id on the raw dry-run failure branch (was carried forward via the **prior spread); document the new 10th _dispatch_eval tuple element. - tolerant_grader.py: use str.casefold() (Unicode-aware) instead of lower() in the relaxed cell predicate. - harness.py: filter legacy flat single_file best-of variants by the primary row's (selected_database, benchmark) so a same-instance_id row from another DB/benchmark can't leak foreign SQL into best-of grading. - Regression tests for the stale-id clear, the foreign-SQL filter, and the Unicode casefold. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Three grading-stack defects made in-task / cascade grading stricter than the benchmark's authoritative
ex_basegrader, causing valid-answer thrash (120+ turns, many resubmits) and zeroing correct answers. Surfaced by the DEV-1589reverse_logisticsrun, where both non-passes (_17,_18) were grader artifacts, not reasoning misses.Defect 1 — in-task submit grades against ONE gold, not best-of audited variants
Under
--use-audited-gold-sqlthe in-task grader scored the agent ONLY against the audited primary variant. On an ambiguous task an agent that commits to a valid NON-primary reading got false-negative feedback and thrashed (reverse_logistics_17: 19 resubmits against the primary while its result exactly matched the audited variant the FINAL grader accepts).apply_audited_gold_overlaynow attaches the full variant set astask["audited_variants"]for any guard-passing grouped row — decoupled from thesol_sql-swap status gate, so a primary withaudit_status="original"still exposes its edited variants.harness.evaluate_best_of_audited_variants(...)accepts the first matching variant (swap-and-call, status restored infinally)._dispatch_evalfalls back to best-of on a primary miss; the matched variant id is surfaced via a non-agent-visible diagnostic (phase1_matched_audited_variant_id) — never in the agent observation.Defect 2 — cascade relaxation tiers applied independently, never composed
N6 (epsilon) / N7 (whitespace) / N8 (column-order) / N9 (case-fold) were evaluated independently, so an answer correct only under (column-reorder + 2dp-round) passed no single tier →
agent_miss(reverse_logistics_18).compare_relaxed(also fixes a pre-existing greedy false-fail incompare_numeric_epsilon) +compare_column_order_relaxed.Defect 3 — cascade precision diverges from
ex_baseN2/N3 + cell tiers compared raw rows;
ex_baserounds floats/Decimals to 2dp (ROUND_HALF_UP) and stripsROUND(...)from SQL. So94.15248…(full-precision agent) vs94.15(2dp gold) FAILED every cascade tier but PASSESex_base.upstream_ex_baseadaptersclean_sqls_like_ex_base(remove_round) +preprocess_rows_like_ex_base(2dp / date / dict canonicalisation), identity fallback when upstream is absent.grade_submissionnormalizes pred/orig/variant rows + strips ROUND; normalized rows are the single variables used by every tier, the judge, persistence and diagnostics. N1's primary path keeps raw SQL (it cleans internally)._pg_execute_submit_action) routes through a new_compare_pg_rows_2dp(+remove_round), with_pg_hashable_rowas the hashable-safe fallback.Tests
Full TDD suite landed first: cascade composition, precision (cascade + adapter), postgres 2dp, and in-task best-of. Edge cases from the plan reviews — overlay-attach when primary is
original, same-order epsilon+case with mismatched column names, bipartite-not-greedy, postgres dict-cell fallback — are all exercised. Full non-integration suite green (3426 passed, 94 skipped).Out of scope
Closes DEV-1606.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes