DEV-1638: unify local benchmark CLI — fold postgres bootstrap into bird-interact

[ZmeiGorynych]

Problem

Local benchmark runs had two entrypoints split by DB backend: sqlite ran via bird-interact (but required pre-resolved --data/--db-path), while postgres needed the separate scripts/run_local_postgres.py orchestrator. The backend leaked into which command you ran.

What this does

bird-interact --dataset <name> is now the one local entrypoint for both backends. It dispatches on benchmark.db_backend:

• Derives --data/--db-path from the registry when both are omitted (both-or-neither; both backends). Explicit paths still honored.
• Postgres: auto-provisions + loads a private sudo-free local cluster and exports BIRD_PG_* — gated on BIRD_PG_HOST being unset (the connection signal), not on whether paths were derived. Postgres connects via BIRD_PG_*, and --db-path is only the data/KB root, so explicit paths do not suppress provisioning.
• Annotations: best-effort syncs the authoritative *.task.json from GCS (all backends; --skip-annotations opt-out; warns loudly on any id still missing so the silent implicit-N1 grading fallback is visible).
• Auth: --env-file loads a dotenv before auth resolution (default $BIRD_ENV_FILE; no machine path baked into the code).

Architecture

The composable logic moved out of the (un-installed) scripts/ into the package so the installed console script can import it:

• bird_interact_agents.env_file.load_env_file
• bird_interact_agents.local_postgres (cluster helpers + provision_and_export)
• bird_interact_agents.local_annotations.sync_annotations

scripts/{setup_local_postgres,fetch_local_annotations}.py became thin CLIs over the package (setup keeps --stop/--recreate as the admin tool). scripts/run_local_postgres.py is retired.

Local ⇄ cloud parity

The same sync_annotations is wired into the cloud submit pre-build (inside the require_annotation gate → self-healing: pull missing annotations from GCS first, then the gate only fires for ids absent in GCS too). So the annotation set the local grader reads == the set the cloud image bakes. Infra/auth errors propagate (fail-fast); per-blob-absent is counted, not raised. The cloud runner (ray_app / run_evaluation) is untouched — main() is the only local entrypoint and the cloud worker never calls it.

One pre-existing divergence is noted but left out of scope: local creates a root role (dumps do OWNER TO root), cloud does not (relies on the non-fatal error) — table data/schema identical, ownership differs, SELECT grading unaffected.

Tests

Full non-integration suite green (3962 passed, 94 skipped). New: test_env_file, test_local_annotations_sync, test_local_postgres_provision, test_run_local_dispatch (helper + CLI-boundary), tests/cloud/test_submit_annotation_presync. Plan + tests were each Codex-reviewed before implementation.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Added automated, isolated local Postgres provisioning and environment exports for postgres-backed benchmarks when no host is set.
  • Added centralized annotation syncing (including a presync “self-healing” step for annotation-required submissions).
  • Added support for loading credentials from a dotenv-style env file.
  • Added more flexible local run path handling by deriving --data/--db-path when omitted.
• Bug Fixes
  • Improved annotation check behavior: missing annotations are synced first; sync failures propagate correctly and missing blobs fail deterministically.

[linear]

DEV-1638 Unify local benchmark CLI: fold postgres bootstrap into `bird-interact` (dispatch by `db_backend`)

Problem

Local benchmark runs currently have two entrypoints, split by DB backend:

• SQLite benchmarks (mini-interact, livesqlbench-base-lite-sqlite) run turnkey via bird-interact (run.py) — but it requires an already-resolved --data / --db-path.
• Postgres benchmarks (livesqlbench-*, bird-interact-lite-exp) have no turnkey local path. The DB-load logic (_ensure_postgres_loaded) originally lived only in the cloud worker. PR Sudo-free local runner for postgres benchmarks + large-v1 dump fix #73 added scripts/run_local_postgres.py, a thin orchestrator that (1) loads auth env, (2) provisions a private sudo-free postgres cluster + loads the dumps (setup_local_postgres), (3) syncs annotations from GCS (fetch_local_annotations), then execs bird-interact with --data / --db-path auto-derived.

So the postgres-vs-sqlite difference leaks into which command the caller must run. The underlying leak is bird-interact's --data/--db-path-required contract — it forces the caller to know the backend and pre-resolve paths.

Proposal

Unify the local side behind one CLI. bird-interact --benchmark <name> looks up the benchmark's db_backend in the registry (benchmark.Benchmark.db_backend, already Literal["sqlite","postgres"]) and dispatches:

• sqlite → derive --data / --db-path from the registry (today they are required flags).
• postgres → internally invoke the same three helpers run_local_postgres.py composes (env load, setup_local_postgres provision+load, fetch_local_annotations sync), export the BIRD_PG_* connection env, then derive paths.

