DEV-1524: add claude_sdk_otf_raw + claude_sdk_otf_ainteract_raw agents - #21
Conversation
…museum runs analyses/: per-run summaries for `20260531t1008-claudes-slayer-890419` (households 11/15) and `20260531t1013-claudes-slayer-48eb0f` (museum 4/10). analyses/raw/: per-task deep dives for the 10 failures (museum_2..10 + households_2/10/12/15) and one cross-cutting observations file. Each references the trajectory items, KB ids, column-meaning lines, and slayer model paths used as evidence; documents whether the answer was derivable in principle from supplied metadata and which signal the agent failed to use (when applicable). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Moves mini-interact from the per_db sidecar layout (audited_gold/<db>/<db>_audited.jsonl) to a single consolidated JSONL at audited_gold/mini_interact_audited.jsonl, matching the livesqlbench layout introduced in DEV-1510. Each row gains three fields: - benchmark: "mini_interact" (required by harness defence-in-depth) - variant_id: "primary" (DEV-1515 multi-variant gold support) - primary: true (interaction-time feedback discriminator) livesqlbench's existing single_file JSONL retro-tagged with the same variant_id / primary fields for layout consistency across benchmarks. The consolidation itself is in scripts/consolidate_mini_interact_audited.py (idempotent: skips fields already present with expected values; rejects benchmark-tag mismatches). audited_gold/ is fully gitignored, so this commit ships the script + descriptor change; the on-disk JSONLs are local-only artefacts other developers regenerate by running the script. apply_audited_gold_overlay's single_file branch already handles this layout (DEV-1510), so the only src change is flipping mini-interact's Benchmark.audited_gold_layout from per_db to single_file. Tests: - test_benchmark.py: assertion flipped per_db → single_file - test_paths.py: per_db_layout_raises → single_file path resolves - test_dual_eval.py: per_db_explicit_matches_default rewritten as single_file dispatch test - Full non-integration suite: 1697 passed, 94 skipped, 50 deselected Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
claude_sdk_otf + claude_sdk_otf_ainteract: add a symmetric companion to
the existing "drop predicate when KB literals are absent from sampled
values" rule. The new rule tells the agent: when the column's
`Sample values` show variants of KB-named literals (case, whitespace,
abbreviations, alternate phrasings of the same concept), normalise and
extend the IN-set to include them.
Targets the households_10 + museum_2 / museum_10 class of failure:
KB enumerations are non-exhaustive ("etc.", "like", "include"); the
sampled values are the authoritative inventory of what's actually
present. A canonical-only IN-set silently misses matching rows.
Also adds a paragraph promoting `search(entities=["<db>.<model>.<col>"],
max_memories=0, max_example_queries=0)` to the front of the slayer-tools
section. EntityHit.text carries Description + Sample values inline;
this is the canonical "read by known ref" primitive for verifying any
filter / projection / join-key column before committing.
Out of scope: the rendered `Sample values` is the truncated comma-string
(motley-slayer's `render_column_text`), not the full structured
`sampled_values: List[str]`. Wider variant coverage requires a separate
slayer-upstream change to expose the structured field via search /
inspect_model.
scripts/verify_audited_gold.py: reads the consolidated
mini_interact_audited.jsonl when audit-set=inhouse and filters by
selected_database (per DEV-1515 consolidation). SAR audit set is
unchanged.
Tests:
- Existing non-integration suite: 1696 passed, 95 skipped, 50 deselected.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…chemas
src/bird_interact_agents/eval/: new module hosting the DEV-1515
annotation infrastructure.
annotation_schema.py:
- TaskAnnotation: per-instance, run-independent. Captures
metadata_sufficiency.verdict ∈ {sufficient, ambiguous, insufficient},
a list of GoldVariantRef (each pointing to a row in the consolidated
audited-gold JSONL by (instance_id, variant_id), with one tagged
primary=True for interaction-time feedback), and an evaluator_prompt
(only invoked when verdict==insufficient).
- SubmissionAnnotation: per-(instance, run). Carries the cascading
SubmissionEvaluation block (original gold → primary variant → any
variant → tie tolerance → LLM judge), FailureClassification
(top-level enum primary + free-form details), decision-point
reference, and UserSimInteraction.
- Every model is ConfigDict(extra="forbid") so silent schema drift is
impossible. Container fields use typed BaseModels rather than
Dict[...].
annotation_io.py:
- read_/write_ helpers for both kinds with Pydantic JSON round-trip.
- task_annotation_path / submission_annotation_path: canonical
`annotations/<benchmark>/<db>/<instance_id>.{task,submission.<run_id>}.json`
anchored at the main checkout (matches audited_gold/results
contract).
- iter_task_annotations / iter_submission_annotations for batch
consumers.
tests/test_eval_annotation_schema.py: 8 cases covering minimal
construction, JSON round-trip, forbid-extra, path helpers, and the
on-disk 2-space-indent + trailing-newline invariant.
Out of scope (follow-up commits):
- generate_task_annotation / generate_submission_annotation that
materialise skeletons from existing artefacts (task data + audited
gold + attempt JSON).
- CLI entry point (`bird-interact-annotate ...`).
- tolerant_grader.py implementing Tier 1 + Tier 2 evaluation that
populates SubmissionEvaluation.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…eract} The cloud-submit snippets shipped with `--workers 1 --actors-per-worker 1` which is correct for the 1-task smokes shown but silently serialises larger runs through a single actor. The DEV-1515 38-task remainder run inherited that and took ~5 hours instead of ~80 minutes (~25 min of cluster boot + 38 × ~7 min per task). Add a sizing note + a 10-instance example using `--actors-per-worker 4` on each framework. e2-standard-4 (4 vCPU / 16 GB) comfortably runs 4 concurrent Opus + Sonnet pairs since the workload is network-bound on the LLM APIs, so 4 is a safe default for 10-50 task batches. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Lands the 8-row cascading verdict (N1=original / N2=audited primary /
N3=any audited variant / N4=tie-order / N5=LLM-judge / N6=numeric-eps /
N7=trailing-whitespace / N8=column-order via case-insensitive name align),
the inline-graded per-task SubmissionAnnotation produced by ray_app and
the local run path, the no-overwrite fetch-side merge into
<main_checkout>/annotations/, and the offline regrade CLI.
* `eval/tolerant_grader.py` — 8-row monotone cascade, ORDER BY parser,
cell-level relaxations, content-hashed LLM-judge cache (model + annotation
hash + gold-variants hash + normalized SQL; runId NOT in key so offline
re-grade reuses cloud-side decisions), Tier 2 informational diff.
* `eval/implicit_annotation.py` — schema-valid in-memory default for
unannotated instances; cascade collapses to N1.
* `eval/grade_in_place.py` — shared inline grader called by both
ray_app (cloud) and run.py (local) so eval.json's `cascading_phase1`
has source data on both paths.
* `eval/cascading_report.py` — aggregator over per-row
submission_annotation.json; enforces cascade monotonicity at aggregate
time; rewrites legacy `phase1_count`/`phase1_rate` from cascade N1.
* `eval/annotate.py` + `scripts/generate_annotation_skeletons.py` —
skeleton CLI with init|refresh|force-all task modes, overwrite|init
submission modes, dry-run, idempotent-preserve human edits.
* `eval/regrade.py` — explicit OVERWRITE re-grade path; writes
eval_regraded.json alongside historical eval.json.
* `paths.annotations_root()` + Dockerfile.cloud `--build-context
annotations=` + `image.data_hash` keyed under `annotations/<rel>` so
worktree builds match main-checkout content.
* `cloud/gcs.{write,read}_submission_annotation` +
`cloud/post_run_merge.merge_submission_annotations` no-overwrite
+ schema-validated + audit-report.
Legacy `phase1_passed_audited` / `phase1_passed_original` raw bool
columns + their plumbing have been removed end-to-end: `run.py`,
`results_db.py`, `cloud/collation.py`, `agents/_submit.py`, all 9 agent
files. The dual-eval block in eval.json is replaced by
`cascading_phase1`; `phase1_count`/`phase1_rate` stay as back-compat
aliases for N1. Two-round Codex review (plan + tests) folded in; 20
findings addressed.
Tests: 1800 passing (+96 net over the 1704 baseline; 12 obsolete
dual-eval tests deleted alongside the legacy columns). New coverage:
test_implicit_task_annotation, test_paths_annotations,
test_schema_extension, test_tolerant_grader_{comparators,orchestration},
test_cascading_report, test_legacy_field_removal,
test_local_run_cascading, test_eval_annotate_cli, test_regrade_cli,
cloud/test_{image_annotations,inline_grader,fetch_annotation_merge}.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…iant audits, audit-correctness pass session-3: - FailurePrimary enum: drop grader_stability; add no_fail, novel_reading_accepted, numerical_precision, row_order, trailing_whitespace, column_order, case_sensitivity. - _auto_failure_class shared classifier mapping cascade tier -> FailurePrimary (no_fail / row_order / novel_reading_accepted / numerical_precision / trailing_whitespace / column_order / other). - InternalInconsistency record on TaskAnnotation for multi-source conflicts (KB vs sql_snippet etc.); MetadataSufficiency docstring sharpened to encode the user_sim disclosure rule. - Multi-variant gold pattern: variant_id + primary on every audited_gold row; audit-gold-sql skill gains source_conflict clause_kind + sidecar example. - annotate-task-submission skill (NEW): canonical decision flow for mini-interact (a-interact) vs livesqlbench (one-shot), audit-correctness gate, failure-class table, cascade-tier -> failure-class mapping. - dev1515 conversion / re-classification / summary scripts under scripts/. session-4: - Multi-variant test refactor in tests/test_livesqlbench_audited_gold.py: _load_audit_rows returns PRIMARY rows; test_no_duplicate_instance_ids -> test_no_duplicate_instance_id_variant_pairs (dedup on the pair, enforce exactly one primary per instance). Households audit-correctness pass (audited_gold/mini_interact_audited.jsonl): - households_5: backfilled justified_by on the R$ 2,640 cutoff (anchored in snippet); task verdict insufficient -> ambiguous with rewritten rationale. - households_10: pruned 10 drifted IN-list literals to snippet-exact; agent flips agent_miss -> no_fail (equal_rowset, 153 housenums). - households_12: audit_status edited -> unrecoverable; one changes entry documents the gap (0..11 ordinal mapping is policy under any monotonic encoding). Museum audit work (audited_gold/livesqlbench_audited.jsonl): - museum_10: re-authored from buggy ERF copy-paste to KB-anchored 7-column DSD+ERPS+CASE per KBs [0,1,4,8,38,52]; agent submission flips to no_fail with gold_audit_quality secondary. - museum_2/4/9: added non-primary variant audit rows (four_dimension_erf, uniform_budget_ratio, conditionassessments_join_path) capturing the KB-defensible alternate readings; submissions flip other -> no_fail with metadata_ambiguity secondary. - museum_5: classified agent_miss (KB 7 latest-reading was clear). - museum_7: filled PENDING_HUMAN_REVIEW rationale + variant interpretation. Full non-integration suite: 1800 passed, 95 skipped, 50 deselected. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…16 Group 1 — Regrade CLI is operational + multi-statement SQL preserves TEMP state (CodeRabbit r3330838210/r3330838211 + Codex): * eval/tolerant_grader.py: _multi_sql_execute opens ONE shared sqlite connection when conn=None and len(sqls)>1, so CREATE TEMP setup statements share state with the final SELECT (closed in a finally so it doesn't leak across grade_submission calls). New regression test in tests/test_tolerant_grader_multi_sql_conn.py exercises the path with conn=None + a real two-statement gold list against a fresh sqlite DB. * eval/regrade.py: stop passing db_path=Path("/dev/null"); resolve the real per-DB sqlite via paths.benchmark_data_root(args.benchmark)/<db>/ <db>.sqlite using the attempt JSON's database key. Wrap the grader call in try/except so a sqlite/SQL error skips the instance rather than crashing the whole CLI. Group 2 — Cascade verdict-label drift (Codex): * eval/grade_in_place.py: extract verdict_label_from_cascade() as the single source of truth for cascade → SubmissionEvaluation.verdict. Both _build_submission_annotation (inline grader) and eval/annotate._eval_from_cascade (skeleton + regrade CLI) now go through it. Without this, N4/N5/N6/N7/N8 cascade-tier passes landed in annotate-side annotations with verdict="invalid" while the inline grader emitted "valid_interpretation". New tests/test_verdict_label_shared.py pins each cascade-tier mapping AND asserts the two persistence paths produce the same label. Group 3 — Cloud eval.json gains the cascading_phase1 block (Codex): * cloud/driver.py: after merge_submission_annotations, walk the downloaded run's <rows>/<inst>/submission_annotation.json files and call emit_cascading_eval_json to rewrite eval.json with the cascading_phase1 block + back-compat phase1_count/phase1_rate aliases. Extracted to _emit_cascading_phase1_on_fetch so it's unit-testable without round-tripping through GCS download + collation. Older runs pre-dating the DEV-1515 worker hook (no per-row annotations) skip cleanly. New tests/cloud/test_fetch_cascading_phase1.py pins the happy path + both no-op edges. Group 4 — Markdown lint + dead code + fresh-checkout build (Codex + CodeRabbit r3330838206/r3330838207/r3330838208 + summary-nitpick): * analyses/raw/households_10.md + museum_2.md + museum_4.md: add language tags to fenced code blocks (text / yaml) to clear MD040. * tests/cloud/test_collation.py: delete dead _read_dual_cols helper (queried phase1_passed_audited / phase1_passed_original columns that were removed in the DEV-1515 schema overhaul; would fail if called). * paths.py: annotations_root() and audited_gold_root() now mkdir(parents=True, exist_ok=True) so a fresh checkout (where both dirs are gitignored) doesn't fail cloud/image.build_and_push's BuildKit --build-context resolve before the data is populated. Full non-integration suite: 1811 passed, 95 skipped, 50 deselected (was 1800; net +11 from the three new test files). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… contradiction + pre-submit mutation guard
Three independent fixes surfaced by the 53-task post-DEV-1478 run audit
(opus-agent / sonnet-sim; 15 households + 38 other tasks; 9 cases
tagged agent_miss + 1 alien_6 secondary).
1. N9 cascade tier — `correct_under_case_fold` (case_sensitivity)
----------------------------------------------------------------
lca_17 returned the correct percentages but `'I/II/III/IV'`
wage-level labels where the gold has `'i/ii/iii/iv'`
(LOWER(TRIM(JSON_EXTRACT(...))) on the dim). The existing
trailing-whitespace tier strips at the cell edge, not inside a
string; column-order tier doesn't touch cell content. The
`case_sensitivity` value was already reserved in `FailurePrimary`
but had no comparator wired up.
* eval/tolerant_grader.py: new `compare_case_fold` mirroring the
`compare_trailing_whitespace` shape (per-cell `.lower()` on
strings, non-strings unchanged); `n9_case_fold` appended to
`_CASCADE_ORDER` and `CascadeVerdict`; `grade_submission` adds
the N9 block iterating variants + `__original__` after N8.
* eval/annotation_schema.py: `SubmissionEvaluation` adds
`correct_under_case_fold: bool = False`.
* eval/grade_in_place.py: `verdict_label_from_cascade` adds n9 to
the `valid_interpretation` disjunction; `_auto_failure_class`
finally wires `case_sensitivity` (was dead enum value).
* eval/annotate.py: mirror n9 in `_eval_from_cascade`.
* scripts/dev1515_convert_runs.py + dev1515_cascade_summary.py:
serialize and label the new tier.
* Tests: 4 new comparator tests, 1 orchestration "n9 lifts failing
n8 to pass", 1 verdict-label parity test, 2 schema round-trip
tests + extends the 2^n cascade-monotonicity loop to 2^9.
2. Multi-variant audit on source contradiction (mandatory)
--------------------------------------------------------
`planets_data_2` exposed the rule's enforcement gap: KB 31
("Retrograde Orbit") defines retrograde as strict
`inclination > 90`; the `critical_ambiguity` sql_snippet for
"spinning backwards" uses inclusive `inclination >= 90`. The
original auditor noticed the conflict in `reasoning_summary` and
collapsed to single primary `>= 90` anyway. The agent followed KB
31 verbatim, returned 0 rows, and got tagged `agent_miss` for a
contradiction the audit should have surfaced as multi-variant.
* _shared/audit-gold-sql.contract.md: new top-level "Multi-variant
audit on source contradiction (MANDATORY)" section with the
contradiction-shapes table, the mechanics, and a synthetic
BAD-vs-GOOD example. Picking one and noting the loser in
reasoning_summary is explicitly forbidden — there is no "labeled
wins" or "KB-anchored is primary" tie-breaker.
* audit-gold-sql/SKILL.md: trim the inline multi-variant rule to
point back at the shared contract; add Step 3.5 "Contradiction
check (MUST precede any single-variant commit)" listing the
mini-interact source set to scan.
* audit-gold-sql-livesqlbench/SKILL.md: parallel Step 4 in the
livesqlbench procedure, listing the livesqlbench source set
(KB items, column meanings, schema FKs — no labeled-ambiguity
blocks).
3. Pre-submit mutation check — prompt guard
-----------------------------------------
Across the 10 agent-miss cases, 5 had the same root cause: the
agent applied a TRIM / LOWER / ROUND / CAST / CASE-canonicalize /
output-shape choice on the final-assembly step that wasn't named in
the question, in an `ask_user` reply, or in an encoded KB —
silently corrupting otherwise-correct rowsets. Households_2,
alien_5, alien_6 (defensive normalizations); credit_7,
organ_transplant_4 (ignored a NAMED transformation in an
`ask_user` reply).
* claude_sdk_otf_ainteract/prompts.py: new step 6 PRE-SUBMIT
MUTATION CHECK before SUBMIT. Each mutation MUST be (a) named in
the user question, (b) named OR authorized in an `ask_user` reply
this session, or (c) required by an encoded KB; otherwise drop
it. Explicit clause for the inverse: when a reply DID name a
transformation, the reply IS the authorization — apply it.
* claude_sdk_otf/prompts.py: parallel step 5 for the one-shot
framework (no `ask_user`, so only (a) and (c) are valid
authorization sources).
Cascade summary post-changes (53-instance run): agent_miss 9 → 8,
no_fail 34 → 35 (planets_data_2 reclassified to no_fail since the
agent's submission now matches the new `kb_strict` variant via
n3_any_audited_variant). N9 row appears at the bottom with (+0)
delta — existing annotations were written before the tier existed; a
fresh re-grade is out of scope here.
Untouched but worth noting: the on-disk
`audited_gold/mini_interact_audited.jsonl` row for `planets_data_2`
was split into two variants (`labeled_snippet` primary `>= 90`,
`kb_strict` alternate `> 90`), and the corresponding
`annotations/mini-interact/planets_data/planets_data_2.task.json` +
`.submission.20260531t1343-claudes-slayer-b39bfc.json` were updated
with `internal_inconsistency.audit_resolution="multi_variant"` + two
`gold_variants` + `failure_classification.primary="no_fail"`. Both
trees are gitignored (per the existing policy), so these data edits
ride into the cloud image via the build-context, not via this commit.
Test suite: 1819 passed, 95 skipped (60s), no new failures.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two new livesqlbench DBs taken from skeleton to fully-enriched annotations (museum was the only previously-audited DB). 30 new audit rows + 30 task annotations land in the gitignored audited_gold/ and annotations/ trees per the cloud gold delivery split; this commit is the supporting infrastructure. - `scripts/dev1515_convert_livesqlbench.py`: renamed from the museum-specific script; now takes `--db` (required) and optional `--run-id`. Task-only mode when run-id is omitted. Adds a third rationale branch for management-category audits (verdict=sufficient with a deferral-aware rationale instead of the misleading "audit found nothing to change" text the old user-shortcut branch would emit for audited==original verbatim-copy rows). - `tests/test_livesqlbench_audited_gold.py`: generalize beyond museum. `EXPECTED_INSTANCE_IDS_BY_DB` keys per-DB; coverage / db-tag / citation-resolvability tests dispatch by selected_database; the edited/unrecoverable test carves out the management-category deferral shape (audited==original is permitted when clause_kind="management_category", per the shared contract); the changes[].justified_by non-empty check is relaxed for management deferrals (no source to cite — the deferral is documented in why_unjustified). - `.claude/skills/annotate-task-submission/SKILL.md`: restructured around the two-phase split. Task annotation is now explicitly self-contained (phase 1: KB + column meanings + schema + audited gold). Submission annotation is the optional phase 2 layered on top when a cloud run exists. Inputs section partitioned the same way; Workflow section split into phase-1 steps (always run) and phase-2 steps (only with a run). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…16 (round 2) Bundles 11 valid findings from CodeRabbit + Codex triage on PR #16: Group 1 — cascade reporting integrity * cascading_report: _per_row_cascade_bools now includes n9_case_fold; deltas loop extended to n9. Fixes silent counts.n9 = 0 on every published eval.json (n9 tier was added to _CASCADE_ORDER but the aggregator was hardcoded n1..n8). * regrade: run_dir uses paths.results_root() so BIRD_RESULTS_ROOT is honored. Also picks up pre-existing in-flight _build_original_sql_index work so regrade can supply original_sol_sql for the N1 check. * run.py: post-eval, auto-detect a per-row rows/ tree alongside eval.json and enrich it with cascading_phase1 when present (no-op for vanilla local runs; covers the symmetric local↔cloud convention). Group 2 — multi-variant gold preservation in conversion scripts * dev1515_convert_livesqlbench: drop hardcoded LIVESQL_ROOT, use paths.livesqlbench_root(); _load_audit_rows returns dict[str, list[dict]]; _pick_primary helper; _process_one passes the full variant list to grade_submission so N3 (\"any audited variant\") sees every alternate. * dev1515_convert_runs: same shape — dict-of-lists for mini-interact audit, _pick_primary, full list to grade_submission. * dev1515_households_14_multivariant: idempotency guard refuses to re-run on already-multivariant rows; filters pre-existing source_conflict entries so reruns never duplicate change records. Group 3 — annotation path mismatch + zero-primary contract test * annotation_io: _canonical_benchmark() normalizes dash↔underscore for the benchmark segment of every annotation path. Cloud workers (_cloud_benchmark → mini_interact) and CLI callers (--benchmark mini-interact) now land in the same tree; on-disk annotations/mini-interact/ contents migrated to annotations/mini_interact/. * annotate.py docstring updated to canonical form (both accepted). * test_livesqlbench_audited_gold: extracted _check_unique_variant_pairs_and_primary_count helper; added zero-primary / two-primary regression tests so an iid with only non-primary rows can no longer slip through. Group 4 — docs polish in skills * annotate-task-submission/SKILL.md: text-tag the two unlabeled fenced blocks (MD040). * audit-gold-sql/SKILL.md: rewrite the Outputs + Step 8 prose so the sidecar write key is (instance_id, variant_id) — overwriting on instance_id alone would silently drop alternates landed by the DEV-1515 multi-variant audits. Test suite: 1823 passed, 95 skipped, 0 failed. INVALID finding (no thread to reply on): CodeRabbit review-summary nitpick claiming tests/cloud/test_collation.py:31-43 holds a stale _read_dual_cols helper — that symbol does not exist in HEAD; lines 31-43 are the test_collate_picks_latest_attempt header + setup. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The cascade adequately classifies the 42/53 instances that pass at some
tier — each tier (N3 strict, N6 numeric_epsilon, N9 case_fold, ...)
names the tolerance that saved them. For the 11 strict misses that
clear no cascade tier, the cascade only tells us "agent missed"; we
want structured signals at grading time so downstream tooling can
break down failure modes (wrong join path, missing predicate, wrong
projection, empty result, SQL error, ...) without re-running queries
or eyeballing trajectories.
Schema (annotation_schema.py):
- New MissDiagnostics Pydantic model captures rowset shape, column
shape, sqlglot-derived SQL signals (tables_referenced,
has_group_by, has_aggregate, join_count, where_conjunct_count,
has_having, has_limit), execution status, and a multi-flag
miss_patterns list with 14 independent flag categories. Comparison
reference is the BEST-OVERLAP audited variant (multiset cardinality
vs agent rows; tie-break primary > alphabetical variant_id) — not
the primary by default, so multi-variant golds get diagnosed
against the closest reading rather than misclassified as
disjoint_rowset against an unrelated primary.
- Interactive-only signal (user_sim_n_asks) stays Optional[int] =
None on one-shot benchmarks (livesqlbench); never_asked_user flag
fires only when the benchmark is interactive AND the agent didn't
ask the user-sim.
- SQL-derived fields are Optional[T] = None when sqlglot fails to
parse, gated by *_sql_parse_ok booleans + truncated error excerpts.
Silent False/0 defaults would produce spurious wrong_table_set /
aggregation_shape_mismatch flags.
Grader (tolerant_grader.py):
- Wrapped the initial executor(submitted_sql, ...) call in try/except
so a syntax/runtime error becomes pred_rows=[] + error excerpt
+ sql_execution_error flag, not a crash that aborts grading.
- Wrapped per-variant execution defensively too (variant-side SQL
that fails just produces empty rowset; diagnostics catch the
downstream sqlglot parse failure via sql_parse_error flag).
- New grade_submission kwarg user_sim_n_asks: Optional[int].
- _compute_miss_diagnostics with bag-aware row comparison
(_bag_relation matches the grader's existing _set_equal multiset
semantics) + sqlglot AST walkers that excludes CTE / derived-table
aliases from base-table extraction.
- Defensive assert len(sol_sql) == 1 — confirmed empirically that
0/915 SELECT-task gold rows are multi-statement; multi-statement is
M-task territory (explicitly out of scope).
Flag rules are INDEPENDENT (every applicable rule appends to
miss_patterns; the list is sorted alphabetically before persist for
stable JSON diffs). An instance can carry multiple flags
simultaneously — e.g. wrong_table_set AND column_projection_mismatch
AND empty_agent_result.
Persistence (grade_in_place.py + annotate.py):
- _auto_failure_class strict-miss branch flipped "other" -> "agent_miss"
(agent_at_fault=True, remediation_target="agent"). The "other"
bucket disappears entirely; rich detail lives on
ev.miss_diagnostics.
- _build_submission_annotation + _eval_from_cascade both plumb
cascade.miss_diagnostics -> ev.miss_diagnostics so both the cloud
worker path (grade_in_place) and the regrade / skeleton path
(annotate) emit the same shape.
- FailureClassification.details is a free-form one-liner derived from
miss_patterns; structured downstream consumers MUST go through
miss_diagnostics.miss_patterns, not parse the prose.
Workflow:
- TDD-style spec with Codex pre-review of the design plan
(11 findings folded), test-suite-first (39 new tests, all failed
for the right "feature missing" reason before implementation),
then Codex pre-review of the tests (11 more findings folded —
most importantly: best-overlap test asserts exact == "alt"
not in {primary, alt}; tie-break tests use non-zero overlap;
multiset superset detection; mirror alias-exclusion on best-variant
side; negative assertions on per-flag tests for mutual-exclusion
contracts).
- Re-graded all 4 fetched runs (3 mini-interact + livesqlbench
museum); the 12 cascade-fail instances all carry populated
miss_diagnostics with >= 1 flag.
scripts/dev1515_strict_miss_diagnostics.py walks the latest
annotation per instance and prints per-instance table + per-flag
tally + instance x flag matrix. column_projection_mismatch fires on
every instance (slayer's namespaced agent column names vs gold's
bare names — a real signal); disjoint_rowset dominates (9/12)
suggesting wrong predicate / wrong join path; predicate_count_mismatch
fires on 7/12 suggesting missing or extra WHERE conjuncts.
Full non-integration suite: 1862 passed, 95 skipped, 50 deselected
(was 1811; +51 from the new test file).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…_user plumbing Column-shape flag refinement (annotation_schema.py + tolerant_grader.py): - Drop the noisy ``column_projection_mismatch`` flag — fired on every strict miss because slayer-namespaced column names (e.g. ``households.housenum``) consistently differ from gold's bare names (``housenum``). Neither the cascade's ``_set_equal`` nor BIRD-Interact's ``ex_base`` uses column names in equality (both compare tuples of values, not names), so a name-only divergence never causes cascade fail. The flag was muddying the failure-mode tally without identifying real cascade-fail causes. - Add ``column_count_mismatch`` — fires when agent and gold project different column COUNTS. This IS load-bearing: cell tuples have different arity, so bag equality cannot hold. Matches the cascade semantics on both sides. - Add ``column_order_mismatch`` — fires when counts match AND the column-name lists are equal as SETS after normalisation (lowercase + strip longest dot-prefix, so ``households.housenum -> housenum``) but differ as LISTS. Surfaces the "near miss" where the agent picked the right columns in wrong order and N8 column-order tolerance would have rescued the cascade if slayer's namespacing hadn't tripped its column-name set check. - Pure name-only divergence with matching counts and non-matching normalised sets is intentionally unflagged — stylistic noise. - The column-shape FIELDS (column_count_match, column_name_match_case_insensitive, column_order_match, agent_columns, best_variant_columns) stay populated as informational signals on MissDiagnostics; downstream tooling can inspect them without the flag muddying the tally. never_asked_user plumbing (grade_in_place.py + regrade.py): - grade_and_write resolves the benchmark interactivity once and forwards ``user_sim_n_asks`` to grade_submission so the ``never_asked_user`` diagnostic actually fires on interactive runs. Interactive: prefer ``user_sim_interaction.n_asks``, fall back to ``n_ask_user_calls``, then 0. One-shot: ``None`` so the flag stays out of miss_patterns (the signal doesn't apply). - regrade.py::_grader mirrors the same logic, computing n_asks from the attempt's trajectory via the existing ``_user_sim_interaction_from_trajectory`` helper. - Codex flagged this exact gap during the just-completed PR review (minor finding on grade_in_place.py:233-243) — the ``user_sim_n_asks`` parameter existed but no production call site forwarded it, so the diagnostic was systematically absent from every annotation. Summary script (scripts/dev1515_strict_miss_diagnostics.py): - Dedup key changed from ``instance_id`` to ``(benchmark, instance_id)``. Same-iid rows exist in BOTH benchmarks (``credit_4``, ``credit_7`` live in mini-interact AND livesqlbench); the previous latest-wins logic silently hid the mini-interact rows when livesqlbench had the same iid with a later run_id, undercounting the never_asked_user tally by 2. - Same-iid rows from both benchmarks now disambiguated with ``iid@livesqlbench`` suffix in the per-instance table. After re-grading all 5 runs (3 mini-interact + 2 livesqlbench, 20 cascade-fail instances total): * Mini-interact: 11/11 strict misses fire ``never_asked_user`` — the agent never queried the user-sim on any cascade-failing instance, even on ambiguous-metadata tasks. Behavioural signal worth investigating in the prompt. * Livesqlbench: 0/9 fire ``never_asked_user`` (one-shot, correctly excluded via the None sentinel). * ``column_count_mismatch`` fires on the 4 arity-mismatch instances (alien_10, households_2, museum_5, credit_7). * ``column_order_mismatch`` fires on 0 — no agent picked the right columns in wrong order. Useful negative finding. Test suite: 41 miss_diagnostics tests pass (39 + 2 new for the column_count / column_order split); full non-integration suite at 1861 passing (3 pre-existing failures in tests/test_livesqlbench_audited_gold.py about museum_2/4/9 primary flags — unrelated to this work). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
/process-reviews triage (CodeRabbit + Codex carryover) — 7 valid items:
Group 1 (tolerant_grader.py correctness):
- Don't coerce broken audited variants to ([], []); skip with continue
+ logger.exception so the agent's empty rowset can't accidentally
pass N2/N3 against an unevaluable gold (CR r3336709435).
- _where_conjunct_count walks to the OUTER Select and reads .args['where']
directly — expr.find(Where) descends into subqueries / CTEs and picks
whichever WHERE node sqlglot iterates first (CR r3336709443).
- Guard N1 with an explicit empty-original-sql check so _set_equal([], [])
doesn't falsely mark N1 as a strict pass when no original gold exists.
Group 2 (regrade.py ops):
- --force-llm-judge with no --instance-ids now clears the WHOLE judge
cache instead of being a silent no-op; the per-instance scope still
applies when --instance-ids is set.
- regrade_rows scratch dir is reset before each pass (full wipe for an
unfiltered run; per-instance subdirs for a filtered run) so stale
rows can't leak into eval_regraded.json.
Group 3 (path canonicalization + CLI):
- scripts/dev1515_cascade_summary.py: BENCHMARK token 'mini-interact' →
'mini_interact' so it reads the canonical underscore tree
(CR r3335026971).
- annotation_io._annotations_root delegates to paths.annotations_root()
when repo_root is None, honouring the BIRD_ANNOTATIONS_ROOT override.
- eval.annotate.main(): drop the unused --run-id / --submission-mode
args; submission-skeleton writing lives in dev1515_convert_runs.py.
DEV-1519 (separate bug — not flagged by CodeRabbit/Codex):
- claude_sdk_otf_ainteract.agent.run_task wrote accum.model_dump() to
usage but never copied the per-task asks_used counter. Grader then
read n_ask_user_calls from usage as 0, falsely flagging
never_asked_user on every interactive miss. Both error and success
result rows now emit {"n_ask_user_calls": ctx_dict.get("asks_used", 0)}.
Tests:
- tests/test_miss_diagnostics.py: replace the broken-best-variant test
with two — mixed-variants (broken + working) and all-variants-failing —
pinning the new contract.
- tests/test_claude_sdk_otf_ainteract_agent.py: extend _make_fake_client
+ _stub_env with prefill_asks and add three regression tests
(zero / nonzero / exception-path).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…t override Codex follow-up after the round-2 push: Item 1 (tolerant_grader.py — major): - When original_sol_sql is empty AND there are no audited variants, every cascade tier from N4 onward was falling back to comparing the agent rowset against the same empty orig_rows[]. compare_tie_order / compare_numeric_epsilon / compare_trailing_whitespace / compare_column_order / compare_case_fold all return True on ([], []) via set / bag equality — so a missing-gold + empty-agent pair would silently cascade-pass at N4 (and propagate as valid_interpretation even though nothing was actually compared). - Gate the __original__ fallback on `original_sol_sql` truthiness for N4 and consolidate N6-N9's comparator targets into one list that applies the same gate, so the guard can't be missed at any tier. - Regression test in test_tolerant_grader_orchestration.py pins: empty original + empty audited variants + empty agent rowset → every N-tier stays False. Item 2 (ray_app.py — minor): - _load_task_annotation_or_implicit was passing repo_root=paths.main_checkout_root() into task_annotation_path, which after round 2's annotation_io._annotations_root change deliberately bypasses BIRD_ANNOTATIONS_ROOT (the env override only fires when repo_root is None). Drop the explicit kwarg so the cloud worker honours the same override mounted by tests / forks. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Codex caught that the local runner never invoked grade_and_write, so
the cascading_phase1 block was silently absent from local eval.json —
only cloud runs (via cloud.ray_app._grade_one_submission) populated it.
The aggregator at the bottom of run_evaluation was already in place,
but its precondition (per-instance submission_annotation.json files)
was never satisfied locally.
Changes
- Move the three per-task helpers (load_task_annotation_or_implicit,
load_audited_gold_rows_for, grade_one_submission) from cloud.ray_app
into eval.grade_in_place — the canonical home now matches the
docstring's "shared inline grader" claim. ray_app keeps a thin alias
on `_grade_one_submission` and re-imports the two loaders for the
existing call sites (test_inline_grader, test_audited_gold_overlay_wiring,
regrade.py — the last one updated to import from the canonical
location).
- run.py: define `_grade_local_row` and call it after `_persist` in
`_run_with_sem`. Best-effort — a grader raise on one instance logs
and continues so the loop can't be killed by a single bad task.
Loop sees `b.name` (canonical underscore form) and resolves the
per-task sqlite via paths.benchmark_data_root, mirroring the cloud
helper's argument shape.
Tests
- tests/test_run_local_inline_grader.py (new):
- test_local_run_invokes_inline_grader_per_task — stub runner +
stub grader; assert the grader is called once per task, the
benchmark token is canonicalised, per-row submission_annotation.json
files land, and eval.json carries cascading_phase1 with the
expected counts/rates.
- test_local_run_grader_failure_does_not_kill_loop — grader raises
on alien_1; alien_2 still runs to completion and total_tasks==2.
- tests/cloud/test_inline_grader.py: patch the canonical
grade_in_place.grade_and_write (in addition to the back-compat
ray_app alias) so the existing wiring tests still pass.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…nator
Two Codex findings on the round-4 grader plumbing:
Group 1 (Codex major — grade_in_place.py, regrade.py):
Both call sites did ``list(value or [])`` to coerce the source
row's ``sol_sql``/``original_sol_sql`` into a list. When the value
comes through as a bare string (which ``run.py::_persist`` already
handles via ``isinstance(sol, str)`` — so the shape is established
upstream), ``list("SELECT 1")`` returns
``["S","E","L","E","C","T"," ","1"]``. The grader then runs each
character through sqlite as a one-character statement, raises
``sqlite3.OperationalError`` per character, and the row's N1 silently
drops to False under the broad-except.
Fix: ``normalize_sol_sql(value)`` helper that returns ``[]`` for
None/falsy, ``[value]`` for a string, and ``list(value)`` for a
list — used at both call sites.
Group 2 (Codex major — run.py:1007-1012):
``_grade_local_row`` silently returned without writing any
``submission_annotation.json`` when ``submitted_sql`` was missing
OR the inline grader raised. ``aggregate_cascading_phase1`` then
walked ``rows_dir`` and only counted the per-task dirs that did
have annotations — so ``n_dual_eval_tasks`` dropped below
``len(tasks)`` and ``cascading_phase1.rates`` were INFLATED by
silently excluding the never-graded rows.
Fix: ``write_failed_submission_annotation()`` helper writes a
fail-everything ``SubmissionAnnotation`` (verdict=invalid,
failure_classification.primary="other", all N-tiers 0) for both
bypass paths. The aggregator then counts those rows in the
denominator and honest rates fall out.
Tests:
- tests/test_normalize_sol_sql.py (new) — 7 tests pinning the shape
contract: None/empty/string/list/tuple, plus a long-string
regression that asserts no char-split semantics.
- tests/test_run_local_inline_grader.py:
- test_local_run_grader_failure_writes_fail_everything_annotation
(renamed) — extended to assert the fallback annotation lands,
n_dual_eval_tasks==2, all-tiers count 0, alien_1's verdict shape.
- test_local_run_no_submitted_sql_writes_fail_everything_annotation
(new) — analog for the no-submit pre-submit-crash path.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ocal)
Codex caught the symmetric cloud bug after round 5 landed the local
fix: ``_run_one_in_actor``'s inline-grader except branch only printed
a traceback and uploaded nothing. Two downstream consequences in
``driver._emit_cascading_phase1_on_fetch``:
* All-tasks-failed run: ``has_per_row_anns`` is False, the whole
``cascading_phase1`` block is silently dropped from ``eval.json``.
* Some-tasks-failed run: the aggregator's strict
``_per_row_cascade_bools`` raises ``FileNotFoundError`` on the
missing row, which the driver surfaces as
``cascading_phase1_error`` and the block STILL doesn't land. So
one broken task wipes the whole run's cascade metrics.
Fix: on grader bypass (unbound ``data_dir``, no ``submitted_sql``,
broken gold, ``_grade_one_submission`` exception) the except branch now
writes a fail-everything annotation via ``write_failed_submission_annotation``
into the same temp ``annotation_dir`` and uploads it via
``_gcs.write_submission_annotation`` — mirrors ``run._grade_local_row``
shape and the cloud driver-side aggregator now sees a per-row file for
EVERY task. Fallback-of-the-fallback prints a second traceback and
moves on (so a bug in the failed-annotation path can't crash the actor).
Tests:
- tests/cloud/test_inline_grader.py:
- test_cloud_grader_failure_uploads_fail_everything_annotation —
patches ``ray_app._grade_one_submission`` to raise, runs
``_LocalActor.run_one``, asserts the in-memory fake GCS bucket
received a ``runs/<run_id>/rows/<iid>/submission_annotation.json``
blob with verdict=invalid, primary=other, and the exception
message in details.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…sql indexing
Two more Codex findings on the round-4/6 grader plumbing:
Item 1 (Codex major — cloud/ray_app.py:588-590):
The round-6 cloud fallback only fires when the grader RAISES.
When ``row.get("submitted_sql") is None`` the cloud worker still
called ``_grade_one_submission(submitted_sql=str(None or "") == "")``,
passing the empty SQL through to SQLite. Some sqlite3 versions
return an empty rowset (no exception), which then matches an
also-empty gold rowset via ``_set_equal([], []) == True`` and
falsely passes N1/N2/N3. The local runner had the explicit guard
in ``_grade_local_row``; the cloud worker did not.
Fix: hoist the same short-circuit. Read ``row.get("submitted_sql")``
+ ``row.get("database") or task_data.get("selected_database")`` BEFORE
the grader call; raise a ``RuntimeError`` if either is missing so
the existing fail-everything except branch fires the fallback
upload path. Cloud + local now agree on never-submitted rows.
Item 2 (Codex major — eval/regrade.py:103-106, :134-137):
``_build_original_sql_index`` filtered on
``isinstance(sol, list) and sol`` at BOTH branches (mini_interact
data file + livesqlbench gated sidecar). That silently dropped any
source row whose ``sol_sql`` was the bare-string shape that round-5's
``normalize_sol_sql`` was explicitly designed to support. After the
index returns empty for such rows, the per-task lookup falls through
to ``original_sql_by_inst.get(iid) → None → []`` and N1 can never
pass, undercounting ``eval_regraded.json``.
Fix: pass ``r.get("sol_sql")`` through ``normalize_sol_sql`` at both
branches; skip only when the normalised value is empty.
Tests:
- tests/test_regrade_cli.py:
- test_build_original_sql_index_accepts_string_sol_sql — mixed
string/list/missing rows; assert string wraps to ``[s]``, list
passes through, missing-sol_sql is skipped.
- test_build_original_sql_index_does_not_char_split_string —
belt-and-braces against the round-5 char-split regression.
- tests/cloud/test_inline_grader.py:
- test_cloud_no_submitted_sql_short_circuits_before_real_grader —
stubs ``run_one_task`` to return ``submitted_sql=None``, spies
on ``_grade_one_submission`` and asserts it is never called,
and asserts the fail-everything annotation still lands in GCS
with primary=``other`` + the short-circuit details.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Three Codex findings, all on the round 4-7 grader plumbing: Item 1 (Codex major — run.py:1046-1058): Local ``_grade_local_row`` was computing ``per_task_db`` from ``paths.benchmark_data_root(_benchmark_canonical)`` — the GLOBAL benchmark root — instead of the caller-provided ``data_dir`` the agent actually executed against. An alternate checkout, a temp fixture, or a ``BIRD_DB_PATH`` override would silently route the grader at a different sqlite than the agent's, so a correct submission could be marked failing for purely path-routing reasons. Fix: root ``per_task_db`` at ``Path(data_dir)`` (already in scope on the closure). Mirrors the cloud worker's ``cfg["data_dir"]`` pattern. Regression test extended to assert ``db_path`` is rooted under the test's ``data_dir`` arg. Item 2 (Codex major — cloud/ray_app.py:570-631): ``_run_one_in_actor`` uploaded the attempt row BEFORE the inline grader + annotation upload. ``driver.wait_until_done`` returns ``done`` when ``len(attempts) >= total`` (attempt rows in GCS). Non-detached ``submit`` immediately calls ``fetch``; if the row landed first, the in-flight annotation upload could race the download, and the cascade aggregator would either drop the ``cascading_phase1`` block (none present) or surface ``cascading_phase1_error`` (some present). Fix: swap the order. Inline-grade + annotation upload runs FIRST, then ``_gcs.write_row``. The row blob becomes the canonical "task fully done, including annotation" marker. New ordering test records the call sequence and asserts annotation < row. Item 3 (Codex major — cloud/post_run_merge.py:487-497): The merge dest is keyed only by ``(benchmark, db, instance_id, run_id)`` and skipped when already present. Resubmit reuses the same ``run_id`` and bumps ``attempt``; a partial earlier fetch could pin attempt-1's annotation forever while ``eval.json`` / ``results.db`` reflect attempt-2. Fix: parse ``submission.trajectory_path`` (``rows/<iid>/attempt-N.json``) on src and dest; overwrite ONLY when the new attempt number is strictly greater. Unknown / equal / older → keep existing (safety floor preserved). New report counters ``overwritten_newer_attempt`` + ``overwritten_paths``. Prerequisite: ``grade_one_submission`` now takes an ``attempt`` kwarg (default 1) and uses it in ``trajectory_path``; cloud worker threads the real attempt through. Pre-fix the path was hardcoded to ``attempt-1.json`` and the attempt comparison would always short-circuit to ``equal``. 3 new merge tests + 1 helper test. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ostics
Two Codex findings on the round-8 grader plumbing:
Item 1 (Codex major — run.py:984-985 + results_db.py):
``run.py::_persist`` had been passing
``phase1_observation_audited`` / ``phase1_observation_original`` into
``TaskResultRow`` for every per-task insert, but the model + DDL
silently lost those two fields somewhere along the DEV-1515 dual-eval
cleanup. Pydantic's default ``extra="ignore"`` ate them without
warning, so ``results.db`` quietly stopped storing the observation
strings even though ~6 agent flavors (claude_sdk, pydantic_ai,
pydantic_ai_otf_encode, claude_sdk_otf_ainteract, plus
``agents/_submit.py``) continued to emit them on every row.
Fix:
* Add ``phase1_observation_audited: str | None = None`` +
``phase1_observation_original: str | None = None`` to
``TaskResultRow``.
* Append both to ``_DIAGNOSTIC_COLUMNS`` so ``open_db`` ALTERs
pre-existing on-disk DBs to add the columns (mirroring how
``phase1_observation`` etc. are upgraded).
* Append both to the fresh DDL string for documentation parity.
* Extend the ``INSERT OR REPLACE`` statement in
``insert_task_result`` to bind both fields.
* Two regression tests pin (a) a model-INSERT-SELECT round trip in
a fresh DB and (b) the column-upgrade path on a pre-existing DB
that lacks both columns at open time.
Item 2 (Codex minor — tolerant_grader.py:1081-1089):
``_compute_miss_diagnostics`` asserted ``len(sqls) <= 1`` on every
variant's ``audited_sol_sql`` and on ``original_sol_sql``. But the
grader's executor (``_multi_sql_execute``) explicitly supports
multi-statement gold (DDL prelude + final SELECT). When a
multi-statement gold produced a strict miss, the assert raised,
the round-5 outer except clause caught it, and the row fell back
to the round-6/7 fail-everything fallback — losing all structured
miss_patterns + Tier 2 informational detail for that row.
Fix: drop the asserts and use the LAST statement of the gold's
``audited_sol_sql`` for sqlglot parsing. The setup statements
don't constrain miss patterns; the SELECT under diagnosis is
what determines tables / aggregation shape / predicate count.
Replaced the two pre-fix "must raise" tests with two new ones
pinning the post-fix contract:
* test_multi_statement_audited_gold_uses_last_for_sql_signals
— best_variant_tables_referenced parses from the SELECT, not
the DDL.
* test_multi_statement_original_gold_does_not_crash_diagnostics
— original-gold side handles multi-stmt gracefully too.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ustness, cloud parity Four Codex findings on the round 4-9 plumbing: Group 1 (Codex major — run.py:1001-1002): ``run_evaluation`` reused ``output_dir/rows`` with ``mkdir(exist_ok=True)`` — never wiped. The aggregator at ``cascading_report.aggregate_cascading_phase1`` walks every subdirectory under ``rows_dir``, so rerunning with the same output path but a different ``--limit`` / ``--instance-id`` subset carried forward stale annotations from prior passes, inflating ``cascading_phase1.n_dual_eval_tasks`` and rewriting ``phase1_count`` / ``phase1_rate`` from the union of old + new. Fix: wipe before the run. ``filter_ids is None`` → ``shutil.rmtree`` the whole rows dir; filtered run → reset only the per-instance subdirs in the current task set so unrelated prior-run annotations survive and still contribute. Two new tests: stale-iid is wiped on full reruns; filtered rerun preserves unrelated rows. Group 2 (Codex major — harness.py:472-493 + cloud/_audited_gold_check.py:77-94): Both single-file audited gold indexes did ``out[iid] = row`` with latest-wins semantics. DEV-1515 multi-variant audits ship N rows per ``instance_id`` (one ``primary=True`` + non-primary alternates); on a file where an alternate appears AFTER the primary, the alternate's ``audited_sol_sql`` / ``audit_status`` would overwrite the primary at index time. Interaction-time overlay then applied the alt's reading; cloud audit-gold guard checked against the alt's status. Fix: prefer ``primary=True`` over alternates. Once a primary lands in the index, never overwrite. A non-primary recorded first gets upgraded by the primary later. Three new tests pinning the primary-first contract on both index helpers in both file orders. Group 3 (Codex major — tolerant_grader.py:624-626): Original gold execution was unguarded. The agent SQL and audited variant SQL got try/except wrappers in round 3 / 1.1, but ``_multi_sql_execute(original_sol_sql, ...)`` was bare. An invalid original gold (broken upstream SQL, schema drift, …) raised the whole grader; the local/cloud fail-everything fallback then wrote a generic "grader raised" annotation, losing any valid audited- variant passes (N2/N3) the row would have earned. Fix: wrap the original-exec call, log on raise, set ``orig_rows = []`` + ``original_sql_executed_ok = False``. Then extend the round-3 missing-gold guards on N1 (line 682) and the ``__original__`` fallback gates for N4 (line 724) + N6-N9 (line 790) to AND-in ``original_sql_executed_ok`` so the false-pass shape ``_set_equal([], [])`` / ``compare_tie_order([], [])`` is also blocked on exec-failure. Two new tests: valid audited variant still passes through a broken original; empty agent + broken original + no variants stays False at every tier. Group 4 (Codex minor — cloud/collation.py:69-94): Round 9 restored ``phase1_observation_audited`` / ``phase1_observation_original`` on ``TaskResultRow`` + DDL; the local ``run.py::_persist`` path was already plumbing them. But cloud collation's ``_row_to_task_result_row`` never received the update, so cloud-fetched ``results.db`` files quietly lost the observation columns while local runs retained them. Fix: append both kwargs to the ``TaskResultRow(...)`` build in collation.py to mirror local. New test: synthesize a per-task row with both observation fields set, run collate, assert both come back populated when reading ``results.db``. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…y shape
Two Codex findings on the round-8 / round-10 plumbing:
Item 1 (Codex major — regrade.py:193):
The CLI hardcoded ``attempt = sub / "attempt-1.json"``. But cloud
collation has long treated the highest ``attempt-N.json`` as
canonical and round-8's post-fetch merge now compares attempt
numbers — so the regrade CLI was the odd one out: after a resubmit
it would either silently SKIP instances that had only attempt-2
(because attempt-1 didn't exist) or, when both attempts coexisted,
OVERWRITE the submission annotation + ``eval_regraded.json`` with
results computed against STALE attempt-1 SQL.
Fix: ``_latest_attempt_file(sub)`` scans for ``attempt-N.json``,
parses the digit, and returns the max. Mirrors the regex used in
``cloud/post_run_merge._attempt_from_trajectory_path``. Two new
tests: prefer attempt-3 over attempt-1 when both exist; pick up
attempt-2 as the sole file.
Item 2 (Codex major — annotate.py:120 + regrade.py:281 + annotate.py:241):
``_user_sim_interaction_from_trajectory`` iterated ``traj`` and
called ``item.get("role")``. Three call sites wrapped the source
trajectory with ``list(attempt_data.get("trajectory") or [])`` —
fine when ``trajectory`` is a list of turn-step dicts, but
``pydantic_ai_otf_encode/agent.py:1537`` (and the recursive flavor's
``agent.py:892``) emit ``trajectory = {"final_output_excerpt":
..., "agents": [...]}`` (a DICT). ``list(dict)`` returns the dict's
KEYS as strings, then ``str.get("role")`` raises AttributeError —
the grader-fallback / skeleton-build paths crashed after the cascade
computed cleanly.
Fix:
* Drop the function's strict ``traj: list[dict]`` annotation and
short-circuit to ``UserSimInteraction()`` when the value isn't
a list. Skip individual non-dict items in the iteration too
(handles ``list(dict)`` -> list of strings as the safety floor).
Guard the ``traj[i-1]`` previous-step lookup against non-dicts
on the same principle.
* At the three call sites, drop the ``list(...)`` wrap and pass
the raw value (or ``[]`` when None); the helper now handles
every shape defensively.
New tests file ``tests/test_user_sim_interaction_trajectory.py`` —
7 tests pinning: canonical list-of-dicts (load-bearing happy path),
dict-shaped traj (Codex's bug fixture), list-of-strings (the
``list(dict)`` coercion case), None, empty list, mixed list with
stray non-dict entries, and a non-dict ``traj[i-1]`` prev-step.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Codex r11 follow-up on round 10's Group 1 design. Round 10's filtered
rerun preserved unrelated prior ``rows/<iid>/submission_annotation.json``
directories on disk so previously-graded instances would still be
visible for human inspection. The aggregator at the bottom of
``run_evaluation`` then walked EVERY subdirectory under
``output_dir/rows`` to build ``cascading_phase1``, while the rest of
the metrics dict (``total_tasks``, ``results``, durations, usage,
phase counts) was built from only the filtered ``tasks`` list. The
final ``emit_cascading_eval_json`` rewrote ``phase1_count`` /
``phase1_rate`` from the cascade's N1 count — over the union — while
``total_tasks`` stayed at the filtered count. Observable on the
published ``eval.json``:
* ``total_tasks`` = filtered count (e.g. 1)
* ``phase1_count`` = union count (e.g. 3 — 2 prior + 1 fresh)
* ``cascading_phase1.n_dual_eval_tasks`` = union count
``phase1_count > total_tasks`` and rates uninterpretable.
Fix: add ``instance_filter: set[str] | None`` to
``aggregate_cascading_phase1`` and pipe it through
``emit_cascading_eval_json``. When set, only subdirectories whose name
is in the filter are counted (preserved prior annotations stay on
disk but DON'T pollute the published metrics). At the ``run.py`` call
site, build the set from ``{td["instance_id"] for td in tasks}`` so the
cascade describes the same row set as ``total_tasks``. Cloud collation
and full local runs pass ``None`` to keep back-compat.
Tests:
- tests/test_cascading_report.py:
test_aggregator_instance_filter_scopes_to_current_run — pin the
helper's filter contract directly: 3 dirs on disk, filter to 2,
assert the third dir is preserved on disk but excluded from the
count.
- tests/test_run_local_inline_grader.py:
test_local_run_filter_ids_preserves_unrelated_rows — extended with
the metrics-consistency invariant ``total_tasks ==
cascading_phase1.n_dual_eval_tasks`` after a filtered rerun with a
stale unrelated annotation on disk.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Two Codex findings on the cascade comparators: Item 1 (Codex major — tolerant_grader.py:150-156): ``compare_tie_order`` only checks row-count cardinality before indexing every row by ``orderby_indices``. A wrong-projection-width agent submission (fewer columns than the gold ORDER BY references) raised ``IndexError`` inside the ``_key`` builder; the exception bubbled out of the grader and the cloud / local fail-everything fallback wrote a generic "grader raised" annotation instead of the structured cascade verdict with miss_diagnostics — losing the very signal that would tell the analyst the agent's projection was too narrow. Fix: bounds-check ``max(orderby_indices)`` against the NARROWEST row on either side up front. Out-of-range index → return False cleanly (same disposition as the row-count check immediately above). Two new tests: pred too narrow, gold too narrow. Item 2 (Codex major — tolerant_grader.py:281-285): ``compare_column_order`` did ``set(pred_l) != set(gold_l)`` and then ``perm = [pred_l.index(c) for c in gold_l]``. With duplicate column names (e.g. ``a, b, a`` after a join + alias collapse), ``set(...)`` collapsed the duplicates and ``.index()`` returned the FIRST matching position for every later occurrence — so both gold "a" positions mapped to pred's first "a" column, and the second pred "a" column's actual value was silently ignored. A submission with wrong values in the second duplicate column could falsely pass N8. Fix: reject duplicate column names at the boundary. Duplicates make "column order tolerance" ill-defined — the tier was designed for distinct projections. Three new tests: duplicates on pred, duplicates on gold, plus a belt-and-braces regression that the canonical "same names, different order" pass still works. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Two new raw-SQL Claude SDK OTF agents that mirror the SLayer OTF variants but use no SLayer at all — direct SQL via the BIRD tool suite, submit via submit_sql. Refactors existing slayer OTF prompts to share constants from a new _shared_otf_prompts.py, leaving rendered prompts byte-for-byte identical. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds DEV-1515 annotation schemas/IO, a tolerant cascading grader, multi-variant audited-gold behavior, raw OTF agents and prompt fragments, cloud/local inline grading and aggregation (cascading_phase1), image bake-in for annotations, consolidation/convert scripts, and wide test coverage. ChangesDEV-1515 End-to-End Overhaul
Sequence Diagram(s)sequenceDiagram
participant CLI as Local/Cloud CLI
participant Runner as Agent Runner
participant Grader as grade_one_submission
participant Store as Annotations/checkout
participant GCS as Cloud Storage
CLI->>Runner: run_evaluation / submit
Runner->>Grader: grade_one_submission(instance)
Grader-->>Runner: submission_annotation.json (cascade)
Runner->>Store: write annotations/<benchmark>/<db>/<instance>.submission.<run>.json
Runner->>Store: emit_cascading_eval_json(out_patch)
Runner->>GCS: upload submission_annotation.json
Runner->>GCS: upload attempt row/log
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
src/bird_interact_agents/eval/annotate.py (1)
212-216: ⚡ Quick winConsider error handling for missing attempt file.
The code reads
attempt-1.jsonwithout handling potential file-not-found or JSON parsing errors. While the docstring mentions tests pass a stub grader, production usage could encounter missing or malformed attempt files.🛡️ Proposed defensive handling
attempt_path = Path(rows_dir) / instance_id / "attempt-1.json" + if not attempt_path.exists(): + raise FileNotFoundError( + f"Missing attempt file for {instance_id}: {attempt_path}" + ) + try: - attempt = json.loads(attempt_path.read_text()) + attempt = json.loads(attempt_path.read_text()) + except json.JSONDecodeError as e: + raise ValueError( + f"Invalid JSON in attempt file {attempt_path}: {e}" + ) from e submitted_sql = attempt.get("submitted_sql", "")🤖 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/eval/annotate.py` around lines 212 - 216, The code assumes attempt-1.json always exists and is valid; update the logic around attempt_path, attempt and the subsequent submitted_sql/traj/usage extraction to handle missing or malformed files by first checking attempt_path.exists(), and wrapping attempt_path.read_text() + json.loads(...) in a try/except that catches FileNotFoundError and json.JSONDecodeError; on error either log/raise a clear message including attempt_path and instance_id or set attempt = {} so submitted_sql, traj and usage fall back to their existing defaults, ensuring the rest of the function (where submitted_sql, traj, usage are used) won’t crash.tests/test_claude_sdk_otf_ainteract_raw_agent.py (1)
257-258: ⚡ Quick winPrefix unused unpacked variables with underscore.
The test unpacks
counter_aandnag_bbut never uses them. Prefix with underscore to signal the intentional omission.✨ Suggested fix
- gate_a, counter_a, nag_a = m._make_ask_user_guards() - gate_b, counter_b, nag_b = m._make_ask_user_guards() + gate_a, _counter_a, nag_a = m._make_ask_user_guards() + gate_b, counter_b, _nag_b = m._make_ask_user_guards()🤖 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_claude_sdk_otf_ainteract_raw_agent.py` around lines 257 - 258, The test unpacks values from m._make_ask_user_guards into gate_a, counter_a, nag_a and gate_b, counter_b, nag_b but others are unused; update the unpacking to prefix unused variables with underscores (e.g., keep gate_a and nag_a as-is if used, change counter_a to _counter_a, and change counter_b or nag_b to _counter_b/_nag_b as appropriate) so intent is clear and linters won't flag unused variables.src/bird_interact_agents/eval/tolerant_grader.py (1)
221-224: ⚡ Quick winConsider adding
strict=Trueto zip() for defensive coding.While the length check at line 219 protects against mismatched row lengths, explicitly passing
strict=Truetozip()would make the invariant more explicit and catch unexpected length mismatches earlier in future refactorings.🛡️ Proposed change
if all( - _numeric_cell_equal(a, b, epsilon=epsilon) - for a, b in zip(pr, gr) + _numeric_cell_equal(a, b, epsilon=epsilon) + for a, b in zip(pr, gr, strict=True) ):🤖 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/eval/tolerant_grader.py` around lines 221 - 224, Add strict=True to the zip call in the equality check to make the row-length invariant explicit: inside the block using _numeric_cell_equal(...) for a, b in zip(pr, gr) (with epsilon passed through), change the iterator to zip(pr, gr, strict=True) so any unexpected length mismatch between pr and gr fails immediately even if there is a prior length check. Ensure the import/target runtime supports Python 3.10+ where zip(..., strict=True) is available.src/bird_interact_agents/eval/cascading_report.py (1)
86-92: ⚡ Quick winPrefer deriving delta keys from the counts dict.
The deltas loop hardcodes
"n1"through"n9", while line 73 dynamically derives count keys from_CASCADE_ORDER. If_CASCADE_ORDERis extended or reordered, the deltas loop won't automatically adapt, creating a maintenance hazard.♻️ Derive delta keys from counts
- for k in ("n1", "n2", "n3", "n4", "n5", "n6", "n7", "n8", "n9"): + for k in counts.keys(): if prev is None: deltas[k] = 0 else:🤖 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/eval/cascading_report.py` around lines 86 - 92, The delta computation currently hardcodes keys "n1".."n9"; instead iterate the keys derived from the same source as counts (e.g., iterate over _CASCADE_ORDER or list(counts.keys()) in the same order used to build counts) and compute deltas into deltas[k] using the existing prev logic (set delta 0 for the first key, else counts[k] - prev, then set prev = counts[k]); update references to prev, deltas, counts and _CASCADE_ORDER so the delta loop adapts if the cascade order changes.src/bird_interact_agents/eval/regrade.py (1)
121-127: 💤 Low valueConsider making the gold sidecar filename configurable.
The hardcoded filename
"livesqlbench_sqlite_gt_kg_testcases_0528.jsonl"at line 123 could become stale if the gold sidecar is updated. While the code safely checkscandidate.exists()and respects theBIRD_LIVESQLBENCH_GOLD_FILEenv override, a future-proof approach would source the filename from benchmark configuration or make it a CLI option.🤖 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/eval/regrade.py` around lines 121 - 127, The code currently hardcodes the sidecar filename "livesqlbench_sqlite_gt_kg_testcases_0528.jsonl" when searching under paths.benchmark_data_root(benchmark) — change this to use a configurable value (preferably from the benchmark configuration or a CLI flag) and fall back to the existing BIRD_LIVESQLBENCH_GOLD_FILE env override and then a sensible default; update the lookup around gold_path and the loop that checks candidate.exists() to reference that configurable name (e.g., use benchmark.get("gold_sidecar") or a parsed CLI option) so future changes to the gold filename don’t require code edits.
🤖 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 `@scripts/consolidate_mini_interact_audited.py`:
- Around line 23-24: Replace the repo-root-based path computation (ROOT and
AUDITED) with the path helper: remove the Path(__file__)-derived ROOT and
AUDITED variables and instead call paths.audited_gold_root(); add an import for
the helper (from bird_interact_agents import paths) and update any references to
AUDITED to use the value returned by paths.audited_gold_root() so the script
honors git worktrees and the BIRD_AUDITED_GOLD_ROOT override.
In `@src/bird_interact_agents/eval/annotation_io.py`:
- Around line 47-59: The function _annotations_root currently constructs
Path(repo_root) / ANNOTATIONS_DIRNAME which bypasses the centralized helper;
change it to always resolve via the paths helper (call paths.annotations_root()
and do not build repo_root/ANNOTATIONS_DIRNAME directly). Update the
implementation of _annotations_root to return paths.annotations_root()
unconditionally (or, if you must keep the repo_root parameter, forward it to
paths.annotations_root(repo_root) rather than concatenating ANNOTATIONS_DIRNAME
yourself) so ANNOTATIONS_DIRNAME is resolved by the canonical
paths.annotations_root helper.
- Around line 61-73: The path builders (e.g., task_annotation_path and the
similar functions around lines 76-89) place user-provided components
(selected_database, instance_id, run_id) directly into filesystem paths and need
sanitization to prevent path traversal; update each function to validate and
normalize these components by ensuring each is a single path segment (no path
separators), not equal to '.' or '..', and using the basename form (e.g.,
Path(value).name) or explicit checks, and raise a ValueError on invalid input
before composing the final Path with _annotations_root and _canonical_benchmark
so no supplied value can escape the annotations tree.
In `@src/bird_interact_agents/run.py`:
- Around line 1129-1153: The inline grader reconstructs per-task DB path
(per_task_db) from data_dir/selected_database/... which ignores the materialized
local copy placed into task_data by materialize_task_db; replace the constructed
per_task_db with the materialized path from the task data (e.g.,
td.get("db_file_path") or task_data["db_file_path"]) when calling
grade_one_submission so the grader uses the same sqlite file the agent executed
against (ensure it's converted to a Path if needed and still passed as the
db_path argument to grade_one_submission).
In `@tests/test_local_run_cascading.py`:
- Around line 30-32: The FakeExecutor.__call__ currently returns identical
results for the conditional, so update it to return distinct outcomes for the
submitted SQL vs the other case: inside FakeExecutor.__call__ (method name
__call__, class FakeExecutor) keep the if sql == submitted branch returning the
intended "submitted" tuple and change the else branch to return a different
tuple representing the "gold"/original result (e.g., different rows or column
names) so the grader can distinguish submitted vs original execution results.
---
Nitpick comments:
In `@src/bird_interact_agents/eval/annotate.py`:
- Around line 212-216: The code assumes attempt-1.json always exists and is
valid; update the logic around attempt_path, attempt and the subsequent
submitted_sql/traj/usage extraction to handle missing or malformed files by
first checking attempt_path.exists(), and wrapping attempt_path.read_text() +
json.loads(...) in a try/except that catches FileNotFoundError and
json.JSONDecodeError; on error either log/raise a clear message including
attempt_path and instance_id or set attempt = {} so submitted_sql, traj and
usage fall back to their existing defaults, ensuring the rest of the function
(where submitted_sql, traj, usage are used) won’t crash.
In `@src/bird_interact_agents/eval/cascading_report.py`:
- Around line 86-92: The delta computation currently hardcodes keys "n1".."n9";
instead iterate the keys derived from the same source as counts (e.g., iterate
over _CASCADE_ORDER or list(counts.keys()) in the same order used to build
counts) and compute deltas into deltas[k] using the existing prev logic (set
delta 0 for the first key, else counts[k] - prev, then set prev = counts[k]);
update references to prev, deltas, counts and _CASCADE_ORDER so the delta loop
adapts if the cascade order changes.
In `@src/bird_interact_agents/eval/regrade.py`:
- Around line 121-127: The code currently hardcodes the sidecar filename
"livesqlbench_sqlite_gt_kg_testcases_0528.jsonl" when searching under
paths.benchmark_data_root(benchmark) — change this to use a configurable value
(preferably from the benchmark configuration or a CLI flag) and fall back to the
existing BIRD_LIVESQLBENCH_GOLD_FILE env override and then a sensible default;
update the lookup around gold_path and the loop that checks candidate.exists()
to reference that configurable name (e.g., use benchmark.get("gold_sidecar") or
a parsed CLI option) so future changes to the gold filename don’t require code
edits.
In `@src/bird_interact_agents/eval/tolerant_grader.py`:
- Around line 221-224: Add strict=True to the zip call in the equality check to
make the row-length invariant explicit: inside the block using
_numeric_cell_equal(...) for a, b in zip(pr, gr) (with epsilon passed through),
change the iterator to zip(pr, gr, strict=True) so any unexpected length
mismatch between pr and gr fails immediately even if there is a prior length
check. Ensure the import/target runtime supports Python 3.10+ where zip(...,
strict=True) is available.
In `@tests/test_claude_sdk_otf_ainteract_raw_agent.py`:
- Around line 257-258: The test unpacks values from m._make_ask_user_guards into
gate_a, counter_a, nag_a and gate_b, counter_b, nag_b but others are unused;
update the unpacking to prefix unused variables with underscores (e.g., keep
gate_a and nag_a as-is if used, change counter_a to _counter_a, and change
counter_b or nag_b to _counter_b/_nag_b as appropriate) so intent is clear and
linters won't flag unused variables.
🪄 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: 3d1d7544-8850-4903-9e69-1faf6978952b
📒 Files selected for processing (105)
.claude/skills/_shared/audit-gold-sql.contract.md.claude/skills/annotate-task-submission/SKILL.md.claude/skills/audit-gold-sql-livesqlbench/SKILL.md.claude/skills/audit-gold-sql/SKILL.md.gitignoreDockerfile.cloudREADME.mdanalyses/households_failure_analysis_20260531t1008-claudes-slayer-890419.mdanalyses/museum_failure_analysis_20260531t1013-claudes-slayer-48eb0f.mdanalyses/raw/cross_cutting_observations.mdanalyses/raw/households_10.mdanalyses/raw/households_12.mdanalyses/raw/households_15.mdanalyses/raw/households_2.mdanalyses/raw/museum_10.mdanalyses/raw/museum_2.mdanalyses/raw/museum_3.mdanalyses/raw/museum_4.mdanalyses/raw/museum_5.mdanalyses/raw/museum_9.mdscripts/consolidate_mini_interact_audited.pyscripts/dev1515_cascade_summary.pyscripts/dev1515_convert_livesqlbench.pyscripts/dev1515_convert_runs.pyscripts/dev1515_households_14_multivariant.pyscripts/dev1515_reclassify_sufficiency.pyscripts/dev1515_remap_failure_classes.pyscripts/dev1515_strict_miss_diagnostics.pyscripts/generate_annotation_skeletons.pyscripts/verify_audited_gold.pysrc/bird_interact_agents/agents/_shared_otf_prompts.pysrc/bird_interact_agents/agents/_submit.pysrc/bird_interact_agents/agents/agno/agent.pysrc/bird_interact_agents/agents/claude_sdk/agent.pysrc/bird_interact_agents/agents/claude_sdk_otf/agent.pysrc/bird_interact_agents/agents/claude_sdk_otf/prompts.pysrc/bird_interact_agents/agents/claude_sdk_otf_ainteract/agent.pysrc/bird_interact_agents/agents/claude_sdk_otf_ainteract/prompts.pysrc/bird_interact_agents/agents/claude_sdk_otf_ainteract_raw/__init__.pysrc/bird_interact_agents/agents/claude_sdk_otf_ainteract_raw/agent.pysrc/bird_interact_agents/agents/claude_sdk_otf_ainteract_raw/prompts.pysrc/bird_interact_agents/agents/claude_sdk_otf_raw/__init__.pysrc/bird_interact_agents/agents/claude_sdk_otf_raw/agent.pysrc/bird_interact_agents/agents/claude_sdk_otf_raw/prompts.pysrc/bird_interact_agents/agents/mcp_agent/agent.pysrc/bird_interact_agents/agents/pydantic_ai/agent.pysrc/bird_interact_agents/agents/pydantic_ai_otf_encode/agent.pysrc/bird_interact_agents/agents/pydantic_ai_recursive/agent.pysrc/bird_interact_agents/agents/smolagents/agent.pysrc/bird_interact_agents/benchmark.pysrc/bird_interact_agents/cloud/_audited_gold_check.pysrc/bird_interact_agents/cloud/cli.pysrc/bird_interact_agents/cloud/collation.pysrc/bird_interact_agents/cloud/driver.pysrc/bird_interact_agents/cloud/gcs.pysrc/bird_interact_agents/cloud/image.pysrc/bird_interact_agents/cloud/post_run_merge.pysrc/bird_interact_agents/cloud/ray_app.pysrc/bird_interact_agents/eval/__init__.pysrc/bird_interact_agents/eval/annotate.pysrc/bird_interact_agents/eval/annotation_io.pysrc/bird_interact_agents/eval/annotation_schema.pysrc/bird_interact_agents/eval/cascading_report.pysrc/bird_interact_agents/eval/grade_in_place.pysrc/bird_interact_agents/eval/implicit_annotation.pysrc/bird_interact_agents/eval/regrade.pysrc/bird_interact_agents/eval/tolerant_grader.pysrc/bird_interact_agents/harness.pysrc/bird_interact_agents/paths.pysrc/bird_interact_agents/results_db.pysrc/bird_interact_agents/run.pytests/cloud/test_cli.pytests/cloud/test_collation.pytests/cloud/test_fetch_annotation_merge.pytests/cloud/test_fetch_cascading_phase1.pytests/cloud/test_image_annotations.pytests/cloud/test_inline_grader.pytests/test_benchmark.pytests/test_cascading_report.pytests/test_claude_sdk_otf_agent.pytests/test_claude_sdk_otf_ainteract_agent.pytests/test_claude_sdk_otf_ainteract_raw_agent.pytests/test_claude_sdk_otf_raw_agent.pytests/test_claude_sdk_otf_raw_run_wiring.pytests/test_claude_sdk_usage.pytests/test_dual_eval.pytests/test_eval_annotate_cli.pytests/test_eval_annotation_schema.pytests/test_implicit_task_annotation.pytests/test_legacy_field_removal.pytests/test_livesqlbench_audited_gold.pytests/test_local_run_cascading.pytests/test_miss_diagnostics.pytests/test_normalize_sol_sql.pytests/test_paths.pytests/test_paths_annotations.pytests/test_regrade_cli.pytests/test_results_db.pytests/test_run_local_inline_grader.pytests/test_schema_extension.pytests/test_shared_otf_prompts.pytests/test_tolerant_grader_comparators.pytests/test_tolerant_grader_multi_sql_conn.pytests/test_tolerant_grader_orchestration.pytests/test_verdict_label_shared.py
💤 Files with no reviewable changes (9)
- tests/test_claude_sdk_otf_agent.py
- src/bird_interact_agents/agents/agno/agent.py
- src/bird_interact_agents/agents/smolagents/agent.py
- src/bird_interact_agents/agents/mcp_agent/agent.py
- src/bird_interact_agents/agents/pydantic_ai/agent.py
- src/bird_interact_agents/agents/claude_sdk/agent.py
- src/bird_interact_agents/agents/pydantic_ai_otf_encode/agent.py
- src/bird_interact_agents/agents/pydantic_ai_recursive/agent.py
- tests/test_claude_sdk_usage.py
…ation
Three Codex findings, all addressed:
Item 1 (Codex major — regrade.py + tolerant_grader.py):
Regrade never threaded an ``llm_judge`` into ``grade_submission``,
so N5 was permanently dormant for insufficient-task rows. Now the
judge fires automatically as part of the regular grader flow:
* New ``LiteLLMJudge`` concrete class in ``tolerant_grader.py`` —
wraps ``litellm.completion`` with a fixed ACCEPT/REJECT prompt
contract and parses the last line of the model's reply. Network
/ shape errors return ``None`` per ``LLMJudgeProtocol`` so the
cascade falls through cleanly.
* Regrade CLI reads ``agent_model`` from
``<run_dir>/manifest.json`` and builds
``CachedLLMJudge(inner=LiteLLMJudge(model=agent_model),
cache_path=<run_dir>/llm_judge_cache.json)`` ONCE per regrade —
no new flags, no extra scaffolding.
* Dropped ``--force-llm-judge`` and the matching
``clear_llm_judge_cache`` helper (and the obsolete tests). The
cache key already includes ``model_name`` + content hashes, so
changing the agent's model on a resubmit naturally invalidates
entries without a separate cache-clear flag.
5 new tests in test_tolerant_grader_orchestration.py: ACCEPT /
REJECT / inconclusive parsing, litellm exceptions degrade to None,
and a mocked-response happy path that validates the OpenAI-shaped
reply path.
Item 2 (Codex major — annotation_schema.py):
``TaskAnnotation.external_knowledge`` was ``List[int]`` only. All
observed mini-interact / livesqlbench data currently ships int-only,
but some livesqlbench fixtures + forward-looking benchmark variants
carry the KB body inline as a dict (``{"id": 31, "label": "TETL",
"definition": "..."}``); pydantic would hard-reject those.
Fix: widen to ``List[Union[int, dict]]`` + add a regression test
that round-trips a mixed list through model_dump / model_validate.
Item 3 (Codex minor — scripts/dev1515_convert_runs.py):
Same char-splitting bug round 5 fixed elsewhere — the script did
``list(task_row.get("sol_sql") or [])``, which on a bare-string
``sol_sql`` (mini-interact does both shapes) would produce
``["S","E","L","E","C","T",...]`` and the grader would mis-execute.
Replaced with ``normalize_sol_sql(...)`` from grade_in_place.
(No new test — the helper is already tested in
tests/test_normalize_sol_sql.py round 5.)
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…er, hoist data_dir, fix dead test conditional Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…-annotations-tolerant-grader-post-dev-1478' into egor/dev-1524-slayer-less-claude-sdk-agent
…ree contract) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/bird_interact_agents/eval/regrade.py (1)
194-221: ⚡ Quick winGuard the attempt-JSON load so one corrupt file doesn't abort the whole re-grade.
json.loads(attempt.read_text())on Line 198 sits outside thetry/exceptthat begins on Line 209. A single truncated or malformedattempt-N.json(e.g. an interrupted write) raisesJSONDecodeErrorand aborts the entire offline regrade rather than skipping that one instance — the opposite of the resilient per-instance behavior the grader-exception handler already provides.♻️ Skip unreadable attempt files instead of aborting
attempt = _latest_attempt_file(sub) if attempt is None: report.skipped += 1 continue - attempt_data = json.loads(attempt.read_text()) + try: + attempt_data = json.loads(attempt.read_text()) + except (json.JSONDecodeError, OSError) as exc: + print(f" skip {instance_id}: unreadable attempt {attempt.name}: {exc}") + report.skipped += 1 + continue submitted_sql = attempt_data.get("submitted_sql", "")🤖 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/eval/regrade.py` around lines 194 - 221, The JSON load of per-attempt data (attempt_data = json.loads(attempt.read_text())) must be guarded so a corrupted attempt-N.json doesn't abort the whole regrade: wrap the file read + json.loads in the existing per-instance try/except (or add a small try/except) around that operation, catch JSONDecodeError (or Exception) and on error print a short message including instance_id and the exception, increment report.skipped, and continue; keep the rest of the grader invocation (grader(...), report.skipped handling) unchanged so only unreadable attempt files are skipped.
🤖 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.
Nitpick comments:
In `@src/bird_interact_agents/eval/regrade.py`:
- Around line 194-221: The JSON load of per-attempt data (attempt_data =
json.loads(attempt.read_text())) must be guarded so a corrupted attempt-N.json
doesn't abort the whole regrade: wrap the file read + json.loads in the existing
per-instance try/except (or add a small try/except) around that operation, catch
JSONDecodeError (or Exception) and on error print a short message including
instance_id and the exception, increment report.skipped, and continue; keep the
rest of the grader invocation (grader(...), report.skipped handling) unchanged
so only unreadable attempt files are skipped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 47c6b682-af6a-446b-8c0f-ca10e4865803
📒 Files selected for processing (17)
scripts/consolidate_mini_interact_audited.pyscripts/dev1515_convert_runs.pysrc/bird_interact_agents/cloud/ray_app.pysrc/bird_interact_agents/eval/annotate.pysrc/bird_interact_agents/eval/annotation_schema.pysrc/bird_interact_agents/eval/cascading_report.pysrc/bird_interact_agents/eval/regrade.pysrc/bird_interact_agents/eval/tolerant_grader.pysrc/bird_interact_agents/run.pytests/test_cascading_report.pytests/test_eval_annotation_schema.pytests/test_local_run_cascading.pytests/test_regrade_cli.pytests/test_run_local_inline_grader.pytests/test_tolerant_grader_comparators.pytests/test_tolerant_grader_orchestration.pytests/test_user_sim_interaction_trajectory.py
🚧 Files skipped from review as they are similar to previous changes (11)
- scripts/consolidate_mini_interact_audited.py
- tests/test_local_run_cascading.py
- src/bird_interact_agents/cloud/ray_app.py
- scripts/dev1515_convert_runs.py
- src/bird_interact_agents/eval/annotate.py
- src/bird_interact_agents/eval/annotation_schema.py
- tests/test_cascading_report.py
- src/bird_interact_agents/eval/cascading_report.py
- tests/test_run_local_inline_grader.py
- src/bird_interact_agents/run.py
- src/bird_interact_agents/eval/tolerant_grader.py
Summary
execute_sql,get_schema, etc.) and submit viasubmit_sqlClaudeSDKOtfRawAgent— bound to--dataset livesqlbench --mode one-shot --query-mode raw; mirrorsClaudeSDKOtfAgentClaudeSDKOtfAInteractRawAgent— bound to--dataset mini_interact --mode a-interact --query-mode raw; mirrorsClaudeSDKOtfAInteractAgentwith the sameask_user-before-submit_sqlgate (_make_ask_user_guards+ nag hook)claude_sdk_otf/prompts.py,claude_sdk_otf_ainteract/prompts.py) to import shared constants from a newagents/_shared_otf_prompts.py; rendered prompts are verified byte-for-byte identical via SHA-256 snapshot testsrun.py(_FRAMEWORK_DATASET_MODE_BINDING,_validate_slayer_setup,_validate_one_shot_framework,_make_runner,--frameworkchoices) and adds early returns incloud/driver.py+cloud/ray_app.py(no SLayer artifacts to upload/download)Test plan
env -u SSH_AUTH_SOCK uv run --extra all --extra dev --extra pydantic-ai pytest— 2020 passed, 95 skippedtests/test_shared_otf_prompts.py— SHA-256 snapshot tests verify slayer prompts are unchanged after refactor; structural tests cover shared constant format-placeholder contractstests/test_claude_sdk_otf_raw_agent.py— 29 tests covering tool selection, prompt building, validation, turn-budget hook, usage, error pathstests/test_claude_sdk_otf_ainteract_raw_agent.py— 43 tests covering the above plus all three_make_ask_user_guardshooks (gate, counter, nag), state isolation across factory calls, and ask_user call tracking in usage outputtests/test_claude_sdk_otf_raw_run_wiring.py— 29 tests coveringrun.pyvalidation logic for both new frameworks🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation