DEV-1518: annotator agent — full agentic task audit via Claude Agent SDK - #18
Merged
Conversation
New annotator agent that audits a single benchmark task end-to-end and produces a TaskAnnotation + audited gold variants, deployable to the same GCE/Ray cloud infrastructure as the existing eval agent. Key additions: - src/bird_interact_agents/agents/annotator/: ClaudeSDKClient-based agent with get_ambiguity_resolutions (mini-interact only) + submit_annotation tools; _fill_audited_gold_ref_files harness helper fills sentinel paths - src/bird_interact_agents/cloud/ray_app_annotator.py: per-task worker with dual-blob skip check (both stable blobs required), attempt-1.json written for every outcome, dual GCS write on success (run-specific + stable) - cloud/gcs.py: task_annotation_blob, audited_gold_variants_blob, stable_*, write_*, blob_exists helpers - cloud/post_run_merge.py: merge_task_annotations (always-overwrite) and merge_audited_gold_variants (dedup by instance_id+variant_id) - cloud/cluster.py: ray_app_path kwarg on submit_job - tests/test_livesqlbench_audited_gold.py: fix museum_2/4/9 primary field and update primary-selection logic to treat single-row instances as primary Co-Authored-By: Claude Sonnet 4.6 <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 DEV-1515 multi‑variant audit contract and skills; Pydantic annotation schemas and IO; tolerant N1–N9 grader with miss diagnostics and LLM-judge caching; inline/cloud grading and annotator agent; GCS blob/merge/image wiring; consolidation/conversion scripts; and broad tests. Legacy dual‑eval DB fields removed; cascading aggregation uses per‑row submission annotations. ChangesDEV-1515: Annotation, tolerant grader, and cloud wiring
Sequence Diagram (high-level grading flow) sequenceDiagram
participant Run as Runner (run.py / ray_app)
participant Grade as GradeInPlace
participant Grader as TolerantGrader
participant GCS as GCS
Run->>Grade: grade_one_submission(task_data, attempt_row)
Grade->>Grader: grade_submission(submitted_sql, audited_variants, db_path)
Grader-->>Grade: CascadeVerdict + MissDiagnostics
Grade->>GCS: write_submission_annotation(run_id, instance_id, submission_annotation.json)
Run->>GCS: upload attempt row and artifacts
GCS-->>Run: per-row annotations available for fetch/aggregation
Estimated code review effort Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
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/cloud/collation.py (1)
167-168:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrite
eval.jsonthrough the cascading emitter.
collate()still dumps_build_metrics()directly, so this path never adds the newcascading_phase1block and never rewritesphase1_count/phase1_ratefrom the merged per-row annotations. That leaveseval.jsonstale against the new contract.Proposed fix
+from bird_interact_agents.eval.cascading_report import emit_cascading_eval_json + def collate(run_dir: Path, manifest: dict) -> dict[str, Any]: @@ - metrics = _build_metrics(manifest, ordered_rows, attempts) - (run_dir / "eval.json").write_text(json.dumps(metrics, indent=2, default=str) + "\n") + base_metrics = _build_metrics(manifest, ordered_rows, attempts) + metrics = emit_cascading_eval_json( + run_dir / "rows", + run_dir / "eval.json", + base_metrics=base_metrics, + ) return metrics🤖 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/collation.py` around lines 167 - 168, collate() currently writes _build_metrics(manifest, ordered_rows, attempts) directly to eval.json, bypassing the cascading emitter so the cascading_phase1 block and the merged per-row rewrites of phase1_count/phase1_rate are never applied; update collate() to emit the metrics via the cascading emitter (i.e., pass metrics through the same emitter/merge path used for rows) and then have the emitter write eval.json so cascading_phase1 is added and phase1_count/phase1_rate are rewritten from merged annotations produced by the emitter rather than writing the raw _build_metrics output.src/bird_interact_agents/cloud/image.py (1)
195-208: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUnify the
annotations_rootdefault between tagging and building.
build_and_push()bakespaths.annotations_root()when the kwarg is omitted, butimage_tag()/data_hash()ignore annotations unless the caller passes it explicitly. That makes the tag blind to annotation edits for any caller that forgets the new kwarg, so the manifest probe can reuse a stale image. Either default both sides the same way or make the kwarg required across both APIs.Also applies to: 315-316
🤖 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/image.py` around lines 195 - 208, The tagging path is blind to annotation edits because image_tag (and data_hash) ignore annotations when annotations_root is omitted while build_and_push implicitly uses paths.annotations_root(); make them consistent by either (A) defaulting image_tag and data_hash to use paths.annotations_root() when annotations_root is None (mirror build_and_push's behavior) or (B) make annotations_root a required kwarg on image_tag and data_hash so callers must pass paths.annotations_root(); update both image_tag and data_hash signatures/entry logic (and the related callers around the 315-316 usage) to apply the chosen approach.
🟠 Major comments (24)
scripts/consolidate_mini_interact_audited.py-39-40 (1)
39-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDerive default
primaryfromvariant_idto avoid invalid multi-primary rows.If a row has
variant_id != "primary"and lacksprimary, this setsprimary=True, which can silently create multiple primary variants for the same instance.Suggested fix
out.setdefault("benchmark", benchmark) out.setdefault("variant_id", "primary") - out.setdefault("primary", True) + out.setdefault("primary", out["variant_id"] == "primary")🤖 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/consolidate_mini_interact_audited.py` around lines 39 - 40, The current lines calling out.setdefault("variant_id", "primary") and out.setdefault("primary", True) can wrongly mark non-"primary" variants as primary; change the defaulting for "primary" to derive from the (possibly defaulted) variant_id by setting primary to True only when out.get("variant_id") == "primary" (e.g., use out.setdefault("primary", out.get("variant_id") == "primary")), so that primary is consistent with variant_id; update the logic around the out.setdefault calls in this block accordingly.scripts/consolidate_mini_interact_audited.py-23-27 (1)
23-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
bird_interact_agents.paths.*_root()helpers instead of repo-relativePath(__file__)roots.This script hardcodes repository-relative paths, which breaks the path-root contract for Python-reachable data paths and makes execution location-sensitive.
Suggested refactor
from __future__ import annotations import json -from pathlib import Path -ROOT = Path(__file__).resolve().parents[1] -AUDITED = ROOT / "audited_gold" +from bird_interact_agents import paths + +AUDITED = paths.audited_gold_root()As per coding guidelines: "
**/*.py: ... All reachable-from-Python data paths must be resolved viabird_interact_agents.paths.*_root()helpers..."🤖 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/consolidate_mini_interact_audited.py` around lines 23 - 27, Replace the repo-relative Path(__file__) root with the provided path helper: import the appropriate helper from bird_interact_agents.paths (e.g., audited_root or the relevant *_root() function) and set AUDITED = audited_root() (or audited_root().joinpath(...)) then compute MINI_INTERACT_OUT and LIVESQLBENCH relative to that AUDITED; remove the ROOT = Path(__file__).resolve() usage so all data paths use bird_interact_agents.paths.*_root() helpers (update the import to include the helper and keep variable names MINI_INTERACT_OUT and LIVESQLBENCH).scripts/dev1515_convert_runs.py-292-299 (1)
292-299:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRead the per-DB audit sidecars instead of a consolidated root file.
This PR’s updated mini-interact audit contract still writes sidecars as
audited_gold/<db>/<db>_audited.jsonl. Loadingaudited_gold/mini_interact_audited.jsonlhere will fail on a normal checkout that only has the documented per-DB files.Suggested fix
- audit_rows: dict[str, list[dict]] = {} - for line in ( - paths.audited_gold_root() / "mini_interact_audited.jsonl" - ).read_text().splitlines(): - if not line.strip(): - continue - d = json.loads(line) - audit_rows.setdefault(d["instance_id"], []).append(d) + audit_rows: dict[str, list[dict]] = {} + for audit_file in paths.audited_gold_root().glob("*/*_audited.jsonl"): + for line in audit_file.read_text().splitlines(): + if not line.strip(): + continue + d = json.loads(line) + audit_rows.setdefault(d["instance_id"], []).append(d)🤖 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_convert_runs.py` around lines 292 - 299, The code currently loads a single consolidated file "mini_interact_audited.jsonl" into audit_rows; instead, iterate the audited_gold root directory (paths.audited_gold_root()) and read each per-DB sidecar (pattern "<db>/<db>_audited.jsonl" or use Path.glob("*/*_audited.jsonl")), parse each non-empty JSONL line and append to audit_rows keyed by d["instance_id"]; ensure you skip missing files and continue on empty lines—replace the existing single-file read block that populates audit_rows with this directory-based loop using the same audit_rows variable.scripts/dev1515_convert_livesqlbench.py-190-200 (1)
190-200:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the audited row’s actual
variant_idhere.The updated audit contract only guarantees one row with
primary: true; it does not guarantee that row’s slug is literally"primary". Hardcoding"primary"here will create a danglingAuditedGoldReffor multi-variant audits whose primary variant is named something else.Suggested fix
gold_variants = [ GoldVariantRef( - variant_id="primary", + variant_id=str(audit_row.get("variant_id", "primary")), interpretation=PENDING, - primary=True, + primary=bool(audit_row.get("primary", True)), anchored_in=[], audited_gold_ref=AuditedGoldRef( file=AUDIT_FILE_REL, instance_id=instance_id, - variant_id="primary", + variant_id=str(audit_row.get("variant_id", "primary")), ), notes=(🤖 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_convert_livesqlbench.py` around lines 190 - 200, The hardcoded "primary" in the AuditedGoldRef.variant_id should instead use the actual variant id of the audited row marked primary; locate where GoldVariantRef objects are created (GoldVariantRef and AuditedGoldRef) and replace the literal "primary" with the primary audited row's variant_id (e.g., the variant_id value from the GoldVariantRef/row where primary=True or from the audit row variable representing the primary instance). Ensure you grab the primary variant_id programmatically so multi-variant audits use the real slug rather than the string "primary".scripts/dev1515_convert_runs.py-122-132 (1)
122-132:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t hardcode the primary variant slug.
primary: truedoes not implyvariant_id == "primary". On any multi-variant audit whose primary row uses a different slug, this annotation will point at a non-existent audited variant.Suggested fix
gold_variants = [ GoldVariantRef( - variant_id="primary", + variant_id=str(audit_row.get("variant_id", "primary")) if audit_row else "primary", interpretation=PENDING, - primary=True, + primary=bool(audit_row.get("primary", True)) if audit_row else True, anchored_in=[], audited_gold_ref=AuditedGoldRef( file=AUDIT_FILE_REL, instance_id=iid, - variant_id="primary", + variant_id=str(audit_row.get("variant_id", "primary")) if audit_row else "primary", ), notes=(🤖 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_convert_runs.py` around lines 122 - 132, The code hardcodes audited_gold_ref.variant_id as the literal "primary" which can be incorrect when the primary variant uses a different slug; update the construction of the GoldVariantRef so audited_gold_ref.variant_id (and variant_id for the primary GoldVariantRef) use the actual primary variant slug value instead of the string "primary" — e.g., replace the literal "primary" with the variable that holds the primary variant's slug (or compute it from the source audit row like primary_variant_slug / primary_row.variant_id) when creating GoldVariantRef and its AuditedGoldRef so they point to the real primary variant ID.scripts/dev1515_households_14_multivariant.py-197-200 (1)
197-200:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't swallow unexpected rewrite failures here.
This
except Exceptionpath converts any bug intoaudited_sample_row_status="error"and still writes the patched audit file. If the SQL rewrite or row shape is wrong, the script will persist bad audit data instead of failing fast. Catch the expected DB error class only and let everything else abort the run.Suggested fix
- except Exception as exc: + except sqlite3.Error as exc: snippet_row["audited_sample_row"] = [] snippet_row["audited_sample_row_status"] = "error" snippet_row["audited_sample_row_error"] = f"{type(exc).__name__}: {exc}"🤖 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_households_14_multivariant.py` around lines 197 - 200, Replace the broad "except Exception as exc:" that swallows all errors with a handler that only catches the expected database rewrite exception type(s) (e.g., psycopg2.Error or sqlalchemy.exc.SQLAlchemyError as appropriate for this codebase) around the rewrite logic; set snippet_row["audited_sample_row"], snippet_row["audited_sample_row_status"], and snippet_row["audited_sample_row_error"] inside that specific except block, and let any other exceptions propagate (re-raise) so the run fails fast instead of persisting bad audit data.scripts/verify_audited_gold.py-78-94 (1)
78-94:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't raise before trying the existing per-db sidecar.
If
mini_interact_audited.jsonlexists but hasn't been populated for thisdbyet, this branch raises immediately and never reaches the legacy<db>/<db>_audited.jsonlfallback below. That makes partial migrations fail verification unnecessarily.Suggested fix
if audit_set == "inhouse": single = audited_root_for(audit_set) / "mini_interact_audited.jsonl" if single.exists(): rows: list[dict] = [] with single.open() as f: for line in f: line = line.strip() if not line: continue d = json.loads(line) if d.get("selected_database") == db: rows.append(d) - if not rows: - raise FileNotFoundError( - f"No rows for db={db!r} in {single}" - ) - return rows + if rows: + return rows path = audited_root_for(audit_set) / db / audited_filename_for(db, audit_set)🤖 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/verify_audited_gold.py` around lines 78 - 94, The current branch under audit_set == "inhouse" immediately raises if mini_interact_audited.jsonl exists but contains no rows for the given db, preventing the legacy per-db fallback from being attempted; change the logic in that block (around audited_root_for(...) and the single variable) so that if mini_interact_audited.jsonl exists you collect rows and if rows is non-empty return them, but if rows is empty do NOT raise there—let execution continue to try the per-db sidecar (<db>/<db>_audited.jsonl) and only raise FileNotFoundError after both the aggregate mini_interact_audited.jsonl (if present) and the per-db audited file have been checked and found empty/missing.scripts/dev1515_remap_failure_classes.py-119-123 (1)
119-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScan both benchmark directory spellings here.
This migration only walks
annotations_root()/mini-interact, but the PR already acknowledges amini-interactvsmini_interactsplit elsewhere. Any residual underscore-path submission files will be skipped and never remapped.Suggested fix
def main() -> None: - annroot = paths.annotations_root() / "mini-interact" n_seen = n_changed = 0 - print(f"Walking {annroot}") - for p in sorted(annroot.glob("*/*.submission.*.json")): - n_seen += 1 - raw = json.loads(p.read_text()) - changed, reason = _remap_raw(raw) - # Always re-validate to guarantee schema conformance. - SubmissionAnnotation.model_validate(raw) - if changed: - n_changed += 1 - p.write_text(json.dumps(raw, indent=2) + "\n") - print(f" {raw['instance_id']:40s} {reason}") + annroots = ( + paths.annotations_root() / "mini-interact", + paths.annotations_root() / "mini_interact", + ) + for annroot in annroots: + print(f"Walking {annroot}") + for p in sorted(annroot.glob("*/*.submission.*.json")): + n_seen += 1 + raw = json.loads(p.read_text()) + changed, reason = _remap_raw(raw) + # Always re-validate to guarantee schema conformance. + SubmissionAnnotation.model_validate(raw) + if changed: + n_changed += 1 + p.write_text(json.dumps(raw, indent=2) + "\n") + print(f" {raw['instance_id']:40s} {reason}")🤖 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_remap_failure_classes.py` around lines 119 - 123, The script's main() only walks paths.annotations_root() / "mini-interact" so files under the alternate directory name "mini_interact" are skipped; update main() to scan both spellings (e.g., iterate over names ["mini-interact", "mini_interact"] or build two annroot variants from paths.annotations_root()) and run the same glob logic (the "*/*.submission.*.json" pattern) for each root so n_seen/n_changed count and remapping logic apply to files in both directories.src/bird_interact_agents/agents/annotator/prompts.py-42-43 (1)
42-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
sol_sqlas eitherstrorlist[str]here.This builder currently treats any string gold SQL as an iterable of characters, so the prompt ends up with one character per line. The repo already handles
sol_sqlas a mixedstr | list[str]shape elsewhere.Suggested fix
def build_system_prompt(task_data: dict, benchmark: str) -> str: sol_sql = task_data.get("sol_sql", []) - sol_str = "\n".join(sol_sql) if sol_sql else "(none)" + if isinstance(sol_sql, str): + sol_str = sol_sql or "(none)" + elif sol_sql: + sol_str = "\n".join(sol_sql) + else: + sol_str = "(none)" has_ambiguity_tool = benchmark == "mini_interact"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bird_interact_agents/agents/annotator/prompts.py` around lines 42 - 43, The code treats sol_sql as an iterable of characters; adjust the sol_sql handling so it supports both str and list[str]: retrieve sol_sql = task_data.get("sol_sql") then if sol_sql is a str use sol_sql as a single entry (or wrap into a one-element list) and if it's a list join with "\n"; finally set sol_str = "(none)" when sol_sql is empty or None; update the variables referenced (sol_sql and sol_str) in prompts.py accordingly.src/bird_interact_agents/cloud/ray_app_annotator.py-121-131 (1)
121-131:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the success-path GCS writes fail closed and still record an error attempt.
If any of these four uploads fails, the function exits before
attempt-1.jsonis written. That violates the worker contract in this PR and can leave partially published annotation blobs behind with no terminal attempt row.Suggested structure
- _gcs.write_task_annotation(run_id, instance_id, ann, client=client) - _gcs.write_audited_gold_variants(run_id, instance_id, variants, client=client) - _gcs.write_stable_task_annotation(benchmark, db, instance_id, ann, client=client) - _gcs.write_stable_audited_gold_variants(benchmark, db, instance_id, variants, client=client) - - attempt_row = { - "instance_id": instance_id, - "status": "annotated", - "duration_s": result.duration_s, - } - _write_attempt(run_id, instance_id, attempt_row, client=client) + try: + _gcs.write_task_annotation(run_id, instance_id, ann, client=client) + _gcs.write_audited_gold_variants(run_id, instance_id, variants, client=client) + _gcs.write_stable_task_annotation(benchmark, db, instance_id, ann, client=client) + _gcs.write_stable_audited_gold_variants( + benchmark, db, instance_id, variants, client=client + ) + except Exception as exc: + logger.error("[%s] failed to persist annotation outputs: %s", instance_id, exc) + _write_attempt( + run_id, + instance_id, + { + "instance_id": instance_id, + "status": "error", + "error": str(exc), + "duration_s": time.monotonic() - t0, + }, + client=client, + ) + return + + _write_attempt( + run_id, + instance_id, + { + "instance_id": instance_id, + "status": "annotated", + "duration_s": result.duration_s, + }, + client=client, + )tests/test_livesqlbench_audited_gold.py-300-332 (1)
300-332:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAssert coverage for every expected DB, not only DBs already present in the file.
This loop only validates databases that already appear in
primary_rows. If the audit file dropscreditormentalentirely, the test still passes, which undercuts the new multi-DB contract.Suggested fix
def test_audit_rows_cover_select_tasks_per_db(): @@ primary_rows = _load_audit_rows() by_db: dict[str, set[str]] = {} for iid, row in primary_rows.items(): by_db.setdefault(row["selected_database"], set()).add(iid) - for db, ids in by_db.items(): - expected = EXPECTED_INSTANCE_IDS_BY_DB.get(db) - assert expected is not None, ( - f"audit file contains DB {db!r} without a coverage entry in " - f"EXPECTED_INSTANCE_IDS_BY_DB — add one when authoring a new DB" - ) + unexpected_dbs = set(by_db) - set(EXPECTED_INSTANCE_IDS_BY_DB) + assert not unexpected_dbs, ( + f"audit file contains DBs without coverage entries: {sorted(unexpected_dbs)}" + ) + for db, expected in EXPECTED_INSTANCE_IDS_BY_DB.items(): + ids = by_db.get(db, set()) missing = expected - ids assert not missing, ( f"{db}: missing SELECT-task audits {sorted(missing)}" )🤖 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_livesqlbench_audited_gold.py` around lines 300 - 332, The test only iterates databases present in primary_rows (by_db) so it misses cases where an expected DB (e.g., "credit" or "mental") is entirely absent; update test_audit_rows_cover_select_tasks_per_db to ensure every key in EXPECTED_INSTANCE_IDS_BY_DB is validated: either assert that EXPECTED_INSTANCE_IDS_BY_DB.keys() is a subset of by_db.keys() (and fail with a clear message listing missing DBs) before the existing loop, or change the loop to iterate over EXPECTED_INSTANCE_IDS_BY_DB.items() and treat missing ids as an empty set so the subsequent missing/extra checks work; reference primary_rows, by_db, EXPECTED_INSTANCE_IDS_BY_DB and the existing missing/extras logic when making the change.src/bird_interact_agents/agents/annotator/agent.py-152-176 (1)
152-176:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject cross-task submissions before marking the annotation complete.
submit_annotationtrusts whateverinstance_id/selected_databasethe model sends. If the agent hallucinates or replays a payload for a different task, this code still sets_submission_doneand the caller will persist mismatched data under the current task's blobs.Suggested guard
async def submit_annotation(args: dict) -> dict: from pydantic import ValidationError + task_data = _ctx.get("task_data", {}) + expected_instance_id = task_data.get("instance_id") + expected_database = task_data.get("selected_database") + ta_json = args.get("task_annotation_json", "") av_json = args.get("audited_gold_variants_json", "[]") @@ try: task_annotation = TaskAnnotation.model_validate(ta_dict) @@ except Exception as e: return _text(f"Error parsing task_annotation_json: {e}") + + if task_annotation.instance_id != expected_instance_id: + return _text( + "Validation error in task_annotation_json: " + f"instance_id must be {expected_instance_id!r}" + ) + if task_annotation.selected_database != expected_database: + return _text( + "Validation error in task_annotation_json: " + f"selected_database must be {expected_database!r}" + ) try: audited_gold_variants: list[dict] = json.loads(av_json) if not isinstance(audited_gold_variants, list): raise TypeError("Expected a JSON array") + for i, variant in enumerate(audited_gold_variants): + if not isinstance(variant, dict): + raise TypeError(f"Expected object at index {i}") + if variant.get("instance_id") not in (None, expected_instance_id): + raise TypeError( + f"variant[{i}].instance_id must be {expected_instance_id!r}" + ) + if variant.get("selected_database") not in (None, expected_database): + raise TypeError( + f"variant[{i}].selected_database must be {expected_database!r}" + ) except (json.JSONDecodeError, TypeError) as e: return _text(f"Error: invalid JSON in audited_gold_variants_json: {e}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bird_interact_agents/agents/annotator/agent.py` around lines 152 - 176, After parsing and validating ta_json into task_annotation (via TaskAnnotation.model_validate) and audited_gold_variants, verify that the submitted annotation actually belongs to the current task before persisting: compare task_annotation.instance_id and task_annotation.selected_database (or analogous identifying fields on TaskAnnotation) against the current context values in _ctx (e.g., _ctx.get("instance_id") and _ctx.get("selected_database")); if they do not match, return an error via _text and do NOT set _ctx["annotation_result"] or _ctx["_submission_done"]. Place this guard immediately after validation and before the existing assignments to _ctx.src/bird_interact_agents/eval/annotate.py-349-383 (1)
349-383:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe CLI never writes submission skeletons.
main()parses--run-idand--submission-mode, but it only iterateswrite_task_skeleton(). That meanspython -m bird_interact_agents.eval.annotatedoes not perform the per-submission half advertised by the module docstring and parser description, and the required--run-idis effectively dead.🤖 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 349 - 383, main currently parses --run-id and --submission-mode but only processes task skeletons via write_task_skeleton, so submission skeletons are never written; update main to also load and iterate submission rows using the provided args.run_id and args.submission_mode (and args.instance_ids if applicable) and call the submission-writing helper (e.g., write_submission_skeleton or the module's submission writer) for each submission, honoring args.submission_mode and args.dry_run; ensure the added logic runs after task processing and uses the same instance_ids handling as the task loop so the --run-id and --submission-mode flags take effect.src/bird_interact_agents/eval/annotation_schema.py-128-147 (1)
128-147: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winEnforce the gold-variant invariants in the model, not just the docstrings.
Right now the schema accepts impossible states like multiple
primary=Truevariants, zero primaries whengold_variantsis non-empty, ororiginal_gold_is_correct=Truealongside populatedgold_variants. Becauseannotation_iouses these models as the validation boundary, bad annotation JSON can still be persisted and only fail later in the grader.💡 Suggested validator shape
from pydantic import BaseModel, ConfigDict, Field +from pydantic import model_validator ... class TaskAnnotation(BaseModel): model_config = ConfigDict(extra="forbid") + + `@model_validator`(mode="after") + def _check_gold_invariants(self) -> "TaskAnnotation": + primary_count = sum(1 for variant in self.gold_variants if variant.primary) + if self.gold_variants and primary_count != 1: + raise ValueError("gold_variants must contain exactly one primary variant") + if self.original_gold_is_correct and self.gold_variants: + raise ValueError( + "gold_variants must be empty when original_gold_is_correct is True" + ) + return selfAlso applies to: 237-241
🤖 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/annotation_schema.py` around lines 128 - 147, Add model-level validation to enforce invariants described in the docstrings: in GoldVariantRef and the parent model that owns gold_variants and original_gold_is_correct, implement pydantic validators (or model root_validator) to 1) ensure exactly one GoldVariantRef has primary=True when gold_variants is non-empty, 2) forbid multiple primaries, 3) prevent original_gold_is_correct=True when gold_variants is populated, and 4) reject zero primaries if gold_variants exists. Reference the GoldVariantRef class and the containing field gold_variants and flag original_gold_is_correct to locate where to add these validators and raise clear ValidationError messages on violations.src/bird_interact_agents/eval/annotate.py-63-66 (1)
63-66:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCanonicalize
benchmarkbefore persisting paths and refs.These fields currently store the caller’s raw benchmark string, but the actual files are written via
annotation_io’s underscore-normalized layout. With--benchmark mini-interact, this emitsmini-interact.jsonlandannotations/mini-interact/...even though the real artifacts live undermini_interact, so the generated provenance/reference paths can point at non-existent files.💡 Minimal fix
+from bird_interact_agents.eval.annotation_io import _canonical_benchmark + def _benchmark_task_jsonl_name(benchmark: str) -> str: - if benchmark in set(benchmark_names()): - return get_benchmark(benchmark).data_file - return f"{benchmark}.jsonl" + canonical = _canonical_benchmark(benchmark) + if canonical in set(benchmark_names()): + return get_benchmark(canonical).data_file + return f"{canonical}.jsonl"+ task_annotation_ref=( + f"annotations/{_canonical_benchmark(benchmark)}/{selected_database}/" + f"{instance_id}.task.json" + ), - task_annotation_ref=( - f"annotations/{benchmark}/{selected_database}/" - f"{instance_id}.task.json" - ),Also applies to: 229-232
🤖 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 63 - 66, The code is using the caller's raw benchmark string to build filenames/paths, causing mismatches for hyphenated names; update _benchmark_task_jsonl_name to first canonicalize the benchmark to the annotation_io underscore-normalized form (e.g., via get_benchmark(benchmark) or the same normalization used by annotation_io/benchmark_names()), then use that canonical name when checking benchmark_names(), calling get_benchmark(...).data_file, and when constructing f"{...}.jsonl"; apply the same canonicalization to the other code block that builds annotation paths (the lines referenced around the second occurrence) so all persisted refs use the normalized (underscore) benchmark identifier.src/bird_interact_agents/eval/annotation_io.py-47-51 (1)
47-51: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRoute the annotations tree through
bird_interact_agents.pathsinstead of open-coding it here.This new root is another Python-reachable gitignored artifact path, but it is anchored by hand with
main_checkout_root()rather than a dedicatedpaths.*_root()helper. That bypasses the repo’s path contract and makes future callers duplicate the layout again.As per coding guidelines, "
**/*.py: Never depend on a path insiderepo_rootfor gitignored input or output ... All reachable-from-Python data paths must be resolved viabird_interact_agents.paths.*_root()helpers."🤖 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/annotation_io.py` around lines 47 - 51, The _annotations_root function is currently building the path by combining paths.main_checkout_root() and ANNOTATIONS_DIRNAME; replace this manual assembly with a dedicated helper in bird_interact_agents.paths (e.g. paths.annotations_root(repo_root) or similar) so callers use the repo path contract; update _annotations_root to call that paths helper, and if the helper does not exist add a paths.annotations_root(repo_root: Optional[Path]) -> Path that encapsulates the main_checkout_root() + ANNOTATIONS_DIRNAME logic (and keep the same signature for _annotations_root).src/bird_interact_agents/eval/tolerant_grader.py-1027-1039 (1)
1027-1039:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStrict misses can crash on multi-statement gold SQL.
_multi_sql_execute()explicitly supports multi-statement gold/variant lists, but_compute_miss_diagnostics()asserts them away. Any strict miss on a task that uses setup SQL will raise here instead of returning aCascadeVerdict.Keep grading alive here and just skip SQL-structure diagnostics for multi-statement inputs by leaving the parse-derived fields as
Noneand adding a dedicated miss pattern if needed, instead of asserting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bird_interact_agents/eval/tolerant_grader.py` around lines 1027 - 1039, The assert guards in _compute_miss_diagnostics crash on multi-statement gold/variant SQL even though _multi_sql_execute supports them; remove the assertions on variant_results' v_meta[].get("audited_sol_sql") and original_sol_sql and instead detect when any of those lists has length > 1, skip SQL-structure diagnostics for that variant by leaving parse-derived fields as None (and avoid calling any parsing/diagnostic helpers), and ensure the function returns the normal miss handling (e.g., a CascadeVerdict or the existing miss pattern) rather than raising; update logic around variant_results, v_meta/audited_sol_sql, and original_sol_sql in _compute_miss_diagnostics to short-circuit diagnostics when multi-statement is detected.src/bird_interact_agents/eval/tolerant_grader.py-277-285 (1)
277-285:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
compare_column_order()misgrades duplicate column names.
pred_l.index(c)always picks the first matching position, so a result like["id", "id"]maps both gold columns to the same source column. That makes N8 produce false positives/negatives on perfectly legal duplicate labels.Proposed fix
+from collections import defaultdict, deque + def compare_column_order( @@ - if set(pred_l) != set(gold_l): + if Counter(pred_l) != Counter(gold_l): return False - # Permutation: position in pred for each gold column. - perm = [pred_l.index(c) for c in gold_l] + positions: dict[str, deque[int]] = defaultdict(deque) + for i, name in enumerate(pred_l): + positions[name].append(i) + perm = [positions[name].popleft() for name in gold_l] aligned = [tuple(r[i] for i in perm) for r in pred] return _set_equal(aligned, gold)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bird_interact_agents/eval/tolerant_grader.py` around lines 277 - 285, The compare_column_order() logic mismaps duplicate column names because perm = [pred_l.index(c) for c in gold_l] always returns the first matching index; update it to assign for each gold column the next unused matching index from pred_l (e.g., iterate gold_l and for each value find a pred_l index equal to c that is not already used, track used indices), then build aligned = [tuple(r[i] for i in perm) for r in pred] as before; ensure variables referenced are pred_l, gold_l, perm, pred, aligned and handle the error case where no unused match exists.src/bird_interact_agents/eval/regrade.py-150-154 (1)
150-154:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
--force-llm-judgeis ignored for full-run regrades.When
--instance-idsis omitted,filter_setisNone, so this branch never clears anything. A full-run regrade will still reuse cached judge decisions, which defeats the flag's purpose.Proposed fix
- filter_set = set(instance_ids) if instance_ids else None - if force_llm_judge and filter_set: + filter_set = set(instance_ids) if instance_ids else None + if force_llm_judge: + ids_to_clear = filter_set + if ids_to_clear is None: + ids_to_clear = { + p.name for p in rows_dir.iterdir() if p.is_dir() + } clear_llm_judge_cache( cache_path=run_dir / "llm_judge_cache.json", - instance_ids=filter_set, + instance_ids=ids_to_clear, )🤖 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 branch only clears LLM judge cache when both force_llm_judge and filter_set are truthy, so full-run regrades (filter_set is None) don't clear cached decisions; change the logic in regrade.py so that when force_llm_judge is true you always call clear_llm_judge_cache: if filter_set is set call clear_llm_judge_cache(cache_path=run_dir / "llm_judge_cache.json", instance_ids=filter_set) as now, otherwise call clear_llm_judge_cache(cache_path=run_dir / "llm_judge_cache.json") (or pass an explicit sentinel/empty list if the clear function expects that) to clear the entire cache.src/bird_interact_agents/eval/regrade.py-164-167 (1)
164-167:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRegrade should use the latest saved attempt, not
attempt-1.json.This rewrites annotations from the first attempt even when the run later resubmitted the task. That can regress
eval_regraded.jsonand the overwritten per-run submission annotation back to stale data.Proposed fix
- attempt = sub / "attempt-1.json" - if not attempt.exists(): + attempts = sorted(sub.glob("attempt-*.json")) + if not attempts: report.skipped += 1 continue + attempt = attempts[-1]🤖 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 164 - 167, The code currently hardcodes attempt = sub / "attempt-1.json" which overwrites the first attempt; change it to pick the latest saved attempt file under sub (e.g., glob for "attempt-*.json" and select the one with the highest attempt number or newest mtime) before the exists check so you load the most recent submission attempt; update the variable used afterward (attempt) and keep the report.skipped behavior when no attempts are found.src/bird_interact_agents/cloud/driver.py-713-718 (1)
713-718:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRespect
BIRD_ANNOTATIONS_ROOTin the fetch merge path.This call re-bakes the destination to
<main_checkout>/annotations, so fetch ignores the new annotations-root override and writes merged files to the wrong tree whenever tests or forks redirect annotations elsewhere. Please pass the resolved annotations root itself through this API instead ofmain_checkout_root().As per coding guidelines, "All reachable-from-Python data paths must be resolved via
bird_interact_agents.paths.*_root()helpers."🤖 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/driver.py` around lines 713 - 718, The call to _post_run_merge.merge_submission_annotations is forcing the merge output into the main checkout by passing main_checkout_root=paths.main_checkout_root(), which ignores BIRD_ANNOTATIONS_ROOT overrides; change the argument to pass the resolved annotations root (use the paths helper for annotations root, e.g. paths.annotations_root()) so merge_submission_annotations receives the actual annotations tree, keeping downloaded_run_dir=dest, run_id, and benchmark as-is; update the call site where annotation_merge is assigned to use that annotations_root helper instead of main_checkout_root().src/bird_interact_agents/cloud/post_run_merge.py-533-537 (1)
533-537:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequire
instance_idbefore appending audited gold variants.The dedupe key is
(instance_id, variant_id), butinstance_idis not part of_REQUIRED_VARIANT_FIELDS. A malformed row with noinstance_idwill still be appended under the empty-string key, and later malformed rows will collide with it instead of being rejected.Suggested fix
-_REQUIRED_VARIANT_FIELDS = {"selected_database", "benchmark", "audit_status", "audited_sol_sql"} +_REQUIRED_VARIANT_FIELDS = { + "instance_id", + "selected_database", + "benchmark", + "audit_status", + "audited_sol_sql", +} @@ - key = (row.get("instance_id", ""), row.get("variant_id", "")) + key = (row["instance_id"], row.get("variant_id", "")) if key in existing_keys: report.skipped_duplicate += 1 continueAlso applies to: 629-642
🤖 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/post_run_merge.py` around lines 533 - 537, The dedupe key uses (instance_id, variant_id) but _REQUIRED_VARIANT_FIELDS lacks "instance_id", so rows missing instance_id get grouped under an empty key; add "instance_id" to _REQUIRED_VARIANT_FIELDS and update the audited-gold-append logic that builds the dedupe key (the block that appends audited gold variants using (instance_id, variant_id)) to explicitly validate presence/non-empty instance_id (and variant_id) and skip/log malformed rows instead of appending them.src/bird_interact_agents/cloud/post_run_merge.py-562-573 (1)
562-573:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSchema-validate task annotations before overwriting the stable file.
This path currently accepts any JSON payload with
selected_databaseandinstance_id, then overwrites<annotations_root>/.../<instance>.task.json. A partial/corrupt payload will therefore replace a previously valid task annotation and break downstream consumers that expect a fullTaskAnnotation.Suggested fix
def merge_task_annotations( @@ ) -> TaskAnnotationMergeReport: @@ bm = _normalise_benchmark(benchmark) report = TaskAnnotationMergeReport() rows_dir = downloaded_run_dir / "rows" if not rows_dir.exists(): return report + + from bird_interact_agents.eval.annotation_schema import TaskAnnotation + from pydantic import ValidationError for sub in sorted(p for p in rows_dir.iterdir() if p.is_dir()): src = sub / "task_annotation.json" if not src.exists(): continue try: - data = json.loads(src.read_text()) - db = data["selected_database"] - instance_id = data["instance_id"] - except (json.JSONDecodeError, KeyError) as e: + ann = TaskAnnotation.model_validate_json(src.read_text()) + except (ValidationError, ValueError) as e: report.errors += 1 report.error_details.append(f"{src}: {e}") continue - dest = annotations_root / bm / db / f"{instance_id}.task.json" + dest = annotations_root / bm / ann.selected_database / f"{ann.instance_id}.task.json" dest.parent.mkdir(parents=True, exist_ok=True) - dest.write_text(json.dumps(data, indent=2) + "\n") + dest.write_text(ann.model_dump_json(indent=2) + "\n") report.merged += 1🤖 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/post_run_merge.py` around lines 562 - 573, Validate the loaded JSON against the TaskAnnotation schema before writing to dest: after json.loads(src.read_text()) and before computing dest, run a schema/validation function (e.g., validate_task_annotation(data) or use the existing TaskAnnotation model) and if validation fails increment report.errors, append the validation error to report.error_details and continue without touching dest; only when validation passes should you create dest.parent and write the file (optionally write to a temp file and atomically replace to avoid partial writes).src/bird_interact_agents/cloud/ray_app.py-61-69 (1)
61-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve task annotations via
paths.annotations_root()instead ofrepo_root.Line 68 reintroduces a repo-root-based lookup for gitignored annotation data. That breaks the repository’s path contract and can point this worker at the wrong annotations tree in worktree/common-dir setups. Please build this path from the annotations root helper instead of threading
paths.main_checkout_root()throughtask_annotation_path().As per coding guidelines, "Never depend on a path inside
repo_rootfor gitignored input or output ... All reachable-from-Python data paths must be resolved viabird_interact_agents.paths.*_root()helpers."🤖 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 61 - 69, The task annotation path is being resolved using repo_root via paths.main_checkout_root(); update the call to task_annotation_path(...) to use paths.annotations_root() as the repo root for gitignored annotation data (i.e., replace passing repo_root=paths.main_checkout_root() with repo_root=paths.annotations_root() or remove repo_root and pass the annotations root where required) so that task_annotation_path, benchmark, selected_database, and instance_id resolve against the annotations root rather than the main checkout root.
🟡 Minor comments (8)
tests/test_regrade_cli.py-145-150 (1)
145-150:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the filter assertion identify which instance was regraded.
All three fixtures use the same
submitted_sql, solen(seen) == 1still passes ifregrade_runprocesses the wrong row. Give each instance a distinct marker (or captureinstance_idin the stub) and assert that onlyalien_2was graded.🤖 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_regrade_cli.py` around lines 145 - 150, The test currently only asserts len(seen) == 1 which doesn't guarantee the regraded row is alien_2; modify the test so the stub grader records which instance_id was graded (e.g., have StubGrader append the received instance_id or a unique marker per fixture to seen) or give each fixture a distinct submitted_sql value, then assert that the recorded value equals "alien_2" (or that seen contains the marker for alien_2 and no others) when calling regrade_run; update references to StubGrader, regrade_run, and seen accordingly to implement this stronger assertion.tests/test_paths.py-712-720 (1)
712-720:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the surrounding contract comment too.
This test now pins
audited_gold_file(benchmark="mini_interact")to a single-file path, but the header immediately above still says the same call must raise for mini-interact. Keeping both in one module makes the expected contract ambiguous for future callers.🤖 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_paths.py` around lines 712 - 720, The docstring above test_audited_gold_file_mini_interact_anchored_to_main is out of date and still describes a behavior where the call "must raise" for mini-interact; update that header comment to reflect the new contract: state that mini-interact is consolidated to a single file and that audited_gold_file(benchmark="mini_interact") is pinned to "audited_gold/mini_interact_audited.jsonl" (no raise expected), so the test's intent is unambiguous for future readers.tests/test_verdict_label_shared.py-48-98 (1)
48-98:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd N1/N2 verdict assertions to complete the regression matrix.
This suite says it pins each cascade-tier mapping, but it never exercises
n1_original_goldorn2_audited_primary. A divergence on either"correct"path would still pass here.➕ Minimal coverage addition
+def test_n1_only_yields_correct_on_both_paths(): + inline, annotate = _both_verdicts(_cascade(n1=True)) + assert inline == "correct" + assert annotate == "correct" + + +def test_n2_only_yields_correct_on_both_paths(): + inline, annotate = _both_verdicts(_cascade(n2=True)) + assert inline == "correct" + assert annotate == "correct" + + def test_n3_strict_yields_correct_on_both_paths(): inline, annotate = _both_verdicts(_cascade(n3=True)) assert inline == "correct" assert annotate == "correct"🤖 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_verdict_label_shared.py` around lines 48 - 98, Add two tests for the missing tiers to complete the matrix: create test_n1_only_yields_correct_on_both_paths and test_n2_only_yields_correct_on_both_paths that call inline, annotate = _both_verdicts(_cascade(n1=True)) and inline, annotate = _both_verdicts(_cascade(n2=True)) respectively, and assert both inline and annotate == "correct" (reusing the same pattern as the existing tests so they exercise n1_original_gold and n2_audited_primary paths via _cascade and _both_verdicts)..claude/skills/annotate-task-submission/SKILL.md-77-79 (1)
77-79:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign
primaryselection guidance with the shared multi-variant contract.Line 77 currently recommends a preference (
KB-anchored reading typically takes primary), but the shared contract saysprimaryis arbitrary and must not act as an authority tie-breaker. This contradiction can cause inconsistent audits.Suggested wording tweak
- with `primary=true`. The KB-anchored reading typically takes - primary; the snippet-anchored reading is the alternate. + with `primary=true`. Primary selection is bookkeeping-only and + can be arbitrary; do not use it to imply source authority.🤖 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 @.claude/skills/annotate-task-submission/SKILL.md around lines 77 - 79, Update the guidance about choosing `primary` so it matches the shared multi-variant contract: state that `primary` is arbitrary and must not be used as an authority tie-breaker rather than saying the KB-anchored reading "typically takes primary"; explicitly note both `KB-anchored` and `snippet-anchored` readings are valid alternatives and that each row's `reasoning_summary` must cite its source but not elevate the `primary` variant as authoritative.analyses/museum_failure_analysis_20260531t1013-claudes-slayer-48eb0f.md-102-102 (1)
102-102:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the task count in this cost summary.
Line 102 says "4 tasks" but lists five instances:
museum_2,museum_3,museum_4,museum_9, andmuseum_10. Update either the count or the list so the summary stays internally consistent.🤖 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 `@analyses/museum_failure_analysis_20260531t1013-claudes-slayer-48eb0f.md` at line 102, The cost-summary sentence is inconsistent: it claims "4 tasks" but enumerates five task IDs (museum_2, museum_3, museum_4, museum_9, museum_10); update the sentence in analyses/museum_failure_analysis_20260531t1013-claudes-slayer-48eb0f.md so the count matches the list (change "4 tasks" to "5 tasks") or remove one of the listed IDs so the text is consistent with the stated count.scripts/dev1515_strict_miss_diagnostics.py-11-11 (1)
11-11:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace the Unicode multiplication sign with ASCII
x.Ruff is already flagging
×in the doc/comment/output string here. If this file is linted, these warnings will keep the script noisy for no real gain.Also applies to: 125-129
🤖 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_strict_miss_diagnostics.py` at line 11, Replace the Unicode multiplication sign (×) with the ASCII letter "x" wherever it appears in this script, e.g. change the string "An instance × flag matrix" to "An instance x flag matrix" and update the other occurrences of "×" (notably the block around the later multi-line comment/output that includes the same symbol) so the file contains only ASCII "x" characters.tests/test_eval_annotation_schema.py-239-240 (1)
239-240:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRename
lin the indent check.Ruff is flagging the comprehension variable here as ambiguous (
E741). A clearer name likelinekeeps the test readable and clears the lint error.🤖 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 239 - 240, Rename the ambiguous comprehension variable `l` to a clearer name like `line` in both the list comprehension that defines indent_lines and the subsequent all(...) check; specifically update "indent_lines = [l for l in body_lines[1:-1] if l.strip()]" to use "line" and the assertion "assert all(l.startswith(\" \") for l in indent_lines)" to also use "line" so the variables are consistent and the Ruff E741 lint error is resolved.src/bird_interact_agents/eval/implicit_annotation.py-31-39 (1)
31-39:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCanonicalize benchmark aliases before deriving
task_jsonl_path.A valid alias like
"mini-interact"is not inbenchmark_names(), so this falls back tomini-interact.jsonlinstead of the realmini_interact.jsonl. That gives incorrect provenance for alias callers. Usingget_benchmark()first and only falling back onValueErrorkeeps the path canonical.💡 Suggested fix
def _benchmark_task_jsonl_name(benchmark: str) -> str: @@ - if benchmark in set(benchmark_names()): - return get_benchmark(benchmark).data_file + try: + return get_benchmark(benchmark).data_file + except ValueError: + pass 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` around lines 31 - 39, The helper _benchmark_task_jsonl_name currently checks membership against benchmark_names() which misses valid aliases (e.g., "mini-interact") and causes wrong fallback; instead call get_benchmark(benchmark) first and return its .data_file, and only if get_benchmark raises ValueError (or equivalent) fall back to returning f"{benchmark}.jsonl"; update _benchmark_task_jsonl_name to try get_benchmark(...) and handle the exception rather than using benchmark_names() membership.
…-annotations-tolerant-grader-post-dev-1478' into egor/dev-1518-annotator-agent
…-annotations-tolerant-grader-post-dev-1478' into egor/dev-1518-annotator-agent
…ts (keep HEAD multi-DB versions) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tions on merge Groups 1+2 from /process-reviews triage: - Move mid-file `from pydantic import ...` and function-level imports inside merge_submission_annotations to the top-level import block (CLAUDE.md compliance) - merge_task_annotations now validates via TaskAnnotation.model_validate(), matching the pattern already used by merge_submission_annotations Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…r VariantMatch + test field name fixes - _compute_miss_diagnostics: replace bare assert (removed by python -O) with explicit RuntimeError for single-statement contract guards - Tier-2 VariantMatch now uses _bag_relation (duplicate-preserving) instead of set-based classify_rowset_relation - Update test_miss_diagnostics to expect RuntimeError (not AssertionError) - Fix consulted_sources → evidence_sources_consulted in all annotator tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…mpty submitted_sql + collation cascading - cli.py: add `annotate` subcommand (benchmark, model, effort, instance-ids, workers, detach) - driver.py: add submit_annotator(), build_annotator_manifest(), _build_annotator_job_args(); wire merge_task_annotations + merge_audited_gold_variants into fetch() for annotator framework - ray_app_annotator.py: add proper main() + run_annotator_pool() + _build_annotator_actor_class() + _load_annotator_task_data(); replaces inline __main__ single-task stub - ray_app.py: fail fast with RuntimeError when submitted_sql is empty (redirects to write_failed_submission_annotation) - collation.py: use emit_cascading_eval_json in collate() when per-row submission_annotation.json files exist Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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/cloud/ray_app_annotator.py (1)
80-148:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat GCS I/O failures as per-task errors.
Any exception from the stable-blob skip check or the success-path writes currently skips
_write_attempt(). That breaks the module's "attempt-1.json for every outcome" contract and can leavewait_until_done()stuck on a missing attempt row while partial annotation blobs were already written.🤖 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_annotator.py` around lines 80 - 148, Wrap GCS operations so any exception becomes a per-task error and still calls _write_attempt; specifically, surround the stable-blob existence checks that call _gcs.blob_exists(ann_blob, client=client) and _gcs.blob_exists(var_blob, client=client) with a try/except that on exception builds an attempt_row (using run_id, instance_id, t0) with status "error", error=str(exc), duration_s and calls _write_attempt(..., client=client) then returns; likewise wrap the success-path writes (_gcs.write_task_annotation, _gcs.write_audited_gold_variants, _gcs.write_stable_task_annotation, _gcs.write_stable_audited_gold_variants) in a try/except that on any exception constructs the same attempt_row (use result.duration_s if available or time.monotonic()-t0) and calls _write_attempt(..., client=client) then returns so every outcome writes an attempt-1.json.
🤖 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/cloud/collation.py`:
- Around line 180-182: The except block writes metrics to eval_path before
recording the error, causing on-disk eval.json to differ from the returned
metrics; update the handler so metrics["cascading_phase1_error"] = str(exc) is
set before calling eval_path.write_text(json.dumps(metrics, indent=2,
default=str) + "\n") (referencing the metrics dict, eval_path variable and the
except handler in collation.py) so the file and returned metrics stay
consistent.
In `@src/bird_interact_agents/cloud/driver.py`:
- Around line 671-780: The resubmit path doesn't handle framework=="annotator"
so resubmit() will fail or launch the wrong entrypoint; update resubmit() to
detect manifest["framework"] == "annotator" and either (a) reconstruct
annotator-specific args using the same fields used in build_annotator_manifest
and _build_annotator_job_args (e.g., "run_id", "benchmark", "instance_ids",
"model"/agent_model, "benchmark_data_prefix") and submit the job with the
annotator ray app path (_ANNOTATOR_RAY_APP_PATH) and annotator
env_vars/query_mode, or (b) explicitly reject resubmits for annotator runs up
front by raising a clear error when manifest["framework"] == "annotator"; pick
one and implement consistently with submit_annotator's behavior
(image_uri/manifest layout, job args, and detach handling).
---
Outside diff comments:
In `@src/bird_interact_agents/cloud/ray_app_annotator.py`:
- Around line 80-148: Wrap GCS operations so any exception becomes a per-task
error and still calls _write_attempt; specifically, surround the stable-blob
existence checks that call _gcs.blob_exists(ann_blob, client=client) and
_gcs.blob_exists(var_blob, client=client) with a try/except that on exception
builds an attempt_row (using run_id, instance_id, t0) with status "error",
error=str(exc), duration_s and calls _write_attempt(..., client=client) then
returns; likewise wrap the success-path writes (_gcs.write_task_annotation,
_gcs.write_audited_gold_variants, _gcs.write_stable_task_annotation,
_gcs.write_stable_audited_gold_variants) in a try/except that on any exception
constructs the same attempt_row (use result.duration_s if available or
time.monotonic()-t0) and calls _write_attempt(..., client=client) then returns
so every outcome writes an attempt-1.json.
🪄 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: fa784f87-03fa-46b4-becd-fd8dced1ea8a
📒 Files selected for processing (5)
src/bird_interact_agents/cloud/cli.pysrc/bird_interact_agents/cloud/collation.pysrc/bird_interact_agents/cloud/driver.pysrc/bird_interact_agents/cloud/ray_app.pysrc/bird_interact_agents/cloud/ray_app_annotator.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/bird_interact_agents/cloud/ray_app.py
Group 1 — bugs in new annotator code:
* collation.py: set cascading_phase1_error key before write_text so the
error lands in the on-disk eval.json (Thread 1)
* agent.py: pass model= to ClaudeAgentOptions so the annotator uses the
requested model instead of the SDK default (Codex)
* ray_app_annotator.py: wrap 4 success-path GCS writes in try/except;
failure now records an error attempt instead of silently dropping it
* driver.py: add agent_model + effort to annotator manifest; add
_build_annotator_resubmit_args; branch resubmit() to use the annotator
entrypoint + args for framework=="annotator" (Thread 2)
* post_run_merge.py: add instance_id to _REQUIRED_VARIANT_FIELDS so
malformed variants are rejected instead of deduping under ("","")
Group 2 — pre-existing bugs surfaced by review:
* prompts.py: guard sol_sql with isinstance(list) so a string value
is not iterated char-by-char
* regrade.py: replace hardcoded attempt-1.json with sort-by-number
scan matching collation._discover_canonical_rows
* tolerant_grader.py: replace pred_l.index(c) with a per-name
position-queue so duplicate column names get distinct permutation
indices
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/bird_interact_agents/cloud/ray_app_annotator.py (1)
49-57: 💤 Low valueConsider using
asyncio.run()instead ofget_event_loop().run_until_complete().
asyncio.get_event_loop()emits aDeprecationWarningin Python 3.10+ when called outside a running event loop.asyncio.run()is the recommended replacement for synchronous async bridging. However, if there's a specific need to reuse an existing event loop in the Ray actor context, this pattern is acceptable.♻️ Suggested fix
- return asyncio.get_event_loop().run_until_complete( - ann_agent.run_task( - task_data=task_data, - data_path_base=data_path_base, - benchmark=cfg["benchmark"], - model=cfg.get("model", "anthropic/claude-opus-4-7"), - effort=cfg.get("effort", "medium"), - ) - ) + return asyncio.run( + ann_agent.run_task( + task_data=task_data, + data_path_base=data_path_base, + benchmark=cfg["benchmark"], + model=cfg.get("model", "anthropic/claude-opus-4-7"), + effort=cfg.get("effort", "medium"), + ) + )🤖 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_annotator.py` around lines 49 - 57, The code uses asyncio.get_event_loop().run_until_complete(...) which triggers DeprecationWarning on modern Python; replace that call with asyncio.run(ann_agent.run_task(...)) to bridge sync/async correctly—specifically change the return in ray_app_annotator to return asyncio.run(ann_agent.run_task(task_data=task_data, data_path_base=data_path_base, benchmark=cfg["benchmark"], model=cfg.get("model", "anthropic/claude-opus-4-7"), effort=cfg.get("effort", "medium"))) unless you explicitly need to reuse an existing loop in this Ray actor, in which case obtain or create a running loop safely and document the reason.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/bird_interact_agents/cloud/ray_app_annotator.py`:
- Around line 49-57: The code uses
asyncio.get_event_loop().run_until_complete(...) which triggers
DeprecationWarning on modern Python; replace that call with
asyncio.run(ann_agent.run_task(...)) to bridge sync/async correctly—specifically
change the return in ray_app_annotator to return
asyncio.run(ann_agent.run_task(task_data=task_data,
data_path_base=data_path_base, benchmark=cfg["benchmark"],
model=cfg.get("model", "anthropic/claude-opus-4-7"), effort=cfg.get("effort",
"medium"))) unless you explicitly need to reuse an existing loop in this Ray
actor, in which case obtain or create a running loop safely and document the
reason.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 59af0884-7c5d-47bd-9f74-0529e75e9a32
📒 Files selected for processing (8)
src/bird_interact_agents/agents/annotator/agent.pysrc/bird_interact_agents/agents/annotator/prompts.pysrc/bird_interact_agents/cloud/collation.pysrc/bird_interact_agents/cloud/driver.pysrc/bird_interact_agents/cloud/post_run_merge.pysrc/bird_interact_agents/cloud/ray_app_annotator.pysrc/bird_interact_agents/eval/regrade.pysrc/bird_interact_agents/eval/tolerant_grader.py
🚧 Files skipped from review as they are similar to previous changes (5)
- src/bird_interact_agents/agents/annotator/prompts.py
- src/bird_interact_agents/cloud/collation.py
- src/bird_interact_agents/cloud/post_run_merge.py
- src/bird_interact_agents/eval/regrade.py
- src/bird_interact_agents/eval/tolerant_grader.py
Group 1 — annotator critical: * agent.py: strip provider prefix with native_model_id() before passing to ClaudeAgentOptions.model (fixes anthropic/ prefix rejection) * prereqs.py: _is_claude_sdk_framework returns True for "annotator" so CLAUDE_CODE_OAUTH_TOKEN path is entered for annotator runs * ray_app_annotator.py: wrap blob_exists skip-check in try/except so a transient GCS error doesn't silently abort the task * post_run_merge.py + driver.py: add annotations_root param to merge_submission_annotations; driver passes paths.annotations_root() so BIRD_ANNOTATIONS_ROOT env override is honoured Group 2 — pre-existing production bugs: * grade_in_place.py: detect latest attempt-N.json dynamically (was hardcoded to attempt-1.json) via new _latest_attempt_rel helper * annotate.py: canonicalize benchmark via get_benchmark().name in main() * annotate.py: expose --run-id/--submission-mode args in CLI so submission skeletons can be generated from the command line * image.py: data_hash defaults annotations_root to paths.annotations_root() so callers that omit the arg still include annotations content in hash Group 3 — pre-existing script/test bugs: * consolidate_mini_interact_audited.py: derive primary from variant_id instead of hardcoding True * dev1515_convert_runs.py + dev1515_convert_livesqlbench.py: use actual variant_id from audit row instead of hardcoding "primary" * dev1515_households_14_multivariant.py: bare except → sqlite3.Error * verify_audited_gold.py: fall through to per-DB sidecar when consolidated file has no rows for the DB * dev1515_remap_failure_classes.py: mini_interact (underscore) not mini-interact (dash) * test_livesqlbench_audited_gold.py: assert all expected DBs present * test_regrade_cli.py: strengthen filter assertion (check report fields) * test_verdict_label_shared.py: add N1 and N2 coverage * test_paths.py: drop stale comment about "moved in DEV-1515" * test_eval_annotation_schema.py: l → line (shadowed builtin) * SKILL.md: clarify primary=True and variant_id="primary" go together Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_verdict_label_shared.py (1)
48-65: 💤 Low valueConsider more explicit test names (optional).
The test names
test_n1_original_gold_yields_correct_on_both_pathsandtest_n2_audited_primary_yields_correct_on_both_pathsmight initially suggest testing N1 or N2 in isolation. While the inline comments correctly explain that the monotone cascade forces all downstream tiers to True, readers scanning test names might not immediately grasp this. Consider names liketest_monotone_cascade_n1_through_n9_yields_corrector keeping the current names with a more prominent module-level docstring about monotone cascade testing.That said, the current naming is defensible since you're testing the highest tier (N1 or N2) and the comments are clear.
🤖 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_verdict_label_shared.py` around lines 48 - 65, Rename or clarify the two tests to make the monotone cascade intent explicit: either rename test_n1_original_gold_yields_correct_on_both_paths to something like test_monotone_cascade_n1_through_n9_yields_correct and test_n2_audited_primary_yields_correct_on_both_paths to test_monotone_cascade_n2_through_n9_yields_correct, or add a brief module-level docstring at the top of tests/test_verdict_label_shared.py describing that the grader enforces a monotone cascade (e.g., N1=True implies N2..N9=True) so readers immediately understand why downstream tiers are set to True in _cascade; update any references or test discovery expectations if you rename the functions.src/bird_interact_agents/cloud/ray_app_annotator.py (1)
82-94: ⚡ Quick winConsider catching more specific GCS exceptions.
The skip-check catches broad
Exceptionto ensure transient GCS errors don't abort tasks. While the defensive behavior is appropriate, catching specific GCS-related exceptions (e.g.,google.cloud.exceptions.GoogleCloudErrororrequests.exceptions.RequestException) would better distinguish between expected transient failures and unexpected programming errors.♻️ Proposed refinement
try: ann_blob = _gcs.stable_task_annotation_blob(benchmark, db, instance_id) var_blob = _gcs.stable_audited_gold_variants_blob(benchmark, db, instance_id) both_exist = ( _gcs.blob_exists(ann_blob, client=client) and _gcs.blob_exists(var_blob, client=client) ) - except Exception as exc: + except (OSError, RuntimeError) as exc: + # Covers GCS client errors, network issues, and transient failures logger.warning( "[%s] skip-check failed (%s); proceeding with annotation", instance_id, exc, ) both_exist = False🤖 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_annotator.py` around lines 82 - 94, Replace the broad "except Exception" around the _gcs blob existence checks with targeted catches for GCS/network related errors (e.g., google.cloud.exceptions.GoogleCloudError and requests.exceptions.RequestException) and keep the same logging/behavior (set both_exist = False) for those cases; import the chosen exception classes at the top of the module, and if you still want to guard against truly unexpected errors add a final generic except that re-raises the exception so programming errors aren't silently swallowed.
🤖 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/annotate.py`:
- Around line 417-425: The closure _grader captures loop variables ann, audited,
row, and db_path by reference which can lead to late-binding bugs; fix it by
binding those values into the closure signature as default parameters (e.g., def
_grader(*, instance_id: str, submitted_sql: str, task_row: dict, ann=ann,
audited=audited, row=row, db_path=db_path, **_kw):) and then pass those bound
values into grade_submission (use row.get("sol_sql") via the bound row); this
ensures _grader uses the current-iteration values rather than the final loop
values.
---
Nitpick comments:
In `@src/bird_interact_agents/cloud/ray_app_annotator.py`:
- Around line 82-94: Replace the broad "except Exception" around the _gcs blob
existence checks with targeted catches for GCS/network related errors (e.g.,
google.cloud.exceptions.GoogleCloudError and
requests.exceptions.RequestException) and keep the same logging/behavior (set
both_exist = False) for those cases; import the chosen exception classes at the
top of the module, and if you still want to guard against truly unexpected
errors add a final generic except that re-raises the exception so programming
errors aren't silently swallowed.
In `@tests/test_verdict_label_shared.py`:
- Around line 48-65: Rename or clarify the two tests to make the monotone
cascade intent explicit: either rename
test_n1_original_gold_yields_correct_on_both_paths to something like
test_monotone_cascade_n1_through_n9_yields_correct and
test_n2_audited_primary_yields_correct_on_both_paths to
test_monotone_cascade_n2_through_n9_yields_correct, or add a brief module-level
docstring at the top of tests/test_verdict_label_shared.py describing that the
grader enforces a monotone cascade (e.g., N1=True implies N2..N9=True) so
readers immediately understand why downstream tiers are set to True in _cascade;
update any references or test discovery expectations if you rename the
functions.
🪄 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: 93a24198-e887-487b-9dc4-9a1af8aae2f7
📒 Files selected for processing (20)
.claude/skills/annotate-task-submission/SKILL.mdscripts/consolidate_mini_interact_audited.pyscripts/dev1515_convert_livesqlbench.pyscripts/dev1515_convert_runs.pyscripts/dev1515_households_14_multivariant.pyscripts/dev1515_remap_failure_classes.pyscripts/verify_audited_gold.pysrc/bird_interact_agents/agents/annotator/agent.pysrc/bird_interact_agents/cloud/driver.pysrc/bird_interact_agents/cloud/image.pysrc/bird_interact_agents/cloud/post_run_merge.pysrc/bird_interact_agents/cloud/prereqs.pysrc/bird_interact_agents/cloud/ray_app_annotator.pysrc/bird_interact_agents/eval/annotate.pysrc/bird_interact_agents/eval/grade_in_place.pytests/test_eval_annotation_schema.pytests/test_livesqlbench_audited_gold.pytests/test_paths.pytests/test_regrade_cli.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)
- src/bird_interact_agents/cloud/image.py
- tests/test_paths.py
- scripts/consolidate_mini_interact_audited.py
- scripts/dev1515_convert_runs.py
- tests/test_regrade_cli.py
- scripts/dev1515_households_14_multivariant.py
- src/bird_interact_agents/cloud/driver.py
- scripts/dev1515_convert_livesqlbench.py
- src/bird_interact_agents/cloud/post_run_merge.py
- src/bird_interact_agents/eval/grade_in_place.py
- scripts/dev1515_remap_failure_classes.py
- tests/test_livesqlbench_audited_gold.py
…ula, paths helper, per-DB sidecars, annotated_at pre-construction - annotate.py: bind loop vars as default params in _grader closure (B023) - ray_app_annotator.py: use asyncio.run() (avoids DeprecationWarning in Py3.10+) - image.py: move data_hash docstring before the if block so it is the first statement - cascading_report.py: simplify N5 formula to novel_reading_judgment=="pass" only (enforce_monotone_cascade propagates the rest; N3/N4 conditions were semantically incorrect) - consolidate_mini_interact_audited.py: replace Path(__file__) root with paths.audited_gold_root() per CLAUDE.md contract - dev1515_convert_runs.py: read per-DB *_audited.jsonl sidecars instead of consolidated file (more robust on a normal checkout) - regrade.py: compute annotated_at before constructing SubmissionAnnotation so it is never persisted with an empty string on any exception path; add datetime import at top Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… inline graders - agent.py: materialize_task_db was called with db_name (str) instead of task_data (dict), crashing on any non-mini_interact benchmark with AttributeError - run.py + ray_app.py: inline graders reconstructed <db>/<db>.sqlite ignoring task_data["db_file_path"] set by materialize_task_db; prefer the materialized per-task path with fallback to the standard reconstructed path Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…_order, metadata_evidence character-split - regrade.py: _build_original_sql_index now handles string sol_sql (wraps in list) so N1 is not falsely missed for benchmarks that store gold as a bare string - prompts.py: annotator prompt now renders string sol_sql instead of showing (none) - tolerant_grader.py: compare_tie_order catches IndexError when submitted rows have fewer projected columns than the ORDER BY index — returns False instead of crashing - annotate.py: _masked_terms_from normalizes string metadata_evidence to a single-element list instead of splitting it into characters via list(str) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
_build_annotator_resubmit_args reads manifest.get("override") but
build_annotator_manifest never stored it, silently dropping --override
on every resubmit.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ray_app_annotator: _load_annotator_task_data now auto-detects the gold sidecar for gold-required benchmarks (env var → default path), matching the regrade.py pattern, so LiveSQLBench annotator jobs don't crash on task load. driver: submit_annotator passed user_sim_model=agent_model to both prereqs.check and read_api_keys_from_local_env; in the OAuth path this incorrectly required ANTHROPIC_API_KEY for the non-existent user-sim. Pass "" instead — annotator runs have no user simulator. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Without variant_id in _REQUIRED_VARIANT_FIELDS, two variants missing the field both land on dedup key (instance_id, "") and the second is silently dropped. Reject at merge time instead. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ath fixes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Without per-element validation, a missing field like variant_id caused the stable GCS blobs to be written but then rejected at merge time, leaving the task marked done while the consolidated JSONL stayed empty. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ark alias fix * submit_annotation rejects annotations whose instance_id or selected_database don't match the current task context (Group 1 / CR). * ray_app_annotator: on skip, copy stable blobs to run-scoped row paths so fetch can download annotation data for skipped tasks (Group 2 / CR). * _benchmark_task_jsonl_name: use get_benchmark() with ValueError fallback instead of benchmark_names() membership check, fixing alias resolution for "mini-interact" → "mini_interact.jsonl" (Group 3 / Codex). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…h coverage assertion Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Group 1 (critical — cloud actor runtime crashes): - ray_app_annotator: add benchmark= kwarg to _run_with_actors call - ray_app_annotator: call download_benchmark_data in AnnotatorActor.__init__ - ray_app_annotator: add dataset + benchmark_data_prefix to cfg dict in main() Group 2 (major — cross-task variant contamination): - agent.submit_annotation: cross-validate each variant's instance_id and selected_database against the task context Group 3: delete three already-run migration scripts - scripts/consolidate_mini_interact_audited.py (output exists) - scripts/dev1515_convert_runs.py (annotations exist) - scripts/dev1515_convert_livesqlbench.py (annotations exist) Group 4 (schema enforcement): - annotation_schema.TaskAnnotation: add @model_validator enforcing ≤1 primary variant and empty gold_variants when original_gold_is_correct Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Production fixes: - driver.py resubmit: pass "" (not agent_model) as user_sim_model for annotator framework — OAuth path was requiring ANTHROPIC_API_KEY for a user-sim that doesn't exist (Codex finding) - agent.py submit_annotation: validate that every gold_variants entry's audited_gold_ref.variant_id has a matching row in audited_gold_variants_json, so original_gold_is_correct=False submissions with dangling refs are rejected before GCS write (Codex finding) Tests: one new test in test_driver.py; two new tests in test_annotator_tools.py Cosmetic / documentation: - test_eval_annotation_schema.py: rename `l` → `line` (Ruff E741) - scripts/dev1515_strict_miss_diagnostics.py: × → x (3 occurrences) - analyses/museum_failure_analysis…md: "4 tasks" → "5 tasks" (5 listed) - skills/annotate-task-submission/SKILL.md: primary=true is bookkeeping-only, not an authority signal (aligns with multi-variant contract) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
generate_submission_annotation previously hardcoded attempt-1.json, reading stale data when a resubmit produced attempt-2. Now uses _latest_attempt_file() from regrade (no circular import — regrade's imports from annotate are all local/inside-function). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The triple-quoted string at lines 270-274 appeared after the _check_gold_invariants validator's `return self`, making it a dead expression statement rather than a docstring for internal_inconsistency. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- regrade: guard _latest_attempt_file against non-existent directory (returns None instead of raising FileNotFoundError on iterdir) - grade_in_place: remove dead _latest_attempt_rel (unused since annotate.py now imports _latest_attempt_file from regrade) - image: remove default annotations_root injection that broke test hermeticity; all production callers already pass it explicitly - annotation_schema: restore field-level comment for internal_inconsistency (explains coupling to gold_variants; was accidentally dropped as dead code) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…mode
The new --run-id submission-skeleton path resolved DB as {db}.sqlite but
LiveSQLBench uses {db}_template.sqlite. Extract _resolve_db_sqlite_path()
helper (mirrors regrade.py's existing inline fallback) and add 3 unit tests.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- annotate CLI: skip instances whose attempt directory is missing (--run-id path crashed with FileNotFoundError mid-loop; now reports count of skipped instances instead) - test: document that zero-primary gold_variants is valid (grader uses alphabetical tiebreak); update test name for clarity Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…erms_from
Codex r7: the `isinstance(me, list) else [me]` branch in _masked_terms_from
had no test coverage for the string case ('KB 3' → ['KB 3']).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Satisfies the CLAUDE.md rule that all imports belong at the top of the file. Four locations fixed: * agent.py: `build_system_prompt` moved out of `run_task()`. * driver.py: `argparse` moved out of `submit_annotator()`. * ray_app_annotator.py: `argparse`, `get_benchmark`, paths, harness, and `ray_app` helpers moved out of `_load_annotator_task_data()`, `run_annotator_pool()`, and `main()`. Redundant inner `download_benchmark_data` import (already at module top) removed. * test_annotator_ray_app.py: monkeypatches updated from `ray_app.*` to `ray_app_annotator.*` now that the symbols are bound at import time rather than re-imported per call. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…d audited_gold_variants submit_annotation() now rejects submissions where a gold_variants entry is marked primary=True but the corresponding audited_gold_variants row has primary=False. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…sing gold _write_attempt() now takes an explicit attempt parameter instead of hardcoding attempt-1.json, so resubmits write to the correct blob. run_annotator_pool, _run_one_task, and AnnotatorActor all propagate the attempt number; the CLI gains --attempt (default 1). _load_annotator_task_data raises early with a clear message when gold_required=True but no gold sidecar is found, instead of passing None through to the in-cluster worker. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…s and resubmit args Four _write_attempt calls in _run_one_task (agent-raised, result.error, GCS-write-fail, success) were missing attempt=attempt, so retries always wrote attempt-1.json. Also wire next_attempt through _build_annotator_resubmit_args (signature + --attempt flag + call site) to match the non-annotator resubmit path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… submit_annotation Skip-path: copy failure now writes status='error' (not 'skipped') so resubmit retries the copy instead of treating the task as complete with missing run-scoped blobs. submit_annotation: validate each audited_gold_variants row's 'benchmark' field against the expected benchmark from context, matching the existing instance_id and selected_database checks. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…bmit_annotation A string audited_sol_sql passes the presence check but causes tolerant_grader's list(...) to iterate characters; an unknown audit_status is silently ignored by the harness. Both are now rejected at submit time with actionable error messages. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
_load_annotator_task_data gains a gold_file param that short-circuits env/path lookup when provided. main() gains --gold-file CLI arg and passes it through. _build_annotator_resubmit_args emits --gold-file when manifest carries one, matching _build_resubmit_args's behaviour for LiveSQLBench resubmits. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…erge
- build_annotator_manifest now stores the in-cluster gold_file path
- _build_annotator_job_args emits --gold-file when gold_file is set
- merge_audited_gold_variants gains override=True upsert mode (replaces
existing rows by (instance_id, variant_id) key, rewriting the whole file)
- Thread override=manifest.get("override", False) at the fetch call site
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- ClaudeAgentOptions now sets tools=[] and setting_sources=[] to match all other Claude SDK agents and prevent built-in/settings bleed-through - annotate subparser exposes --gold-file so users can pass an explicit gold sidecar path for LiveSQLBench without relying on env-var fallback - JSONL append path guards against a missing trailing newline on the existing consolidated file to prevent row concatenation corruption Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- _submit_benchmark() now falls back to args.benchmark when args.dataset is absent, so _in_cluster_gold_file() returns the correct container path (/data/livesqlbench/...) for annotate runs instead of /data/mini-interact/ - submit_annotator() now calls _validate_gold_under_data_root() before prereqs/build/upload, matching the guard in submit() and failing fast when --gold-file is outside the benchmark data root Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- merge_audited_gold_variants(override=True) now purges all existing rows for every instance_id present in the run directory before upserting new rows; previously an empty incoming file (e.g. re-annotated as original_gold_is_correct=True) would leave stale variants in place - annotation_schema._check_gold_invariants now rejects TaskAnnotation with non-empty gold_variants and zero primary=True entries; previously this silently caused N2=False for all submissions against that task Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ride mode When override=True and all existing rows belong to the re-run instance (i.e. the consolidated file had no unrelated rows), existing_ordered ended up empty after the purge step but the `if existing_ordered:` guard skipped the write, leaving stale content on disk. Fix: add an `elif consolidated.exists()` branch that truncates the file. Add test_merge_audited_gold_variants_override_purges_all_rows_truncates_file to cover the all-stale-all-purged scenario. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…s_correct=True A stray audited-gold row combined with original_gold_is_correct=True would silently poison the consolidated JSONL and allow future submissions to pass N2/N3 against a task annotated as using the original gold. Now submit_annotation returns a validation error in that case. Add test_submit_annotation_original_gold_correct_with_variants_rejected. Fix two pre-existing tests that incorrectly passed original_gold_is_correct=True with non-empty variants; updated them to use original_gold_is_correct=False with proper gold_variants references. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
post_run_merge: limit override purge to dirs with audited_gold_variants.jsonl so failed annotator tasks (attempt-N.json only) don't lose their prior consolidated rows. agent: reject audited_sol_sql=[] for non-unrecoverable audit statuses — an empty-SQL variant is silently treated as non-existent by the grader. agent: reject more than one primary=True in audited_gold_variants_json — multiple primaries produce unstable N2 grading depending on merged-file order. Add tests for all three behaviors. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…d database field driver: move json to top-level import; rewrite eval.json after annotator merge reports are added so they're durable on disk (collate() wrote eval.json before these keys existed). cli: print task_annotation_merge and audited_gold_merge error counts + details after fetch, matching how OTF merge failures are surfaced. ray_app_annotator: add database=db to every attempt_row so collation can populate the database column for filtering/debugging, consistent with normal cloud runs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Conflicts resolved: implicit_annotation.py: adopt origin/main's if/benchmark_names() pattern (avoids try/except); add canonical dash→underscore normalization so the "mini-interact" alias still resolves to "mini_interact.jsonl" (our branch's test_implicit_task_annotation_alias_benchmark_uses_canonical_jsonl_name). Also pull in benchmark_names import added on main. tests/cloud/test_driver.py: keep both sides' new tests (DEV-1518 gold-file plumbing tests from our branch + DEV-1523 BIRD_PG_* forwarding tests from main). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This was referenced Jun 4, 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
agents/annotator/) — Claude Agent SDK loop withget_ambiguity_resolutions(mini-interact) andsubmit_annotationtools; producesTaskAnnotation+ audited gold variants per taskcloud/ray_app_annotator.py) — dual-blob skip check (both stable GCS blobs required), writesattempt-1.jsonfor every outcome (annotated / skipped / error), dual GCS write on successtask_annotation_blob,audited_gold_variants_blob,stable_*path helpers,write_*functions,blob_existsmerge_task_annotations(always-overwrites, benchmark dash↔underscore normalisation) andmerge_audited_gold_variants(dedup by(instance_id, variant_id), rejects entries missing required fields)cluster.submit_job— newray_app_pathkwarg so annotator can point atray_app_annotator.pylivesqlbench_audited.jsonlfix — museum_2/4/9 hadprimary=Falsewith no primary partner; fixed toprimary=True; updated_load_audit_rows()to treat single-row instances as primary regardless of the field valueTest plan
pytest tests/test_annotator_tools.py— 10 tests forget_ambiguity_resolutionsandsubmit_annotationtoolspytest tests/test_annotator_agent.py— 10 tests forrun_task(happy path, turn cap, retry, benchmark tool sets, sentinel fill, non-Anthropic model)pytest tests/cloud/test_annotator_gcs.py— 14 tests for GCS path helpers and write/read functionspytest tests/cloud/test_annotator_merge.py— 13 tests formerge_task_annotationsandmerge_audited_gold_variantspytest tests/cloud/test_annotator_ray_app.py— 12 tests for_run_one_taskskip/success/error logic andsubmit_jobkwarg🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Infrastructure
Tests