DEV-1523: postgres benchmark support via DbConnection abstraction - #20
Merged
ZmeiGorynych merged 26 commits intoJun 3, 2026
Merged
Conversation
Add DbConnection protocol with SqliteDbConnection / PostgresDbConnection
implementations; make_db_connection is the single factory that branches on
benchmark.db_backend. Extend benchmark.py with LIVESQLBENCH_POSTGRES and
MINI_INTERACT_POSTGRES; add db_backend / per_task_db_isolation fields.
Harness: dispatch execute_env_action / execute_submit_action to postgres
path for postgres benchmarks; materialize_task_db uses per_task_db_isolation.
tolerant_grader: make_executor factory, _multi_sql_execute benchmark kwarg,
grade_and_write auto-wires postgres executor.
_submit.py: _dry_run_sql and capture_result_snapshot accept benchmark kwarg
and route through DbConnection for postgres; _dry_run_error_message is now
backend-agnostic ("DB error" not "SQLite error").
orchestrator: _slayer_ingest extracted as patchable helper; _phase1_ingest
accepts db_url; phases 3/4 skip for postgres. cache.py: fingerprint_of hashes
schema text for postgres; ensure_db_cache skips sqlite check; _build_async
constructs postgres db_url from env vars.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
More reviews will be available in 28 minutes and 21 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughAdds a tolerant SQL grader, Postgres DB backend and benchmark variants, a DB connection abstraction with Postgres/SQLite executors, inline grading orchestration and snapshots, harness/agent/run/orchestrator cache wiring for Postgres, and extensive test coverage. ChangesDEV-1515 complete stack
Sequence Diagram(s)(Skipped — changes are broad and aggregated across many components; no single 3+ component sequential flow exclusively captures the entire PR.) Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.claude/skills/_shared/audit-gold-sql.contract.md (1)
11-33: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd
variant_idandprimaryfields to the schema definition.The multi-variant mechanics (lines 130-136) introduce
variant_idandprimaryfields, but these are not documented in the top-level JSON schema. Downstream parsers and validators need these fields defined in the schema.📋 Proposed schema addition
{ "instance_id": "<task instance_id>", + "variant_id": "<optional: kebab-case slug when multi-variant, e.g. 'labeled_snippet'>", + "primary": <optional: true for exactly one variant when multi-variant, false for others>, "selected_database": "<task db>", "benchmark": "<mini_interact | livesqlbench>",🤖 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/_shared/audit-gold-sql.contract.md around lines 11 - 33, The top-level JSON schema in .claude/skills/_shared/audit-gold-sql.contract.md is missing the multi-variant fields introduced later; add "variant_id" (string) and "primary" (boolean) to the top-level object definition so downstream parsers can validate them. Update the schema block that defines keys like "instance_id", "selected_database", "benchmark", etc., to include "variant_id" with a short description (e.g., variant identifier) and "primary" with a description (e.g., true for the canonical variant), and ensure these fields are allowed alongside "original_sol_sql"/"audited_sol_sql" to match the multi-variant mechanics mentioned in the later lines.src/bird_interact_agents/agents/_submit.py (1)
238-246:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical:
capture_result_snapshotcall sites missingbenchmarkparameter.The
capture_result_snapshotfunction now accepts abenchmarkparameter (line 83) to route postgres tasks through theDbConnectionabstraction. However, both call sites in_diagnostic_payload(lines 238-246) omit this parameter:predicted = ( capture_result_snapshot( submitted_sql, db_name, data_path_base, db_file_path=db_file_path, ) if not skip_snapshots else None ) gold = ( capture_result_snapshot( sol_sql, db_name, data_path_base, db_file_path=db_file_path, ) if not skip_snapshots else None )When
benchmarkisNone, the function's postgres conditional (line 96:if getattr(benchmark, "db_backend", "sqlite") == "postgres") will always evaluate toFalse, causing all snapshots—even for postgres tasks—to fall through to the SQLite path. This will fail for postgres benchmarks or produce incorrect diagnostics.🐛 Proposed fix to pass benchmark parameter
_diagnostic_payloadshould derive the benchmark fromsample_status.original_data["dataset"](mirroring the pattern insubmit_raw_sqllines 480-486 andsubmit_slayer_querylines 740-746), then pass it to both snapshot calls:def _diagnostic_payload( *, submitted_sql: str | None, sample_status: Any, data_path_base: str, observation: str | None, p1: bool, p2: bool, json_failed: bool = False, translation_failed: bool = False, dry_run_failed: bool = False, infrastructure_failed: bool = False, phase1_observation_audited: str | None = None, phase1_observation_original: str | None = None, ) -> dict[str, Any]: """...""" db_name = sample_status.original_data["selected_database"] sol_sql = _first_sql(sample_status.original_data.get("sol_sql")) pre_phase = getattr(sample_status, "current_phase", 1) db_file_path = sample_status.original_data.get("db_file_path") + + # Resolve benchmark for postgres routing in capture_result_snapshot + _dataset = sample_status.original_data.get("dataset", "") + _benchmark = None + if _dataset: + try: + _benchmark = _get_benchmark(_dataset) + except ValueError: + pass skip_snapshots = json_failed or translation_failed predicted = ( capture_result_snapshot( submitted_sql, db_name, data_path_base, db_file_path=db_file_path, + benchmark=_benchmark, ) if not skip_snapshots else None ) gold = ( capture_result_snapshot( sol_sql, db_name, data_path_base, db_file_path=db_file_path, + benchmark=_benchmark, ) if not skip_snapshots else None )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bird_interact_agents/agents/_submit.py` around lines 238 - 246, _diagnostic_payload is calling capture_result_snapshot for predicted and gold snapshots but omits the new benchmark parameter, causing postgres tasks to be treated as sqlite; derive the benchmark the same way other submitters do (read dataset from sample_status.original_data["dataset"] and obtain the benchmark object) and pass that benchmark into both capture_result_snapshot calls (the two invocations in _diagnostic_payload) so the function can route postgres work through the DbConnection abstraction.
🧹 Nitpick comments (12)
src/bird_interact_agents/cloud/post_run_merge.py (1)
560-564: 💤 Low valueConsider making the audit-report write best-effort.
The OTF
merge_report.jsonwrite at Line 410-415 intentionally swallowsOSErrorso a logging-side failure can't poison an already-completed merge. This audit write isn't guarded, so a write failure raises after destinations were already mutated.♻️ Optional: mirror the best-effort write
- audit_path = downloaded_run_dir / "annotation_merge_report.json" - audit_path.write_text(report.model_dump_json(indent=2) + "\n") - return report + audit_path = downloaded_run_dir / "annotation_merge_report.json" + try: + audit_path.write_text(report.model_dump_json(indent=2) + "\n") + except OSError: + pass + return report🤖 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 560 - 564, The audit write using audit_path.write_text(report.model_dump_json(indent=2) + "\n") should be best-effort like the OTF merge_report.json write: wrap that call in a try/except that catches OSError (and optionally Exception) and logs a warning instead of letting the exception propagate so downstream state stays consistent; use the existing logging facility in this module (e.g., logger) to emit a clear message including the target path and error, then continue and return report.tests/test_tolerant_grader_postgres.py (2)
183-183: 💤 Low valueUse underscore prefix for unused unpacked variable.
The
colsvariable is unpacked but never used in the assertion. Per Python convention, prefix it with_to indicate it's intentionally unused.♻️ Suggested fix
- rows, cols = _multi_sql_execute( + rows, _cols = _multi_sql_execute(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_tolerant_grader_postgres.py` at line 183, The unpacked variable "cols" returned from _multi_sql_execute is unused; update the unpacking to use a leading underscore (e.g., "_cols" or simply "_") to follow Python convention for intentionally unused variables. Locate the call to _multi_sql_execute (the assignment "rows, cols = _multi_sql_execute(...)") and change "cols" to "_cols" (or "_") so linters/readers know it's unused, leaving "rows" intact and not changing any other logic.
156-156: 💤 Low valueUse underscore prefix for unused unpacked variable.
The
colsvariable is unpacked but never used in the assertion. Per Python convention, prefix it with_to indicate it's intentionally unused.♻️ Suggested fix
- rows, cols = _multi_sql_execute( + rows, _cols = _multi_sql_execute(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_tolerant_grader_postgres.py` at line 156, The test unpacks "rows, cols = _multi_sql_execute(...)" but never uses cols; rename the unused unpacked variable to "_cols" (or simply "_" ) to follow Python convention and signal it's intentionally unused. Locate the unpack in the test function where _multi_sql_execute is called and update "cols" to "_cols" so linter/readers know the value is ignored.tests/test_eval_annotation_schema.py (1)
239-240: 💤 Low valueAvoid ambiguous single-letter variable name
l.The variable name
l(lowercase L) is easily confused with1(digit one) orI(uppercase i). Use a more descriptive name likeline.♻️ Suggested fix
- indent_lines = [l for l in body_lines[1:-1] if l.strip()] - assert all(l.startswith(" ") for l in indent_lines) + indent_lines = [line for line in body_lines[1:-1] if line.strip()] + assert all(line.startswith(" ") for line in indent_lines)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_eval_annotation_schema.py` around lines 239 - 240, Replace the ambiguous single-letter variable `l` with a descriptive name `line` in the list comprehension and assertion: change "indent_lines = [l for l in body_lines[1:-1] if l.strip()]" to use "line" and update the assertion "assert all(l.startswith(\" \") for l in indent_lines)" to "assert all(line.startswith(\" \") for line in indent_lines)" so the variable name is clear and consistent.tests/test_local_run_cascading.py (2)
79-85: 💤 Low valueSimilar cleanup opportunity in FakePass and FakeFail.
The
FakePassexecutor can also drop the unused noqa and be simplified to a direct return, whileFakeFailcorrectly uses the conditional to distinguish predicted vs gold.♻️ Optional simplification for FakePass
class FakePass: - def __call__(self, sql, *, db_path, conn): # noqa: ARG002,ARG005 # noqa: ARG002 + def __call__(self, sql, *, db_path, conn): # noqa: ARG002 return ([(1,)], ["a"])🤖 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_local_run_cascading.py` around lines 79 - 85, The FakePass and FakeFail executor implementations can be cleaned up: in FakePass remove the redundant noqa comments from its __call__ signature and simplify the body to directly return ([(1,)], ["a"]) without extra annotations; leave FakeFail's __call__ as-is but ensure its conditional remains to return ([(99,)], ["a"]) when "predicted" is in sql and ([(1,)], ["a"]) otherwise, and also remove any duplicated noqa tokens from its signature if present.
31-32: 💤 Low valueClean up noqa comments and simplify the ternary.
The noqa comment has two issues:
- It appears twice (
# noqa: ARG002,ARG005 # noqa: ARG002)- ARG005 is for unused lambda arguments, not method arguments
Additionally, both branches of the ternary return identical values, making the conditional useless (Ruff RUF034). Since the test simulates a pass scenario where both SQLs produce the same result, you can simplify to a direct return.
♻️ Proposed cleanup
- class FakeExecutor: - def __call__(self, sql, *, db_path, conn): # noqa: ARG002,ARG005 # noqa: ARG002 - return ([(1,)], ["a"]) if sql == submitted else ([(1,)], ["a"]) + class FakeExecutor: + def __call__(self, sql, *, db_path, conn): # noqa: ARG002 + return ([(1,)], ["a"])🤖 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_local_run_cascading.py` around lines 31 - 32, The __call__ method's noqa comment should be cleaned up and the redundant ternary simplified: remove the duplicated "# noqa" and drop ARG005 (it's not applicable to a method), leaving a single appropriate noqa if needed; then replace the conditional return (which returns the same value for both branches) with a direct unconditional return of ([(1,)], ["a"]) in the __call__ implementation (the variable submitted is not needed for the result).src/bird_interact_agents/eval/implicit_annotation.py (1)
37-37: 💤 Low valueMicro-optimization: avoid redundant
set()conversion.Calling
set(benchmark_names())on every invocation creates a new set each time. Ifbenchmark_names()returns a small collection, checking membership directly (benchmark in benchmark_names()) would be simpler and equally fast for the typical 2-3 benchmark case.♻️ Optional simplification
- if benchmark in set(benchmark_names()): + if benchmark in benchmark_names(): return get_benchmark(benchmark).data_file🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bird_interact_agents/eval/implicit_annotation.py` at line 37, The membership check currently wraps benchmark_names() in set(...) causing an unnecessary allocation each call; change the condition to test membership directly against the result of benchmark_names() (i.e., remove the set(...) wrapper) so the code reads a direct "benchmark in benchmark_names()" using the existing benchmark variable and the benchmark_names() function.scripts/consolidate_mini_interact_audited.py (1)
23-27: ⚡ Quick winUse
paths.audited_gold_root()instead of computing the path relative to the script file.As per coding guidelines, all data paths must be resolved via
bird_interact_agents.paths.*_root()helpers. This ensures consistency with how the rest of the codebase locates data directories.🔧 Proposed fix
-ROOT = Path(__file__).resolve().parents[1] -AUDITED = ROOT / "audited_gold" +from bird_interact_agents import paths + +AUDITED = paths.audited_gold_root() -MINI_INTERACT_OUT = AUDITED / "mini_interact_audited.jsonl" -LIVESQLBENCH = AUDITED / "livesqlbench_audited.jsonl" +MINI_INTERACT_OUT = AUDITED / "mini_interact_audited.jsonl" +LIVESQLBENCH = AUDITED / "livesqlbench_audited.jsonl"As per coding guidelines:
**/*.pyfiles must resolve data paths 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 manual path construction (ROOT, AUDITED, MINI_INTERACT_OUT, LIVESQLBENCH) with the audited-root helper: call bird_interact_agents.paths.audited_gold_root() to obtain the audited_gold directory and then derive MINI_INTERACT_OUT and LIVESQLBENCH from that returned Path; update any references to ROOT/AUDITED to use the helper-returned Path and remove the relative Path(__file__).resolve().parents[1] logic.src/bird_interact_agents/cloud/driver.py (1)
725-755: ⚡ Quick winConsider logging the FileNotFoundError details.
When
_cascading_report.emit_cascading_eval_jsonraisesFileNotFoundError(line 750), the error is stored inmetrics["cascading_phase1_error"]but not logged. For diagnosability, consider adding alogger.warning(...)before storing the error string so operators can see the issue in logs without inspecting the metrics dict.📋 Add logging for cascade aggregation errors
except FileNotFoundError as exc: # Aggregator is strict — a missing per-row file raises so we # never silently under-count. Surface it as a side-channel entry # and leave the in-memory metrics writeable. + logger.warning( + "[fetch] cascading_phase1 aggregation failed for run %s: %s", + dest.name, exc, + ) metrics["cascading_phase1_error"] = str(exc) 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/driver.py` around lines 725 - 755, In _emit_cascading_phase1_on_fetch, when calling _cascading_report.emit_cascading_eval_json catch FileNotFoundError and log the exception details before updating metrics["cascading_phase1_error"]; specifically, add a logger.warning or logger.exception call (including exc info and context about rows_dir/eval_path) in the except FileNotFoundError block so the FileNotFoundError raised by _cascading_report.emit_cascading_eval_json is visible in logs as well as stored in metrics.src/bird_interact_agents/cloud/ray_app.py (1)
592-668: ⚡ Quick winVerify
_grader_data_diris always bound when needed.Line 592 uses
locals().get("data_dir")to retrieve the data_dir that was bound at line 543. However, if the try block at line 536 raises an exception before reaching line 543,data_dirwill not exist in locals, and_grader_data_dirwill beNone. The guard at line 614 correctly handles this by raising a RuntimeError, but the variable naming is confusing.Consider binding
_grader_data_dir = Nonebefore the outer try block (line 534), then assigning_grader_data_dir = data_dirafter line 543 within the try. This makes the scoping explicit and avoids relying onlocals().♻️ Clearer scoping
log_dir = Path(tempfile.mkdtemp(prefix="cloud_log_")) log_tmp = log_dir / "task.log" task_start_ts = time.time() +_grader_data_dir = None try: with fd_capture(log_tmp): try: # `cfg["data_dir"]` is the benchmark's container_data_dir... data_dir = cfg.get("data_dir") or "/data/mini-interact" + _grader_data_dir = data_dir row = asyncio.run( _run_one_task_async( ... ) ) except Exception as e: # noqa: BLE001 row = _build_error_row(iid, database, str(e)) finally: pass # DEV-1515: inline grader produces a SubmissionAnnotation per task -_grader_data_dir = locals().get("data_dir")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bird_interact_agents/cloud/ray_app.py` around lines 592 - 668, The code relies on locals().get("data_dir") to set _grader_data_dir which can be None if an earlier exception prevents binding; initialize _grader_data_dir = None before the outer try, then after you assign data_dir (the binding at/after where data_dir is created) set _grader_data_dir = data_dir, and replace the locals().get("data_dir") call with direct use of _grader_data_dir in the grader block (the call sites include the _grade_one_submission db_path construction and the RuntimeError guard that checks _grader_data_dir); this makes scoping explicit and avoids relying on locals().scripts/dev1515_strict_miss_diagnostics.py (1)
32-40: ⚡ Quick winClarify/remove the redundant
mini-interactdirectory inROOTS["mini_interact"].
tests/test_paths_annotations.pyassertsmini-interact(dash) andmini_interact(underscore) are canonicalized to the same on-disk directory underannotations/(the underscore form, i.e.annotations/mini_interact/...). If the script can assume annotations were written via the path helpers, thepaths.annotations_root() / "mini-interact"entry is redundant; if you need to support legacy already-writtenannotations/mini-interact/..., keep it but document that it’s a backward-compat fallback.🤖 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` around lines 32 - 40, The ROOTS mapping for "mini_interact" contains both paths.annotations_root() / "mini-interact" and paths.annotations_root() / "mini_interact", which is redundant if annotations are always created via the path helpers; remove the dashed entry (paths.annotations_root() / "mini-interact") from ROOTS["mini_interact"] to keep only paths.annotations_root() / "mini_interact", or if you must support legacy on-disk names keep the dashed entry but add a clear comment documenting it as a backward-compatibility fallback; update the ROOTS definition and accompanying comment near the ROOTS constant and ensure code that consumes ROOTS relies on paths.annotations_root() canonicalization.src/bird_interact_agents/agents/_submit.py (1)
114-114: 💤 Low valueConsider adding
strict=Truetozip()calls for safer column/type pairing.The
zip()calls building column metadata assumecol_namesandtypes(ortypes_) are always the same length. Addingstrict=Truemakes this assumption explicit and will raiseValueErrorif the lengths mismatch, catching potential logic errors.♻️ Optional refinement
return { "columns": [ - {"name": n, "type": t} for n, t in zip(col_names, types) + {"name": n, "type": t} for n, t in zip(col_names, types, strict=True) ], ...(Similarly for line 155 in the SQLite path.)
Also applies to: 155-155
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bird_interact_agents/agents/_submit.py` at line 114, The list-comprehension that builds column metadata uses zip(col_names, types) which silently truncates on length mismatch; change it to zip(col_names, types, strict=True) so a ValueError is raised for length mismatches and prevents silent mispairing, and apply the same change to the other zip usage that pairs col_names with types_ in the SQLite path; update the comprehensions that reference col_names, types and col_names, types_ respectively to include strict=True.
🤖 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/db_connection.py`:
- Around line 118-141: The execute() method currently runs
cur.execute("ROLLBACK") in both the except and finally blocks causing a
redundant rollback; remove the rollback logic from the except clause and keep
the single guarded rollback in the finally block (i.e., when self._read_only is
true call cur.execute("ROLLBACK") inside the finally only), preserving
re-raising the psycopg2.Error and the existing try/except around the finally
rollback to avoid swallowing errors; reference symbols: execute,
self._read_only, cur.execute("ROLLBACK").
In `@src/bird_interact_agents/harness.py`:
- Around line 173-179: The current comparison in harness.py iterates sol_sqls
and uses set(map(tuple, ...)) which drops duplicate rows; change the comparison
to use multisets so row multiplicity is preserved (e.g., import Counter from
collections and compare Counter(map(tuple, pred_rows)) == Counter(map(tuple,
gold_rows))). Update the block that calls _run for each gold_sql and sets p1 so
it uses Counter-based equality between pred_rows and gold_rows.
In `@src/bird_interact_agents/run.py`:
- Around line 1070-1094: The grader currently always builds a sqlite path
(per_task_db) and passes it to grade_one_submission, which breaks tasks whose
td["db_backend"] == "postgres"; change the logic around per_task_db to be
backend-aware: inspect task_data/td for db_backend (and preferably
db_connection_uri/db_conn) and if db_backend == "postgres" pass the Postgres
connection URI (td["db_connection_uri"] or an env-provided BIRD_DB_PATH) into
grade_one_submission instead of the sqlite path; otherwise keep the existing
Path(data_dir)/selected_database/... .sqlite construction. Ensure you update the
call-site to pass the connection string/URI (or an explicit db_backend param) so
grade_one_submission can open the correct backend.
In `@src/bird_interact_agents/slayer_otf/cache.py`:
- Around line 329-355: The validation misses Postgres schema files causing
fingerprint_of(db_name=db, data_root=effective_root, benchmark=benchmark) to
later fail; update the ensure_db_cache logic so that when is_postgres is True
you construct schema_path = effective_root / db / f"{db}_schema.txt" (or the
exact schema filename used by fingerprint_of) and append (schema_path, "schema")
to required_files before the existence loop, so the FileNotFoundError is raised
during validation rather than inside fingerprint_of; alternatively, if you
prefer resilience in fingerprint_of, change fingerprint_of to skip hashing the
schema file if it does not exist, but the preferred change is to add schema_path
to required_files in the block that checks is_postgres.
---
Outside diff comments:
In @.claude/skills/_shared/audit-gold-sql.contract.md:
- Around line 11-33: The top-level JSON schema in
.claude/skills/_shared/audit-gold-sql.contract.md is missing the multi-variant
fields introduced later; add "variant_id" (string) and "primary" (boolean) to
the top-level object definition so downstream parsers can validate them. Update
the schema block that defines keys like "instance_id", "selected_database",
"benchmark", etc., to include "variant_id" with a short description (e.g.,
variant identifier) and "primary" with a description (e.g., true for the
canonical variant), and ensure these fields are allowed alongside
"original_sol_sql"/"audited_sol_sql" to match the multi-variant mechanics
mentioned in the later lines.
In `@src/bird_interact_agents/agents/_submit.py`:
- Around line 238-246: _diagnostic_payload is calling capture_result_snapshot
for predicted and gold snapshots but omits the new benchmark parameter, causing
postgres tasks to be treated as sqlite; derive the benchmark the same way other
submitters do (read dataset from sample_status.original_data["dataset"] and
obtain the benchmark object) and pass that benchmark into both
capture_result_snapshot calls (the two invocations in _diagnostic_payload) so
the function can route postgres work through the DbConnection abstraction.
---
Nitpick comments:
In `@scripts/consolidate_mini_interact_audited.py`:
- Around line 23-27: Replace the manual path construction (ROOT, AUDITED,
MINI_INTERACT_OUT, LIVESQLBENCH) with the audited-root helper: call
bird_interact_agents.paths.audited_gold_root() to obtain the audited_gold
directory and then derive MINI_INTERACT_OUT and LIVESQLBENCH from that returned
Path; update any references to ROOT/AUDITED to use the helper-returned Path and
remove the relative Path(__file__).resolve().parents[1] logic.
In `@scripts/dev1515_strict_miss_diagnostics.py`:
- Around line 32-40: The ROOTS mapping for "mini_interact" contains both
paths.annotations_root() / "mini-interact" and paths.annotations_root() /
"mini_interact", which is redundant if annotations are always created via the
path helpers; remove the dashed entry (paths.annotations_root() /
"mini-interact") from ROOTS["mini_interact"] to keep only
paths.annotations_root() / "mini_interact", or if you must support legacy
on-disk names keep the dashed entry but add a clear comment documenting it as a
backward-compatibility fallback; update the ROOTS definition and accompanying
comment near the ROOTS constant and ensure code that consumes ROOTS relies on
paths.annotations_root() canonicalization.
In `@src/bird_interact_agents/agents/_submit.py`:
- Line 114: The list-comprehension that builds column metadata uses
zip(col_names, types) which silently truncates on length mismatch; change it to
zip(col_names, types, strict=True) so a ValueError is raised for length
mismatches and prevents silent mispairing, and apply the same change to the
other zip usage that pairs col_names with types_ in the SQLite path; update the
comprehensions that reference col_names, types and col_names, types_
respectively to include strict=True.
In `@src/bird_interact_agents/cloud/driver.py`:
- Around line 725-755: In _emit_cascading_phase1_on_fetch, when calling
_cascading_report.emit_cascading_eval_json catch FileNotFoundError and log the
exception details before updating metrics["cascading_phase1_error"];
specifically, add a logger.warning or logger.exception call (including exc info
and context about rows_dir/eval_path) in the except FileNotFoundError block so
the FileNotFoundError raised by _cascading_report.emit_cascading_eval_json is
visible in logs as well as stored in metrics.
In `@src/bird_interact_agents/cloud/post_run_merge.py`:
- Around line 560-564: The audit write using
audit_path.write_text(report.model_dump_json(indent=2) + "\n") should be
best-effort like the OTF merge_report.json write: wrap that call in a try/except
that catches OSError (and optionally Exception) and logs a warning instead of
letting the exception propagate so downstream state stays consistent; use the
existing logging facility in this module (e.g., logger) to emit a clear message
including the target path and error, then continue and return report.
In `@src/bird_interact_agents/cloud/ray_app.py`:
- Around line 592-668: The code relies on locals().get("data_dir") to set
_grader_data_dir which can be None if an earlier exception prevents binding;
initialize _grader_data_dir = None before the outer try, then after you assign
data_dir (the binding at/after where data_dir is created) set _grader_data_dir =
data_dir, and replace the locals().get("data_dir") call with direct use of
_grader_data_dir in the grader block (the call sites include the
_grade_one_submission db_path construction and the RuntimeError guard that
checks _grader_data_dir); this makes scoping explicit and avoids relying on
locals().
In `@src/bird_interact_agents/eval/implicit_annotation.py`:
- Line 37: The membership check currently wraps benchmark_names() in set(...)
causing an unnecessary allocation each call; change the condition to test
membership directly against the result of benchmark_names() (i.e., remove the
set(...) wrapper) so the code reads a direct "benchmark in benchmark_names()"
using the existing benchmark variable and the benchmark_names() function.
In `@tests/test_eval_annotation_schema.py`:
- Around line 239-240: Replace the ambiguous single-letter variable `l` with a
descriptive name `line` in the list comprehension and assertion: change
"indent_lines = [l for l in body_lines[1:-1] if l.strip()]" to use "line" and
update the assertion "assert all(l.startswith(\" \") for l in indent_lines)" to
"assert all(line.startswith(\" \") for line in indent_lines)" so the variable
name is clear and consistent.
In `@tests/test_local_run_cascading.py`:
- Around line 79-85: The FakePass and FakeFail executor implementations can be
cleaned up: in FakePass remove the redundant noqa comments from its __call__
signature and simplify the body to directly return ([(1,)], ["a"]) without extra
annotations; leave FakeFail's __call__ as-is but ensure its conditional remains
to return ([(99,)], ["a"]) when "predicted" is in sql and ([(1,)], ["a"])
otherwise, and also remove any duplicated noqa tokens from its signature if
present.
- Around line 31-32: The __call__ method's noqa comment should be cleaned up and
the redundant ternary simplified: remove the duplicated "# noqa" and drop ARG005
(it's not applicable to a method), leaving a single appropriate noqa if needed;
then replace the conditional return (which returns the same value for both
branches) with a direct unconditional return of ([(1,)], ["a"]) in the __call__
implementation (the variable submitted is not needed for the result).
In `@tests/test_tolerant_grader_postgres.py`:
- Line 183: The unpacked variable "cols" returned from _multi_sql_execute is
unused; update the unpacking to use a leading underscore (e.g., "_cols" or
simply "_") to follow Python convention for intentionally unused variables.
Locate the call to _multi_sql_execute (the assignment "rows, cols =
_multi_sql_execute(...)") and change "cols" to "_cols" (or "_") so
linters/readers know it's unused, leaving "rows" intact and not changing any
other logic.
- Line 156: The test unpacks "rows, cols = _multi_sql_execute(...)" but never
uses cols; rename the unused unpacked variable to "_cols" (or simply "_" ) to
follow Python convention and signal it's intentionally unused. Locate the unpack
in the test function where _multi_sql_execute is called and update "cols" to
"_cols" so linter/readers know the value is ignored.
🪄 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: 8f3b1e48-3ef1-408d-96a8-05fae58b970c
📒 Files selected for processing (106)
.claude/skills/_shared/audit-gold-sql.contract.md.claude/skills/annotate-task-submission/SKILL.md.claude/skills/audit-gold-sql-livesqlbench/SKILL.md.claude/skills/audit-gold-sql/SKILL.md.gitignoreDockerfile.cloudREADME.mdanalyses/households_failure_analysis_20260531t1008-claudes-slayer-890419.mdanalyses/museum_failure_analysis_20260531t1013-claudes-slayer-48eb0f.mdanalyses/raw/cross_cutting_observations.mdanalyses/raw/households_10.mdanalyses/raw/households_12.mdanalyses/raw/households_15.mdanalyses/raw/households_2.mdanalyses/raw/museum_10.mdanalyses/raw/museum_2.mdanalyses/raw/museum_3.mdanalyses/raw/museum_4.mdanalyses/raw/museum_5.mdanalyses/raw/museum_9.mdscripts/consolidate_mini_interact_audited.pyscripts/dev1515_cascade_summary.pyscripts/dev1515_convert_livesqlbench.pyscripts/dev1515_convert_runs.pyscripts/dev1515_households_14_multivariant.pyscripts/dev1515_reclassify_sufficiency.pyscripts/dev1515_remap_failure_classes.pyscripts/dev1515_strict_miss_diagnostics.pyscripts/generate_annotation_skeletons.pyscripts/verify_audited_gold.pysrc/bird_interact_agents/agents/_submit.pysrc/bird_interact_agents/agents/agno/agent.pysrc/bird_interact_agents/agents/claude_sdk/agent.pysrc/bird_interact_agents/agents/claude_sdk_otf/agent.pysrc/bird_interact_agents/agents/claude_sdk_otf/prompts.pysrc/bird_interact_agents/agents/claude_sdk_otf_ainteract/agent.pysrc/bird_interact_agents/agents/claude_sdk_otf_ainteract/prompts.pysrc/bird_interact_agents/agents/mcp_agent/agent.pysrc/bird_interact_agents/agents/pydantic_ai/agent.pysrc/bird_interact_agents/agents/pydantic_ai_otf_encode/agent.pysrc/bird_interact_agents/agents/pydantic_ai_recursive/agent.pysrc/bird_interact_agents/agents/smolagents/agent.pysrc/bird_interact_agents/benchmark.pysrc/bird_interact_agents/cloud/_audited_gold_check.pysrc/bird_interact_agents/cloud/cli.pysrc/bird_interact_agents/cloud/collation.pysrc/bird_interact_agents/cloud/driver.pysrc/bird_interact_agents/cloud/gcs.pysrc/bird_interact_agents/cloud/image.pysrc/bird_interact_agents/cloud/post_run_merge.pysrc/bird_interact_agents/cloud/ray_app.pysrc/bird_interact_agents/db_connection.pysrc/bird_interact_agents/eval/__init__.pysrc/bird_interact_agents/eval/annotate.pysrc/bird_interact_agents/eval/annotation_io.pysrc/bird_interact_agents/eval/annotation_schema.pysrc/bird_interact_agents/eval/cascading_report.pysrc/bird_interact_agents/eval/grade_in_place.pysrc/bird_interact_agents/eval/implicit_annotation.pysrc/bird_interact_agents/eval/regrade.pysrc/bird_interact_agents/eval/tolerant_grader.pysrc/bird_interact_agents/harness.pysrc/bird_interact_agents/paths.pysrc/bird_interact_agents/results_db.pysrc/bird_interact_agents/run.pysrc/bird_interact_agents/slayer_otf/cache.pysrc/bird_interact_agents/slayer_pipeline/orchestrator.pytests/cloud/test_cli.pytests/cloud/test_collation.pytests/cloud/test_fetch_annotation_merge.pytests/cloud/test_fetch_cascading_phase1.pytests/cloud/test_image_annotations.pytests/cloud/test_inline_grader.pytests/slayer_pipeline/test_orchestrator_conn.pytests/test_benchmark.pytests/test_benchmark_postgres.pytests/test_cascading_report.pytests/test_claude_sdk_otf_agent.pytests/test_claude_sdk_otf_ainteract_agent.pytests/test_claude_sdk_usage.pytests/test_db_connection.pytests/test_dry_run_gate.pytests/test_dual_eval.pytests/test_eval_annotate_cli.pytests/test_eval_annotation_schema.pytests/test_harness_postgres_dispatch.pytests/test_implicit_task_annotation.pytests/test_legacy_field_removal.pytests/test_livesqlbench_audited_gold.pytests/test_local_run_cascading.pytests/test_miss_diagnostics.pytests/test_normalize_sol_sql.pytests/test_otf_postgres.pytests/test_paths.pytests/test_paths_annotations.pytests/test_regrade_cli.pytests/test_results_db.pytests/test_run_local_inline_grader.pytests/test_schema_extension.pytests/test_slayer_otf_cache.pytests/test_submit_postgres.pytests/test_tolerant_grader_comparators.pytests/test_tolerant_grader_multi_sql_conn.pytests/test_tolerant_grader_orchestration.pytests/test_tolerant_grader_postgres.pytests/test_verdict_label_shared.py
💤 Files with no reviewable changes (10)
- src/bird_interact_agents/agents/pydantic_ai_recursive/agent.py
- src/bird_interact_agents/agents/pydantic_ai_otf_encode/agent.py
- src/bird_interact_agents/agents/mcp_agent/agent.py
- src/bird_interact_agents/agents/agno/agent.py
- src/bird_interact_agents/agents/pydantic_ai/agent.py
- src/bird_interact_agents/agents/smolagents/agent.py
- src/bird_interact_agents/agents/claude_sdk/agent.py
- src/bird_interact_agents/agents/claude_sdk_otf/agent.py
- tests/test_claude_sdk_usage.py
- tests/test_claude_sdk_otf_agent.py
…-annotations-tolerant-grader-post-dev-1478' into egor/dev-1523-support-postgres-based-benchmarks
- db_connection: remove redundant ROLLBACK in except block (finally covers it) - harness: normalize str sol_sqls to list; use Counter for row comparison - run: postgres inline grader uses Path(db_name) not a .sqlite path - cache: add schema_path to required_files for postgres benchmarks - _submit: thread benchmark through _diagnostic_payload → capture_result_snapshot; initialize _benchmark=None before _record closure to fix NameError on early exits - tolerant_grader: _multi_sql_execute opens shared postgres conn for multi-stmt SQL; _pg_executor reuses provided PostgresDbConnection instead of opening a fresh one - regrade: postgres-aware db_path + wire make_executor for postgres benchmarks - tests: update spy signatures + mock _open_psycopg2_connection in shared-conn test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Critical: - PostgresDbConnection.execute: guard fetchall() for non-SELECT (description=None) - grade_submission: add benchmark= param, thread to _multi_sql_execute for shared-conn logic - _multi_sql_execute: open shared postgres conn with read_only=False so TEMP TABLE state persists - grade_in_place / regrade: forward benchmark= to grade_submission Minor: - cache.py: URL-escape postgres credentials in connection string - ray_app.py: bind _grader_data_dir=None before try, assign inside (drop locals().get()) - post_run_merge.py: best-effort OSError on annotation audit write - driver.py: log FileNotFoundError before storing in metrics - _submit.py: strict=True on zip(col_names, types) - implicit_annotation.py: drop redundant set() wrapper on benchmark_names() - consolidate_mini_interact_audited.py: use paths.audited_gold_root() - dev1515_strict_miss_diagnostics.py: remove redundant mini_interact underscore root - tests: fix duplicate noqa, rename l→line, add regression tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/bird_interact_agents/agents/_submit.py (1)
96-124:⚠️ Potential issue | 🟠 MajorFix Postgres snapshot bounding — truncation happens after
fetchall()loads all rowsIn
src/bird_interact_agents/agents/_submit.py, the Postgres snapshot truncatesrowsonly afterconn.execute(sql)returns, butsrc/bird_interact_agents/db_connection.pyimplementsPostgresDbConnection.execute()usingcur.fetchall(), so_SNAPSHOT_MAX_ROWSdoesn’t prevent loading the full result set into memory/latency. The SQLite path is bounded viacur.fetchmany(_SNAPSHOT_MAX_ROWS + 1). Fix by bounding Postgres reads (e.g., cursorfetchmany()/streaming) or rewriting the query with an outerLIMITfor snapshotting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bird_interact_agents/agents/_submit.py` around lines 96 - 124, The Postgres branch currently calls conn.execute(sql) which (via PostgresDbConnection.execute) uses cur.fetchall() and loads the entire result set before truncation; change the Postgres path in the block that checks getattr(benchmark, "db_backend", "sqlite") == "postgres" so that you bound the returned rows at source — either (preferred) modify the call to use a fetchmany/streaming API on the Postgres cursor (invoke a new method on PostgresDbConnection that uses cur.fetchmany(_SNAPSHOT_MAX_ROWS + 1)) or rewrite the SQL passed to conn.execute to append a LIMIT _SNAPSHOT_MAX_ROWS+1 for snapshotting; ensure the subsequent logic still sets truncated, builds sample using _SNAPSHOT_SAMPLE_SIZE, and returns the same keys ("columns", "row_count", "row_count_truncated", "sample_rows").src/bird_interact_agents/cloud/ray_app.py (1)
617-626:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the inline grader's
db_pathbackend-aware.Line 623 still hardcodes a SQLite file path. For
livesqlbench_postgres, that path will never exist, so the inline grader drops into the exception path and uploads a failed annotation for every task even when execution succeeded. Because the attempt row is still written afterward, cloud fetch will publish incorrectcascading_phase1results for Postgres runs.Proposed fix
+ from bird_interact_agents.benchmark import get_benchmark + + bench = get_benchmark(_cloud_benchmark(cfg)) + grader_db_path = ( + Path(str(_row_selected_db)) + if getattr(bench, "db_backend", "sqlite") == "postgres" + else Path(_grader_data_dir) / str(_row_selected_db) / f"{_row_selected_db}.sqlite" + ) + ann_path = _grade_one_submission( task_data=task_data, submitted_sql=str(_row_submitted_sql), rows_dir=annotation_dir, run_id=run_id, - benchmark=_cloud_benchmark(cfg), - db_path=Path(_grader_data_dir) - / str(_row_selected_db) - / f"{_row_selected_db}.sqlite", + benchmark=bench.name, + db_path=grader_db_path, cost_usd_agent=row.get("usage", {}).get("cost_usd_agent") if isinstance(row.get("usage"), dict) else None,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bird_interact_agents/cloud/ray_app.py` around lines 617 - 626, The inline grader call to _grade_one_submission currently passes a hardcoded SQLite file via the db_path parameter (built from _grader_data_dir and _row_selected_db), which breaks for Postgres backends; change the code around the _grade_one_submission invocation so db_path is backend-aware: query the backend via _cloud_benchmark(cfg) (or inspect cfg) and when the benchmark is "livesqlbench_postgres" (or the Postgres sentinel) supply the appropriate Postgres connection info (or None/a backend token) instead of a SQLite file, otherwise keep the existing Path-based SQLite DB; ensure the receiving function _grade_one_submission (and any callers) handle the alternate Postgres value correctly.
🤖 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/regrade.py`:
- Around line 326-329: get and use the canonical benchmark id from
get_benchmark() instead of the raw CLI token: after _bench =
get_benchmark(args.benchmark) assign args.benchmark to the canonical identifier
on _bench (e.g. _bench.name or _bench.canonical_name) so downstream callers like
regrade_run() and the code paths around lines where _is_postgres/_executor are
constructed receive the normalized benchmark name; apply the same change to the
other occurrence noted (around lines 422-425) so annotation/task paths use the
canonical form.
---
Outside diff comments:
In `@src/bird_interact_agents/agents/_submit.py`:
- Around line 96-124: The Postgres branch currently calls conn.execute(sql)
which (via PostgresDbConnection.execute) uses cur.fetchall() and loads the
entire result set before truncation; change the Postgres path in the block that
checks getattr(benchmark, "db_backend", "sqlite") == "postgres" so that you
bound the returned rows at source — either (preferred) modify the call to use a
fetchmany/streaming API on the Postgres cursor (invoke a new method on
PostgresDbConnection that uses cur.fetchmany(_SNAPSHOT_MAX_ROWS + 1)) or rewrite
the SQL passed to conn.execute to append a LIMIT _SNAPSHOT_MAX_ROWS+1 for
snapshotting; ensure the subsequent logic still sets truncated, builds sample
using _SNAPSHOT_SAMPLE_SIZE, and returns the same keys ("columns", "row_count",
"row_count_truncated", "sample_rows").
In `@src/bird_interact_agents/cloud/ray_app.py`:
- Around line 617-626: The inline grader call to _grade_one_submission currently
passes a hardcoded SQLite file via the db_path parameter (built from
_grader_data_dir and _row_selected_db), which breaks for Postgres backends;
change the code around the _grade_one_submission invocation so db_path is
backend-aware: query the backend via _cloud_benchmark(cfg) (or inspect cfg) and
when the benchmark is "livesqlbench_postgres" (or the Postgres sentinel) supply
the appropriate Postgres connection info (or None/a backend token) instead of a
SQLite file, otherwise keep the existing Path-based SQLite DB; ensure the
receiving function _grade_one_submission (and any callers) handle the alternate
Postgres value correctly.
🪄 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: dd202c2d-d3fa-4a80-a37d-88b80e988444
📒 Files selected for processing (28)
scripts/consolidate_mini_interact_audited.pyscripts/dev1515_convert_runs.pyscripts/dev1515_strict_miss_diagnostics.pysrc/bird_interact_agents/agents/_submit.pysrc/bird_interact_agents/cloud/driver.pysrc/bird_interact_agents/cloud/post_run_merge.pysrc/bird_interact_agents/cloud/ray_app.pysrc/bird_interact_agents/db_connection.pysrc/bird_interact_agents/eval/annotate.pysrc/bird_interact_agents/eval/annotation_schema.pysrc/bird_interact_agents/eval/cascading_report.pysrc/bird_interact_agents/eval/grade_in_place.pysrc/bird_interact_agents/eval/implicit_annotation.pysrc/bird_interact_agents/eval/regrade.pysrc/bird_interact_agents/eval/tolerant_grader.pysrc/bird_interact_agents/harness.pysrc/bird_interact_agents/run.pysrc/bird_interact_agents/slayer_otf/cache.pytests/test_cascading_report.pytests/test_dry_run_gate.pytests/test_eval_annotation_schema.pytests/test_local_run_cascading.pytests/test_regrade_cli.pytests/test_run_local_inline_grader.pytests/test_tolerant_grader_comparators.pytests/test_tolerant_grader_orchestration.pytests/test_tolerant_grader_postgres.pytests/test_user_sim_interaction_trajectory.py
💤 Files with no reviewable changes (1)
- scripts/dev1515_strict_miss_diagnostics.py
🚧 Files skipped from review as they are similar to previous changes (16)
- tests/test_dry_run_gate.py
- src/bird_interact_agents/eval/implicit_annotation.py
- scripts/consolidate_mini_interact_audited.py
- src/bird_interact_agents/cloud/driver.py
- tests/test_local_run_cascading.py
- src/bird_interact_agents/db_connection.py
- tests/test_cascading_report.py
- src/bird_interact_agents/eval/grade_in_place.py
- src/bird_interact_agents/harness.py
- src/bird_interact_agents/eval/annotation_schema.py
- scripts/dev1515_convert_runs.py
- src/bird_interact_agents/slayer_otf/cache.py
- tests/test_run_local_inline_grader.py
- src/bird_interact_agents/cloud/post_run_merge.py
- src/bird_interact_agents/eval/annotate.py
- src/bird_interact_agents/eval/tolerant_grader.py
Group 1 — cloud grader + snapshot: - ray_app.py: resolve benchmark object from cfg; use Path(<db>) for postgres grader db_path instead of hardcoded SQLite path - _submit.py: wrap postgres snapshot SQL in SELECT * FROM (...) LIMIT N+1 so fetchall() is bounded at source instead of loading full result set Group 2 — read-only enforcement + OTF caller gap: - db_connection.py: BEGIN READ ONLY instead of bare BEGIN; prevents DML on permanent tables at server level even if SQL embeds a COMMIT - runtime.py: pass benchmark= to ensure_db_cache for postgres OTF path - reference_build.py: add benchmark= param to ensure_db_reference and thread to ensure_db_cache; update test spies to accept benchmark=None Group 3 — minor / regression fix: - regrade.py: canonicalize args.benchmark = _bench.name after get_benchmark() so task_annotation_ref uses mini_interact (underscore) not mini-interact - dev1515_strict_miss_diagnostics.py: keep mini_interact (underscore) dir, not mini-interact (hyphen) — canonical on-disk form is underscored Tests: add test_postgres_read_only_uses_begin_read_only regression test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Merge origin/main (claude_sdk_otf_raw agents, db_file_path grader routing) and resolve conflicts in ray_app.py and run.py, combining postgres backend-awareness (our branch) with the db_file_path per-task sqlite isolation path (main). - harness.py: load_livesqlbench_tasks now accepts dataset_marker param; load_benchmark_tasks passes b.dataset_marker so livesqlbench_postgres tasks get dataset="livesqlbench_postgres" instead of "livesqlbench". Without this, execute_submit_action routed postgres tasks to the SQLite path (Codex finding, DEV-1523). - _submit.py: add strict=True to zip(col_names, types_) in the SQLite snapshot path for symmetry with the postgres path at line 120. - test_benchmark_postgres.py: two new tests pin the dataset_marker threading through load_benchmark_tasks → load_livesqlbench_tasks. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Pass benchmark= to ensure_db_reference in pydantic_ai_otf_encode so the OTF encoder uses the per-benchmark cache/reference roots for postgres tasks - Fix misleading db_connection.py comment: BEGIN READ ONLY does not prevent an embedded COMMIT in a multi-statement string from escaping the transaction - Clarify harness.py p2=False: correct for one-shot postgres, noted limitation for future interactive postgres benchmarks - Update fake_ensure_db_reference stub in test to accept the new benchmark= kwarg Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tion harness.py: _pg_execute_submit_action now returns finished=benchmark.one_shot (not hardcoded True) so mini_interact_postgres tasks in a-/c-interact mode can continue after a failed or errored submission, matching SQLite interactive semantics. cache.py / orchestrator.py: strip BIRD_PG_PASSWORD from the postgresql:// URL passed to `slayer datasources create`; inject it via PGPASSWORD env var instead so the credential never appears in subprocess command-line args (visible via ps) or persists in the datasources/<db>.yaml file written by SLayer. Tests: five _pg_execute_submit_action finished-flag assertions in test_benchmark_postgres.py; three password-isolation assertions in test_otf_postgres.py (PGPASSWORD set, not in URL, _build_async passes pg_password separately); fake_phase1 stubs in test_slayer_otf_cache.py updated to accept new pg_password kwarg. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
post_run_merge.py: take origin/main's logger.warning on audit-log write failure (better than HEAD's silent pass). ray_app.py: merge backend-aware grader db_path (postgres vs SQLite, HEAD) with db_file_path support for per-task-isolated SQLite copies (origin/main). regrade.py: merge postgres db_path carrier (HEAD) with _template.sqlite fallback for per-task-isolated copies (origin/main). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
sol_sqls is a sequence of dependent steps (not independent alternatives). Replace the per-element loop that opened a fresh connection for each gold SQL with a single _run_gold_sequence helper that executes all statements on one shared connection and compares the last result — mirroring tolerant_grader._multi_sql_execute semantics. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
PostgresDbConnection.execute wraps each call in its own BEGIN/ROLLBACK, so the old _run_gold_sequence loop lost temp-table state between steps. Add execute_sequence() that issues ONE BEGIN READ ONLY / ROLLBACK for the whole list, then call it from _run_gold_sequence. Update tests to expect execute_sequence on the gold connection (not per-stmt execute). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…postgres variants - _FRAMEWORK_DATASET_MODE_BINDING now accepts both sqlite and postgres dataset names per framework (frozenset); _validate_framework_dataset_mode updated to match (V3) - claude_sdk_otf/agent.py and claude_sdk_otf_ainteract/agent.py: relax hardcoded single-name check to accept the postgres sibling dataset name (V3) - pydantic_ai_recursive _resolve_otf_task_storage_dir: pass benchmark= to ensure_db_cache so postgres tasks get the correct cache backend (V2) - Update test stubs in test_one_shot_run.py and test_recursive_runtime_db_root.py to accept the new benchmark= kwarg on ensure_db_cache Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… + import cleanup - harness.load_benchmark_tasks: stamp task["dataset"] = b.dataset_marker for non-gold-required benchmarks (mini_interact_postgres was silently falling through to the SQLite dispatch path — Codex, DEV-1523) - tests/test_benchmark_postgres.py: add two new marker-stamp tests for mini_interact_postgres and mini_interact backward-compat - claude_sdk_otf + claude_sdk_otf_ainteract: pass benchmark= to grade_submission in the autopsy/cascade path so postgres tasks use the postgres executor instead of trying to open a non-existent SQLite file - db_connection.DbConnection Protocol: declare execute_sequence so callers don't need type: ignore[union-attr]; SqliteDbConnection implements it with NotImplementedError (postgres-only); harness.py drops the suppression - slayer_otf/cache._build_async: replace `import os as _os` (inside function) with the module-level `os` import, per project import rules - cloud/driver._build_missing_otf_caches: pass benchmark= to ensure_db_cache so OTF cache builds use the right DB backend if ever called for a non-SQLite benchmark Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…oting
- pyproject.toml: add psycopg2-binary>=2.9 to cloud and all extras so
postgres connectivity is available without the mini-interact-agent
transitive dep
- db_connection.py: PostgresDbConnection.execute / execute_sequence now
call self._conn.rollback() on psycopg2.Error when read_only=False,
leaving the connection in a clean state for reuse
- harness.py: fix execute() SQL extraction — strip("'\"") was removing
ALL consecutive quotes; replace with single-pair outer-quote removal
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Users installing narrower extras (pydantic-ai, slayer, claude-sdk) who choose a postgres-backend benchmark would hit ModuleNotFoundError at runtime. Adding a dedicated postgres extra makes the dependency opt-in without requiring cloud or all. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
_pg_execute_submit_action returns "SQL execution error: ..." but classify_submission only matched SQLite/upstream error strings, so postgres SQL errors were silently bucketed as wrong_result. Add the postgres error pattern and cover it with a test. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rading
Two bugs in _pg_execute_submit_action:
1. Counter(map(tuple, rows)) crashed with TypeError on JSONB/JSON columns
(psycopg2 returns dict/list). Add _pg_hashable_row() that serialises
dict/list cells to canonical JSON strings before hashing.
2. conditions["order"] was silently ignored — all comparisons were unordered
Counter equality, so ordered queries could pass with wrong row order.
Now branches on conditions.get("order", False), matching ex_base behaviour.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
A submitted query such as SELECT pg_sleep(3600) would hang a task worker indefinitely. Set -c statement_timeout=30000ms (30s default) on the psycopg2 connection via BIRD_PG_STATEMENT_TIMEOUT env var (0 = no limit). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Two correctness bugs: 1. harness: _pg_execute_submit_action with sol_sql=[] called execute_sequence on an empty list, which returned ([], no-error); any prediction returning empty results then matched gold and got p1=True. Guard: treat empty sol_sql as gold_err=True (ungraded), matching SQLite evaluator behaviour. 2. harness: slayer_mcp_stdio_config env copy missed PGPASSWORD when only BIRD_PG_PASSWORD was set. Postgres SLayer datasources store a password-free URL; the MCP subprocess inherits the env and needs PGPASSWORD to connect. Derive PGPASSWORD from BIRD_PG_PASSWORD when not already present. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The persisted SLayer datasource YAML contains a connection URL (postgresql://user@host:port/db). If BIRD_PG_HOST/PORT/USER changes after the cache is built, the cached YAML still points at the old server — the MCP server then fails to authenticate. Extend _impl_fingerprint_of to include (host:port:user) when benchmark is postgres, and pass benchmark through the _impl_ok() closure and the marker-write call. The no-benchmark / SQLite callers in tests are unaffected (default argument = sqlite path). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…in OTF agents Postgres submissions were silently graded via sqlite3 because both claude_sdk_otf and claude_sdk_otf_ainteract agents imported make_executor but didn't pass it to grade_submission. The import was there but unused; default_executor would try to open a non-existent .sqlite file. Adds two regression tests that verify postgres executor vs sqlite3 dispatch at the grade_submission level. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_slayer_otf_cache.py (1)
332-333:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
pg_passwordparameter to theboomstub for signature compatibility.The
boomstub is missing thepg_password=Noneparameter that was added to_phase1_ingest. If a Postgres benchmark test calls this code path, the stub will raise aTypeErrorwhenensure_db_cachepasses thepg_passwordkeyword argument.🔧 Proposed fix
- def boom(db, storage, *, sqlite_path=None, db_url=None): + def boom(db, storage, *, sqlite_path=None, db_url=None, pg_password=None): raise RuntimeError("orchestrator died")🤖 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_slayer_otf_cache.py` around lines 332 - 333, The stub function boom currently has signature boom(db, storage, *, sqlite_path=None, db_url=None) but callers (ensure_db_cache/_phase1_ingest) may pass pg_password, causing a TypeError; update boom to accept pg_password=None (e.g., boom(db, storage, *, sqlite_path=None, db_url=None, pg_password=None)) and simply ignore it so the stub remains compatible with Postgres benchmark calls.
🧹 Nitpick comments (2)
src/bird_interact_agents/db_connection.py (1)
150-170: ⚡ Quick winClarify Postgres
read_only=Falsesemantics: it preserves session state but doesn’t commit writes
PostgresDbConnection.execute/execute_sequencenever callcommit()when_read_onlyisFalse, so any non-TEMP changes will be rolled back when the connection is closed. That matches the only currentmake_db_connection(..., read_only=False)usage intolerant_grader._multi_sql_execute, which explicitly relies on keeping TEMP/session state across multiple statements within the call (and expects writes not to persist beyond it).Update docs/comments (and/or rename the parameter) to make clear that
read_only=Falsemeans “don’t wrap each call in BEGIN/ROLLBACK for read-only dry-run” rather than “commit writes on success.”🤖 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/db_connection.py` around lines 150 - 170, The current semantics of PostgresDbConnection.execute / execute_sequence and the _read_only flag are unclear: _read_only=False does not commit writes but only avoids wrapping calls in BEGIN/ROLLBACK to preserve session/TEMP state for the call. Update inline docstrings/comments for PostgresDbConnection.execute and execute_sequence and the constructor/parameter description of make_db_connection to state explicitly that _read_only=False preserves session state within the connection but does not commit persistent changes (writes will be rolled back on close), and mention tolerant_grader._multi_sql_execute relies on this behavior; alternatively, rename the parameter to something clearer (e.g., preserve_session_state or transactional=False) across these symbols to reflect the true semantics.src/bird_interact_agents/cloud/ray_app.py (1)
113-113: 💤 Low valueRedundant import:
get_benchmarkis already imported at module level (line 27).This in-function import can be removed since
get_benchmarkis now imported at the top of the module.♻️ Suggested fix
def download_benchmark_data(cfg: dict[str, Any], *, client=None) -> None: ... prefix = cfg.get("benchmark_data_prefix") if not prefix: return - from bird_interact_agents.benchmark import get_benchmark - b = get_benchmark(_cloud_benchmark(cfg))🤖 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` at line 113, Remove the redundant in-function import of get_benchmark (the line "from bird_interact_agents.benchmark import get_benchmark") since get_benchmark is already imported at module level; locate the in-function import in ray_app.py (where get_benchmark is imported again inside a function) and delete that line so the function uses the module-level get_benchmark import.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/test_slayer_otf_cache.py`:
- Around line 332-333: The stub function boom currently has signature boom(db,
storage, *, sqlite_path=None, db_url=None) but callers
(ensure_db_cache/_phase1_ingest) may pass pg_password, causing a TypeError;
update boom to accept pg_password=None (e.g., boom(db, storage, *,
sqlite_path=None, db_url=None, pg_password=None)) and simply ignore it so the
stub remains compatible with Postgres benchmark calls.
---
Nitpick comments:
In `@src/bird_interact_agents/cloud/ray_app.py`:
- Line 113: Remove the redundant in-function import of get_benchmark (the line
"from bird_interact_agents.benchmark import get_benchmark") since get_benchmark
is already imported at module level; locate the in-function import in ray_app.py
(where get_benchmark is imported again inside a function) and delete that line
so the function uses the module-level get_benchmark import.
In `@src/bird_interact_agents/db_connection.py`:
- Around line 150-170: The current semantics of PostgresDbConnection.execute /
execute_sequence and the _read_only flag are unclear: _read_only=False does not
commit writes but only avoids wrapping calls in BEGIN/ROLLBACK to preserve
session/TEMP state for the call. Update inline docstrings/comments for
PostgresDbConnection.execute and execute_sequence and the constructor/parameter
description of make_db_connection to state explicitly that _read_only=False
preserves session state within the connection but does not commit persistent
changes (writes will be rolled back on close), and mention
tolerant_grader._multi_sql_execute relies on this behavior; alternatively,
rename the parameter to something clearer (e.g., preserve_session_state or
transactional=False) across these symbols to reflect the true semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c3841c04-7d66-426f-9e29-28876561852b
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
pyproject.tomlscripts/dev1515_strict_miss_diagnostics.pysrc/bird_interact_agents/agents/_submit.pysrc/bird_interact_agents/agents/claude_sdk_otf/agent.pysrc/bird_interact_agents/agents/claude_sdk_otf_ainteract/agent.pysrc/bird_interact_agents/agents/pydantic_ai_otf_encode/agent.pysrc/bird_interact_agents/agents/pydantic_ai_recursive/agent.pysrc/bird_interact_agents/cloud/driver.pysrc/bird_interact_agents/cloud/ray_app.pysrc/bird_interact_agents/db_connection.pysrc/bird_interact_agents/eval/grade_in_place.pysrc/bird_interact_agents/eval/regrade.pysrc/bird_interact_agents/harness.pysrc/bird_interact_agents/run.pysrc/bird_interact_agents/slayer_otf/cache.pysrc/bird_interact_agents/slayer_otf/reference_build.pysrc/bird_interact_agents/slayer_otf/runtime.pysrc/bird_interact_agents/slayer_pipeline/orchestrator.pytests/test_benchmark_postgres.pytests/test_db_connection.pytests/test_harness_postgres_dispatch.pytests/test_local_run_cascading.pytests/test_one_shot_run.pytests/test_otf_encode_reference_root.pytests/test_otf_postgres.pytests/test_recursive_runtime_db_root.pytests/test_slayer_otf_cache.pytests/test_slayer_otf_reference_build.pytests/test_submit_classification.pytests/test_tolerant_grader_postgres.py
✅ Files skipped from review due to trivial changes (1)
- tests/test_otf_encode_reference_root.py
🚧 Files skipped from review as they are similar to previous changes (14)
- src/bird_interact_agents/agents/claude_sdk_otf/agent.py
- tests/test_one_shot_run.py
- src/bird_interact_agents/slayer_otf/reference_build.py
- tests/test_slayer_otf_reference_build.py
- src/bird_interact_agents/cloud/driver.py
- tests/test_local_run_cascading.py
- src/bird_interact_agents/slayer_otf/runtime.py
- src/bird_interact_agents/agents/pydantic_ai_recursive/agent.py
- src/bird_interact_agents/run.py
- tests/test_harness_postgres_dispatch.py
- src/bird_interact_agents/agents/claude_sdk_otf_ainteract/agent.py
- src/bird_interact_agents/eval/grade_in_place.py
- src/bird_interact_agents/slayer_pipeline/orchestrator.py
- src/bird_interact_agents/slayer_otf/cache.py
…alse semantics ray_app.py:download_benchmark_data imported get_benchmark a second time inside the function body; the module-level import at line 27 is sufficient. PostgresDbConnection docstring now explicitly states that read_only=False preserves session state (TEMP tables) within the connection but never commits writes — psycopg2 default autocommit=False means the implicit open transaction is rolled back on close. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… jsonb_meaning_entries _detect_jsonb_columns was extracted in this PR so tests can monkeypatch it and verify that postgres builds skip JSONB detection. But _phase3_jsonb was still calling jsonb_meaning_entries directly, making the monkeypatch a no-op and the test trivially passing regardless of the postgres early-return guard. Now _phase3_jsonb calls _detect_jsonb_columns(meanings_path), so test_phase3_skipped_for_postgres actually tests what it claims. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…s_from_local_env Both postgres benchmarks added in this PR have container_data_dir set, making them cloud-submittable. Without this fix, a cloud submit would silently use the defaults (localhost:5432 / bird_interact) on the worker node instead of the user's actual postgres server. Both the OAuth (claude_sdk) and the standard (API key) branches now include any BIRD_PG_* vars that are set locally; vars that are absent are not forwarded (no empty-string keys that would shadow worker defaults). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…-postgres-based-benchmarks
cur.execute("ROLLBACK") on an aborted psycopg2 connection raises
InFailedSqlTransaction and silently fails (caught by the bare except).
connection.rollback() works in aborted-transaction state and correctly
resets the connection after any error during execute/execute_sequence.
Also add test_postgres_connection_read_only_error_uses_conn_rollback
which specifically exercises the error path, and update the two existing
tests that checked cursor-level ROLLBACK invocations.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
5 tasks
10 tasks
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
DbConnectionprotocol (db_connection.py) —SqliteDbConnectionandPostgresDbConnection(psycopg2-backed, read_only wraps in BEGIN/ROLLBACK);make_db_connectionis the single backend-branching factory, keyed onbenchmark.db_backendLIVESQLBENCH_POSTGRESandMINI_INTERACT_POSTGRESadded tobenchmark.py; newdb_backend: Literal["sqlite","postgres"]andper_task_db_isolation: boolfieldsexecute_env_action/execute_submit_actiondispatch to postgres path;materialize_task_dbusesper_task_db_isolationinstead of hardcoded dataset stringsmake_executor(benchmark)factory;_multi_sql_executebenchmarkkwarg;grade_and_writeauto-wires postgres executor_submit.py—_dry_run_sqlandcapture_result_snapshotacceptbenchmarkkwarg and route throughDbConnectionfor postgres;_dry_run_error_messageis now backend-agnostic_slayer_ingestextracted as patchable helper;_phase1_ingestacceptsdb_url; phases 3 & 4 skip for postgres (native types)cache.py—fingerprint_ofhashes schema text for postgres;ensure_db_cacheskips sqlite file check;_build_asyncconstructs postgresdb_urlfrom env varsTest plan
tests/test_db_connection.py— 11 unit tests for both backendstests/test_benchmark_postgres.py— 15 tests for new benchmark descriptorstests/test_tolerant_grader_postgres.py— make_executor, _multi_sql_execute, grade_submission with mocked postgrestests/test_harness_postgres_dispatch.py— harness dispatch to postgres pathtests/test_otf_postgres.py— fingerprint_of, ensure_db_cache, phase1_ingest, phases 3/4 skiptests/test_submit_postgres.py— _dry_run_sql, capture_result_snapshot, submit_raw_sql wiring🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Enhancements
Tests