DEV-1515: tolerant grader + cascading-phase1 report + annotation CLI - #16
Merged
ZmeiGorynych merged 27 commits intoJun 3, 2026
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>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds per-task and per-submission JSON annotation schema/IO and CLI; implements a 9-tier tolerant SQL grader with miss diagnostics and LLM-judge caching; wires inline cloud grading, GCS upload, fetch-time merge, and cascading-phase1 aggregation; bakes annotations into cloud image; removes legacy dual-eval fields; adds conversion scripts and many tests/docs. ChangesDEV-1515 Annotations & Grading
Sequence Diagram(s)sequenceDiagram
participant Worker
participant GradeInPlace
participant GCS
participant Fetch
Worker->>GradeInPlace: grade_and_write(rows_dir, instance_id)
GradeInPlace-->>Worker: writes submission_annotation.json
Worker->>GCS: write_submission_annotation(run_id, instance_id, annotation)
Fetch->>GCS: download per-run rows/*
Fetch->>Fetch: merge_submission_annotations(downloaded_run_dir, main_checkout)
Fetch->>Fetch: emit_cascading_phase1_on_fetch(dest)
🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (10)
tests/cloud/test_collation.py (1)
31-43: 💤 Low valueOptional: Remove unused helper function.
The
_read_dual_colsfunction is no longer called after the dual-eval test removal. Consider removing it to keep the test suite clean.🧹 Proposed cleanup
-def _read_dual_cols(db_path: Path) -> dict[str, dict]: - conn = sqlite3.connect(str(db_path)) - conn.row_factory = sqlite3.Row - out = { - r["instance_id"]: dict(r) - for r in conn.execute( - "SELECT instance_id, phase1_passed_audited, phase1_passed_original, " - "phase1_observation_audited, phase1_observation_original " - "FROM task_results" - ) - } - conn.close() - return out - -🤖 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/cloud/test_collation.py` around lines 31 - 43, Remove the now-unused helper function _read_dual_cols from tests/cloud/test_collation.py: locate the def _read_dual_cols(...) block and delete it (including its sqlite3 usage and return value) so the test file no longer contains dead code; ensure no other references to _read_dual_cols remain in the file after removal.tests/test_eval_annotation_schema.py (1)
218-219: 💤 Low valueConsider using a more descriptive variable name.
The single-letter variable
l(lowercase L) can be confused with1(one) orI(uppercase i) in some fonts. Consider renaming tolinefor clarity.♻️ Suggested refactor
- indent_lines = [l for l in body_lines[1:-1] if l.strip()] - assert all(l.startswith(" ") for l in indent_lines) + indent_lines = [line for line in body_lines[1:-1] if line.strip()] + assert all(line.startswith(" ") for line in indent_lines)🤖 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_eval_annotation_schema.py` around lines 218 - 219, Rename the ambiguous loop variable `l` used in the list comprehension and generator expression to a clearer name like `line` for readability: update the list comprehension that assigns `indent_lines = [l for l in body_lines[1:-1] if l.strip()]` to use `line` (e.g., `indent_lines = [line for line in body_lines[1:-1] if line.strip()]`) and likewise change the subsequent assertion `assert all(l.startswith(" ") for l in indent_lines)` to use `line` (`assert all(line.startswith(" ") for line in indent_lines)`), ensuring both references to the variable in this test are renamed consistently.tests/test_tolerant_grader_orchestration.py (1)
735-753: 💤 Low valueMinor: Consider consistent attribute initialization.
The
CountingInnerstub uses a class attributecalls = 0(line 736), while other test stubs in this file use instance attributes in__init__(e.g., line 676). This works due to attribute shadowing on write, but is unconventional. For consistency, consider:♻️ Optional refactor for consistency
class CountingInner: - calls = 0 + def __init__(self): + self.calls = 0 + def judge(self, **kwargs): self.calls += 1 return 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 `@tests/test_tolerant_grader_orchestration.py` around lines 735 - 753, The CountingInner test stub uses a class-level attribute calls = 0 which is unconventional; change it to an instance attribute by adding an __init__ that sets self.calls = 0 and keep the judge(self, **kwargs) method to increment self.calls, so the behavior with CachedLLMJudge (inner, judge) remains the same but follows the same instance-initialization pattern used elsewhere in the tests.src/bird_interact_agents/eval/tolerant_grader.py (1)
507-507: 💤 Low value
rowset_relationsis declared but never populated.
grade_submissiononly fillsvariant_matches;rowset_relationsalways stays the empty default andgrade_in_placenever reads it. Either wire it up or drop the field to avoid a misleading empty list on every persisted verdict.🤖 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` at line 507, The model has an unused field rowset_relations: List[VariantMatch] defaulting to empty; either remove it or actually populate and consume it. If you want to keep it, update grade_submission (where variant_matches is populated) to also build and assign rowset_relations from the same matching logic or source (use the same VariantMatch construction logic), and modify grade_in_place to read/serialize rowset_relations alongside variant_matches; otherwise drop the rowset_relations Field declaration to avoid persisting a misleading empty list. Ensure you reference and update the methods grade_submission and grade_in_place and the VariantMatch construction to keep behavior consistent.src/bird_interact_agents/eval/regrade.py (2)
200-227: ⚡ Quick winMove imports outside the inline function.
Importing inside the
_graderfunction (lines 203-208) is inefficient and runs on every regrade invocation. Move these to module-level imports.♻️ Proposed fix
+from bird_interact_agents.cloud.ray_app import ( + _load_audited_gold_rows_for, + _load_task_annotation_or_implicit, +) +from bird_interact_agents.eval.implicit_annotation import ( + implicit_task_annotation, +) +from bird_interact_agents.eval.tolerant_grader import grade_submissionThen remove the imports from inside
_grader.🤖 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 200 - 227, The inline imports inside the _grader function (implicit_task_annotation, _load_audited_gold_rows_for, _load_task_annotation_or_implicit) should be moved to module-level imports to avoid re-importing on every regrade call; add those import statements at the top of the module and remove the corresponding from ... import ... lines from inside the _grader function so _grader simply calls implicit_task_annotation/_load_task_annotation_or_implicit/_load_audited_gold_rows_for directly.
113-120: ⚡ Quick winMove imports outside the loop.
Importing
_eval_from_cascade,_user_sim_interaction_from_trajectory,FailureClassification, andSubmissionMetadatainside the per-instance loop is inefficient. Move these to module-level imports.♻️ Proposed fix
from bird_interact_agents.eval.annotation_io import ( submission_annotation_path, write_submission_annotation, ) from bird_interact_agents.eval.annotation_schema import SubmissionAnnotation +from bird_interact_agents.eval.annotation_schema import ( + FailureClassification, + SubmissionMetadata, +) +from bird_interact_agents.eval.annotate import ( + _eval_from_cascade, + _user_sim_interaction_from_trajectory, +) from bird_interact_agents.eval.cascading_report import emit_cascading_eval_jsonThen remove lines 113-120 from inside the loop.
🤖 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 113 - 120, The imports for _eval_from_cascade, _user_sim_interaction_from_trajectory, FailureClassification, and SubmissionMetadata are currently inside the per-instance loop; move those import statements to the top-level of the module (module-level imports) so they are executed once, and then remove the in-loop import lines (the ones importing _eval_from_cascade, _user_sim_interaction_from_trajectory from bird_interact_agents.eval.annotate and FailureClassification, SubmissionMetadata from bird_interact_agents.eval.annotation_schema) so the loop uses the already-imported symbols.src/bird_interact_agents/cloud/ray_app.py (2)
690-699: ⚡ Quick winSimplify repeated
row.get("usage", {}).get(...)pattern.Extracting
usage = row.get("usage", {})once and checkingisinstance(usage, dict)would reduce repetition and improve readability.♻️ Proposed fix
+ usage = row.get("usage", {}) + usage_safe = usage if isinstance(usage, dict) else {} ann_path = _grade_one_submission( task_data=task_data, submitted_sql=str(row.get("submitted_sql") or ""), rows_dir=annotation_dir, run_id=run_id, benchmark=_cloud_benchmark(cfg), db_path=Path(data_dir) / str(task_data.get("selected_database", "")) / f"{task_data.get('selected_database', '')}.sqlite", - cost_usd_agent=row.get("usage", {}).get("cost_usd_agent") - if isinstance(row.get("usage"), dict) else None, - cost_usd_user_sim=row.get("usage", {}).get("cost_usd_user_sim") - if isinstance(row.get("usage"), dict) else None, + cost_usd_agent=usage_safe.get("cost_usd_agent"), + cost_usd_user_sim=usage_safe.get("cost_usd_user_sim"), duration_s=row.get("duration_s"), - n_agent_turns=row.get("usage", {}).get("n_agent_turns") - if isinstance(row.get("usage"), dict) else None, - n_ask_user_calls=row.get("usage", {}).get("n_ask_user_calls") - if isinstance(row.get("usage"), dict) else None, + n_agent_turns=usage_safe.get("n_agent_turns"), + n_ask_user_calls=usage_safe.get("n_ask_user_calls"), predicted_row_count=None, )🤖 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/cloud/ray_app.py` around lines 690 - 699, Extract usage = row.get("usage") once, check if isinstance(usage, dict), and then replace repeated row.get("usage", {}).get(...) calls with usage.get(...) so cost_usd_agent, cost_usd_user_sim, n_agent_turns, and n_ask_user_calls read from the local usage variable; keep duration_s = row.get("duration_s") and predicted_row_count=None unchanged and ensure the fallback when usage is not a dict remains None for those fields (use a single boolean or conditional expression based on isinstance(usage, dict)).
676-679: ⚡ Quick winAvoid relying on
locals().get("data_dir").Using
locals().get("data_dir")is fragile—if an exception occurs beforedata_diris assigned (line 644), this will beNone. While the code raisesRuntimeErrorand swallows it, it's clearer to referencedata_dirdirectly or use a try/except block.♻️ Proposed fix
- _grader_data_dir = locals().get("data_dir") try: - if _grader_data_dir is None: - raise RuntimeError("data_dir unbound; grader skipped") + # data_dir is bound at line 644; if not, NameError is caught below annotation_dir = Path(tempfile.mkdtemp(prefix="bird_submission_annot_")) ann_path = _grade_one_submission( task_data=task_data, submitted_sql=str(row.get("submitted_sql") or ""), rows_dir=annotation_dir, run_id=run_id, benchmark=_cloud_benchmark(cfg), - db_path=Path(_grader_data_dir) + db_path=Path(data_dir)🤖 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/cloud/ray_app.py` around lines 676 - 679, The code currently uses _grader_data_dir = locals().get("data_dir"), which is fragile; instead access data_dir directly and handle the case where it isn't defined by wrapping the reference in a try/except NameError (or test for None after a direct lookup) and then raise the existing RuntimeError("data_dir unbound; grader skipped") if it's missing; update the block around _grader_data_dir and the subsequent try/except so the code references data_dir directly (or assigns _grader_data_dir = data_dir inside a try) and does not rely on locals().get, keeping the same error message and behavior in the grader setup code.src/bird_interact_agents/eval/implicit_annotation.py (1)
37-37: ⚡ Quick winAvoid creating a set on every call.
Converting
benchmark_names()to a set on each invocation is inefficient. Either cache the set at module level or compare directly against the list if it's small.♻️ Proposed fix
+_KNOWN_BENCHMARKS = set(benchmark_names()) + def _benchmark_task_jsonl_name(benchmark: str) -> str: """Return the basename of the benchmark's task JSONL for provenance. Falls back to a generic ``"<benchmark>.jsonl"`` placeholder when the benchmark token doesn't match a registered ``Benchmark`` descriptor (which is fine for tests / forks that haven't registered theirs).""" - if benchmark in set(benchmark_names()): + if benchmark in _KNOWN_BENCHMARKS: return get_benchmark(benchmark).data_file return f"{benchmark}.jsonl"🤖 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/implicit_annotation.py` at line 37, The condition currently builds a set each call with "if benchmark in set(benchmark_names()):" which is wasteful; create a module-level cached set (e.g., BENCHMARK_NAMES_SET = set(benchmark_names())) and replace the call-site check with "if benchmark in BENCHMARK_NAMES_SET:" (or, if the list is tiny, use direct list membership "if benchmark in benchmark_names():"). Update implicit_annotation.py to define the cached set once at import and reference BENCHMARK_NAMES_SET in the function that checks benchmark membership.src/bird_interact_agents/eval/annotate.py (1)
57-60: ⚡ Quick winDeduplicate
_benchmark_task_jsonl_name.This helper is identical to
implicit_annotation._benchmark_task_jsonl_name. Import it from there instead of duplicating.♻️ Proposed fix
+from bird_interact_agents.eval.implicit_annotation import ( + _benchmark_task_jsonl_name, +) + -def _benchmark_task_jsonl_name(benchmark: str) -> str: - if benchmark in set(benchmark_names()): - return get_benchmark(benchmark).data_file - return f"{benchmark}.jsonl"🤖 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 57 - 60, The function _benchmark_task_jsonl_name is duplicated here; remove this local definition and import the canonical helper from implicit_annotation instead. Replace the function block in this file with an import statement that brings in _benchmark_task_jsonl_name from the module implicit_annotation (the same module that currently defines the original helper), ensuring all references in this file use the imported symbol. Keep the import at the top with other imports and run tests/linters to ensure no name collisions.
🤖 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 `@analyses/raw/households_10.md`:
- Around line 41-45: The markdown fenced code blocks listing KB values (e.g.,
the blocks containing "KB 6 Dwelling Type", "KB 7 Cable TV Status", and "KB 10
Vehicle Year Range") are missing a language tag and trigger MD040; update each
opening triple-backtick to include a language identifier (use "text") for those
specific fenced blocks and likewise add "text" to the other missing fenced
blocks referenced (the blocks around lines noted in the review, e.g., the long
value lists at the later locations) so every ``` becomes ```text.
In `@analyses/raw/museum_2.md`:
- Around line 38-42: The fenced code block containing the YAML mapping starting
with "erf:" should declare its language for MD040 compliance; update the
backticks that open the block to include "yaml" (i.e., change the code fence for
the block that contains erf, formula, and description to "```yaml") so the block
is explicitly marked as YAML.
In `@analyses/raw/museum_4.md`:
- Around line 16-18: The fenced code blocks in the markdown contain math
expressions but lack a language tag, triggering MD040; update each
triple-backtick fence (the blocks containing "CBE = \frac{\sum_{i \in artifacts}
(CPI_i \times BudgetRatio_i)}{|artifacts|}, where BudgetRatio..." and the block
with "CBE = sum(CPI*1/N)/N = sum(CPI)/N^2 assuming uniform BudgetRatio") to
include a language identifier such as "text" (e.g., ```text) so both fenced
blocks validate against the linter and remain unchanged otherwise.
In `@src/bird_interact_agents/eval/regrade.py`:
- Around line 218-227: The code currently calls grade_submission(...) with
db_path=Path("/dev/null") and conn=None which lets grade_submission fall back to
default_executor and actually execute SQL; fix by passing an explicit safe
executor (e.g., a no-op or validation-only function) via the executor parameter
to grade_submission so it will not open or execute against a DB, and also wrap
the call site in regrade_run (the function that invokes grader(...)) in a
try/except to catch sqlite/SQL or executor errors and skip/fail the item
gracefully; reference grade_submission, default_executor, regrade_run, db_path,
and conn when making these changes.
In `@src/bird_interact_agents/eval/tolerant_grader.py`:
- Around line 551-569: grade_submission currently lets default_executor
open/close a new SQLite connection per call so multi-statement submissions lose
TEMP/session state; fix by creating a single shared SQLite connection when conn
is None at the start of grade_submission (and ensuring executor is the
default_executor if unset), pass that shared conn into every call to executor
and _multi_sql_execute (including calls for submitted_sql, original_sol_sql, and
audited_sol_sql variants), and close the connection in a finally block; update
references in this function (grade_submission, default_executor, and calls to
_multi_sql_execute) so all statements for a submission use the same connection.
---
Nitpick comments:
In `@src/bird_interact_agents/cloud/ray_app.py`:
- Around line 690-699: Extract usage = row.get("usage") once, check if
isinstance(usage, dict), and then replace repeated row.get("usage", {}).get(...)
calls with usage.get(...) so cost_usd_agent, cost_usd_user_sim, n_agent_turns,
and n_ask_user_calls read from the local usage variable; keep duration_s =
row.get("duration_s") and predicted_row_count=None unchanged and ensure the
fallback when usage is not a dict remains None for those fields (use a single
boolean or conditional expression based on isinstance(usage, dict)).
- Around line 676-679: The code currently uses _grader_data_dir =
locals().get("data_dir"), which is fragile; instead access data_dir directly and
handle the case where it isn't defined by wrapping the reference in a try/except
NameError (or test for None after a direct lookup) and then raise the existing
RuntimeError("data_dir unbound; grader skipped") if it's missing; update the
block around _grader_data_dir and the subsequent try/except so the code
references data_dir directly (or assigns _grader_data_dir = data_dir inside a
try) and does not rely on locals().get, keeping the same error message and
behavior in the grader setup code.
In `@src/bird_interact_agents/eval/annotate.py`:
- Around line 57-60: The function _benchmark_task_jsonl_name is duplicated here;
remove this local definition and import the canonical helper from
implicit_annotation instead. Replace the function block in this file with an
import statement that brings in _benchmark_task_jsonl_name from the module
implicit_annotation (the same module that currently defines the original
helper), ensuring all references in this file use the imported symbol. Keep the
import at the top with other imports and run tests/linters to ensure no name
collisions.
In `@src/bird_interact_agents/eval/implicit_annotation.py`:
- Line 37: The condition currently builds a set each call with "if benchmark in
set(benchmark_names()):" which is wasteful; create a module-level cached set
(e.g., BENCHMARK_NAMES_SET = set(benchmark_names())) and replace the call-site
check with "if benchmark in BENCHMARK_NAMES_SET:" (or, if the list is tiny, use
direct list membership "if benchmark in benchmark_names():"). Update
implicit_annotation.py to define the cached set once at import and reference
BENCHMARK_NAMES_SET in the function that checks benchmark membership.
In `@src/bird_interact_agents/eval/regrade.py`:
- Around line 200-227: The inline imports inside the _grader function
(implicit_task_annotation, _load_audited_gold_rows_for,
_load_task_annotation_or_implicit) should be moved to module-level imports to
avoid re-importing on every regrade call; add those import statements at the top
of the module and remove the corresponding from ... import ... lines from inside
the _grader function so _grader simply calls
implicit_task_annotation/_load_task_annotation_or_implicit/_load_audited_gold_rows_for
directly.
- Around line 113-120: The imports for _eval_from_cascade,
_user_sim_interaction_from_trajectory, FailureClassification, and
SubmissionMetadata are currently inside the per-instance loop; move those import
statements to the top-level of the module (module-level imports) so they are
executed once, and then remove the in-loop import lines (the ones importing
_eval_from_cascade, _user_sim_interaction_from_trajectory from
bird_interact_agents.eval.annotate and FailureClassification, SubmissionMetadata
from bird_interact_agents.eval.annotation_schema) so the loop uses the
already-imported symbols.
In `@src/bird_interact_agents/eval/tolerant_grader.py`:
- Line 507: The model has an unused field rowset_relations: List[VariantMatch]
defaulting to empty; either remove it or actually populate and consume it. If
you want to keep it, update grade_submission (where variant_matches is
populated) to also build and assign rowset_relations from the same matching
logic or source (use the same VariantMatch construction logic), and modify
grade_in_place to read/serialize rowset_relations alongside variant_matches;
otherwise drop the rowset_relations Field declaration to avoid persisting a
misleading empty list. Ensure you reference and update the methods
grade_submission and grade_in_place and the VariantMatch construction to keep
behavior consistent.
In `@tests/cloud/test_collation.py`:
- Around line 31-43: Remove the now-unused helper function _read_dual_cols from
tests/cloud/test_collation.py: locate the def _read_dual_cols(...) block and
delete it (including its sqlite3 usage and return value) so the test file no
longer contains dead code; ensure no other references to _read_dual_cols remain
in the file after removal.
In `@tests/test_eval_annotation_schema.py`:
- Around line 218-219: Rename the ambiguous loop variable `l` used in the list
comprehension and generator expression to a clearer name like `line` for
readability: update the list comprehension that assigns `indent_lines = [l for l
in body_lines[1:-1] if l.strip()]` to use `line` (e.g., `indent_lines = [line
for line in body_lines[1:-1] if line.strip()]`) and likewise change the
subsequent assertion `assert all(l.startswith(" ") for l in indent_lines)` to
use `line` (`assert all(line.startswith(" ") for line in indent_lines)`),
ensuring both references to the variable in this test are renamed consistently.
In `@tests/test_tolerant_grader_orchestration.py`:
- Around line 735-753: The CountingInner test stub uses a class-level attribute
calls = 0 which is unconventional; change it to an instance attribute by adding
an __init__ that sets self.calls = 0 and keep the judge(self, **kwargs) method
to increment self.calls, so the behavior with CachedLLMJudge (inner, judge)
remains the same but follows the same instance-initialization pattern used
elsewhere in the tests.
🪄 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: a57df611-9330-4d19-ac38-6ca160fe4249
📒 Files selected for processing (72)
.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/generate_annotation_skeletons.pyscripts/verify_audited_gold.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/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/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/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_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_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_local_run_cascading.pytests/test_paths.pytests/test_paths_annotations.pytests/test_schema_extension.pytests/test_tolerant_grader_comparators.pytests/test_tolerant_grader_orchestration.py
💤 Files with no reviewable changes (12)
- src/bird_interact_agents/agents/smolagents/agent.py
- tests/test_claude_sdk_otf_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
- src/bird_interact_agents/agents/pydantic_ai/agent.py
- src/bird_interact_agents/agents/mcp_agent/agent.py
- src/bird_interact_agents/agents/claude_sdk/agent.py
- tests/test_claude_sdk_otf_ainteract_agent.py
- src/bird_interact_agents/agents/claude_sdk_otf_ainteract/agent.py
- src/bird_interact_agents/agents/agno/agent.py
- src/bird_interact_agents/agents/claude_sdk_otf/agent.py
…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>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 @.claude/skills/annotate-task-submission/SKILL.md:
- Around line 163-179: Replace the unlabeled fenced code blocks that contain the
"Audit-correctness gate — is the audited gold itself correct?" steps and the
later block referencing user_query_ambiguity.critical_ambiguity[].sql_snippet /
knowledge_ambiguity[].sql_snippet by adding a language tag (e.g., change ``` to
```text) so the two fenced blocks are labeled; locate the blocks by the text
"Audit-correctness gate" and the block that mentions
user_query_ambiguity.critical_ambiguity[].sql_snippet /
knowledge_ambiguity[].sql_snippet and update their opening fences to ```text.
In @.claude/skills/audit-gold-sql/SKILL.md:
- Around line 81-87: The docs and write semantics currently describe
overwrite-by-instance_id which will delete alternate variants; change the
write-key / overwrite logic and documentation to use the composite key
(instance_id, variant_id) so each variant row is upserted independently. Update
any code paths or descriptions that perform deletes/overwrites keyed only by
instance_id to instead key by both instance_id and variant_id, and ensure the
primary boolean semantics (primary default true for single-variant) remain
unchanged when emitting multi-variant rows so exactly one variant per instance
can be marked primary.
In `@scripts/dev1515_cascade_summary.py`:
- Around line 152-155: The f-string prefix on the constant branch is unnecessary
and triggers Ruff F541; in the delta assignment (the variable delta computed
using prev_total and tot) replace f" (+0)" with a plain string " (+0)" so only
the branch that needs interpolation uses an f-string (i.e., keep f on f" (+{tot
- prev_total})" but remove the leading f on the constant branch).
In `@scripts/dev1515_convert_livesqlbench_museum.py`:
- Line 69: The script currently hardcodes LIVESQL_ROOT = Path("/home/james/...")
(and other hardcoded dataset paths around lines 76-90 and 206-207); replace
those hardcoded absolute paths with calls to the repository path helpers in
bird_interact_agents.paths (e.g., use the appropriate paths.<dataset>_root()
function) so all data paths are resolved via the helper functions; update
references to LIVESQL_ROOT and any other hardcoded Path(...) variables to call
the proper paths.*_root() and then join subpaths with .joinpath(...) so the
script becomes portable and follows the repo path-resolution policy.
- Around line 99-109: _load_audit_rows currently maps rows by instance_id
overwriting previous variants; change it to collect a list of variants per
instance_id (rows should be dict[str, list]) by appending each parsed d to
rows[d["instance_id"]]; then adjust the call site(s) around grade_submission
(the block referenced at lines ~231–237) to pass the full list of audited
variants (rows[instance_id]) or iterate them according to the "any audited
variant" grading rule so grade_submission receives/handles all variants instead
of a single row. Use the existing symbols _load_audit_rows, rows, instance_id
and grade_submission to locate and update the code.
In `@scripts/dev1515_convert_runs.py`:
- Around line 269-277: audit_rows currently maps each instance_id to a single
dict and later variants overwrite prior ones; change audit_rows to collect all
variants per instance_id (e.g., initialize audit_rows as a defaultdict(list) or
use setdefault and append each d) so mini_interact_audited.jsonl entries are
preserved; also update the grading logic that consumes audit_rows (the code that
previously assumed a single dict per instance_id) to iterate over the list of
variants for that instance_id and compute cascade/ambiguous outcomes across all
variants instead of using only the last variant.
In `@scripts/dev1515_households_14_multivariant.py`:
- Around line 96-183: Detect if the instance is already multivariant and skip
creating duplicates: before creating kb_row/snippet_row, check existing rows for
the same d["instance_id"] (look for variant_id "kb_definition_reading" or
"snippet_reading" or any change with clause_kind "source_conflict") and if
found, just append d and continue; when constructing snippet_row ensure the
audited_sol_sql rewrite is idempotent by only replacing "lcs > 3" -> "lcs > 2"
when the string "lcs > 3" is present (and avoid double-replacing), and when
updating "changes" filter out any existing source_conflict entries (use the same
filter used earlier) before appending the new source_conflict so you never
create duplicate source_conflict entries for kb_row or snippet_row (references:
INSTANCE_ID, rows, kb_row, snippet_row, variant_id, changes, clause_kind
"source_conflict", audited_sol_sql).
In `@tests/test_livesqlbench_audited_gold.py`:
- Around line 349-372: The test test_no_duplicate_instance_id_variant_pairs only
records counts for instance_ids that have a primary row, so instance_ids with
zero primaries slip through; modify the loop over _iter_audit_rows to ensure
primaries_per_iid is initialized for every iid (e.g., set primaries_per_iid[iid]
= primaries_per_iid.get(iid, 0) before counting) or separately track a seen_iids
set and then compute counts for all seen iids, then build over_primaries from
primaries_per_iid.items() and assert n == 1 for every seen iid; keep the
existing logic for seen_pairs/dupes and defaulting variant_id via
row.get("variant_id", "primary").
🪄 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: 51a7c81c-840a-4faa-8a1b-f3e72f227e27
📒 Files selected for processing (14)
.claude/skills/annotate-task-submission/SKILL.md.claude/skills/audit-gold-sql/SKILL.mdscripts/dev1515_cascade_summary.pyscripts/dev1515_convert_livesqlbench_museum.pyscripts/dev1515_convert_runs.pyscripts/dev1515_households_14_multivariant.pyscripts/dev1515_reclassify_sufficiency.pyscripts/dev1515_remap_failure_classes.pysrc/bird_interact_agents/eval/annotate.pysrc/bird_interact_agents/eval/annotation_schema.pysrc/bird_interact_agents/eval/grade_in_place.pysrc/bird_interact_agents/eval/regrade.pytests/test_livesqlbench_audited_gold.pytests/test_regrade_cli.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/bird_interact_agents/eval/regrade.py
- src/bird_interact_agents/eval/annotate.py
…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>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bird_interact_agents/eval/regrade.py (1)
210-210:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
paths.results_root()for the regraderun_dirresults path
run_diris currently built aspaths.main_checkout_root() / "results" / "cloud" / args.run_id, which bypasses theBIRD_RESULTS_ROOToverride thatpaths.results_root()honors (and it already matches howcloud/driver.pybuildslocal_results_root).♻️ Proposed change
- run_dir = paths.main_checkout_root() / "results" / "cloud" / args.run_id + run_dir = paths.results_root() / "cloud" / args.run_id🤖 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` at line 210, The run_dir is built using paths.main_checkout_root() which ignores the BIRD_RESULTS_ROOT override; change the construction in regrade.py so run_dir uses paths.results_root() and then appends "cloud" and args.run_id (matching cloud/driver.py's local_results_root pattern) so the BIRD_RESULTS_ROOT override is honored when locating results for regrade.
🤖 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.
Outside diff comments:
In `@src/bird_interact_agents/eval/regrade.py`:
- Line 210: The run_dir is built using paths.main_checkout_root() which ignores
the BIRD_RESULTS_ROOT override; change the construction in regrade.py so run_dir
uses paths.results_root() and then appends "cloud" and args.run_id (matching
cloud/driver.py's local_results_root pattern) so the BIRD_RESULTS_ROOT override
is honored when locating results for regrade.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 54b49eec-8e9b-4652-a705-1affb2b35691
📒 Files selected for processing (14)
analyses/raw/households_10.mdanalyses/raw/museum_2.mdanalyses/raw/museum_4.mdscripts/dev1515_convert_livesqlbench.pysrc/bird_interact_agents/cloud/driver.pysrc/bird_interact_agents/eval/annotate.pysrc/bird_interact_agents/eval/grade_in_place.pysrc/bird_interact_agents/eval/regrade.pysrc/bird_interact_agents/eval/tolerant_grader.pysrc/bird_interact_agents/paths.pytests/cloud/test_collation.pytests/cloud/test_fetch_cascading_phase1.pytests/test_tolerant_grader_multi_sql_conn.pytests/test_verdict_label_shared.py
💤 Files with no reviewable changes (2)
- tests/cloud/test_collation.py
- scripts/dev1515_convert_livesqlbench.py
✅ Files skipped from review due to trivial changes (3)
- analyses/raw/museum_2.md
- analyses/raw/museum_4.md
- analyses/raw/households_10.md
🚧 Files skipped from review as they are similar to previous changes (3)
- src/bird_interact_agents/paths.py
- src/bird_interact_agents/cloud/driver.py
- src/bird_interact_agents/eval/tolerant_grader.py
… 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>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/bird_interact_agents/eval/regrade.py (1)
150-154:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
--force-llm-judgeis a silent no-op without--instance-ids.
clear_llm_judge_cacheonly runs whenfilter_setis truthy, so passing--force-llm-judgealone (regrade all instances) leaves the cache fully populated and the grader reuses every cached judge decision — the opposite of the flag's intent. If clearing the whole cache when unfiltered is undesirable, consider warning the user; otherwise clear all entries.🤖 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 150 - 154, The current check only calls clear_llm_judge_cache when filter_set is truthy, making --force-llm-judge a no-op without --instance-ids; change the logic so that when force_llm_judge is true you always invoke clear_llm_judge_cache for the llm_judge_cache.json: call clear_llm_judge_cache(cache_path=run_dir / "llm_judge_cache.json", instance_ids=filter_set if filter_set else None) (or pass an explicit empty/None to indicate clearing all), or alternatively emit a user warning and skip clearing only if you prefer; update the branch around force_llm_judge/filter_set to unconditionally call clear_llm_judge_cache with instance_ids determined as described.scripts/dev1515_cascade_summary.py (1)
27-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix DEV-1515 cascade summary to use the canonical annotations/ benchmark directory name (underscore).
scripts/dev1515_cascade_summary.pysetsBENCHMARK = "mini-interact"and_gather_files()buildsannroot = paths.annotations_root() / BENCHMARK, so it globsannotations/mini-interact/.... Butsrc/bird_interact_agents/eval/annotation_io.pynormalizes the benchmark token (_canonical_benchmark()replaces-with_) when building bothsubmission_annotation_pathandtask_annotation_path, andtests/test_paths_annotations.py::test_annotation_paths_canonicalize_mini_interactasserts the on-disk directory ismini_interact(dash is not used on disk). Writers such assrc/bird_interact_agents/eval/regrade.pyandsrc/bird_interact_agents/cloud/post_run_merge.pywrite viasubmission_annotation_path(...), so the cascade summary will miss the written files and report0pairs.Set
BENCHMARK = "mini_interact"(and update the docstring path) or otherwise construct the directory using the same canonical benchmark token asannotation_iouses.🤖 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 `@scripts/dev1515_cascade_summary.py` at line 27, The script sets BENCHMARK = "mini-interact" causing _gather_files() to look under annotations/mini-interact while on-disk writers (submission_annotation_path, task_annotation_path in src/bird_interact_agents/eval/annotation_io.py which use _canonical_benchmark() to replace '-' with '_') write to annotations/mini_interact; fix by changing BENCHMARK to "mini_interact" (or build the annroot using the same canonicalization as annotation_io) and update the docstring path to annotations/mini_interact so _gather_files() and the rest of the pipeline reference the same directory name.
🤖 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/dev1515_cascade_summary.py`:
- Line 3: The docstring in scripts/dev1515_cascade_summary.py incorrectly
documents the directory as annotations/mini-interact/*/...; update that
docstring to use the canonical underscore form
annotations/mini_interact/*/<inst>.submission.<run>.json (matching the BENCHMARK
fix) and ensure any other docstring examples in the same file use the underscore
form so the documented layout matches the actual directory-resolution logic.
---
Outside diff comments:
In `@scripts/dev1515_cascade_summary.py`:
- Line 27: The script sets BENCHMARK = "mini-interact" causing _gather_files()
to look under annotations/mini-interact while on-disk writers
(submission_annotation_path, task_annotation_path in
src/bird_interact_agents/eval/annotation_io.py which use _canonical_benchmark()
to replace '-' with '_') write to annotations/mini_interact; fix by changing
BENCHMARK to "mini_interact" (or build the annroot using the same
canonicalization as annotation_io) and update the docstring path to
annotations/mini_interact so _gather_files() and the rest of the pipeline
reference the same directory name.
In `@src/bird_interact_agents/eval/regrade.py`:
- Around line 150-154: The current check only calls clear_llm_judge_cache when
filter_set is truthy, making --force-llm-judge a no-op without --instance-ids;
change the logic so that when force_llm_judge is true you always invoke
clear_llm_judge_cache for the llm_judge_cache.json: call
clear_llm_judge_cache(cache_path=run_dir / "llm_judge_cache.json",
instance_ids=filter_set if filter_set else None) (or pass an explicit empty/None
to indicate clearing all), or alternatively emit a user warning and skip
clearing only if you prefer; update the branch around force_llm_judge/filter_set
to unconditionally call clear_llm_judge_cache with instance_ids determined as
described.
🪄 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: 4512e354-c71c-46fd-8f2f-233a199777cc
📒 Files selected for processing (28)
.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.mdscripts/dev1515_cascade_summary.pyscripts/dev1515_convert_livesqlbench.pyscripts/dev1515_convert_runs.pyscripts/dev1515_households_14_multivariant.pysrc/bird_interact_agents/agents/claude_sdk_otf/prompts.pysrc/bird_interact_agents/agents/claude_sdk_otf_ainteract/prompts.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/regrade.pysrc/bird_interact_agents/eval/tolerant_grader.pysrc/bird_interact_agents/run.pytests/cloud/test_fetch_annotation_merge.pytests/test_cascading_report.pytests/test_eval_annotate_cli.pytests/test_eval_annotation_schema.pytests/test_livesqlbench_audited_gold.pytests/test_paths_annotations.pytests/test_regrade_cli.pytests/test_tolerant_grader_comparators.pytests/test_tolerant_grader_orchestration.pytests/test_verdict_label_shared.py
✅ Files skipped from review due to trivial changes (1)
- .claude/skills/annotate-task-submission/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (12)
- tests/test_cascading_report.py
- tests/test_verdict_label_shared.py
- tests/test_eval_annotate_cli.py
- src/bird_interact_agents/eval/annotation_io.py
- tests/cloud/test_fetch_annotation_merge.py
- tests/test_eval_annotation_schema.py
- src/bird_interact_agents/eval/annotation_schema.py
- src/bird_interact_agents/eval/cascading_report.py
- scripts/dev1515_convert_runs.py
- scripts/dev1515_households_14_multivariant.py
- src/bird_interact_agents/run.py
- src/bird_interact_agents/eval/annotate.py
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>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/eval/tolerant_grader.py`:
- Around line 970-988: The current implementation uses expr.find(sg_expr.Where)
which can match a nested WHERE (e.g., in a subquery/CTE); update
_where_conjunct_count to first locate the outermost SELECT node (sg_expr.Select)
starting at the provided expr (do not search nested selects) and then read that
Select's WHERE child (the Select's where/args["where"] or equivalent) instead of
calling expr.find(sg_expr.Where); then count conjuncts from that outer Select's
Where as before.
- Around line 633-643: The current except block around _multi_sql_execute
coerces failures into empty rowsets (v_rows, v_cols = [], []), which can produce
false positives; instead, on exception capture the failure and record a distinct
unevaluable marker (e.g., v_rows = v_cols = None or append a tuple like (v,
None, None)) to variant_results so downstream comparison logic can skip or mark
unevaluable variants (update any logic that reads variant_results to treat None
as "skip/unevaluable" rather than an empty result). Ensure the exception still
gets logged (include the exception info) and do not convert failures into empty
lists.
🪄 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: 17eeaa32-8241-4734-891e-ccb8d7800172
📒 Files selected for processing (6)
scripts/dev1515_strict_miss_diagnostics.pysrc/bird_interact_agents/eval/annotate.pysrc/bird_interact_agents/eval/annotation_schema.pysrc/bird_interact_agents/eval/grade_in_place.pysrc/bird_interact_agents/eval/tolerant_grader.pytests/test_miss_diagnostics.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/bird_interact_agents/eval/grade_in_place.py
- src/bird_interact_agents/eval/annotate.py
…_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>
…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>
…ariant-gold-annotations-tolerant-grader-post-dev-1478 # Conflicts: # tests/test_livesqlbench_audited_gold.py
This was referenced Jun 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
<main_checkout>/annotations/.phase1_passed_audited/phase1_passed_originalraw bool columns end-to-end (run.py, results_db.py, cloud/collation.py, agents/_submit.py, 9 agent files) — per-task verdicts now live inSubmissionAnnotation.eval/annotate.py+scripts/generate_annotation_skeletons.py) with init/refresh/force-all modes and a sentinel-preserving idempotency contract, plus the offlineeval/regrade.pyCLI.Closes DEV-1515.
Design notes
ex_basedefault; preservesphase1_count → counts["n1"]back-compat). N4 (bucket-by-ORDER-BY-then-set-equal) is therefore a no-op vs N3 in practice but kept for spec compliance.ORDER BY(deliberate simplification).annotations/is gitignored (separate-repo planned; same posture asaudited_gold/); the cloud image bakes it in via--build-context annotations=<paths.annotations_root()>so worktree builds match main-checkout content.Codex review
Two rounds folded in:
grade_in_place; LLM cache key incomplete.--force-llm-judgewas a no-op assertion; GCS roundtrip not tested; schema-extension fields not pinned;VariantInformationalweakly covered; N5 gating + N4 ORDER-BY-source tests didn't actually distinguish; image-hash path-keying tested by source grep instead of behaviorally; cache key dimensions incomplete; aggregator monotone test passed without enforcement;phase1_countrewrite test passed without rewrite; submission-mode overwrite + dry-run untested.All findings have specific commits / tests.
Test plan
tests/test_legacy_field_removal.pygrep-sweep — no production source emits the legacy fields.tests/test_tolerant_grader_orchestration.py::test_cascade_is_monotone_for_every_possible_pass_pattern— property test over all 256 raw bool patterns.tests/cloud/test_image_annotations.py::test_data_hash_invariant_to_host_path_of_annotations_root— worktree-safety regression mirrors the existingaudited_goldposture.tests/cloud/test_fetch_annotation_merge.py— end-to-end GCS round-trip via the existing fake-bucket fixture + no-overwrite + schema-validate + audit-report.20260531t1343-claudes-slayer-b39bfc, terminal); 53-instance annotation pass; validate prompt + grader fix on a small re-run from this branch. Tracked in the DEV-1515 Linear comment.🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
New Features
Improvements
Analysis