Net: one local entrypoint; the postgres bootstrap becomes an internal implementation detail dispatched by db_backend, not a separate script. run_local_postgres.py collapses to a thin shim over the flag (or is retired).

Scope / non-goals

• LOCAL only. bird-interact-cloud (cluster lifecycle: submit/list/fetch/kill) stays a separate surface — genuinely different concern (GCE/Ray provisioning + orchestration), not a task runner.
• Keep setup_local_postgres / fetch_local_annotations independently importable; the CLI just composes them.
• Preserve run.py's pure-runner contract used by the cloud worker — provisioning stays additive, gated on local + --benchmark, so the cloud path is untouched.

Origin

Surfaced while setting up a local v2 run of the 15 livesqlbench-large tasks under DEV-1591: a postgres benchmark needs scripts/run_local_postgres.py instead of plain bird-interact. Confirmed the split lives on origin/main (PR #73 = ded8409) and the db_backend dispatch key already exists in benchmark.py, so this is a branch-off-main refactor.

Review in Linear

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 8 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 1fa10e04-6ddf-47a1-aa2d-4e9f1b007916

📥 Commits

Reviewing files that changed from the base of the PR and between 7179fd8 and 75ab8a6.

📒 Files selected for processing (7)

• scripts/setup_local_postgres.py
• src/bird_interact_agents/local_annotations.py
• src/bird_interact_agents/local_postgres.py
• src/bird_interact_agents/run.py
• tests/scripts/test_local_postgres_helpers.py
• tests/test_local_annotations_sync.py
• tests/test_run_local_dispatch.py

📝 Walkthrough

Walkthrough

Local Postgres provisioning and annotation sync logic move into package modules, with run.py and cloud submit using those helpers. The old one-shot Postgres runner is removed, the script entrypoints become wrappers, and docs/tests are updated for the new local benchmark flow.

Changes

Local Postgres and annotation sync consolidation

Layer / File(s)	Summary
Dotenv auth loader src/bird_interact_agents/env_file.py, tests/test_env_file.py	Adds load_env_file for dotenv-style auth loading and tests for missing files, parsing, quoting, and comment handling.
Local Postgres provisioning src/bird_interact_agents/local_postgres.py, scripts/setup_local_postgres.py, tests/scripts/test_local_postgres_helpers.py, tests/test_local_postgres_provision.py	Adds the package-local Postgres provisioner, keeps setup_local_postgres.py as a wrapper, and updates helper/orchestration tests for cluster setup, role creation, database loading, and export generation.
Task annotation sync src/bird_interact_agents/local_annotations.py, scripts/fetch_local_annotations.py, src/bird_interact_agents/cloud/cli.py, tests/cloud/test_cli.py, tests/cloud/test_submit_annotation_presync.py, tests/test_local_annotations_sync.py	Adds annotation sync, turns fetch_local_annotations.py into a wrapper, makes cloud submit sync before gating, and adds tests for sync behavior, gating, and stubbing.
Local run dispatch wiring src/bird_interact_agents/run.py, CLAUDE.md, tests/test_run_local_dispatch.py	Adds run-time helpers for data path derivation, instance-id selection, local Postgres bootstrap, and annotation sync, then wires them into CLI parsing and main execution with new flags and updated docs/tests.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant run_main as run.main
  participant local_postgres
  participant local_annotations
  participant run_evaluation

  CLI->>run_main: parse_args(argv)
  run_main->>run_main: load_env_file(env_file)
  run_main->>run_main: _resolve_data_paths(args)
  run_main->>run_main: _effective_instance_ids(args)
  run_main->>local_postgres: _maybe_bootstrap_local_postgres()
  local_postgres-->>run_main: BIRD_PG_* exports
  run_main->>local_annotations: _maybe_sync_annotations()
  local_annotations-->>run_main: fetched/missing counts
  run_main->>run_evaluation: run_evaluation(args)

Loading

sequenceDiagram
  participant Client
  participant submit_cli as cloud submit
  participant local_annotations
  participant annotation_gate as missing_annotation_ids

  Client->>submit_cli: parse_args(argv)
  submit_cli->>local_annotations: sync_annotations(dataset, instance_ids)
  local_annotations-->>submit_cli: fetched/missing_in_gcs
  submit_cli->>annotation_gate: missing_annotation_ids(dataset, instance_ids)
  annotation_gate-->>submit_cli: missing ids or none
  submit_cli-->>Client: continue or SystemExit

Loading

Possibly related PRs

• MotleyAI/bird-agents#20: Both PRs change the Postgres-backed benchmark path and BIRD_PG_*-driven connection flow.
• MotleyAI/bird-agents#73: Both PRs touch the sudo-free local Postgres benchmark workflow and the supporting wrapper scripts.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is concise and accurately captures the main change: moving local benchmark execution and Postgres bootstrap into bird-interact.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (5)

src/bird_interact_agents/local_postgres.py (2)

169-171: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Prefer shutil.rmtree over shelling out to rm -rf.

Avoids depending on an external rm binary being on PATH and is more portable.

♻️ Proposed fix

+import shutil
...
     if recreate and p["data"].exists():
         stop_cluster(bindir)
-        subprocess.run(["rm", "-rf", str(p["data"])], check=True)
+        shutil.rmtree(p["data"])

🤖 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/local_postgres.py` around lines 169 - 171, Replace
the shell call to remove the data directory in local_postgres.py with a
Python-native deletion using shutil.rmtree. The recreate branch in the cluster
cleanup logic currently shells out via subprocess.run(["rm", "-rf", ...]), so
update that block to use the existing p["data"] path directly and keep the
stop_cluster(bindir) flow intact. Make sure the cleanup still runs only when
recreate is true and the data path exists.

---

45-49: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value

Hardcoded PG_PASSWORD flagged by static analysis (S105).

Given roles are created with -A trust (line 175), the password is never actually used for authentication, so this is effectively a dev-cluster placeholder rather than a real secret. Worth a brief # noqa: S105 (or nosec-style) comment to document intent and silence the lint noise for future readers.

🤖 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/local_postgres.py` around lines 45 - 49, The
hardcoded PG_PASSWORD in local_postgres.py is a deliberate dev-cluster
placeholder and should be annotated to silence static analysis. Add a brief
intent comment or lint suppression рядом with the PG_PASSWORD constant in the
module-level defaults so S105 is ignored, and keep the existing
PG_USER/_REQUIRED_ROLES context unchanged.

Source: Linters/SAST tools

src/bird_interact_agents/local_annotations.py (1)

57-57: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Empty instance_ids list silently syncs the whole benchmark.

targets = instance_ids or list(inst_to_db) treats [] the same as None. Currently unreachable from the provided callers (the cloud CLI already rejects an empty resolved id list before this point), but as a public library function this is a latent footgun for any future caller that legitimately wants "sync nothing."

🛡️ Proposed fix

-    targets = instance_ids or list(inst_to_db)
+    targets = list(inst_to_db) if instance_ids is None else instance_ids

🤖 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/local_annotations.py` at line 57, The `sync`/target
selection logic in `local_annotations.py` treats an empty `instance_ids` list
the same as no input by using `instance_ids or list(inst_to_db)`, which can
accidentally sync everything. Update the target selection in the relevant
function to distinguish `None` from `[]` so an explicit empty list stays empty,
and only fall back to `list(inst_to_db)` when `instance_ids` is not provided at
all.

tests/test_local_annotations_sync.py (1)

1-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Missing coverage for the not-found-instance-id path.

None of the tests exercise sync_annotations being called with an instance_id absent from inst_to_db (the SystemExit branch at local_annotations.py lines 53‑56). Worth a quick added case, especially given the exit-type inconsistency flagged in local_annotations.py.

🤖 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_annotations_sync.py` around lines 1 - 147, Add a test for
the not-found instance-id path in sync_annotations: cover the branch where an
input instance_id is missing from inst_to_db and the function exits via
SystemExit. Use the existing local_annotations.sync_annotations fixture setup
and extend tests/test_local_annotations_sync.py with a case that passes an
unknown instance_id, asserting the exit behavior rather than download/count
results. Keep the coverage focused on the sync_annotations lookup logic and the
_load_dataset_instance_db_map-based mapping used to resolve instance IDs.

src/bird_interact_agents/cloud/cli.py (1)

379-389: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Silent network I/O and error contract during parse_args.

sync_annotations(ns.dataset, ns.instance_ids) performs GCS network calls inside argument parsing with no observability (no print/log of what/whether it fetched anything), unlike local_annotations.main()'s script wrapper which reports fetched=/already_local=/missing_in_gcs=. Since this now runs on every submit with --require-annotation (the default), a slow or partially-failing GCS call is invisible to the operator until the subsequent gate fails or an unrelated-looking exception surfaces. Consider emitting a short stderr summary (or reusing the counts already returned by sync_annotations) so the self-healing step is visible.

Separately — flagged in local_annotations.py — an instance_id typo causes sync_annotations to raise a bare SystemExit here, which exits with a different code/format than the p.error() calls used everywhere else in this same gate.

The submit gate now runs sync_annotations first before the require_annotation missing-annotation gate.

🤖 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/cli.py` around lines 379 - 389, The submit
parsing flow now calls sync_annotations(ns.dataset, ns.instance_ids) during
parse_args, but it does so silently, so add a short stderr/log summary using the
counts returned by sync_annotations (like fetched/already_local/missing_in_gcs)
before the require_annotation gate runs. Also fix the instance_id typo path in
sync_annotations so it does not raise a bare SystemExit; instead surface the
error through the same parser-style error handling used by the
require_annotation gate (for example via p.error). Keep the changes centered on
parse_args in cli.py and sync_annotations in local_annotations.py so the
self-healing step is visible and exits consistently.

🤖 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/local_annotations.py`:
- Around line 53-56: The sync_annotations validation currently raises SystemExit
for missing instance_ids, which bypasses the normal CLI error handling path.
Update sync_annotations in local_annotations.py to raise ValueError instead of
exiting, and then handle/format that exception at each CLI boundary that calls
it so the cloud CLI can continue using p.error() consistently across callers.

In `@src/bird_interact_agents/local_postgres.py`:
- Around line 104-120: Make the bindir resolver a public API instead of a
private helper: rename `_resolve_bindir` in
`bird_interact_agents.local_postgres` to a non-underscored name (or add a public
alias) because `scripts/setup_local_postgres.py` imports it across the module
boundary. Update the internal call sites in `provision_and_export` and the
import in `scripts/setup_local_postgres.py` to use the public helper name so the
cross-file contract is explicit and stable.

---

Nitpick comments:
In `@src/bird_interact_agents/cloud/cli.py`:
- Around line 379-389: The submit parsing flow now calls
sync_annotations(ns.dataset, ns.instance_ids) during parse_args, but it does so
silently, so add a short stderr/log summary using the counts returned by
sync_annotations (like fetched/already_local/missing_in_gcs) before the
require_annotation gate runs. Also fix the instance_id typo path in
sync_annotations so it does not raise a bare SystemExit; instead surface the
error through the same parser-style error handling used by the
require_annotation gate (for example via p.error). Keep the changes centered on
parse_args in cli.py and sync_annotations in local_annotations.py so the
self-healing step is visible and exits consistently.

In `@src/bird_interact_agents/local_annotations.py`:
- Line 57: The `sync`/target selection logic in `local_annotations.py` treats an
empty `instance_ids` list the same as no input by using `instance_ids or
list(inst_to_db)`, which can accidentally sync everything. Update the target
selection in the relevant function to distinguish `None` from `[]` so an
explicit empty list stays empty, and only fall back to `list(inst_to_db)` when
`instance_ids` is not provided at all.

In `@src/bird_interact_agents/local_postgres.py`:
- Around line 169-171: Replace the shell call to remove the data directory in
local_postgres.py with a Python-native deletion using shutil.rmtree. The
recreate branch in the cluster cleanup logic currently shells out via
subprocess.run(["rm", "-rf", ...]), so update that block to use the existing
p["data"] path directly and keep the stop_cluster(bindir) flow intact. Make sure
the cleanup still runs only when recreate is true and the data path exists.
- Around line 45-49: The hardcoded PG_PASSWORD in local_postgres.py is a
deliberate dev-cluster placeholder and should be annotated to silence static
analysis. Add a brief intent comment or lint suppression рядом with the
PG_PASSWORD constant in the module-level defaults so S105 is ignored, and keep
the existing PG_USER/_REQUIRED_ROLES context unchanged.

In `@tests/test_local_annotations_sync.py`:
- Around line 1-147: Add a test for the not-found instance-id path in
sync_annotations: cover the branch where an input instance_id is missing from
inst_to_db and the function exits via SystemExit. Use the existing
local_annotations.sync_annotations fixture setup and extend
tests/test_local_annotations_sync.py with a case that passes an unknown
instance_id, asserting the exit behavior rather than download/count results.
Keep the coverage focused on the sync_annotations lookup logic and the
_load_dataset_instance_db_map-based mapping used to resolve instance IDs.

🪄 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: 661bf342-14b9-4285-a755-8fc59f822130

📥 Commits

Reviewing files that changed from the base of the PR and between ded8409 and 0761e85.

📒 Files selected for processing (16)

• CLAUDE.md
• scripts/fetch_local_annotations.py
• scripts/run_local_postgres.py
• scripts/setup_local_postgres.py
• src/bird_interact_agents/cloud/cli.py
• src/bird_interact_agents/env_file.py
• src/bird_interact_agents/local_annotations.py
• src/bird_interact_agents/local_postgres.py
• src/bird_interact_agents/run.py
• tests/cloud/test_cli.py
• tests/cloud/test_submit_annotation_presync.py
• tests/scripts/test_local_postgres_helpers.py
• tests/test_env_file.py
• tests/test_local_annotations_sync.py
• tests/test_local_postgres_provision.py
• tests/test_run_local_dispatch.py

💤 Files with no reviewable changes (1)

• scripts/run_local_postgres.py