diff --git a/Dockerfile.cloud b/Dockerfile.cloud index c5a57e83..1acae53b 100644 --- a/Dockerfile.cloud +++ b/Dockerfile.cloud @@ -90,5 +90,23 @@ COPY --from=audited-gold . /app/bird-interact-agents/audited_gold/ # inside the worker (via git's `--git-common-dir`). COPY --from=annotations . /app/bird-interact-agents/annotations/ +# DEV-1550: bake the upstream BIRD-Interact and livesqlbench grader +# subtrees so `eval.upstream_ex_base` can load `test_utils.py` + +# `db_utils.py` in the cloud actor. Without this the cascade-tier-N1 +# dispatch silently falls back to legacy `_set_equal` (the loader's +# `FileNotFoundError` is caught upstream and downgraded), so the PR's +# N1=ex_base promise never engages in production. +# +# Build contexts (`bird-interact-evaluation`, `livesqlbench-evaluation`) +# are wired by `cloud.image.build_and_push` and point at the evaluation +# subdirs of `paths.bird_interact_root()` / `paths.livesqlbench_root()`. +# The deeper rel paths preserved here match the host-tree layout so +# `_MINI_INTERACT_REL` / `_LIVESQLBENCH_REL` in `upstream_ex_base` resolve +# identically on cloud and on the developer laptop. +COPY --from=bird-interact-evaluation . \ + /app/upstream_graders/bird-interact/mini_interact/knowledge_based/mini_interact_conv/evaluation/ +COPY --from=livesqlbench-evaluation . \ + /app/upstream_graders/livesqlbench/evaluation/src/ + ENV BIRD_RESULTS_ROOT=/tmp/results \ BIRD_INTERACT_AGENTS_CLOUD=1 diff --git a/pyproject.toml b/pyproject.toml index adfb62ec..ea2de67a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -33,7 +33,7 @@ pydantic-ai = [ smolagents = ["smolagents>=1.0"] agno = ["agno>=2.0"] mcp-agent = ["mcp-agent>=0.0.16"] -slayer = ["motley-slayer[embedding-search]>=0.7.3"] +slayer = ["motley-slayer[embedding-search]>=0.7.4"] # `original` brings in the upstream BIRD-Interact mini_interact_agent harness # so we can call its main.py directly via `uv run python -m batch_run_bird_interact.main`. original = ["mini-interact-agent"] @@ -51,7 +51,7 @@ all = [ "smolagents>=1.0", "agno>=2.0", "mcp-agent>=0.0.16", - "motley-slayer[embedding-search]>=0.7.3", + "motley-slayer[embedding-search]>=0.7.4", "mini-interact-agent", "ray[default]>=2.30", "google-cloud-storage>=2.18", diff --git a/scripts/regrade_n1_ex_base.py b/scripts/regrade_n1_ex_base.py new file mode 100644 index 00000000..55cdd28c --- /dev/null +++ b/scripts/regrade_n1_ex_base.py @@ -0,0 +1,395 @@ +#!/usr/bin/env python3 +"""Backfill cascade N1 for mini-interact stored runs using upstream's +``ex_base``-equivalent grader. + +The pre-fix N1 ("phase1_against_original_gold") was bag equality on +``repr(cell)``. Upstream mini-interact's grader applies 2-dp Decimal/ +float rounding + date normalisation + ``set()``-dedup comparison via +``test_case_default`` + ``ex_base`` + ``preprocess_results``. Result: ~6 +slayer + ~3 raw mini-interact cases that pass upstream were being +demoted to N6 epsilon (or worse) in our cascade. + +This script walks every stored result under +``paths.runs_root() / 'mini-interact' / / / .json`` +and, for each one: + +* loads ``submitted_sql`` from the JSON and ``sol_sql`` / ``conditions`` + from the per-task annotation (falling back to the canonical + ``mini_interact.jsonl`` row when no annotation exists); +* skips conservatively if either side mutates the DB (``INSERT`` / + ``UPDATE`` / ``DELETE`` / ``CREATE`` / ``DROP`` / ``ALTER`` / + ``TRUNCATE`` / ``REPLACE`` at statement start) — the pristine + backfill DB diverges from the inline-grader's post-mutation state + for those tasks; +* resolves the SQLite db_path via + ``paths.benchmark_data_root('mini-interact') / / f"{}.sqlite"``; +* re-runs the FULL cascade (via ``tolerant_grader.grade_submission``, + which now dispatches N1 to upstream ``ex_base`` for mini-interact) + so every field downstream of N1 (audited tiers, tolerance booleans, + verdict, ``failure_classification``) gets recomputed consistently — + NOT a single-field patch (Codex round-1 finding #1); +* rewrites the JSON in place when fields change. + +The script is mini-interact only (livesqlbench backfill needs Postgres ++ per-task DB load; deferred to a separate follow-up). + +Usage:: + + uv run python scripts/regrade_n1_ex_base.py # apply changes + uv run python scripts/regrade_n1_ex_base.py --dry-run # print would-flips only + +Exits 0 when all tasks were processed without script-level errors +(state-sensitive / missing-input skips are normal, not failures). +""" + +from __future__ import annotations + +import argparse +import json +import logging +import os +import sqlite3 +import sys +from dataclasses import dataclass, field +from pathlib import Path +from typing import Optional + +from bird_interact_agents import paths +from bird_interact_agents.benchmark import get_benchmark +from bird_interact_agents.eval.annotation_schema import ( + SubmissionAnnotation, + TaskAnnotation, +) +from bird_interact_agents.eval.grade_in_place import ( + _auto_failure_class, + _build_submission_annotation, + _verdict_to_phase, + load_audited_gold_rows_for, + load_task_annotation_or_implicit, + normalize_sol_sql, +) +from bird_interact_agents.eval.tolerant_grader import ( + grade_submission, +) +from bird_interact_agents.eval.upstream_ex_base import is_mutation_sql + +logger = logging.getLogger("regrade_n1_ex_base") + +_BENCHMARK = "mini-interact" + + +@dataclass +class Report: + processed: int = 0 + regraded_flipped: int = 0 + regraded_unchanged: int = 0 + would_flip: int = 0 + skipped_state_sensitive: int = 0 + skipped_missing_inputs: int = 0 + errors: int = 0 + flipped_iids: list[str] = field(default_factory=list) + + def to_one_line(self) -> str: + return ( + f"processed={self.processed} " + f"regraded_flipped={self.regraded_flipped} " + f"regraded_unchanged={self.regraded_unchanged} " + f"would_flip={self.would_flip} " + f"skipped_state_sensitive={self.skipped_state_sensitive} " + f"skipped_missing_inputs={self.skipped_missing_inputs} " + f"errors={self.errors}" + ) + + +def _load_task_annotation_or_jsonl( + instance_id: str, selected_database: str, +) -> tuple[Optional[TaskAnnotation], list[str], Optional[dict]]: + """Load ``(task_annotation, sol_sql, conditions)`` from the per-task + annotation when present, else from the canonical jsonl row. + + Returns ``(None, [], None)`` when neither source carries this task — + the caller skips with ``skipped_missing_inputs``. + """ + annotations_root = paths.annotations_root() + ann_path = ( + annotations_root / _BENCHMARK / selected_database + / f"{instance_id}.task.json" + ) + sol_sql: list[str] = [] + conditions: Optional[dict] = None + task_ann: Optional[TaskAnnotation] = None + + if ann_path.is_file(): + payload = json.loads(ann_path.read_text()) + # sol_sql / conditions live on the task json next to the + # TaskAnnotation fields; TaskAnnotation uses `extra="forbid"`, so + # peel them off before validating. + sol_sql = normalize_sol_sql(payload.get("sol_sql")) + conditions = payload.get("conditions") + validation_payload = { + k: v for k, v in payload.items() + if k not in ("sol_sql", "conditions") + } + try: + task_ann = TaskAnnotation.model_validate(validation_payload) + except Exception: # noqa: BLE001 + task_ann = None + + if not sol_sql: + jsonl_path = os.environ.get("BIRD_MINI_INTERACT_DATA_PATH") + candidates: list[Path] = [] + if jsonl_path: + candidates.append(Path(jsonl_path)) + # Default canonical mini_interact.jsonl location. + candidates.append(paths.benchmark_data_root(_BENCHMARK) / "mini_interact.jsonl") + for cand in candidates: + if not cand.is_file(): + continue + for line in cand.read_text().splitlines(): + if not line.strip(): + continue + row = json.loads(line) + if row.get("instance_id") == instance_id: + sol_sql = normalize_sol_sql(row.get("sol_sql")) + if conditions is None: + conditions = row.get("conditions") + break + if sol_sql: + break + + if task_ann is None: + try: + task_ann = load_task_annotation_or_implicit( + instance_id=instance_id, + selected_database=selected_database, + benchmark=_BENCHMARK, + amb_user_query="", + ) + except Exception: # noqa: BLE001 + task_ann = None + + return task_ann, sol_sql, conditions + + +def _resolve_db_path(selected_database: str) -> Path: + """Codex round-1 finding #4: the SQLite layout is + ``benchmark_data_root('mini-interact') / / .sqlite``.""" + return ( + paths.benchmark_data_root(_BENCHMARK) + / selected_database + / f"{selected_database}.sqlite" + ) + + +def _evaluation_diffs(before: SubmissionAnnotation, after: SubmissionAnnotation) -> bool: + """True iff at least one persisted field changed (``evaluation`` or + ``failure_classification``). Identity-on-content; bytes-equal JSON + elsewhere on idempotent re-run.""" + return ( + before.evaluation.model_dump() != after.evaluation.model_dump() + or before.failure_classification.model_dump() + != after.failure_classification.model_dump() + ) + + +def _process_one( + result_path: Path, *, dry_run: bool, report: Report, +) -> None: + payload = json.loads(result_path.read_text()) + try: + before = SubmissionAnnotation.model_validate(payload) + except Exception: # noqa: BLE001 + logger.exception("Could not parse SubmissionAnnotation at %s", result_path) + report.errors += 1 + return + + instance_id = before.instance_id + selected_database = before.selected_database + submitted_sql = before.submitted_sql or "" + + task_ann, sol_sql, conditions = _load_task_annotation_or_jsonl( + instance_id, selected_database, + ) + if task_ann is None or not sol_sql or not submitted_sql: + logger.info( + "[skip missing_inputs] %s/%s", + selected_database, instance_id, + ) + report.skipped_missing_inputs += 1 + return + + # Mutation-bearing skip (Codex round-1 finding #5). + all_sqls = [submitted_sql, *sol_sql] + if any(is_mutation_sql(s) for s in all_sqls): + logger.info( + "[skip state_sensitive] %s/%s (mutation-bearing SQL)", + selected_database, instance_id, + ) + report.skipped_state_sensitive += 1 + return + + db_path = _resolve_db_path(selected_database) + if not db_path.is_file(): + logger.info( + "[skip missing_inputs] %s/%s — db_path %s not found", + selected_database, instance_id, db_path, + ) + report.skipped_missing_inputs += 1 + return + + try: + audited_rows = load_audited_gold_rows_for( + benchmark=_BENCHMARK, + instance_id=instance_id, + ) + except Exception: # noqa: BLE001 + audited_rows = [] + + benchmark_obj = get_benchmark(_BENCHMARK) + try: + cascade = grade_submission( + task_annotation=task_ann, + audited_gold_rows=audited_rows, + original_sol_sql=sol_sql, + submitted_sql=submitted_sql, + db_path=db_path, + benchmark=benchmark_obj, + # DEV-1550 round-2 (Codex): we loaded `conditions` above; pass + # it through so ordered-comparison tasks regrade with + # positional semantics, matching upstream. + conditions=conditions, + ) + except Exception: # noqa: BLE001 + logger.exception( + "grade_submission raised for %s/%s", selected_database, instance_id, + ) + report.errors += 1 + return + + after = _build_submission_annotation( + task_annotation=task_ann, + cascade=cascade, + benchmark=_BENCHMARK, + run_id=before.submission.cloud_run_id, + trajectory_path=before.submission.trajectory_path, + predicted_row_count=before.submission.predicted_row_count, + duration_s=before.submission.duration_s, + cost_usd_agent=before.submission.cost_usd_agent, + cost_usd_user_sim=before.submission.cost_usd_user_sim, + n_agent_turns=before.submission.n_agent_turns, + n_ask_user_calls=before.submission.n_ask_user_calls, + submitted_sql=submitted_sql, + user_sim_interaction=before.user_sim_interaction, + config=before.submission.config, + ) + # Preserve `annotated_by` / `annotated_at` from the original + # annotation so the regrade isn't mistakenly attributed to the + # auto-annotator. We only changed the evaluation block. + after = after.model_copy(update={ + "annotated_by": before.annotated_by, + "annotated_at": before.annotated_at, + "autopsy": before.autopsy, + "decision_point": before.decision_point, + }) + + # Every task we successfully re-graded counts as processed, + # whether or not the result flipped — the report line's + # processed counter then matches the size of the work-list minus + # the skip / error buckets. Without this, idempotent re-runs + # report processed=0 even though every JSON was loaded, graded, + # and compared. (CodeRabbit round 2.) + report.processed += 1 + + flipped = _evaluation_diffs(before, after) + if not flipped: + report.regraded_unchanged += 1 + return + + if dry_run: + report.would_flip += 1 + n1_before = before.evaluation.phase1_against_original_gold + n1_after = after.evaluation.phase1_against_original_gold + primary_before = before.failure_classification.primary + primary_after = after.failure_classification.primary + logger.info( + "[would_flip] %s/%s n1 %s→%s primary %s→%s", + selected_database, instance_id, + n1_before, n1_after, primary_before, primary_after, + ) + return + + report.regraded_flipped += 1 + report.flipped_iids.append(f"{selected_database}/{instance_id}") + # Rewrite the JSON in place. + out = after.model_dump(mode="json") + result_path.write_text(json.dumps(out, indent=2) + "\n") + logger.info( + "[regraded] %s/%s n1=%s primary=%s", + selected_database, instance_id, + after.evaluation.phase1_against_original_gold, + after.failure_classification.primary, + ) + + +def regrade( + *, runs_root: Optional[Path] = None, dry_run: bool = False, +) -> Report: + """Top-level entrypoint: walk runs_root and process each + mini-interact result JSON. Returns a final :class:`Report`.""" + if runs_root is None: + runs_root = paths.runs_root() + benchmark_root = runs_root / _BENCHMARK + report = Report() + if not benchmark_root.is_dir(): + logger.warning("[regrade] no mini-interact root at %s", benchmark_root) + return report + + # Discover result JSONs. The skip-on-non-mini-interact contract is + # already enforced by only walking mini-interact's subtree (other + # benchmarks' subtrees are never visited). + for f in sorted(benchmark_root.glob("*/*/*.json")): + if f.name.endswith(".trajectory.json"): + continue + try: + _process_one(f, dry_run=dry_run, report=report) + except Exception: # noqa: BLE001 + logger.exception("[regrade] unexpected error on %s", f) + report.errors += 1 + return report + + +def main(argv: Optional[list[str]] = None) -> int: + parser = argparse.ArgumentParser( + description=( + "Backfill mini-interact cascade N1 to upstream ex_base " + "semantics (DEV-1550 follow-up)." + ), + ) + parser.add_argument("--dry-run", action="store_true") + parser.add_argument( + "--runs-root", default=None, + help="Override runs/ root (default: $BIRD_RUNS_ROOT or paths.runs_root()).", + ) + parser.add_argument( + "--log-level", default="INFO", + choices=("DEBUG", "INFO", "WARNING", "ERROR"), + ) + args = parser.parse_args(argv) + + logging.basicConfig( + format="[%(levelname)s] %(name)s: %(message)s", + level=getattr(logging, args.log_level), + ) + + runs_root = Path(args.runs_root) if args.runs_root else None + report = regrade(runs_root=runs_root, dry_run=args.dry_run) + print(report.to_one_line()) + if report.flipped_iids: + print(f"flipped {len(report.flipped_iids)} tasks:") + for iid in report.flipped_iids: + print(f" {iid}") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/src/bird_interact_agents/agents/claude_sdk_otf/agent.py b/src/bird_interact_agents/agents/claude_sdk_otf/agent.py index c7d01570..e6c5e5e8 100644 --- a/src/bird_interact_agents/agents/claude_sdk_otf/agent.py +++ b/src/bird_interact_agents/agents/claude_sdk_otf/agent.py @@ -662,6 +662,7 @@ async def run_task( benchmark=benchmark, executor=make_executor(benchmark), user_sim_n_asks=None, + conditions=task_data.get("conditions"), ) if _ann_from_disk and _is_genuine_miss(_cascade): _autopsy_result = await run_autopsy( diff --git a/src/bird_interact_agents/agents/claude_sdk_otf_ainteract/agent.py b/src/bird_interact_agents/agents/claude_sdk_otf_ainteract/agent.py index b83bad3f..ec794815 100644 --- a/src/bird_interact_agents/agents/claude_sdk_otf_ainteract/agent.py +++ b/src/bird_interact_agents/agents/claude_sdk_otf_ainteract/agent.py @@ -503,6 +503,7 @@ async def run_task( benchmark=benchmark, executor=make_executor(benchmark), user_sim_n_asks=ctx_dict.get("asks_used", 0), + conditions=task_data.get("conditions"), ) if _ann_from_disk and _is_genuine_miss(_cascade): _autopsy_result = await run_autopsy( diff --git a/src/bird_interact_agents/cloud/cli.py b/src/bird_interact_agents/cloud/cli.py index 81d182ae..f2f199f4 100644 --- a/src/bird_interact_agents/cloud/cli.py +++ b/src/bird_interact_agents/cloud/cli.py @@ -380,16 +380,23 @@ def main(argv: Sequence[str] | None = None) -> int: from bird_interact_agents.cloud import image repo_root = driver.submitter_repo_root() + bird_interact_eval, livesqlbench_eval = ( + image.default_grader_eval_roots() + ) tag = image.image_tag( repo_root, paths.audited_gold_root(), allow_dirty=False, annotations_root=paths.annotations_root(), + bird_interact_evaluation_root=bird_interact_eval, + livesqlbench_evaluation_root=livesqlbench_eval, ) uri = image.build_and_push( tag, repo_root, audited_gold_root=paths.audited_gold_root(), annotations_root=paths.annotations_root(), + bird_interact_evaluation_root=bird_interact_eval, + livesqlbench_evaluation_root=livesqlbench_eval, force=ns.force, ) print(uri) diff --git a/src/bird_interact_agents/cloud/driver.py b/src/bird_interact_agents/cloud/driver.py index f4b56ccb..e8db28fd 100644 --- a/src/bird_interact_agents/cloud/driver.py +++ b/src/bird_interact_agents/cloud/driver.py @@ -646,16 +646,21 @@ def submit(args) -> str: slayer_dbs: list[str] = [] if args.query_mode == "slayer": slayer_dbs = _check_slayer_setup_present(args) + bird_interact_eval, livesqlbench_eval = image.default_grader_eval_roots() tag = image.image_tag( repo_root, paths.audited_gold_root(), allow_dirty=args.allow_dirty, annotations_root=paths.annotations_root(), + bird_interact_evaluation_root=bird_interact_eval, + livesqlbench_evaluation_root=livesqlbench_eval, ) image_uri = image.build_and_push( tag, repo_root, audited_gold_root=paths.audited_gold_root(), annotations_root=paths.annotations_root(), + bird_interact_evaluation_root=bird_interact_eval, + livesqlbench_evaluation_root=livesqlbench_eval, force=False, ) # De-bake: upload the benchmark dataset ONCE to its content-hashed GCS @@ -841,16 +846,21 @@ def submit_annotator(args) -> str: ) prereqs.check(_prereq_args) repo_root = submitter_repo_root() + bird_interact_eval, livesqlbench_eval = image.default_grader_eval_roots() tag = image.image_tag( repo_root, paths.audited_gold_root(), allow_dirty=args.allow_dirty, annotations_root=paths.annotations_root(), + bird_interact_evaluation_root=bird_interact_eval, + livesqlbench_evaluation_root=livesqlbench_eval, ) image_uri = image.build_and_push( tag, repo_root, audited_gold_root=paths.audited_gold_root(), annotations_root=paths.annotations_root(), + bird_interact_evaluation_root=bird_interact_eval, + livesqlbench_evaluation_root=livesqlbench_eval, force=False, ) _check_gold_present(get_benchmark(args.benchmark).name) diff --git a/src/bird_interact_agents/cloud/image.py b/src/bird_interact_agents/cloud/image.py index 5b07fcbe..9b720a16 100644 --- a/src/bird_interact_agents/cloud/image.py +++ b/src/bird_interact_agents/cloud/image.py @@ -19,6 +19,18 @@ class DirtyWorktreeError(RuntimeError): the worktree has uncommitted changes touching image-input paths.""" +class UpstreamGraderUnavailableError(RuntimeError): + """Raised by ``build_and_push`` when an upstream grader tree the + Dockerfile expects to bake via BuildKit ``--build-context`` is + missing on the host. Surfacing this at submit time (instead of + letting ``docker build`` fail with an opaque BuildKit error) gives + the user an actionable remediation: clone the upstream tree next to + the main checkout, or set the corresponding env var. Without it the + cascade-tier-N1 dispatch in the cloud actor silently degrades to + legacy ``_set_equal`` — the exact regression this PR was meant to + eliminate. (Codex round 6.)""" + + # Paths whose contents bind into the docker image's CODE layers. _CODE_RELATIVE_PATHS: tuple[str, ...] = ( "src", @@ -35,6 +47,33 @@ def _iter_files_under(root: Path) -> Iterable[Path]: return (p for p in sorted(root.rglob("*")) if p.is_file()) +def _iter_upstream_grader_files(root: Path) -> Iterable[Path]: + """Iterate hashable source files under an upstream-grader tree. + + Filters out ``__pycache__/`` (rebuilt non-deterministically by any + local import, which would otherwise churn the data-layer hash and + force needless rebuilds) and only keeps Python source (the loader + only reads ``.py`` — ``.sh`` / ``.ipynb`` siblings don't influence + the grader's behaviour). + + Symlinks are skipped on purpose: a stray ``foo.py`` symlink whose + target is under ``__pycache__/`` (or a path outside the grader root + entirely) would otherwise bypass both the ``__pycache__`` parts + filter (the symlink's own ``parts`` don't contain it) and the + in-tree assumption (``read_bytes()`` follows the link). Hashing + out-of-context bytes would make the image tag depend on whatever + the symlink resolves to. (Codex round 6.) + """ + if not root.exists(): + return iter(()) + return ( + p for p in sorted(root.rglob("*.py")) + if p.is_file() + and not p.is_symlink() + and "__pycache__" not in p.parts + ) + + def _hash_files(files: Iterable[Path], *, base: Path) -> str: """SHA-256 over (relative_path, bytes) for each file, in sorted order.""" h = hashlib.sha256() @@ -88,6 +127,8 @@ def data_hash( audited_gold_root: Path, *, annotations_root: Path | None = None, + bird_interact_evaluation_root: Path | None = None, + livesqlbench_evaluation_root: Path | None = None, ) -> str: """Content-based hash over the inputs that compose the DATA layers of `Dockerfile.cloud`: ``audited_gold/`` and the Dockerfile's DATA @@ -117,7 +158,16 @@ def data_hash( DEV-1515: ``annotations_root`` is optional; pass ``paths.annotations_root()`` to include annotations content. Defaults to ``None`` (no annotations hashed) - so test callers that don't supply it remain hermetic.""" + so test callers that don't supply it remain hermetic. + + DEV-1550: ``bird_interact_evaluation_root`` / + ``livesqlbench_evaluation_root`` are the upstream grader ``evaluation/`` + subdirs that ``Dockerfile.cloud`` bakes via ``COPY --from=`` so the + cascade tier N1 dispatch can load ``test_utils.py`` + ``db_utils.py`` + in the cloud actor. Hashed under stable on-image keys so the digest + is independent of where the host files live (the build context is + relocatable). Default ``None`` keeps hermetic tests hermetic; the + runtime driver populates both via ``image.build_and_push``.""" h = hashlib.sha256() # audited_gold (main-checkout-anchored, gitignored). Keyed under @@ -145,6 +195,26 @@ def data_hash( h.update(f.read_bytes()) h.update(b"\x00") + # DEV-1550: upstream grader subtrees. Keys mirror the in-image bake + # paths under ``upstream_graders/{bird-interact,livesqlbench}/`` so + # the digest stays the same whether the host root is the developer's + # checkout sibling or a CI mount. ``_iter_upstream_grader_files`` + # filters ``__pycache__`` so a stale .pyc on the host doesn't churn + # the hash. + for grader_root, hash_key_prefix in ( + (bird_interact_evaluation_root, "upstream_graders/bird-interact/"), + (livesqlbench_evaluation_root, "upstream_graders/livesqlbench/"), + ): + if grader_root is None or not grader_root.exists(): + continue + for f in _iter_upstream_grader_files(grader_root): + rel = f.relative_to(grader_root) + h.update(b"repo/") + h.update((hash_key_prefix + rel.as_posix()).encode()) + h.update(b"\x00") + h.update(f.read_bytes()) + h.update(b"\x00") + # Dockerfile DATA-LAYERS section sections = _split_dockerfile_sections(repo_root) h.update(b"dockerfile-data/") @@ -202,14 +272,24 @@ def image_tag( *, allow_dirty: bool, annotations_root: Path | None = None, + bird_interact_evaluation_root: Path | None = None, + livesqlbench_evaluation_root: Path | None = None, ) -> str: """`-` (+ `-dirty` when `allow_dirty=True` and the worktree is dirty). See :func:`data_hash` for why ``audited_gold_root`` is a separate input (worktree-safety). ``annotations_root`` is the DEV-1515 sibling input — - same rationale.""" - dh = data_hash(repo_root, audited_gold_root, annotations_root=annotations_root) + same rationale. ``bird_interact_evaluation_root`` / + ``livesqlbench_evaluation_root`` (DEV-1550) point at the upstream + grader ``evaluation/`` subdirs Dockerfile.cloud bakes into the image.""" + dh = data_hash( + repo_root, + audited_gold_root, + annotations_root=annotations_root, + bird_interact_evaluation_root=bird_interact_evaluation_root, + livesqlbench_evaluation_root=livesqlbench_evaluation_root, + ) ch = code_hash(repo_root, allow_dirty=allow_dirty) tag = f"{dh[:12]}-{ch[:12]}" if allow_dirty and _dirty_image_input_paths(repo_root): @@ -280,6 +360,78 @@ def _dirty_image_input_paths(repo_root: Path) -> set[str]: return dirty +def _ensure_upstream_grader_tree_present( + eval_root: Path, *, env_var: str, tree_label: str, +) -> None: + """Raise :class:`UpstreamGraderUnavailableError` when the upstream + grader tree at ``eval_root`` is missing OR is missing any marker + listed in + :data:`bird_interact_agents.eval.upstream_ex_base.REQUIRED_UPSTREAM_GRADER_MARKERS`. + + Without this check, ``docker build --build-context =`` + fails downstream with an opaque BuildKit error, OR (Codex round 7) + a partial / shallow upstream copy with only ``test_utils.py`` bakes + a degraded image that imports successfully through ``test_utils`` + but later raises on ``from db_utils import ...`` at first cascade + tier-N1 call, downgrading to legacy ``_set_equal``. Catching it up + front lets us point the user at the env-var override + the expected + sibling-of-checkout layout. + + The required marker list lives next to the loader + (:mod:`bird_interact_agents.eval.upstream_ex_base`) so build-time + and runtime checks share a single source of truth. + """ + from bird_interact_agents.eval.upstream_ex_base import ( + REQUIRED_UPSTREAM_GRADER_MARKERS, + ) + + if not eval_root.is_dir(): + raise UpstreamGraderUnavailableError( + f"Upstream {tree_label} grader tree directory not found: " + f"{eval_root}. Either clone the upstream {tree_label} repo " + f"next to the bird-agents main checkout, or set ${env_var} " + f"to the upstream repo root. Without it, the cloud actor's " + f"cascade-tier-N1 dispatch silently falls back to legacy " + f"`_set_equal`." + ) + missing = [ + m for m in REQUIRED_UPSTREAM_GRADER_MARKERS + if not (eval_root / m).is_file() + ] + if missing: + raise UpstreamGraderUnavailableError( + f"Upstream {tree_label} grader tree at {eval_root} is " + f"missing required marker(s): {missing}. The cloud actor " + f"loads these at cascade-tier-N1 call time; without them " + f"the import silently fails and N1 falls back to legacy " + f"`_set_equal`. Re-clone the upstream {tree_label} repo " + f"next to the bird-agents main checkout (or set ${env_var} " + f"to a complete checkout)." + ) + + +def default_grader_eval_roots() -> tuple[Path, Path]: + """Host-side eval-dir paths the cloud build bakes into the image. + + Returns ``(bird_interact_evaluation_root, livesqlbench_evaluation_root)``. + Centralised so the driver / CLI callsites pass the same paths to + :func:`image_tag` (data-layer hash) AND :func:`build_and_push` + (BuildKit ``--build-context``) — drift between the two would mean + rebuilding under a stale tag. + """ + from bird_interact_agents import paths + + bird_interact_eval = ( + paths.bird_interact_upstream_root() + / "mini_interact" / "knowledge_based" + / "mini_interact_conv" / "evaluation" + ) + livesqlbench_eval = ( + paths.livesqlbench_upstream_root() / "evaluation" / "src" + ) + return bird_interact_eval, livesqlbench_eval + + def build_and_push( tag: str, repo_root: Path, @@ -287,6 +439,8 @@ def build_and_push( image_uri_prefix: str | None = None, audited_gold_root: Path | None = None, annotations_root: Path | None = None, + bird_interact_evaluation_root: Path | None = None, + livesqlbench_evaluation_root: Path | None = None, force: bool = False, ) -> str: """Build (if needed) and push the image, returning the full URI. @@ -318,6 +472,39 @@ def build_and_push( audited_gold_root = paths.audited_gold_root() if annotations_root is None: annotations_root = paths.annotations_root() + # DEV-1550: upstream BIRD-Interact + livesqlbench grader subtrees. + # The build contexts point at the host evaluation/ subdirs that + # `Dockerfile.cloud`'s `COPY --from=bird-interact-evaluation` / + # `COPY --from=livesqlbench-evaluation` lines bake into the image. + # `_MINI_INTERACT_REL` / `_LIVESQLBENCH_REL` in `upstream_ex_base` + # are anchored under the eval grader root, so the dirs we point at + # here must be the deeper `evaluation/` subdirs, not the upstream + # repo roots. + if bird_interact_evaluation_root is None: + bird_interact_evaluation_root = ( + paths.bird_interact_upstream_root() + / "mini_interact" / "knowledge_based" + / "mini_interact_conv" / "evaluation" + ) + if livesqlbench_evaluation_root is None: + livesqlbench_evaluation_root = ( + paths.livesqlbench_upstream_root() / "evaluation" / "src" + ) + # Fail-fast prereq check (Codex round 6): a missing upstream-grader + # tree would otherwise blow up `docker build` with an opaque BuildKit + # error about an unresolved `--build-context`. Surface an actionable + # message naming the env-var remediation so the user can clone the + # tree or repoint the var instead of decoding BuildKit's output. + _ensure_upstream_grader_tree_present( + bird_interact_evaluation_root, + env_var="BIRD_BIRD_INTERACT_ROOT", + tree_label="BIRD-Interact", + ) + _ensure_upstream_grader_tree_present( + livesqlbench_evaluation_root, + env_var="BIRD_LIVESQLBENCH_ROOT", + tree_label="livesqlbench", + ) uri = f"{image_uri_prefix}:{tag}" if not force: probe = subprocess.run( @@ -332,6 +519,10 @@ def build_and_push( "docker", "build", "--build-context", f"audited-gold={audited_gold_root}", "--build-context", f"annotations={annotations_root}", + "--build-context", + f"bird-interact-evaluation={bird_interact_evaluation_root}", + "--build-context", + f"livesqlbench-evaluation={livesqlbench_evaluation_root}", "-t", uri, "-f", "Dockerfile.cloud", ".", diff --git a/src/bird_interact_agents/eval/annotate.py b/src/bird_interact_agents/eval/annotate.py index d6b98b4d..7ddae43b 100644 --- a/src/bird_interact_agents/eval/annotate.py +++ b/src/bird_interact_agents/eval/annotate.py @@ -519,6 +519,7 @@ def _grader( # noqa: E731 submitted_sql=submitted_sql, db_path=_db_path, conn=None, + conditions=_row.get("conditions"), ) dest = write_submission_skeleton( diff --git a/src/bird_interact_agents/eval/autopsy.py b/src/bird_interact_agents/eval/autopsy.py index 72809161..17d0540a 100644 --- a/src/bird_interact_agents/eval/autopsy.py +++ b/src/bird_interact_agents/eval/autopsy.py @@ -472,87 +472,73 @@ def _map_output( ) -# JSON schema for the autopsy_output tool (derived from AutopsyLLMOutput). -_AUTOPSY_TOOL_SCHEMA = { - "name": "autopsy_output", - "description": "Report the root-cause analysis of the agent failure.", - "input_schema": { - "type": "object", - "properties": { - "pattern": { - "type": "string", - "enum": [ - "never_asked_key_question", - "asked_but_ignored_answer", - "user_sim_misleading", - "late_mutation_corrupted_result", - "wrong_join_path", - "output_schema_misread", - "slayer_generation_artifact", - "slayer_overaggregation", - "exhausted_budget_guessing", - "other", - ], - }, - "other_details": {"type": ["string", "null"]}, - "narrative": {"type": "string"}, - "remediation": {"type": "string"}, - "decision_point_trajectory_index": {"type": ["integer", "null"]}, - "decision_point_description": {"type": ["string", "null"]}, - "n_asks": {"type": "integer", "default": 0}, - "key_asks": { - "type": "array", - "items": { - "type": "object", - "properties": { - "trajectory_idx": {"type": "integer"}, - "summary": {"type": "string"}, - }, - "required": ["trajectory_idx", "summary"], - }, - }, - "disclosed_resolutions": {"type": "array", "items": {"type": "string"}}, - "undisclosed_resolutions": {"type": "array", "items": {"type": "string"}}, - }, - "required": [ - "pattern", "narrative", "remediation", - "key_asks", "disclosed_resolutions", "undisclosed_resolutions", - ], - }, -} - - -# DEV-1541: one-shot tool schema. Drops four ask_user-shaped properties -# (n_asks, key_asks, disclosed_resolutions, undisclosed_resolutions) and -# three of those from `required` (n_asks was never required). Pattern -# enum is the six-value subset. -_AUTOPSY_TOOL_SCHEMA_ONE_SHOT = { - "name": "autopsy_output", - "description": "Report the root-cause analysis of the agent failure.", - "input_schema": { - "type": "object", - "properties": { - "pattern": { - "type": "string", - "enum": [ - "late_mutation_corrupted_result", - "wrong_join_path", - "output_schema_misread", - "slayer_generation_artifact", - "slayer_overaggregation", - "exhausted_budget_guessing", - "other", - ], - }, - "other_details": {"type": ["string", "null"]}, - "narrative": {"type": "string"}, - "remediation": {"type": "string"}, - "decision_point_trajectory_index": {"type": ["integer", "null"]}, - "decision_point_description": {"type": ["string", "null"]}, - }, - "required": ["pattern", "narrative", "remediation"], - }, -} +def _inline_refs(schema: dict) -> dict: + """Resolve and inline ``$ref``/``$defs`` in a JSON Schema. + + Pydantic's ``model_json_schema()`` emits ``$ref``s for nested models. + Anthropic's tool ``input_schema`` accepts standard JSON Schema, but + inlining keeps the schema flat — fewer moving parts, easier to read + in error logs, and no chance of a future SDK version stumbling on a + discovery step.""" + defs = schema.pop("$defs", {}) + + def resolve(node: object) -> object: + if isinstance(node, dict): + ref = node.get("$ref") + if isinstance(ref, str) and ref.startswith("#/$defs/"): + key = ref.split("/")[-1] + if key not in defs: + raise ValueError( + f"Unresolved $ref: {ref}; " + f"$defs keys = {sorted(defs)}" + ) + return resolve(defs[key]) + return {k: resolve(v) for k, v in node.items()} + if isinstance(node, list): + return [resolve(x) for x in node] + return node + + return resolve(schema) # type: ignore[return-value] + + +def _pydantic_to_tool_schema( + model_cls: type[BaseModel], + *, + name: str, + description: str, +) -> dict: + """Generate an Anthropic tool descriptor from a Pydantic model. + + Hand-mirroring drifts: a new field or pattern-enum value on the + Pydantic model silently diverges from the hand-written tool schema; + the LLM returns output the (looser) tool schema accepts but Pydantic + rejects, and the autopsy lands as ``validation_error`` (the failure + that motivated this helper). Generating from the model keeps a single + source of truth.""" + return { + "name": name, + "description": description, + "input_schema": _inline_refs(model_cls.model_json_schema()), + } + + +_AUTOPSY_TOOL_SCHEMA = _pydantic_to_tool_schema( + AutopsyLLMOutput, + name="autopsy_output", + description="Report the root-cause analysis of the agent failure.", +) + + +# DEV-1541: one-shot variant drops the four ``ask_user``-shaped fields +# (``n_asks``, ``key_asks``, ``disclosed_resolutions``, +# ``undisclosed_resolutions``) and restricts ``pattern`` to the six +# one-shot-valid values. Both the field set and the enum live on +# ``AutopsyLLMOutputOneShot`` — the tool schema follows automatically. +_AUTOPSY_TOOL_SCHEMA_ONE_SHOT = _pydantic_to_tool_schema( + AutopsyLLMOutputOneShot, + name="autopsy_output", + description="Report the root-cause analysis of the agent failure.", +) # --------------------------------------------------------------------------- @@ -636,6 +622,9 @@ async def run_autopsy( # never completed. kb_text = "" prompt = "" + tool_schema: dict = {} + schema_cls: type[BaseModel] = AutopsyLLMOutput + client: Optional["anthropic.AsyncAnthropic"] = None try: kb_text = _read_kb_text( slayer_storage_dir, @@ -656,49 +645,9 @@ async def run_autopsy( AutopsyLLMOutputOneShot if is_one_shot else AutopsyLLMOutput ) client = _build_anthropic_client() - response = await client.messages.create( - model=native_model_id(model), - max_tokens=2048, - tools=[tool_schema], - tool_choice={"type": "tool", "name": "autopsy_output"}, - messages=[{"role": "user", "content": prompt}], - ) - except anthropic.BadRequestError as exc: - kind = "context_overflow" if _looks_like_context_overflow(exc) else "api_error" - logger.error( - "[autopsy] BadRequestError on %s (kind=%s): %s", - task_annotation.instance_id, kind, exc, - exc_info=True, - ) - return _autopsy_error_result( - kind=kind, exc=exc, prompt=prompt, kb_text=kb_text, - trajectory=trajectory, model=model, - ) - except anthropic.APIConnectionError as exc: - # Codex r1 #5: must be ordered BEFORE APIError, since - # APIConnectionError is a sibling (not subclass) of APIStatusError - # in the anthropic SDK; APITimeoutError is a subclass of - # APIConnectionError and resolves here too. - logger.error( - "[autopsy] APIConnectionError on %s: %s", - task_annotation.instance_id, exc, exc_info=True, - ) - return _autopsy_error_result( - kind="network_error", exc=exc, prompt=prompt, kb_text=kb_text, - trajectory=trajectory, model=model, - ) - except anthropic.APIError as exc: - logger.error( - "[autopsy] APIError on %s: %s", - task_annotation.instance_id, exc, exc_info=True, - ) - return _autopsy_error_result( - kind="api_error", exc=exc, prompt=prompt, kb_text=kb_text, - trajectory=trajectory, model=model, - ) except Exception as exc: # noqa: BLE001 logger.error( - "[autopsy] unexpected error on %s", + "[autopsy] prep failed on %s", task_annotation.instance_id, exc_info=True, ) return _autopsy_error_result( @@ -706,48 +655,154 @@ async def run_autopsy( trajectory=trajectory, model=model, ) - try: - tool_use = next( - b for b in response.content if getattr(b, "type", None) == "tool_use" - ) - except StopIteration as exc: - logger.error( - "[autopsy] no tool_use block in response on %s", - task_annotation.instance_id, exc_info=True, - ) - return _autopsy_error_result( - kind="missing_tool_use", exc=exc, prompt=prompt, kb_text=kb_text, - trajectory=trajectory, model=model, - ) - except Exception as exc: # noqa: BLE001 - logger.error( - "[autopsy] iterating response.content failed on %s", - task_annotation.instance_id, exc_info=True, - ) - return _autopsy_error_result( - kind="unknown", exc=exc, prompt=prompt, kb_text=kb_text, - trajectory=trajectory, model=model, - ) + assert client is not None # the prep try/except returned otherwise. + + # One LLM call + one corrective retry on Pydantic validation failure. + # The retry sends the validation error back via a ``tool_result`` block + # with ``is_error=True`` so the model sees exactly which fields it + # dropped. Anthropic's ``required`` enforcement on tool input is + # best-effort; with long prompts the model occasionally omits a + # leading field (this was the ``archeology_10`` regression — 353k + # chars, ``pattern`` missing). Other failure kinds (API errors, + # missing ``tool_use`` block, BadRequest) do NOT retry — they are + # not model self-correctable. + messages: list = [{"role": "user", "content": prompt}] + last_validation_exc: Optional[pydantic.ValidationError] = None + for attempt in range(2): + try: + response = await client.messages.create( + model=native_model_id(model), + max_tokens=2048, + tools=[tool_schema], + tool_choice={"type": "tool", "name": "autopsy_output"}, + messages=messages, + ) + except anthropic.BadRequestError as exc: + kind = "context_overflow" if _looks_like_context_overflow(exc) else "api_error" + logger.error( + "[autopsy] BadRequestError on %s (kind=%s): %s", + task_annotation.instance_id, kind, exc, + exc_info=True, + ) + return _autopsy_error_result( + kind=kind, exc=exc, prompt=prompt, kb_text=kb_text, + trajectory=trajectory, model=model, + ) + except anthropic.APIConnectionError as exc: + # Codex r1 #5: must be ordered BEFORE APIError, since + # APIConnectionError is a sibling (not subclass) of + # APIStatusError in the anthropic SDK; APITimeoutError is a + # subclass of APIConnectionError and resolves here too. + logger.error( + "[autopsy] APIConnectionError on %s: %s", + task_annotation.instance_id, exc, exc_info=True, + ) + return _autopsy_error_result( + kind="network_error", exc=exc, prompt=prompt, kb_text=kb_text, + trajectory=trajectory, model=model, + ) + except anthropic.APIError as exc: + logger.error( + "[autopsy] APIError on %s: %s", + task_annotation.instance_id, exc, exc_info=True, + ) + return _autopsy_error_result( + kind="api_error", exc=exc, prompt=prompt, kb_text=kb_text, + trajectory=trajectory, model=model, + ) + except Exception as exc: # noqa: BLE001 + logger.error( + "[autopsy] unexpected error on %s", + task_annotation.instance_id, exc_info=True, + ) + return _autopsy_error_result( + kind="unknown", exc=exc, prompt=prompt, kb_text=kb_text, + trajectory=trajectory, model=model, + ) - try: - llm_output = schema_cls.model_validate(tool_use.input) - return _map_output(llm_output, is_one_shot=is_one_shot) - except pydantic.ValidationError as exc: - logger.error( - "[autopsy] LLM output failed schema validation on %s: %s", - task_annotation.instance_id, exc, - exc_info=True, - ) - return _autopsy_error_result( - kind="validation_error", exc=exc, prompt=prompt, kb_text=kb_text, - trajectory=trajectory, model=model, - ) - except Exception as exc: # noqa: BLE001 - logger.error( - "[autopsy] mapping LLM output failed on %s", - task_annotation.instance_id, exc_info=True, - ) - return _autopsy_error_result( - kind="unknown", exc=exc, prompt=prompt, kb_text=kb_text, - trajectory=trajectory, model=model, - ) + try: + tool_use = next( + b for b in response.content if getattr(b, "type", None) == "tool_use" + ) + except StopIteration as exc: + logger.error( + "[autopsy] no tool_use block in response on %s", + task_annotation.instance_id, exc_info=True, + ) + return _autopsy_error_result( + kind="missing_tool_use", exc=exc, prompt=prompt, kb_text=kb_text, + trajectory=trajectory, model=model, + ) + except Exception as exc: # noqa: BLE001 + logger.error( + "[autopsy] iterating response.content failed on %s", + task_annotation.instance_id, exc_info=True, + ) + return _autopsy_error_result( + kind="unknown", exc=exc, prompt=prompt, kb_text=kb_text, + trajectory=trajectory, model=model, + ) + + try: + llm_output = schema_cls.model_validate(tool_use.input) + return _map_output(llm_output, is_one_shot=is_one_shot) + except pydantic.ValidationError as exc: + last_validation_exc = exc + logger.warning( + "[autopsy] LLM output failed schema validation on %s " + "(attempt %d/2): %s", + task_annotation.instance_id, attempt + 1, exc, + ) + if attempt == 0: + # Append the model's failed tool_use turn, then a user + # turn carrying the validation error as a tool_result. + # ``is_error=True`` signals the model that the previous + # call was rejected — Anthropic's tool-use docs recommend + # exactly this shape for corrective retries. + messages.append({"role": "assistant", "content": response.content}) + messages.append({ + "role": "user", + "content": [{ + "type": "tool_result", + "tool_use_id": tool_use.id, + "content": ( + "Your previous autopsy_output failed Pydantic " + "schema validation:\n\n" + f"{exc}\n\n" + "Return a corrected autopsy_output that " + "satisfies the schema. Every field listed in " + "the tool's `required` array MUST be present." + ), + "is_error": True, + }], + }) + continue + logger.error( + "[autopsy] LLM output failed schema validation on %s " + "after retry: %s", + task_annotation.instance_id, exc, + exc_info=True, + ) + return _autopsy_error_result( + kind="validation_error", exc=exc, prompt=prompt, kb_text=kb_text, + trajectory=trajectory, model=model, + ) + except Exception as exc: # noqa: BLE001 + logger.error( + "[autopsy] mapping LLM output failed on %s", + task_annotation.instance_id, exc_info=True, + ) + return _autopsy_error_result( + kind="unknown", exc=exc, prompt=prompt, kb_text=kb_text, + trajectory=trajectory, model=model, + ) + + # Unreachable in practice (the loop returns on every branch), but + # falls through here if the retry-loop bound is ever raised without + # re-checking the validation-error return path. + assert last_validation_exc is not None + return _autopsy_error_result( + kind="validation_error", exc=last_validation_exc, + prompt=prompt, kb_text=kb_text, + trajectory=trajectory, model=model, + ) diff --git a/src/bird_interact_agents/eval/grade_in_place.py b/src/bird_interact_agents/eval/grade_in_place.py index 56d1ec5d..2ffb19d6 100644 --- a/src/bird_interact_agents/eval/grade_in_place.py +++ b/src/bird_interact_agents/eval/grade_in_place.py @@ -528,6 +528,7 @@ def grade_and_write( llm_judge: Any = None, epsilon: float = 1e-6, autopsy_result: Optional["AutopsyResult"] = None, + conditions: Optional[dict] = None, ) -> Path: """Run the tolerant grader and write the SubmissionAnnotation to ``//submission_annotation.json``. @@ -577,6 +578,7 @@ def grade_and_write( llm_judge=llm_judge, epsilon=epsilon, user_sim_n_asks=_user_sim_n_asks, + conditions=conditions, ) ann = _build_submission_annotation( task_annotation=task_annotation, @@ -919,4 +921,8 @@ def grade_one_submission( user_sim_interaction=user_sim_interaction, config=config, autopsy_result=autopsy_result, + # DEV-1550 round-2 (Codex): order-sensitive tasks carry + # `conditions={"order": True}`. Without this forward, the new + # ex_base N1 path would silently grade them as set-dedup. + conditions=task_data.get("conditions"), ) diff --git a/src/bird_interact_agents/eval/regrade.py b/src/bird_interact_agents/eval/regrade.py index 5412b3fd..fee9e2b3 100644 --- a/src/bird_interact_agents/eval/regrade.py +++ b/src/bird_interact_agents/eval/regrade.py @@ -467,6 +467,7 @@ def _grader(*, instance_id: str, submitted_sql: str, task_row: dict, **_kw): executor=_executor, benchmark=_bench, user_sim_n_asks=_user_sim_n_asks, + conditions=task_row.get("conditions"), ) report = regrade_run( diff --git a/src/bird_interact_agents/eval/tolerant_grader.py b/src/bird_interact_agents/eval/tolerant_grader.py index a64255b1..8807cf6a 100644 --- a/src/bird_interact_agents/eval/tolerant_grader.py +++ b/src/bird_interact_agents/eval/tolerant_grader.py @@ -18,6 +18,7 @@ import logging import os import sqlite3 +import threading from collections import Counter from pathlib import Path from typing import Any, Callable, Iterable, List, Optional, Protocol, Sequence, Tuple @@ -38,6 +39,18 @@ logger = logging.getLogger(__name__) +# Per-process dedup for the `ExBaseUnavailableError` warning emitted from +# the N1 dispatch catch site. The error fires once per grading instance +# (10s–100s per run); without dedup the log fills with identical lines. +# Keyed on the rendered message text so distinct failure shapes (missing +# tree vs partial markers vs stale override) each surface exactly once. +# The lock protects the check-then-add sequence against any future +# concurrent caller — today's grading paths are process-parallel (Ray +# actors), but the cost of pre-empting a threadpool refactor is one +# lock acquire per ex_base failure, which is negligible. (Codex round 10.) +_EX_BASE_UNAVAILABLE_SEEN: set[str] = set() +_EX_BASE_UNAVAILABLE_SEEN_LOCK = threading.Lock() + # --------------------------------------------------------------------------- # ORDER BY parser @@ -205,6 +218,154 @@ def _set_equal(pred: Sequence[Sequence], gold: Sequence[Sequence]) -> bool: ) +# --------------------------------------------------------------------------- +# N1 dispatch: upstream `ex_base` for mini-interact + livesqlbench; +# legacy `_set_equal` everywhere else / on shim failure. +# --------------------------------------------------------------------------- + +# Re-imported into the module namespace (rather than imported lazily inside +# `_compute_n1`) so tests can monkeypatch +# `tolerant_grader.compare_pred_vs_gold_ex_base` directly. +try: + from bird_interact_agents.eval.upstream_ex_base import ( + ExBaseUnavailableError, + compare_pred_vs_gold_ex_base, + is_mutation_sql, # noqa: F401 (re-export for callers) + ) +except Exception: # noqa: BLE001 (defensive — module-load failure) + class ExBaseUnavailableError(Exception): # type: ignore[no-redef] + pass + + def compare_pred_vs_gold_ex_base(**_kw): # type: ignore[no-redef] + raise ExBaseUnavailableError("upstream_ex_base shim unavailable") + + def is_mutation_sql(_sql: str) -> bool: # type: ignore[no-redef] + # When the shim isn't importable we can't even check; treat as + # non-mutation so grading degrades to the legacy comparison + # via the ExBaseUnavailableError path below. + return False + + +_EX_BASE_N1_BENCHMARKS = frozenset({ + "mini-interact", + "livesqlbench-base-lite-sqlite", + "livesqlbench-base-lite", + "livesqlbench-base-full", + "livesqlbench-large", +}) + + +def _compute_n1( + *, + benchmark: Any, + pred_sqls: List[str], + sol_sqls: List[str], + db_path: Path, + conn: Any, + pred_rows: Sequence[Sequence], + orig_rows: Sequence[Sequence], + conditions: Optional[dict] = None, +) -> bool: + """Compute N1 via upstream ``ex_base`` for supported benchmarks; on + any failure (unsupported benchmark, missing upstream tree, missing + SQL strings, mutation-bearing SQL) fall back to the legacy multiset + row comparison on the already-fetched ``pred_rows`` / ``orig_rows``. + + ``conditions`` forwards through to upstream's ``ex_base`` so + order-sensitive tasks (``conditions={"order": True}``) are graded + positionally instead of with set-dedup semantics (Codex / CodeRabbit + round 2). Defaults to ``None`` (set-dedup), matching upstream when + the task source row carries no override. + """ + benchmark_name = getattr(benchmark, "name", None) or str(benchmark or "") + if ( + benchmark_name not in _EX_BASE_N1_BENCHMARKS + or not pred_sqls + or not sol_sqls + ): + return _set_equal(pred_rows, orig_rows) + # Mutation-bearing SQL would commit through the upstream's writeable + # SQLite open path AND any caller-supplied conn (the upstream + # ``execute_queries`` runs pred first, then gold, on the same conn). + # The cascade's own primary executor already gave us pre-mutation + # rows; stay on the legacy comparison to keep grading honest and to + # avoid persisting state into the benchmark DB. (Codex round-2 + # finding on cloud inline grader; the backfill script has its own + # belt-and-braces skip.) + if any(is_mutation_sql(s) for s in pred_sqls): + return _set_equal(pred_rows, orig_rows) + if any(is_mutation_sql(s) for s in sol_sqls): + return _set_equal(pred_rows, orig_rows) + # If we have no path to a real DB and no caller-supplied conn, the + # upstream ``ex_base`` cannot execute. Stay on the legacy path so + # stubbed-executor callers (tests, scripted regrades on synthetic + # rows) keep producing a verdict. + # + # Only check ``db_path.is_file()`` for SQLite-backed benchmarks. + # Postgres callers (cloud actor, livesqlbench non-sqlite variants) + # pass ``db_path = Path()`` — a DB-name carrier, NOT a + # filesystem path — and ``conn=None``; upstream livesqlbench's + # ``perform_query_on_postgresql_databases`` auto-opens from a + # connection pool when conn is None, so the dispatcher does not need + # to gate on file existence there. Without this carve-out, every + # Postgres livesqlbench task silently fell back to legacy + # ``_set_equal`` despite being listed in + # ``_EX_BASE_N1_BENCHMARKS`` (Codex round 3). + is_postgres = getattr(benchmark, "db_backend", "sqlite") == "postgres" + if conn is None and not is_postgres: + try: + if not db_path or not Path(db_path).is_file(): + return _set_equal(pred_rows, orig_rows) + except Exception: # noqa: BLE001 + return _set_equal(pred_rows, orig_rows) + # Postgres-backed benchmarks (livesqlbench non-sqlite, bird-interact + # full/lite-exp) get a DB stem; SQLite-backed benchmarks get the + # full path. Upstream livesqlbench ``perform_query_on_postgresql_ + # databases`` switches connections via the DB NAME, not via a + # filesystem path — passing ``str(db_path)`` there silently routes + # to the wrong DB (CodeRabbit round 2). + if getattr(benchmark, "db_backend", "sqlite") == "postgres": + db_name = db_path.stem + else: + db_name = str(db_path) + try: + return compare_pred_vs_gold_ex_base( + benchmark=benchmark_name, + pred_sqls=pred_sqls, + sol_sqls=sol_sqls, + db_name=db_name, + conn=conn, + conditions=conditions, + ) + except ExBaseUnavailableError as exc: + # Codex round 9: the round-8 resolver raises detailed + # actionable errors when no upstream candidate validates, but + # this catch site was silently downgrading to legacy + # `_set_equal` — so the operator never saw why N1 wasn't + # engaging. Emit ONE warning per unique error message + # (per-process dedup) so the actionable detail surfaces in + # logs without filling them with N1 dispatch retries. + msg = str(exc) + with _EX_BASE_UNAVAILABLE_SEEN_LOCK: + should_log = msg not in _EX_BASE_UNAVAILABLE_SEEN + if should_log: + _EX_BASE_UNAVAILABLE_SEEN.add(msg) + if should_log: + logger.warning( + "[N1 dispatch] cascade tier N1 is downgrading to legacy " + "_set_equal because the upstream ex_base grader is " + "unavailable. Remediate to engage strict N1 grading:\n%s", + msg, + ) + return _set_equal(pred_rows, orig_rows) + except Exception: # noqa: BLE001 (defensive — never crash grading) + logger.exception( + "[N1 dispatch] compare_pred_vs_gold_ex_base raised; " + "falling back to legacy _set_equal" + ) + return _set_equal(pred_rows, orig_rows) + + def _canonical_repr(row: Sequence) -> str: """Stable string repr for a row so heterogeneous tuples sort.""" return "|".join(repr(c) for c in row) @@ -824,6 +985,7 @@ def grade_submission( llm_judge: Optional[Any] = None, epsilon: float = 1e-6, user_sim_n_asks: Optional[int] = None, + conditions: Optional[dict] = None, ) -> CascadeVerdict: """Compute the 8-row cascade for a single submission. @@ -922,7 +1084,16 @@ def grade_submission( if not original_sol_sql or not original_sql_executed_ok: n1 = False else: - n1 = _set_equal(pred_rows, orig_rows) + n1 = _compute_n1( + benchmark=benchmark, + pred_sqls=[submitted_sql] if submitted_sql else [], + sol_sqls=list(original_sol_sql), + db_path=db_path, + conn=conn, + pred_rows=pred_rows, + orig_rows=orig_rows, + conditions=conditions, + ) # 3) N2/N3 — audited primary / any variant strict. primary = next( diff --git a/src/bird_interact_agents/eval/upstream_ex_base.py b/src/bird_interact_agents/eval/upstream_ex_base.py new file mode 100644 index 00000000..43ce33a9 --- /dev/null +++ b/src/bird_interact_agents/eval/upstream_ex_base.py @@ -0,0 +1,658 @@ +"""Shim around BIRD-Interact's upstream graders so cascade tier N1 lines +up with the harness our reported numbers compare to. + +Tier N1 ("phase1_against_original_gold") used to be a bag-equality on +``repr(cell)`` over the in-process pred / gold row sets. Upstream's +``test_case_default`` actually does more: + +* strips comments / ``DISTINCT`` / ``ROUND`` from BOTH SQLs; +* runs ``preprocess_results`` (2-dp Decimal/float rounding, date + normalisation, dict/list canonicalisation); +* compares ``set(...) == set(...)``, i.e. dedup. + +For mini-interact (SQLite) the upstream lives at +``BIRD-Interact/mini_interact/knowledge_based/mini_interact_conv/ +evaluation/test_utils.py``; for the livesqlbench family (Postgres + the +sqlite-shimmed lite variant) at +``livesqlbench/evaluation/src/test_utils.py``. Both expose the same +``ex_base`` / ``remove_*`` API — the only delta is the driver. + +Root resolution (per-tree, in order): + +1. ``$BIRD_BIRD_INTERACT_ROOT`` / ``$BIRD_LIVESQLBENCH_ROOT`` env var if set. +2. The in-image bake path under ``/app/upstream_graders/{bird-interact, + livesqlbench}/`` (populated by ``Dockerfile.cloud`` via BuildKit + ``--build-context``) — wins inside the cloud actor. +3. Sibling of the main checkout via ``paths.bird_interact_root()`` / + ``paths.livesqlbench_root()`` — common local-dev layout. + +NEVER bake author-private absolute paths into the defaults: if the cloud +image silently fell back to legacy ``_set_equal`` because the upstream +tree was unreachable, the N1 cascade tier would report fake numbers. + +Deliberate deviation from upstream: when BOTH preprocessed result lists +come out empty, the shim returns ``True`` (matches our legacy +``_set_equal([], [])`` behaviour). Upstream returns ``0`` in that +branch; the deviation keeps zero-row gold/pred matches as passes the way +the existing cascade analysis was scored. + +For the Postgres livesqlbench variants the caller is responsible for +passing a fresh psycopg2 connection; the shim issues +``conn.rollback()`` in ``try/finally`` so mutation-bearing prediction +SQL cannot leak into the next grade on the same conn. +""" + +from __future__ import annotations + +import importlib.util +import logging +import os +import re +import sys +import threading +from pathlib import Path +from types import ModuleType +from typing import Optional, Sequence + +logger = logging.getLogger(__name__) + + +class ExBaseUnavailableError(Exception): + """The upstream grader could not be loaded or the benchmark is not + in the ex_base-backed N1 supported set. Callers (the N1 dispatch in + ``tolerant_grader``) catch this and fall back to the legacy + ``_set_equal`` path so a missing upstream tree never crashes + grading.""" + + +# --------------------------------------------------------------------------- +# is_mutation_sql — regex on SQL keywords that imply DB state change. +# --------------------------------------------------------------------------- + +_MUTATION_KEYWORDS = ( + "INSERT", "UPDATE", "DELETE", "CREATE", "DROP", + "ALTER", "TRUNCATE", "REPLACE", +) + +# Match a mutation keyword AT statement start: either after ``;`` +# (with optional whitespace) or at the very beginning of the SQL. +# Keeps ``SELECT REPLACE(...)`` (function call inside a SELECT) from +# being misclassified as a mutation while still catching real +# ``INSERT INTO`` / ``UPDATE`` / multi-statement ``...; DELETE ...``. +_MUTATION_AT_STMT_START_RE = re.compile( + r"(?:^|;)\s*(?:" + "|".join(_MUTATION_KEYWORDS) + r")(?![A-Za-z0-9_])", + re.IGNORECASE, +) + +# CTE-prefixed mutations: ``WITH x AS (...) DELETE FROM t WHERE ...`` +# (SQLite + Postgres both accept this). The statement-start regex above +# only sees ``WITH``, so we also scan for a verb-target pair anywhere in +# the cleaned SQL. The verb-target shape is tight enough that ordinary +# SELECT clauses don't trip it (``SELECT INSERT_NUM FROM t`` doesn't +# contain ``INSERT INTO``). (Codex round 5.) +_MUTATION_VERB_TARGET_RE = re.compile( + r"\b(?:" + r"INSERT\s+INTO|" + r"REPLACE\s+INTO|" + r"UPDATE\s+[A-Za-z_][\w.]*\s+SET|" + r"DELETE\s+FROM|" + r"CREATE\s+(?:TEMP\s+|TEMPORARY\s+|OR\s+REPLACE\s+)?" + r"(?:TABLE|VIEW|INDEX|TRIGGER|SCHEMA|DATABASE)|" + r"DROP\s+(?:TABLE|VIEW|INDEX|TRIGGER|SCHEMA|DATABASE)|" + r"ALTER\s+(?:TABLE|VIEW|INDEX|SCHEMA)|" + r"TRUNCATE(?:\s+TABLE)?\s+[A-Za-z_]" + r")\b", + re.IGNORECASE, +) + +# SQL comment forms upstream's `remove_comments` strips before exec: +# ``-- ... `` (single-line) and ``/* ... */`` (multi-line, non-greedy +# across newlines). Strip both before the mutation regex match so a +# commented mutation (``-- explanation\nINSERT INTO ...``) is still +# caught (Codex round 3). Upstream's exec path also drops the comments, +# so without this strip the dispatcher would think the SQL is read-only +# but upstream's cleaned-up SQL would still execute the mutation. +_SQL_LINE_COMMENT_RE = re.compile(r"--[^\n]*") +_SQL_BLOCK_COMMENT_RE = re.compile(r"/\*.*?\*/", re.DOTALL) + + +def _strip_sql_comments(sql: str) -> str: + no_block = _SQL_BLOCK_COMMENT_RE.sub("", sql) + return _SQL_LINE_COMMENT_RE.sub("", no_block) + + +def is_mutation_sql(sql: str) -> bool: + """Return True iff ``sql`` carries an SQL mutation. + + Detects two shapes: + + 1. ``MUTATION_KEYWORD`` as the LEADING token of any statement + (statements split on ``;``). A SELECT calling ``REPLACE(...)`` + as a function is NOT a mutation — only ``REPLACE INTO`` + (statement-leading) is. + 2. A verb-target pair (``DELETE FROM``, ``INSERT INTO``, + ``UPDATE SET``, ``CREATE TABLE/VIEW/...``, etc.) anywhere + in the cleaned SQL — catches CTE-prefixed mutations like + ``WITH x AS (...) DELETE FROM t WHERE id IN (SELECT id FROM x)`` + that the statement-start regex misses because ``WITH`` itself + isn't a mutation keyword (Codex round 5). + + Comments (``-- ...`` / ``/* ... */``) are stripped before matching + so ``-- explanation\\nINSERT INTO ...`` is still recognised as a + mutation. Mirrors upstream's ``remove_comments`` cleanup so the + dispatcher and the exec path agree on what counts as a mutation. + """ + if not sql: + return False + cleaned = _strip_sql_comments(sql) + if _MUTATION_AT_STMT_START_RE.search(cleaned): + return True + return bool(_MUTATION_VERB_TARGET_RE.search(cleaned)) + + +# --------------------------------------------------------------------------- +# Lazy upstream module loaders +# --------------------------------------------------------------------------- + +# In-image bake locations: Dockerfile.cloud's `COPY --from=` lines +# (matched 1:1 by `cloud.image.build_and_push`'s BuildKit +# `--build-context` flags) populate these dirs at image build time. The +# host-tree directory structure is preserved (so `_MINI_INTERACT_REL` and +# `_LIVESQLBENCH_REL` resolve identically on cloud and on the developer +# laptop). Tests pin these against author-private paths via +# `tests/eval/test_upstream_ex_base.py::test_default_roots_*`. +_CLOUD_BIRD_INTERACT_ROOT = Path("/app/upstream_graders/bird-interact") +_CLOUD_LIVESQLBENCH_ROOT = Path("/app/upstream_graders/livesqlbench") + +_MINI_INTERACT_REL = ( + "mini_interact/knowledge_based/mini_interact_conv/evaluation/test_utils.py" +) +_LIVESQLBENCH_REL = "evaluation/src/test_utils.py" + +# Marker files the upstream loader actually depends on at runtime. +# ``_load_module_from_file`` executes ``test_utils.py``, which does a +# bare ``from db_utils import ...`` — resolved through +# ``sys_path_addition=``. If ``db_utils.py`` is missing the +# import explodes at first cascade-tier-N1 call. Both build-time guard +# (``image._ensure_upstream_grader_tree_present``) and runtime resolver +# (``_resolve_upstream_root``) consult this list, so neither path can +# silently accept a partial upstream tree (Codex rounds 7 + 8). +REQUIRED_UPSTREAM_GRADER_MARKERS: tuple[str, ...] = ( + "test_utils.py", + "db_utils.py", +) + + +def _candidate_has_complete_grader(root: Path, marker_rel: str) -> bool: + """Return True iff every entry in + :data:`REQUIRED_UPSTREAM_GRADER_MARKERS` is present under the eval + dir that ``marker_rel`` resolves to (the upstream loader's + `sys_path_addition`). Mirrors the build-time guard in + ``cloud.image`` — every branch of :func:`_resolve_upstream_root` + that returns a candidate root must validate against this so the + runtime resolver never silently accepts a degraded tree (Codex + round 8).""" + eval_dir = (root / marker_rel).parent + return all( + (eval_dir / m).is_file() + for m in REQUIRED_UPSTREAM_GRADER_MARKERS + ) + + +def _resolve_upstream_root( + env_var: str, + cloud_path: Path, + sibling_name: str, + *, + marker_rel: str, +) -> Path: + """Resolve the host root for one upstream tree. + + Order tried: env override → in-image bake path (cloud actor) → + sibling of main checkout (local dev convention). Every candidate is + validated against :data:`REQUIRED_UPSTREAM_GRADER_MARKERS` at the + eval dir derived from ``marker_rel`` — the first complete tree + wins. If no candidate carries the full marker set we raise + :class:`ExBaseUnavailableError` with the per-candidate failure list + so the operator can see exactly which tree was incomplete and how + to remediate (rather than silently degrading cascade tier N1 to + legacy ``_set_equal``). + + Validating every candidate — not just the cloud bake — is the + Codex round 8 tightening: a stale ``$BIRD_*_ROOT`` override or an + incomplete sibling checkout previously masked a valid baked tree + and downgraded silently. + + The sibling-discovery import of :mod:`bird_interact_agents.paths` + is lazy so the package's path machinery doesn't leak into modules + that don't need it. + """ + # Each candidate is a (label, lazy thunk → Path). Lazy thunks let us + # skip the import of :mod:`bird_interact_agents.paths` (and its + # main-checkout discovery side effects) when an earlier candidate + # already wins — important because tests monkeypatch those helpers + # to raise, and they should only fire when the resolver actually + # consults that branch. + candidates: list[tuple[str, "callable[[], Path]"]] = [] + + override = os.environ.get(env_var) + if override: + override_path = Path(override).expanduser() + candidates.append( + (f"${env_var} override", lambda p=override_path: p), + ) + + candidates.append(( + f"in-image bake path ({cloud_path})", lambda: cloud_path, + )) + + def _sibling() -> Path: + from bird_interact_agents import paths + return ( + paths.bird_interact_upstream_root() + if sibling_name == "BIRD-Interact" + else paths.livesqlbench_upstream_root() + ) + + candidates.append(("sibling-of-checkout", _sibling)) + + failures: list[str] = [] + for label, getter in candidates: + root = getter() + eval_dir = (root / marker_rel).parent + missing = [ + m for m in REQUIRED_UPSTREAM_GRADER_MARKERS + if not (eval_dir / m).is_file() + ] + if not missing: + return root + failures.append(f"{label} ({root}): missing {missing} under {eval_dir}") + + raise ExBaseUnavailableError( + f"No usable upstream {sibling_name} grader tree. Tried:\n" + + "\n".join(f" - {f}" for f in failures) + + f"\nClone the upstream {sibling_name} repo next to the " + f"bird-agents main checkout (or set ${env_var} to a complete " + f"checkout). Without a complete tree, cascade tier N1 falls " + f"back to legacy `_set_equal` — the exact silent-degrade case " + f"this PR was meant to eliminate." + ) + + +# Sibling-helper module names the upstream files import via the bare +# (un-namespaced) name. Each upstream tree ships its own ``db_utils`` — +# mini-interact's wraps sqlite3, livesqlbench's wraps psycopg2 — so a +# shared ``sys.modules["db_utils"]`` cached from the first load would +# leak the wrong DB driver into whichever tree is loaded second. +_UPSTREAM_SIBLING_MODULES = ("db_utils",) + +# Module-load serialisation lock (Codex round 11). ``_load_module_from_file`` +# mutates process-global ``sys.path`` + ``sys.modules`` and runs +# ``exec_module``; two concurrent loads of DIFFERENT upstream trees can +# interleave (re-fronting sys.path, evicting db_utils, exec_module) and +# bind one tree's grader to the other tree's ``db_utils`` — silently +# wrong cascade results. Today's grading paths are process-parallel +# (Ray actors), but pre-empting a threadpool refactor here is cheap and +# the cost (one lock per N1 invocation, contention only on cold loads) +# is negligible against the SQL exec already happening downstream. +_UPSTREAM_LOAD_LOCK = threading.Lock() + + +def _load_module_from_file( + name: str, path: Path, *, sys_path_addition: Optional[Path] = None, +) -> ModuleType: + """Load a Python file as a module by path. Optionally prepend + ``sys_path_addition`` so the module can import its sibling + ``db_utils.py`` (upstream files use ``from db_utils import ...``). + + Both upstream trees define ``db_utils`` with the same bare name — + once Python caches the first tree's ``db_utils`` in ``sys.modules``, + subsequent loads reuse it instead of re-importing from the second + tree's path. We snapshot + temporarily evict the sibling cache + entries during ``exec_module`` so each load re-resolves them + relative to ``sys_path_addition``, then restore the snapshot. The + upstream module itself keeps its own bound references to the right + helpers in its module globals after exec — the cache restore only + affects future bare imports. + """ + if not path.is_file(): + raise FileNotFoundError(f"upstream module not found at {path}") + # Codex round 11: serialise the entire sys.path / sys.modules + # mutation + exec_module against any concurrent loader. Two + # threads loading DIFFERENT upstream trees would otherwise + # interleave the re-fronting / eviction / exec steps, and one + # tree's grader could bind the other tree's `db_utils`. The lock + # scope MUST cover from the sys.path mutation through to the + # snapshot restore — anything narrower leaks the race window. + with _UPSTREAM_LOAD_LOCK: + # Codex round 12: snapshot the FULL sys.path and the prior + # `sys.modules[name]` before any mutation so that an exec_module + # failure rolls back to a clean state. Without this, a partially + # initialised upstream module survives in `sys.modules[name]` + # (an unrelated caller importing by that name would see the + # half-initialised stub), and the prepended sys.path addition + # lingers — the next retry re-fronts itself anyway, but the + # stale entry can still bias sibling lookups for an unrelated + # tree in the meantime. The sibling-module restore at the + # finally below already cleans up `db_utils` — we just extend + # the same hygiene to `sys.path` + the outer module name. + sys_path_snapshot = list(sys.path) + name_snapshot = sys.modules.get(name) + sibling_snapshot: dict[str, Optional[ModuleType]] = { + s: sys.modules.get(s) for s in _UPSTREAM_SIBLING_MODULES + } + + success = False + try: + if sys_path_addition is not None: + # ALWAYS re-front the per-load directory, even if it's already + # somewhere in sys.path. Otherwise the second tree we load + # leaves its own dir at the front, and a subsequent reload of + # the first tree still walks sys.path in [new_first_dir, ..., + # mini_dir, ...] order — the bare `from db_utils import ...` + # would then bind to the WRONG tree's sibling because the + # sys.path search hits the second tree first. (Codex round 2.) + path_str = str(sys_path_addition) + while path_str in sys.path: + sys.path.remove(path_str) + sys.path.insert(0, path_str) + spec = importlib.util.spec_from_file_location(name, str(path)) + if spec is None or spec.loader is None: + raise ImportError(f"could not build module spec for {path}") + mod = importlib.util.module_from_spec(spec) + sys.modules[name] = mod + # Evict the sibling cache so the upcoming exec re-imports + # them from the right tree (resolved via the prepended + # sys.path addition). Restore happens in the finally below. + for sibling in _UPSTREAM_SIBLING_MODULES: + sys.modules.pop(sibling, None) + spec.loader.exec_module(mod) + success = True + return mod + finally: + # Sibling cache ALWAYS restored — success keeps future bare + # imports clean, failure undoes the evictions. + for sibling, prior in sibling_snapshot.items(): + if prior is not None: + sys.modules[sibling] = prior + else: + sys.modules.pop(sibling, None) + if not success: + # Roll sys.path back so the prepended addition doesn't + # linger for unrelated callers, and restore the outer + # `sys.modules[name]` so an unrelated importer doesn't + # pick up a partially initialised stub. On SUCCESS we + # keep the sticky mutations: `sys.path[0] = eval_dir` + # so a subsequent reload of THIS same tree re-fronts + # cleanly, and `sys.modules[name] = mod` so the module + # globals stay bound (the grader API holds references + # into them). + sys.path[:] = sys_path_snapshot + if name_snapshot is None: + sys.modules.pop(name, None) + else: + sys.modules[name] = name_snapshot + + +def _load_mini_interact_module() -> ModuleType: + """Load the mini-interact upstream comparator module on demand.""" + root = _resolve_upstream_root( + "BIRD_BIRD_INTERACT_ROOT", _CLOUD_BIRD_INTERACT_ROOT, "BIRD-Interact", + marker_rel=_MINI_INTERACT_REL, + ) + target = root / _MINI_INTERACT_REL + return _load_module_from_file( + "_bird_interact_mini_interact_test_utils", + target, + sys_path_addition=target.parent, + ) + + +def _load_livesqlbench_module() -> ModuleType: + """Load the livesqlbench upstream comparator module on demand.""" + root = _resolve_upstream_root( + "BIRD_LIVESQLBENCH_ROOT", _CLOUD_LIVESQLBENCH_ROOT, "livesqlbench", + marker_rel=_LIVESQLBENCH_REL, + ) + target = root / _LIVESQLBENCH_REL + return _load_module_from_file( + "_bird_interact_livesqlbench_test_utils", + target, + sys_path_addition=target.parent, + ) + + +# --------------------------------------------------------------------------- +# Benchmark dispatch +# --------------------------------------------------------------------------- + +_MINI_INTERACT_BENCHMARKS = frozenset({"mini-interact"}) +_LIVESQLBENCH_BENCHMARKS = frozenset({ + # The lite-sqlite variant uses upstream livesqlbench's comparator — + # same algorithm, different driver. (We grade against our own local + # SQLite copies, but the comparator code is in the livesqlbench tree.) + "livesqlbench-base-lite-sqlite", + "livesqlbench-base-lite", + "livesqlbench-base-full", + "livesqlbench-large", +}) + + +def _benchmark_loader(benchmark: str): + if benchmark in _MINI_INTERACT_BENCHMARKS: + return _load_mini_interact_module, "sqlite" + if benchmark in _LIVESQLBENCH_BENCHMARKS: + return _load_livesqlbench_module, ( + "sqlite" if benchmark.endswith("-sqlite") else "postgres" + ) + raise ExBaseUnavailableError( + f"benchmark {benchmark!r} is not in the ex_base-backed N1 supported " + f"set; caller must fall back to legacy _set_equal" + ) + + +# --------------------------------------------------------------------------- +# Public surface +# --------------------------------------------------------------------------- + + +def compare_pred_vs_gold_ex_base( + *, + benchmark: str, + pred_sqls: Sequence[str], + sol_sqls: Sequence[str], + db_name: str, + conn, + conditions: Optional[dict] = None, +) -> bool: + """Grade predicted SQL against gold via the upstream BIRD-Interact + ``test_case_default`` pipeline. + + On any upstream-load failure (missing tree, ImportError, + FileNotFoundError, ...) we re-raise as :class:`ExBaseUnavailableError` + so the caller can fall back to legacy ``_set_equal``. + + For livesqlbench Postgres conns we ``conn.rollback()`` in + ``try/finally`` to keep pred mutations from leaking into the next + grade on the same connection. + """ + try: + loader, driver = _benchmark_loader(benchmark) + except ExBaseUnavailableError: + raise + except Exception as exc: # noqa: BLE001 + raise ExBaseUnavailableError( + f"benchmark dispatch failed for {benchmark!r}: {exc}" + ) from exc + + try: + upstream = loader() + except ExBaseUnavailableError: + raise + except (ImportError, FileNotFoundError) as exc: + raise ExBaseUnavailableError( + f"upstream comparator unavailable for {benchmark!r}: {exc}" + ) from exc + except Exception as exc: # noqa: BLE001 + raise ExBaseUnavailableError( + f"loading upstream comparator for {benchmark!r} raised: {exc}" + ) from exc + + # Apply upstream cleanup (matches `test_case_default`, NOT raw + # `ex_base` — `ex_base` itself does not strip). + cleaned_pred = upstream.remove_round( + upstream.remove_distinct( + upstream.remove_comments(list(pred_sqls)) + ) + ) + cleaned_sol = upstream.remove_round( + upstream.remove_distinct( + upstream.remove_comments(list(sol_sqls)) + ) + ) + + needs_rollback = ( + benchmark in _LIVESQLBENCH_BENCHMARKS and driver == "postgres" + ) + # Codex round 4 #1: when the caller passes ``conn=None``, upstream's + # ``execute_queries`` opens a connection per call (sqlite3.connect / + # pool.getconn) but never closes it — every N1 comparison leaks at + # least one conn. Open one ourselves and close it in the finally. + # This makes BOTH preprocessed-result executions reuse the same + # conn AND the rollback/close path own the lifecycle. + owned_conn = None + if conn is None: + try: + if driver == "sqlite": + import sqlite3 as _sqlite3 + conn = _sqlite3.connect(db_name, timeout=30) + owned_conn = conn + elif driver == "postgres": + # Local import to avoid a hard dep on bird_interact_agents' + # postgres helper when only mini-interact is in use. + from bird_interact_agents.db_connection import ( + _open_psycopg2_connection, + ) + import os as _os + host = _os.environ.get("BIRD_PG_HOST", "localhost") + port = int(_os.environ.get("BIRD_PG_PORT", "5432")) + user = _os.environ.get("BIRD_PG_USER", "bird_interact") + password = _os.environ.get("BIRD_PG_PASSWORD", "bird_interact") + stmt_timeout = int( + _os.environ.get("BIRD_PG_STATEMENT_TIMEOUT", "30000") + ) + # `db_name` is the DB short name; upstream livesqlbench's + # `perform_query_on_postgresql_databases` will issue + # queries through this raw psycopg2 conn. + conn = _open_psycopg2_connection( + db_name, host, port, user, password, stmt_timeout, + ) + owned_conn = conn + except Exception as exc: # noqa: BLE001 + raise ExBaseUnavailableError( + f"could not open conn for {benchmark!r} on {db_name!r}: {exc}" + ) from exc + + # Upstream's SQLite `perform_query_on_sqlite_databases` runs + # `PRAGMA synchronous = OFF` / `journal_mode = WAL` on the conn, + # which SQLite rejects when a transaction is open ("Safety level + # may not be changed inside a transaction"). Pre-commit any pending + # tx so the upstream PRAGMAs land cleanly. No-op for psycopg2 since + # the rollback in the finally block reclaims state either way. + if driver == "sqlite": + try: + conn.commit() + except Exception: # noqa: BLE001 + # Conn may not support .commit() (e.g. some custom DB-API + # impl); pressing on is fine, the upstream call will surface + # the original failure if any. + pass + try: + try: + try: + result = upstream.ex_base( + cleaned_pred, cleaned_sol, db_name, conn, conditions, + ) + except Exception: + # Re-raise after the rollback finally below; the outer + # try/finally closes ``owned_conn`` either way. + raise + finally: + if needs_rollback: + try: + conn.rollback() + except Exception: # noqa: BLE001 + logger.warning( + "[upstream_ex_base] conn.rollback() failed after " + "compare_pred_vs_gold_ex_base — next caller may " + "see leaked state", + exc_info=True, + ) + + # Codex round-1 finding #3: legacy deviation from upstream — + # both empty preprocessed results = pass. + if result == 0 and _both_results_empty( + upstream, cleaned_pred, cleaned_sol, db_name, conn, + ): + return True + return bool(result == 1) + finally: + # Codex round 4 #1: close any conn we opened ourselves. The + # outer finally fires on both the success and exception paths, + # AFTER ``_both_results_empty`` reuses the same conn (which is + # why the empty-check runs inside the same try block). + if owned_conn is not None: + try: + owned_conn.close() + except Exception: # noqa: BLE001 + logger.warning( + "[upstream_ex_base] owned-conn close failed", + exc_info=True, + ) + + +def _both_results_empty( + upstream: ModuleType, + pred_sqls: Sequence[str], + sol_sqls: Sequence[str], + db_name: str, + conn, +) -> bool: + """Best-effort check that BOTH sides produced an empty preprocessed + result list (the only case where we deviate from upstream). + + ``ex_base`` returns ``0`` for any non-match. We need to disambiguate + "both empty → kept as pass" from "real mismatch → fail". Re-run the + two pred/gold queries through upstream's execute + preprocess_results + and check explicitly. + """ + try: + execute_queries = upstream.execute_queries + preprocess_results = upstream.preprocess_results + except AttributeError: + return False + try: + # The mini-interact and livesqlbench `execute_queries` signatures + # accept ``(sqls, db, conn, ...)``; mini-interact has the SQLite + # variant ``(sqls, db_path, conn)`` while livesqlbench has + # ``(sqls, db_name, conn, None, "")``. Probe both shapes. + try: + pred_rows, p_err, p_to = execute_queries( + pred_sqls, db_name, conn, None, "", + ) + except TypeError: + pred_rows, p_err, p_to = execute_queries(pred_sqls, db_name, conn) + try: + gold_rows, g_err, g_to = execute_queries( + sol_sqls, db_name, conn, None, "", + ) + except TypeError: + gold_rows, g_err, g_to = execute_queries(sol_sqls, db_name, conn) + except Exception: # noqa: BLE001 + return False + if any([p_err, p_to, g_err, g_to]): + return False + return ( + not preprocess_results(pred_rows or []) + and not preprocess_results(gold_rows or []) + ) diff --git a/src/bird_interact_agents/paths.py b/src/bird_interact_agents/paths.py index 5b1bb0d7..e1bf273a 100644 --- a/src/bird_interact_agents/paths.py +++ b/src/bird_interact_agents/paths.py @@ -191,6 +191,47 @@ def sar_audited_gold_root() -> Path: return main_checkout_root() / "sar_audited_gold" +def bird_interact_upstream_root() -> Path: + """Upstream BIRD-Interact tree root — host-side sibling of the main + checkout. Used by ``eval.upstream_ex_base`` to load the mini-interact + ``test_utils.py`` + ``db_utils.py`` grader, and by + ``cloud.image.build_and_push`` to wire a BuildKit ``--build-context`` + that bakes that grader into the cloud image. + + Distinct from :func:`benchmark_data_root` — this is the upstream + *source* tree (graders + KB schemas + ingest helpers), not the + benchmark *data dir*. Name is deliberately spelled ``_upstream_`` + to avoid collision with the removed ``benchmark_data_root`` shim. + + Honours ``BIRD_BIRD_INTERACT_ROOT`` override (same env-var the loader + reads, so a single setting flips both the host loader path and the + in-image bake source). + """ + override = os.environ.get("BIRD_BIRD_INTERACT_ROOT") + if override: + return Path(override).expanduser() + return main_checkout_root().parent / "BIRD-Interact" + + +def livesqlbench_upstream_root() -> Path: + """Upstream livesqlbench tree root — host-side sibling of the main + checkout. Same posture as :func:`bird_interact_upstream_root`: feeds + the livesqlbench-family ex_base loader on the host AND the cloud + build's BuildKit ``--build-context``. + + Distinct from :func:`benchmark_data_root` — this is the upstream + *source* tree, not a benchmark data dir. Name is spelled + ``_upstream_`` to avoid collision with the removed shim of the same + base name. + + Honours ``BIRD_LIVESQLBENCH_ROOT`` override. + """ + override = os.environ.get("BIRD_LIVESQLBENCH_ROOT") + if override: + return Path(override).expanduser() + return main_checkout_root().parent / "livesqlbench" + + def slayer_models_root() -> Path: """Per-DB SLayer model YAML — committed, must live in the main checkout. diff --git a/src/bird_interact_agents/run.py b/src/bird_interact_agents/run.py index 67c14f39..e92f9af9 100644 --- a/src/bird_interact_agents/run.py +++ b/src/bird_interact_agents/run.py @@ -281,15 +281,18 @@ def make_runner( ) -_DEFAULT_PER_TASK_TIMEOUT_S = 900.0 -"""Per-task wall-clock cap (DEV-1535). 0 / negative = no cap. - -In a sweep of 76 mini-interact retry tasks every `correct` verdict -landed at <= 891 s and every `valid_interpretation` at <= 659 s; 27/53 -`agent_miss` runs burned past 900 s thrashing. The 15-min cap kills -the thrash early with a 0% false-negative rate on the correct/valid -buckets. Override per-run via the BIRD_INTERACT_PER_TASK_TIMEOUT_S -env var (set 0 to disable).""" +_DEFAULT_PER_TASK_TIMEOUT_S = 0.0 +"""Per-task wall-clock cap. 0 / negative = no cap (the default). + +Originally landed under DEV-1535 at 900 s after a 76-task sweep showed +every `correct` verdict at <= 891 s and every `valid_interpretation` +at <= 659 s, so a 15-min cap killed `agent_miss` thrash with a 0% +false-negative rate on the correct/valid buckets. The cap was removed +as a default after rate-limited cloud runs revealed that LLM-side +back-offs (subscription throttles, provider 429s) routinely push +otherwise-correct tasks past the cap, turning recoverable retries into +permanent `eval_failed`s. Re-enable for a specific run via the +BIRD_INTERACT_PER_TASK_TIMEOUT_S env var (set to the desired seconds).""" def _per_task_timeout_s() -> float: diff --git a/src/bird_interact_agents/slayer_otf/cache.py b/src/bird_interact_agents/slayer_otf/cache.py index 3f75d95c..4ab0ef8c 100644 --- a/src/bird_interact_agents/slayer_otf/cache.py +++ b/src/bird_interact_agents/slayer_otf/cache.py @@ -87,6 +87,23 @@ logger = logging.getLogger(__name__) + +# DEV-1550 / DEV-1557: bump when the embedding-text pipeline changes +# semantically. Hashed into `_impl_fingerprint_of` so already-built +# caches invalidate automatically on bump — no manual `rm -rf +# _cache_fp.txt` required. +# +# version=1: initial. +# version=2: bird-agents pre-truncated rendered memory text via a +# local tiktoken helper before `embed_batch` (workaround for +# slayer ≤ 0.7.3 all-batch failure on one over-cap input). +# version=3: delegated per-text truncation to SLayer 0.7.4+ per DEV-1557. +# Bird-agents passes rendered text verbatim; slayer truncates +# per-text via `truncate_text_for_model` (cap - 256 margin) +# with per-input retry fallback. +_EMBEDDING_BUILDER_VERSION = 3 + + # Completeness marker for a per-DB cache dir. Present ⇒ complete; written # LAST in the build tmp dir. Content = the build-time fingerprint (provenance). _CACHE_MARKER = "_cache_fp.txt" @@ -247,6 +264,7 @@ def _impl_fingerprint_of(benchmark: object = None) -> str: h = hashlib.sha256() h.update(f"slayer={_slayer_version()}\n".encode()) h.update(f"embed={_active_embedding_model_or_none()}\n".encode()) + h.update(f"embed_builder={_EMBEDDING_BUILDER_VERSION}\n".encode()) if getattr(benchmark, "db_backend", "sqlite") == "postgres": pg_host = os.environ.get("BIRD_PG_HOST", "localhost") pg_port = os.environ.get("BIRD_PG_PORT", "5432") @@ -538,9 +556,25 @@ async def _materialise_cache_memories( # Memory.model_validate is cheap; the encoder's round-trip test # already proves all dicts are valid. memories = [Memory.model_validate(d) for d in mems] - texts = [ - render_memory_text_for_embedding(memory=m) for m in memories - ] + # DEV-1557 / Stage 2: hand the rendered memory text to slayer + # verbatim. SLayer 0.7.4+ `embed_batch` token-truncates per text via + # `truncate_text_for_model` (cap - 256 margin) and falls back to + # per-input retry if the batch still raises. Bird-agents used to + # pre-truncate here as a workaround for the all-batch failure on + # ≤ 0.7.3; that workaround is deleted (single source of truth = + # slayer). + texts = [render_memory_text_for_embedding(memory=m) for m in memories] + # Per-memory observability: SLayer's truncation log carries only a + # sha256 prefix (it can't see our memory id / db). Emit an INFO + # mapping line so operators can correlate slayer's hashed warnings + # back to the offending memory. + for memory, text in zip(memories, texts, strict=True): + _digest = hashlib.sha256(text.encode("utf-8", errors="replace")).hexdigest() + logger.info( + "[slayer_otf] embedding input for memory %s in db=%s " + "chars=%d sha256=%s", + memory.id, db, len(text), _digest[:16], + ) vectors = await embed_batch(texts, model=model_name) rows: list[Embedding] = [] # strict=True so an embed_batch length mismatch raises instead of diff --git a/tests/cloud/test_cli.py b/tests/cloud/test_cli.py index 54afb723..9cff6a7a 100644 --- a/tests/cloud/test_cli.py +++ b/tests/cloud/test_cli.py @@ -701,7 +701,7 @@ def test_build_subcommand_passes_audited_gold_root_to_image_tag( push_calls: list[tuple] = [] def fake_image_tag(repo_root, audited_gold_root, *, allow_dirty, - annotations_root=None): + annotations_root=None, **_kw): tag_calls.append((repo_root, audited_gold_root, allow_dirty, annotations_root)) return "deadbeef-cafebabe" diff --git a/tests/cloud/test_driver.py b/tests/cloud/test_driver.py index 1d380d90..1746de98 100644 --- a/tests/cloud/test_driver.py +++ b/tests/cloud/test_driver.py @@ -79,6 +79,14 @@ def _patch_collaborators(monkeypatch: pytest.MonkeyPatch) -> dict[str, MagicMock mocks["image"].build_and_push.return_value = ( "us-central1-docker.pkg.dev/motley-team-475011/x/runner:tag" ) + # DEV-1550: driver.submit unpacks `image.default_grader_eval_roots()` + # into two paths before calling image_tag + build_and_push. The + # generic MagicMock above returns a Mock (not iterable), so the + # unpack would fail with ValueError. Stub a concrete 2-tuple. + mocks["image"].default_grader_eval_roots.return_value = ( + Path("/tmp/bird-interact-eval-stub"), + Path("/tmp/livesqlbench-eval-stub"), + ) # De-bake: submit uploads the dataset to a content-hashed GCS prefix and # threads it through the manifest/job-args. Mocked so submit tests don't # hash the real dataset dir or hit GCS. diff --git a/tests/cloud/test_image.py b/tests/cloud/test_image.py index 6a96992d..3bade9e2 100644 --- a/tests/cloud/test_image.py +++ b/tests/cloud/test_image.py @@ -574,6 +574,410 @@ def fake_run(argv, *_a, **_kw): ) +# --------------------------------------------------------------------------- +# DEV-1550: upstream BIRD-Interact + livesqlbench grader subtrees are baked +# into the image so cascade tier N1 dispatch can load test_utils + db_utils +# in the cloud actor. Without this the cascade silently downgrades to +# legacy ``_set_equal`` (the loader's FileNotFoundError is downgraded to +# ``ExBaseUnavailableError`` and caught upstream). +# --------------------------------------------------------------------------- + + +def test_data_hash_changes_on_upstream_bird_interact_grader_edit( + fake_repo_root: Path, tmp_path: Path, +) -> None: + """An edit to the upstream BIRD-Interact ``test_utils.py`` MUST flip + the data-layer hash — otherwise the cached image keeps the old + grader code and the user has to ``--force`` a rebuild to pick up + upstream fixes.""" + eval_root = tmp_path / "bird-interact-eval" + eval_root.mkdir() + (eval_root / "test_utils.py").write_text("def ex_base(*a, **k): return 1\n") + (eval_root / "db_utils.py").write_text("def open_conn(*a, **k): ...\n") + + h1 = image.data_hash( + fake_repo_root, fake_repo_root / "audited_gold", + bird_interact_evaluation_root=eval_root, + ) + (eval_root / "test_utils.py").write_text( + "def ex_base(*a, **k): return 2 # bumped\n" + ) + h2 = image.data_hash( + fake_repo_root, fake_repo_root / "audited_gold", + bird_interact_evaluation_root=eval_root, + ) + assert h1 != h2, ( + "Upstream BIRD-Interact grader edit didn't move data_hash — the " + "cached image would keep stale grader code and silently miss the " + "upstream fix." + ) + + +def test_data_hash_changes_on_upstream_livesqlbench_grader_edit( + fake_repo_root: Path, tmp_path: Path, +) -> None: + """Same posture as the BIRD-Interact tree, for the livesqlbench family.""" + eval_root = tmp_path / "livesqlbench-eval" + eval_root.mkdir() + (eval_root / "test_utils.py").write_text("def ex_base(*a, **k): return 1\n") + + h1 = image.data_hash( + fake_repo_root, fake_repo_root / "audited_gold", + livesqlbench_evaluation_root=eval_root, + ) + (eval_root / "test_utils.py").write_text( + "def ex_base(*a, **k): return 2 # bumped\n" + ) + h2 = image.data_hash( + fake_repo_root, fake_repo_root / "audited_gold", + livesqlbench_evaluation_root=eval_root, + ) + assert h1 != h2 + + +def test_data_hash_ignores_pycache_under_upstream_grader( + fake_repo_root: Path, tmp_path: Path, +) -> None: + """``__pycache__/`` under the upstream grader tree is non-deterministic — + Python recreates it on import. If data_hash counted it, every local + test run that imported the upstream module would churn the cached + image tag and force a rebuild.""" + eval_root = tmp_path / "bird-interact-eval" + eval_root.mkdir() + (eval_root / "test_utils.py").write_text("def ex_base(*a, **k): return 1\n") + + h_baseline = image.data_hash( + fake_repo_root, fake_repo_root / "audited_gold", + bird_interact_evaluation_root=eval_root, + ) + # Simulate Python's bytecode cache landing under the grader tree. + pycache = eval_root / "__pycache__" + pycache.mkdir() + (pycache / "test_utils.cpython-311.pyc").write_bytes(b"\x42" * 256) + + h_with_pycache = image.data_hash( + fake_repo_root, fake_repo_root / "audited_gold", + bird_interact_evaluation_root=eval_root, + ) + assert h_baseline == h_with_pycache, ( + "__pycache__/ entries leaked into data_hash — that makes the " + "image tag depend on Python bytecode generation, which is not " + "stable across local runs." + ) + + +def test_build_and_push_wires_upstream_grader_build_contexts( + fake_repo_root: Path, tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """``build_and_push`` MUST emit ``--build-context + bird-interact-evaluation=`` AND + ``--build-context livesqlbench-evaluation=``. Without these the + Dockerfile's ``COPY --from=...`` calls would fail at build time, OR + silently bake an empty grader tree if BuildKit tolerated missing + contexts.""" + import subprocess as _sp + + captured: list[list[str]] = [] + + def fake_run(argv, *_a, **_kw): + captured.append(list(argv)) + rc = 1 if argv[:3] == ["docker", "manifest", "inspect"] else 0 + return _sp.CompletedProcess(argv, rc, stdout="", stderr="") + + monkeypatch.setattr(_sp, "run", fake_run) + from bird_interact_agents.cloud import config as _config + monkeypatch.setattr( + _config, "image_uri_prefix", lambda: "registry.example/x/runner", + ) + + audited_root = tmp_path / "main-checkout" / "audited_gold" + audited_root.mkdir(parents=True) + bird_interact_eval = tmp_path / "main-checkout" / "bird-interact-eval" + bird_interact_eval.mkdir(parents=True) + livesqlbench_eval = tmp_path / "main-checkout" / "livesqlbench-eval" + livesqlbench_eval.mkdir(parents=True) + # `_ensure_upstream_grader_tree_present` (round 6 + 7 prereq guard) + # requires BOTH `test_utils.py` AND `db_utils.py` markers under each + # eval root before reaching the docker invocation — otherwise + # build_and_push raises `UpstreamGraderUnavailableError` and the + # docker-build wiring this test inspects is never emitted. Plain + # setup, no behaviour change. + for d in (bird_interact_eval, livesqlbench_eval): + (d / "test_utils.py").write_text("") + (d / "db_utils.py").write_text("") + + image.build_and_push( + "deadbeef-cafebabe", + fake_repo_root, + audited_gold_root=audited_root, + bird_interact_evaluation_root=bird_interact_eval, + livesqlbench_evaluation_root=livesqlbench_eval, + force=False, + ) + + build_argv = next(a for a in captured if a[:2] == ["docker", "build"]) + pairs: dict[str, str] = {} + for i, tok in enumerate(build_argv): + if tok == "--build-context" and i + 1 < len(build_argv): + k, _, v = build_argv[i + 1].partition("=") + pairs[k] = v + assert pairs.get("bird-interact-evaluation") == str(bird_interact_eval), ( + f"bird-interact-evaluation build-context not wired; saw {pairs}" + ) + assert pairs.get("livesqlbench-evaluation") == str(livesqlbench_eval), ( + f"livesqlbench-evaluation build-context not wired; saw {pairs}" + ) + + +def test_dockerfile_cloud_bakes_upstream_graders() -> None: + """The PRODUCTION Dockerfile.cloud must have ``COPY --from=`` lines + that consume the two BuildKit contexts ``build_and_push`` emits AND + land them under the in-image bake dir the + ``upstream_ex_base._CLOUD_*_ROOT`` constants point at. A regression + on either side means the cascade tier N1 silently falls back to + legacy ``_set_equal`` in the cloud.""" + import re as _re + + repo_root = Path(__file__).resolve().parents[2] + df = (repo_root / "Dockerfile.cloud").read_text() + non_comment_body = "\n".join( + ln for ln in df.splitlines() if not ln.lstrip().startswith("#") + ) + # Collapse Dockerfile line continuations so COPY commands written + # across multiple lines (the trailing-backslash form) match a single + # regex. + flat_body = _re.sub(r"\\\s*\n\s*", " ", non_comment_body) + + bird_interact_pat = _re.compile( + r"COPY\s+--from=bird-interact-evaluation\s+\.\s+\S*" + r"/app/upstream_graders/bird-interact/mini_interact/knowledge_based/" + r"mini_interact_conv/evaluation/?", + ) + livesqlbench_pat = _re.compile( + r"COPY\s+--from=livesqlbench-evaluation\s+\.\s+\S*" + r"/app/upstream_graders/livesqlbench/evaluation/src/?", + ) + assert bird_interact_pat.search(flat_body), ( + "Dockerfile.cloud is missing the bird-interact-evaluation COPY line " + "or the destination does not land under the in-image bake path." + ) + assert livesqlbench_pat.search(flat_body), ( + "Dockerfile.cloud is missing the livesqlbench-evaluation COPY line " + "or the destination does not land under the in-image bake path." + ) + + +# --------------------------------------------------------------------------- +# DEV-1550 round 6 (Codex): pre-flight checks. A missing upstream-grader tree +# must trip an actionable `UpstreamGraderUnavailableError` BEFORE we shell +# out to `docker build` (where BuildKit's unresolved-context error is +# opaque and points nowhere). Symlink-to-pycache must not bypass the +# `__pycache__` parts filter in `_iter_upstream_grader_files`. +# --------------------------------------------------------------------------- + + +def _stub_subprocess_run_capturing(captured: list[list[str]]): + """Build a `subprocess.run` stand-in that records each invocation + and makes `docker manifest inspect` return non-zero so build_and_push + proceeds to the build step (where the prereq guard runs).""" + import subprocess as _sp + + def fake_run(argv, *_a, **_kw): + captured.append(list(argv)) + rc = 1 if argv[:3] == ["docker", "manifest", "inspect"] else 0 + return _sp.CompletedProcess(argv, rc, stdout="", stderr="") + + return _sp, fake_run + + +def test_build_and_push_raises_when_bird_interact_grader_dir_missing( + fake_repo_root: Path, tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A missing upstream BIRD-Interact grader tree must trip + `UpstreamGraderUnavailableError` BEFORE the docker shell-out, with a + message naming both the path and the env-var remediation.""" + captured: list[list[str]] = [] + _sp, fake_run = _stub_subprocess_run_capturing(captured) + monkeypatch.setattr(_sp, "run", fake_run) + from bird_interact_agents.cloud import config as _config + monkeypatch.setattr( + _config, "image_uri_prefix", lambda: "registry.example/x/runner", + ) + + audited_root = tmp_path / "main-checkout" / "audited_gold" + audited_root.mkdir(parents=True) + # bird_interact_eval is intentionally absent; livesqlbench is present + # so the test pins which tree triggered the failure. + livesqlbench_eval = tmp_path / "main-checkout" / "livesqlbench-eval" + livesqlbench_eval.mkdir(parents=True) + (livesqlbench_eval / "test_utils.py").write_text("") + (livesqlbench_eval / "db_utils.py").write_text("") + bird_interact_eval = tmp_path / "main-checkout" / "nonexistent-bird-interact" + + with pytest.raises( + image.UpstreamGraderUnavailableError, + ) as excinfo: + image.build_and_push( + "deadbeef-cafebabe", + fake_repo_root, + audited_gold_root=audited_root, + bird_interact_evaluation_root=bird_interact_eval, + livesqlbench_evaluation_root=livesqlbench_eval, + force=False, + ) + msg = str(excinfo.value) + assert "BIRD-Interact" in msg + assert str(bird_interact_eval) in msg + assert "BIRD_BIRD_INTERACT_ROOT" in msg, ( + "error message must name the env-var remediation" + ) + # MUST not have reached the docker build invocation. + assert not any(a[:2] == ["docker", "build"] for a in captured), ( + "prereq guard fired AFTER docker build — the whole point is to " + "fail fast before that subprocess. Saw: " + repr(captured) + ) + + +def test_build_and_push_raises_when_grader_dir_lacks_required_markers( + fake_repo_root: Path, tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """An existing-but-empty eval dir is just as bad as missing — it + would silently bake a degraded grader. The guard must catch that + case too (not just the dir-missing one).""" + captured: list[list[str]] = [] + _sp, fake_run = _stub_subprocess_run_capturing(captured) + monkeypatch.setattr(_sp, "run", fake_run) + from bird_interact_agents.cloud import config as _config + monkeypatch.setattr( + _config, "image_uri_prefix", lambda: "registry.example/x/runner", + ) + + audited_root = tmp_path / "main-checkout" / "audited_gold" + audited_root.mkdir(parents=True) + bird_interact_eval = tmp_path / "main-checkout" / "bird-interact-eval" + bird_interact_eval.mkdir(parents=True) + (bird_interact_eval / "test_utils.py").write_text("") + (bird_interact_eval / "db_utils.py").write_text("") + livesqlbench_eval = tmp_path / "main-checkout" / "livesqlbench-eval" + livesqlbench_eval.mkdir(parents=True) # NO marker files at all + + with pytest.raises(image.UpstreamGraderUnavailableError) as excinfo: + image.build_and_push( + "deadbeef-cafebabe", + fake_repo_root, + audited_gold_root=audited_root, + bird_interact_evaluation_root=bird_interact_eval, + livesqlbench_evaluation_root=livesqlbench_eval, + force=False, + ) + msg = str(excinfo.value) + assert "livesqlbench" in msg + assert "test_utils.py" in msg + assert "BIRD_LIVESQLBENCH_ROOT" in msg + + +def test_build_and_push_raises_when_grader_dir_lacks_db_utils( + fake_repo_root: Path, tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Codex round 7: a partial upstream tree with `test_utils.py` but + NO `db_utils.py` would pass the round-6 guard (which only checked + test_utils.py), bake a degraded image, and silently downgrade N1 + when the loader's `from db_utils import ...` raises in the cloud + actor. The round-7 tightening must catch this BEFORE docker build.""" + captured: list[list[str]] = [] + _sp, fake_run = _stub_subprocess_run_capturing(captured) + monkeypatch.setattr(_sp, "run", fake_run) + from bird_interact_agents.cloud import config as _config + monkeypatch.setattr( + _config, "image_uri_prefix", lambda: "registry.example/x/runner", + ) + + audited_root = tmp_path / "main-checkout" / "audited_gold" + audited_root.mkdir(parents=True) + bird_interact_eval = tmp_path / "main-checkout" / "bird-interact-eval" + bird_interact_eval.mkdir(parents=True) + # Has test_utils.py but NOT db_utils.py — the round-6 guard would + # have wrongly accepted this. + (bird_interact_eval / "test_utils.py").write_text("") + livesqlbench_eval = tmp_path / "main-checkout" / "livesqlbench-eval" + livesqlbench_eval.mkdir(parents=True) + (livesqlbench_eval / "test_utils.py").write_text("") + (livesqlbench_eval / "db_utils.py").write_text("") + + with pytest.raises(image.UpstreamGraderUnavailableError) as excinfo: + image.build_and_push( + "deadbeef-cafebabe", + fake_repo_root, + audited_gold_root=audited_root, + bird_interact_evaluation_root=bird_interact_eval, + livesqlbench_evaluation_root=livesqlbench_eval, + force=False, + ) + msg = str(excinfo.value) + assert "BIRD-Interact" in msg + assert "db_utils.py" in msg + assert "BIRD_BIRD_INTERACT_ROOT" in msg + assert not any(a[:2] == ["docker", "build"] for a in captured), ( + "guard fired AFTER docker build — must fail-fast before that" + ) + + +def test_iter_upstream_grader_files_skips_py_symlinks(tmp_path: Path) -> None: + """A `.py` symlink whose target is under `__pycache__/` must be + skipped — otherwise the hash leaks non-deterministic bytecode bytes.""" + root = tmp_path / "grader" + root.mkdir() + real = root / "test_utils.py" + real.write_text("def ex_base(*a, **k): return 1\n") + pyc_dir = root / "__pycache__" + pyc_dir.mkdir() + pyc_target = pyc_dir / "evil.cpython-311.pyc" + pyc_target.write_bytes(b"\xff" * 64) + link = root / "evil.py" + link.symlink_to(pyc_target) + + files = list(image._iter_upstream_grader_files(root)) + assert real in files + assert link not in files, ( + "Symlink whose target is under __pycache__ leaked into the " + "hash set — _iter_upstream_grader_files must skip symlinks." + ) + + +def test_data_hash_ignores_py_symlink_to_pycache( + fake_repo_root: Path, tmp_path: Path, +) -> None: + """Same posture at the data_hash level: adding a `.py` symlink + whose target is under `__pycache__/` must NOT move the hash.""" + eval_root = tmp_path / "bird-interact-eval" + eval_root.mkdir() + (eval_root / "test_utils.py").write_text("def ex_base(*a, **k): return 1\n") + + h_baseline = image.data_hash( + fake_repo_root, fake_repo_root / "audited_gold", + bird_interact_evaluation_root=eval_root, + ) + + pyc_dir = eval_root / "__pycache__" + pyc_dir.mkdir() + pyc = pyc_dir / "test_utils.cpython-311.pyc" + pyc.write_bytes(b"\x99" * 128) + (eval_root / "evil.py").symlink_to(pyc) + + h_with_symlink = image.data_hash( + fake_repo_root, fake_repo_root / "audited_gold", + bird_interact_evaluation_root=eval_root, + ) + assert h_baseline == h_with_symlink, ( + "Symlink-into-pycache leaked into data_hash — the image tag now " + "depends on bytecode bytes, breaking cache stability." + ) + + def test_dirty_input_paths_no_longer_includes_audited_gold( fake_repo_root: Path, make_git, ) -> None: diff --git a/tests/cloud/test_image_annotations.py b/tests/cloud/test_image_annotations.py index 4b0f7392..167c3d3f 100644 --- a/tests/cloud/test_image_annotations.py +++ b/tests/cloud/test_image_annotations.py @@ -127,12 +127,25 @@ class _R: annotations_root = _make_annotations_dir(fake_repo_root) audited_gold_root = fake_repo_root / "audited_gold" + # DEV-1550 round 7: pass explicit upstream grader eval roots so this + # test is hermetic and doesn't depend on the dev/CI machine having + # sibling `BIRD-Interact` / `livesqlbench` checkouts. Each root must + # carry the markers `_ensure_upstream_grader_tree_present` requires. + bird_interact_eval = fake_repo_root / "_bird_interact_eval" + bird_interact_eval.mkdir() + livesqlbench_eval = fake_repo_root / "_livesqlbench_eval" + livesqlbench_eval.mkdir() + for d in (bird_interact_eval, livesqlbench_eval): + (d / "test_utils.py").write_text("") + (d / "db_utils.py").write_text("") image.build_and_push( "test-tag", fake_repo_root, audited_gold_root=audited_gold_root, annotations_root=annotations_root, + bird_interact_evaluation_root=bird_interact_eval, + livesqlbench_evaluation_root=livesqlbench_eval, force=True, ) # The actual build invocation is the call containing "docker build" — diff --git a/tests/cloud/test_run_one_task.py b/tests/cloud/test_run_one_task.py index dcc57c83..631cdec1 100644 --- a/tests/cloud/test_run_one_task.py +++ b/tests/cloud/test_run_one_task.py @@ -267,6 +267,16 @@ async def thrasher(td, data_dir, patience, user_sim_model): assert row["duration_s"] < 2.0 +def test_per_task_timeout_default_is_uncapped( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The default is no cap. Originally 900 s; flipped after rate-limited + cloud runs showed throttled LLM back-offs were pushing legitimate + retries past the cap and converting them into permanent eval_failed.""" + monkeypatch.delenv("BIRD_INTERACT_PER_TASK_TIMEOUT_S", raising=False) + assert run_mod._per_task_timeout_s() <= 0.0 + + @pytest.mark.asyncio async def test_run_one_task_with_runner_zero_timeout_disables_cap( monkeypatch: pytest.MonkeyPatch, diff --git a/tests/eval/test_n1_dispatch.py b/tests/eval/test_n1_dispatch.py new file mode 100644 index 00000000..476e23a2 --- /dev/null +++ b/tests/eval/test_n1_dispatch.py @@ -0,0 +1,549 @@ +"""Tests for the cascade N1 dispatch in `tolerant_grader.grade_submission`. + +When the benchmark is in the upstream-ex_base supported set +(mini-interact + livesqlbench-*), N1 is computed via +`compare_pred_vs_gold_ex_base`. For unsupported benchmarks (bird-interact-*) +or when the shim raises `ExBaseUnavailableError`, N1 falls back to the +legacy `_set_equal(pred_rows, orig_rows)` path. +""" + +from __future__ import annotations + +import sqlite3 +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest + + +def _make_db_with_value(path: Path, table: str, col: str, val) -> None: + conn = sqlite3.connect(str(path)) + conn.execute(f"CREATE TABLE {table} ({col})") + conn.execute(f"INSERT INTO {table} ({col}) VALUES (?)", (val,)) + conn.commit() + conn.close() + + +def _make_task_annotation(): + """Minimal TaskAnnotation that grade_submission's signature requires.""" + from bird_interact_agents.eval.annotation_schema import ( + MetadataSufficiency, Provenance, TaskAnnotation, + ) + return TaskAnnotation( + instance_id="iid_x", + selected_database="alien", + annotated_by="test", + annotated_at="2026-01-01", + amb_user_query="How many rows?", + metadata_sufficiency=MetadataSufficiency( + verdict="sufficient", rationale="test", + ), + provenance=Provenance( + task_jsonl_path="mini_interact.jsonl", + task_jsonl_instance_id="iid_x", + ), + ) + + +def test_n1_dispatch_calls_ex_base_for_mini_interact(tmp_path: Path): + """Mini-interact benchmark: N1 is computed via + `compare_pred_vs_gold_ex_base`, not legacy `_set_equal`.""" + from bird_interact_agents.benchmark import get_benchmark + from bird_interact_agents.eval import tolerant_grader as tg + + db_path = tmp_path / "x.sqlite" + _make_db_with_value(db_path, "t", "v", 1) + task = _make_task_annotation() + + with patch.object( + tg, "compare_pred_vs_gold_ex_base", return_value=True, + ) as ex_base_call: + verdict = tg.grade_submission( + task_annotation=task, + audited_gold_rows=[], + original_sol_sql=["SELECT v FROM t"], + submitted_sql="SELECT v FROM t", + db_path=db_path, + benchmark=get_benchmark("mini-interact"), + ) + assert ex_base_call.called + assert verdict.n1_original_gold is True + + +def test_n1_dispatch_uses_legacy_for_bird_interact_lite_exp(tmp_path: Path): + """`bird-interact-lite-exp` is NOT scoped this PR. N1 keeps the + legacy `_set_equal` path; the ex_base shim is never called.""" + from bird_interact_agents.benchmark import get_benchmark + from bird_interact_agents.eval import tolerant_grader as tg + + db_path = tmp_path / "x.sqlite" + _make_db_with_value(db_path, "t", "v", 1) + task = _make_task_annotation() + + with patch.object(tg, "compare_pred_vs_gold_ex_base") as ex_base_call: + tg.grade_submission( + task_annotation=task, + audited_gold_rows=[], + original_sol_sql=["SELECT v FROM t"], + submitted_sql="SELECT v FROM t", + db_path=db_path, + benchmark=get_benchmark("bird-interact-lite-exp"), + ) + ex_base_call.assert_not_called() + + +def test_n1_dispatch_uses_legacy_for_no_benchmark(tmp_path: Path): + """`benchmark=None` (legacy callers, tests) keeps the legacy path.""" + from bird_interact_agents.eval import tolerant_grader as tg + + db_path = tmp_path / "x.sqlite" + _make_db_with_value(db_path, "t", "v", 1) + task = _make_task_annotation() + + with patch.object(tg, "compare_pred_vs_gold_ex_base") as ex_base_call: + tg.grade_submission( + task_annotation=task, + audited_gold_rows=[], + original_sol_sql=["SELECT v FROM t"], + submitted_sql="SELECT v FROM t", + db_path=db_path, + benchmark=None, + ) + ex_base_call.assert_not_called() + + +def test_n1_fallback_when_ex_base_unavailable_returns_legacy_verdict( + tmp_path: Path, +): + """When the shim raises `ExBaseUnavailableError`, N1 falls back to + `_set_equal(pred_rows, orig_rows)` so a missing upstream tree never + crashes grading. The fallback verdict is the legacy verdict, not + auto-False.""" + from bird_interact_agents.benchmark import get_benchmark + from bird_interact_agents.eval import tolerant_grader as tg + from bird_interact_agents.eval.upstream_ex_base import ExBaseUnavailableError + + db_path = tmp_path / "x.sqlite" + _make_db_with_value(db_path, "t", "v", 1) + task = _make_task_annotation() + + with patch.object( + tg, "compare_pred_vs_gold_ex_base", + side_effect=ExBaseUnavailableError("upstream not installed"), + ): + verdict = tg.grade_submission( + task_annotation=task, + audited_gold_rows=[], + original_sol_sql=["SELECT v FROM t"], + submitted_sql="SELECT v FROM t", + db_path=db_path, + benchmark=get_benchmark("mini-interact"), + ) + # Both pred and gold execute to [(1,)] — legacy _set_equal returns True. + assert verdict.n1_original_gold is True + + +def test_n1_fallback_warns_on_first_ex_base_unavailable_then_dedups( + tmp_path: Path, caplog, +): + """Codex round 9: the N1 dispatch catch site WAS silently swallowing + the round-8 resolver's actionable error — so the operator never saw + why N1 was downgrading. The fix logs a warning with the exception + message on first occurrence, dedups identical subsequent messages + so per-instance retries don't fill the log.""" + import logging + + from bird_interact_agents.benchmark import get_benchmark + from bird_interact_agents.eval import tolerant_grader as tg + from bird_interact_agents.eval.upstream_ex_base import ExBaseUnavailableError + + db_path = tmp_path / "x.sqlite" + _make_db_with_value(db_path, "t", "v", 1) + task = _make_task_annotation() + + # Clear the per-process dedup set so this test's warning fires + # regardless of prior tests in the same session. + tg._EX_BASE_UNAVAILABLE_SEEN.clear() + detailed_msg = ( + "No usable upstream BIRD-Interact grader tree. Tried:\n" + " - in-image bake path (/app/upstream_graders/bird-interact): " + "missing ['test_utils.py', 'db_utils.py']" + ) + + caplog.set_level(logging.WARNING, logger=tg.logger.name) + + with patch.object( + tg, "compare_pred_vs_gold_ex_base", + side_effect=ExBaseUnavailableError(detailed_msg), + ): + # First call: warning fires. + tg.grade_submission( + task_annotation=task, + audited_gold_rows=[], + original_sol_sql=["SELECT v FROM t"], + submitted_sql="SELECT v FROM t", + db_path=db_path, + benchmark=get_benchmark("mini-interact"), + ) + # Second call with identical message: NO additional warning. + tg.grade_submission( + task_annotation=task, + audited_gold_rows=[], + original_sol_sql=["SELECT v FROM t"], + submitted_sql="SELECT v FROM t", + db_path=db_path, + benchmark=get_benchmark("mini-interact"), + ) + + matching = [ + rec for rec in caplog.records + if rec.levelno == logging.WARNING + and "cascade tier N1 is downgrading" in rec.getMessage() + ] + assert len(matching) == 1, ( + f"Expected exactly one warning across two identical errors " + f"(per-process dedup); got {len(matching)}: " + f"{[r.getMessage() for r in matching]}" + ) + assert detailed_msg in matching[0].getMessage(), ( + "Warning must include the actionable error detail so the operator " + "can see WHY N1 downgraded — that's the whole point of the fix." + ) + + +def test_n1_fallback_dedup_is_thread_safe_under_concurrent_callers( + tmp_path: Path, caplog, +): + """Codex round 10: the check-then-add against + `_EX_BASE_UNAVAILABLE_SEEN` must be lock-protected so two threads + racing the SAME unseen message don't both log. Cloud actors are + process-parallel today, but pre-empting a threadpool refactor + costs essentially nothing — and a duplicated warning would + violate the once-per-process contract this test pins.""" + import logging + import threading + + from bird_interact_agents.benchmark import get_benchmark + from bird_interact_agents.eval import tolerant_grader as tg + from bird_interact_agents.eval.upstream_ex_base import ExBaseUnavailableError + + db_path = tmp_path / "x.sqlite" + _make_db_with_value(db_path, "t", "v", 1) + task = _make_task_annotation() + + tg._EX_BASE_UNAVAILABLE_SEEN.clear() + caplog.set_level(logging.WARNING, logger=tg.logger.name) + detailed_msg = "race-window-marker: upstream tree unreachable" + + barrier = threading.Barrier(8) + + def run_once(): + # All 8 threads converge before calling so the check-then-add + # is the hottest point of contention. Without the lock the + # race window is wide enough for multiple threads to see + # `msg not in set` and all log. + barrier.wait() + with patch.object( + tg, "compare_pred_vs_gold_ex_base", + side_effect=ExBaseUnavailableError(detailed_msg), + ): + tg.grade_submission( + task_annotation=task, + audited_gold_rows=[], + original_sol_sql=["SELECT v FROM t"], + submitted_sql="SELECT v FROM t", + db_path=db_path, + benchmark=get_benchmark("mini-interact"), + ) + + threads = [threading.Thread(target=run_once) for _ in range(8)] + for t in threads: + t.start() + for t in threads: + t.join() + + matching = [ + rec for rec in caplog.records + if rec.levelno == logging.WARNING + and detailed_msg in rec.getMessage() + ] + assert len(matching) == 1, ( + f"check-then-add race fired: expected exactly one warning " + f"across 8 concurrent threads with the same error, got " + f"{len(matching)}. Lock missing or wrongly scoped." + ) + + +# --------------------------------------------------------------------------- +# Round-2 review fold-in (CodeRabbit + Codex): conditions forwarding, +# Postgres db_name shape, and the cloud inline-grader mutation-safety guard. +# --------------------------------------------------------------------------- + + +def test_n1_dispatch_forwards_conditions_to_ex_base(tmp_path: Path): + """`grade_submission(conditions={...})` plumbs to + `compare_pred_vs_gold_ex_base(conditions={...})` so ordered-comparison + tasks (conditions={'order': True}) are graded positionally, not as + sets. Without forwarding, ordered tasks would silently pass under + set-dedup semantics. (CodeRabbit + Codex round 2.)""" + from bird_interact_agents.benchmark import get_benchmark + from bird_interact_agents.eval import tolerant_grader as tg + + db_path = tmp_path / "x.sqlite" + _make_db_with_value(db_path, "t", "v", 1) + task = _make_task_annotation() + + sentinel = {"order": True} + with patch.object( + tg, "compare_pred_vs_gold_ex_base", return_value=True, + ) as ex_base_call: + tg.grade_submission( + task_annotation=task, + audited_gold_rows=[], + original_sol_sql=["SELECT v FROM t"], + submitted_sql="SELECT v FROM t", + db_path=db_path, + benchmark=get_benchmark("mini-interact"), + conditions=sentinel, + ) + assert ex_base_call.called + _, kwargs = ex_base_call.call_args + assert kwargs["conditions"] is sentinel + + +def test_n1_dispatch_skips_file_existence_guard_for_postgres(tmp_path: Path): + """Round 3 (Codex): cloud actors pass `db_path = Path()` (a + bare DB-name carrier, not a filesystem path) and `conn=None` for + Postgres livesqlbench. The dispatcher's file-existence guard MUST + NOT fire — upstream's `perform_query_on_postgresql_databases` auto- + opens from a connection pool when conn is None. Without this + carve-out every Postgres livesqlbench task fell back to legacy + `_set_equal` in production despite being listed as ex_base-backed.""" + from bird_interact_agents.benchmark import get_benchmark + from bird_interact_agents.eval import tolerant_grader as tg + + pg_benchmark = get_benchmark("livesqlbench-base-lite") + task = _make_task_annotation() + # db_path is a DB-NAME path, not a filesystem path — purposely NOT + # creating a file at this location. + db_path = Path("alien") + assert not db_path.is_file() + + with patch.object( + tg, "compare_pred_vs_gold_ex_base", return_value=True, + ) as ex_base_call: + tg.grade_submission( + task_annotation=task, + audited_gold_rows=[], + original_sol_sql=["SELECT 1"], + submitted_sql="SELECT 1", + db_path=db_path, + conn=None, # ← the production shape; pool auto-opens + benchmark=pg_benchmark, + executor=lambda sql, *, db_path, conn: ([(1,)], ["c"]), # noqa: ARG005 + ) + # The dispatcher reached ex_base; production parity restored. + assert ex_base_call.called + + +def test_n1_dispatch_uses_db_stem_for_postgres_benchmarks(tmp_path: Path): + """Postgres-backed livesqlbench variants need `db_name = db_path.stem` + (the DB short name), not `str(db_path)`. Upstream + `perform_query_on_postgresql_databases` switches connections via name; + passing a filesystem path silently routes to the wrong DB (CodeRabbit + round 2). + + SQLite-backed benchmarks keep the full `str(db_path)`, which is what + upstream's SQLite path expects.""" + from bird_interact_agents.benchmark import get_benchmark + from bird_interact_agents.eval import tolerant_grader as tg + + pg_benchmark = get_benchmark("livesqlbench-base-lite") + assert pg_benchmark.db_backend == "postgres" + + pred_rows_seen: list[str] = [] + + def _record(**kwargs): + pred_rows_seen.append(kwargs["db_name"]) + return True + + db_path = tmp_path / "alien.sqlite" # value of `db_path.stem == "alien"` + # File creation is defensive only — `conn=MagicMock()` is truthy so + # the conn-is-None / file-existence branch in `_compute_n1` is + # skipped entirely. (CodeRabbit round 3.) + db_path.write_bytes(b"") + task = _make_task_annotation() + with patch.object(tg, "compare_pred_vs_gold_ex_base", side_effect=_record): + # Pass a sqlite3 conn so we don't fall through to the conn=None + # / db-not-found legacy branch; this isolates the dispatch decision. + tg.grade_submission( + task_annotation=task, + audited_gold_rows=[], + original_sol_sql=["SELECT 1"], + submitted_sql="SELECT 1", + db_path=db_path, + conn=MagicMock(), # non-None so the conn-fallback branch is skipped + benchmark=pg_benchmark, + executor=lambda sql, *, db_path, conn: ([(1,)], ["c"]), # noqa: ARG005 + ) + assert pred_rows_seen == ["alien"] + + +def test_n1_dispatch_skips_mutation_bearing_pred_sql(tmp_path: Path): + """When the agent's submitted SQL starts with a mutation keyword + (INSERT / UPDATE / DELETE / CREATE / DROP / ALTER / TRUNCATE / + REPLACE), `_compute_n1` must NOT route through upstream `ex_base`. + Upstream's `execute_queries` would commit the mutation through the + shared conn before running gold (Codex round 2). Fall back to the + legacy multiset comparison on the cascade's pre-fetched pred/orig + rows instead.""" + from bird_interact_agents.benchmark import get_benchmark + from bird_interact_agents.eval import tolerant_grader as tg + + db_path = tmp_path / "x.sqlite" + _make_db_with_value(db_path, "t", "v", 1) + task = _make_task_annotation() + + with patch.object(tg, "compare_pred_vs_gold_ex_base") as ex_base_call: + verdict = tg.grade_submission( + task_annotation=task, + audited_gold_rows=[], + original_sol_sql=["SELECT v FROM t"], + submitted_sql="INSERT INTO t (v) VALUES (9); SELECT v FROM t", + db_path=db_path, + benchmark=get_benchmark("mini-interact"), + ) + ex_base_call.assert_not_called() + # Legacy verdict: pred_rows from cascade's executor (the agent's last + # SQL returned [(1,)] for the SELECT half) ≠ gold's [(1,)] — but the + # cascade gives us whatever the executor returned for the full pred + # SQL. The contract here is "no ex_base call"; verdict shape is the + # legacy `_set_equal` outcome. + assert isinstance(verdict.n1_original_gold, bool) + + +def test_n1_dispatch_skips_mutation_bearing_gold_sql(tmp_path: Path): + """Symmetric: when ORIGINAL gold mutates (rare but possible), N1 + falls back to legacy `_set_equal` rather than letting upstream + commit gold's mutation.""" + from bird_interact_agents.benchmark import get_benchmark + from bird_interact_agents.eval import tolerant_grader as tg + + db_path = tmp_path / "x.sqlite" + _make_db_with_value(db_path, "t", "v", 1) + task = _make_task_annotation() + + with patch.object(tg, "compare_pred_vs_gold_ex_base") as ex_base_call: + tg.grade_submission( + task_annotation=task, + audited_gold_rows=[], + original_sol_sql=["UPDATE t SET v = 2", "SELECT v FROM t"], + submitted_sql="SELECT v FROM t", + db_path=db_path, + benchmark=get_benchmark("mini-interact"), + ) + ex_base_call.assert_not_called() + + +def test_grade_and_write_forwards_conditions_to_grade_submission( + tmp_path: Path, +): + """Round 2 (Codex): `grade_and_write` must accept `conditions` and + forward it to `grade_submission`. Without this, the production + write_submission_skeleton path drops `task_data['conditions']` + before it reaches the ex_base N1 path.""" + from bird_interact_agents.eval import grade_in_place as gip + + rows_dir = tmp_path / "rows" + rows_dir.mkdir() + sentinel = {"order": True} + + seen: list[dict | None] = [] + + def fake_grade(**kwargs): + seen.append(kwargs.get("conditions")) + # Return a minimal CascadeVerdict-shaped object the caller can use. + from bird_interact_agents.eval.tolerant_grader import CascadeVerdict + return CascadeVerdict( + n1_original_gold=True, n2_audited_primary=True, + n3_any_audited_variant=True, n4_tie_order=True, + n5_llm_judge=False, n6_numeric_epsilon=True, + n7_trailing_whitespace=True, n8_column_order=True, + n9_case_fold=True, matched_variant_id=None, + novel_reading_judgment=None, + ) + + with patch.object(gip, "grade_submission", side_effect=fake_grade): + gip.grade_and_write( + rows_dir=rows_dir, instance_id="iid_x", benchmark="mini-interact", + run_id="rid", task_annotation=_make_task_annotation(), + audited_gold_rows=[], original_sol_sql=["SELECT 1"], + submitted_sql="SELECT 1", db_path=Path("/dev/null"), + executor=lambda *a, **kw: ([(1,)], ["c"]), # noqa: ARG005 + trajectory_path="rows/iid_x/attempt-1.json", + conditions=sentinel, + ) + assert seen == [sentinel] + + +def test_remaining_grade_submission_callers_read_conditions_from_task_dict(): + """Round 4 (Codex): every direct grade_submission caller must pull + ``conditions`` from its task-source dict (either ``task_data`` or + ``task_row``, depending on call site) and forward it. This is a + grep-style regression so a future caller is more likely to follow + the same idiom.""" + repo_src = Path(__file__).resolve().parents[2] / "src" + sites = [ + repo_src / "bird_interact_agents/agents/claude_sdk_otf/agent.py", + repo_src / "bird_interact_agents/agents/claude_sdk_otf_ainteract/agent.py", + repo_src / "bird_interact_agents/eval/annotate.py", + repo_src / "bird_interact_agents/eval/regrade.py", + ] + for path in sites: + text = path.read_text() + assert "grade_submission(" in text, f"{path} no longer calls grade_submission" + # Each call passes conditions= sourced from a task-row dict. + assert ( + "conditions=task_data.get(\"conditions\")" in text + or "conditions=task_row.get(\"conditions\")" in text + or "conditions=_row.get(\"conditions\")" in text + ), f"{path} no longer forwards task[.row].get('conditions') to grade_submission" + + +def test_grade_one_submission_forwards_task_data_conditions( + tmp_path: Path, +): + """Round 2 (Codex): `grade_one_submission` must read + `task_data['conditions']` and pass it through `grade_and_write` so + ordered tasks are graded with positional semantics.""" + from bird_interact_agents.eval import grade_in_place as gip + + rows_dir = tmp_path / "rows" + rows_dir.mkdir() + sentinel = {"order": True} + + seen_kwargs: list[dict] = [] + + def fake_grade_and_write(**kwargs): + seen_kwargs.append(kwargs) + return rows_dir / "out.json" + + task_data = { + "instance_id": "iid_x", + "selected_database": "alien", + "sol_sql": ["SELECT 1"], + "amb_user_query": "q", + "conditions": sentinel, + } + with patch.object(gip, "grade_and_write", side_effect=fake_grade_and_write): + gip.grade_one_submission( + task_data=task_data, + submitted_sql="SELECT 1", + rows_dir=rows_dir, + run_id="rid", + benchmark="mini-interact", + db_path=Path("/dev/null"), + task_annotation=_make_task_annotation(), + ) + assert len(seen_kwargs) == 1 + assert seen_kwargs[0].get("conditions") is sentinel diff --git a/tests/eval/test_regrade_n1_ex_base.py b/tests/eval/test_regrade_n1_ex_base.py new file mode 100644 index 00000000..ddae710d --- /dev/null +++ b/tests/eval/test_regrade_n1_ex_base.py @@ -0,0 +1,604 @@ +"""Tests for `scripts/regrade_n1_ex_base.py` (mini-interact backfill). + +The script: +- walks `runs/mini-interact///.json` +- loads `submitted_sql` from JSON + `sol_sql` + `conditions` from + the per-task annotation file +- detects mutation-bearing SQL (skip + log status `state-sensitive`) +- resolves SQLite db at `paths.benchmark_data_root("mini-interact") / db / f"{db}.sqlite"` +- RE-RUNS the FULL cascade (Codex finding #1 — N1 propagates into N2/N3 + and into `failure_classification`; the regrade must regenerate the + entire `evaluation` block + classifier output, not flip one field) +- rewrites the JSON in place; idempotent on re-run. +""" + +from __future__ import annotations + +import json +import sqlite3 +import subprocess +import sys +from pathlib import Path + +import pytest + + +# --------------------------------------------------------------------------- +# Fixtures +# --------------------------------------------------------------------------- + + +@pytest.fixture +def fake_benchmark_root(tmp_path: Path, monkeypatch) -> Path: + """A fake mini-interact root with one DB ``alien`` containing a single + table ``t(val REAL)`` populated with `1.2345`. The same data shape is + used for both the agent's submitted SQL and the gold.""" + parent = tmp_path / "benchmarks" + parent.mkdir() + root = parent / "mini-interact" + db_dir = root / "alien" + db_dir.mkdir(parents=True) + db_path = db_dir / "alien.sqlite" + conn = sqlite3.connect(str(db_path)) + conn.execute("CREATE TABLE t (val REAL)") + conn.execute("INSERT INTO t (val) VALUES (1.2345)") + conn.commit() + conn.close() + # `paths.benchmark_data_root('mini-interact')` honours + # `BIRD_BENCHMARKS_ROOT` (parent dir), not `BIRD_BENCHMARK_DATA_ROOT`. + monkeypatch.setenv("BIRD_BENCHMARKS_ROOT", str(parent)) + return root + + +@pytest.fixture +def fake_runs_root(tmp_path: Path, monkeypatch) -> Path: + """A fake runs/ root (BIRD_RUNS_ROOT) for both the script's walk and + the test's setup.""" + root = tmp_path / "runs" + root.mkdir() + monkeypatch.setenv("BIRD_RUNS_ROOT", str(root)) + return root + + +@pytest.fixture +def fake_annotations_root(tmp_path: Path, monkeypatch) -> Path: + root = tmp_path / "annotations" + root.mkdir() + monkeypatch.setenv("BIRD_ANNOTATIONS_ROOT", str(root)) + return root + + +def _seed_result_json( + runs_root: Path, + annotations_root: Path, + *, + db: str, + iid: str, + run_id: str, + submitted_sql: str, + sol_sql: list[str], + n1_original_gold: bool, + failure_primary: str, +) -> Path: + """Write a synthetic SubmissionAnnotation JSON + matching TaskAnnotation, + using the project's own schema models so any field default that the + regrade reads is realistic.""" + from bird_interact_agents.eval.annotation_schema import ( + FailureClassification, MetadataSufficiency, Provenance, + SubmissionAnnotation, SubmissionEvaluation, SubmissionMetadata, + TaskAnnotation, + ) + + (annotations_root / "mini-interact" / db).mkdir(parents=True, exist_ok=True) + task = TaskAnnotation( + instance_id=iid, + selected_database=db, + annotated_by="test", + annotated_at="2026-01-01", + amb_user_query="How many rows?", + metadata_sufficiency=MetadataSufficiency( + verdict="sufficient", rationale="test", + ), + provenance=Provenance( + task_jsonl_path="mini_interact.jsonl", + task_jsonl_instance_id=iid, + ), + ) + # Pin sol_sql onto the task annotation so the regrade picks it up. + task_dict = task.model_dump() + task_dict["sol_sql"] = sol_sql + task_dict["conditions"] = None + (annotations_root / "mini-interact" / db / f"{iid}.task.json").write_text( + json.dumps(task_dict, indent=2) + ) + + inst_dir = runs_root / "mini-interact" / db / iid + inst_dir.mkdir(parents=True, exist_ok=True) + ev = SubmissionEvaluation( + phase1_against_original_gold="pass" if n1_original_gold else "fail", + phase1_against_audited_primary="pass" if n1_original_gold else "fail", + phase1_against_any_audited_variant="pass" if n1_original_gold else "fail", + phase1_against_variants=[], + correct_up_to_tie_order=False, + novel_reading_judgment=None, + correct_under_numeric_epsilon=False, + correct_under_trailing_whitespace=False, + correct_under_column_order=False, + correct_under_case_fold=False, + numeric_epsilon=1e-6, + verdict="correct" if n1_original_gold else "agent_miss", + matched_variant_id=None, + rationale="", + miss_diagnostics=None, + ) + fc = FailureClassification( + primary=failure_primary, + secondary=[], + agent_at_fault=(failure_primary == "agent_miss"), + remediation_target="other" if failure_primary == "no_fail" else "agent", + remediation_text="", + details="seeded by test", + ) + submission = SubmissionMetadata( + cloud_run_id=run_id, + trajectory_path=f"rows/{iid}/attempt-1.json", + submitted_sql_path=None, + predicted_row_count=None, + duration_s=1.0, + cost_usd_agent=None, + cost_usd_user_sim=None, + n_agent_turns=None, + n_ask_user_calls=None, + ) + ann = SubmissionAnnotation( + schema_version=1, + kind="submission_annotation", + instance_id=iid, + selected_database=db, + task_annotation_ref=f"annotations/mini-interact/{db}/{iid}.task.json", + annotated_by="seed", + annotated_at="2026-06-11T00:00:00Z", + submission=submission, + evaluation=ev, + failure_classification=fc, + decision_point=None, + user_sim_interaction=None, + autopsy=None, + submitted_sql=submitted_sql, + predicted_result=None, + gold_result=None, + original_gold_annotated_correct=None, + ) + out = inst_dir / f"{run_id}.json" + out.write_text(json.dumps(ann.model_dump(mode="json"), indent=2)) + return out + + +def _run_regrade(*args: str, cwd: Path | None = None) -> subprocess.CompletedProcess: + repo = Path(__file__).resolve().parents[2] + script = repo / "scripts" / "regrade_n1_ex_base.py" + return subprocess.run( + [sys.executable, str(script), *args], + cwd=str(cwd or repo), + check=False, + capture_output=True, + text=True, + ) + + +# --------------------------------------------------------------------------- +# Tests +# --------------------------------------------------------------------------- + + +def test_regrade_script_flips_float_precision_case_full_cascade( + fake_benchmark_root: Path, fake_runs_root: Path, + fake_annotations_root: Path, +): + """A task that previously failed N1 (4th-decimal float divergence) + must flip to pass N1 AND propagate through every dependent field: + `phase1_against_*`, `verdict`, `failure_classification.primary`, + `failure_classification.agent_at_fault`.""" + p = _seed_result_json( + fake_runs_root, fake_annotations_root, + db="alien", iid="alien_1", + run_id="20260611t1200-claudes-slayer-aaa111", + # Pred returns 1.2345; gold returns 1.23 -> rounds to 1.23 at 2 dp. + submitted_sql="SELECT val FROM t", + sol_sql=["SELECT 1.23 AS val"], + n1_original_gold=False, + failure_primary="numerical_precision", + ) + proc = _run_regrade() + assert proc.returncode == 0, proc.stderr + after = json.loads(p.read_text()) + # N1 flipped to pass. + assert after["evaluation"]["phase1_against_original_gold"] == "pass" + # Audited tiers and verdict consistent with the new N1. + assert after["evaluation"]["phase1_against_audited_primary"] == "pass" + assert after["evaluation"]["phase1_against_any_audited_variant"] == "pass" + assert after["evaluation"]["verdict"] == "correct" + # failure_classification re-derived. + assert after["failure_classification"]["primary"] == "no_fail" + assert after["failure_classification"]["agent_at_fault"] is False + + +def test_regrade_script_idempotent( + fake_benchmark_root: Path, fake_runs_root: Path, + fake_annotations_root: Path, +): + """Running the script twice produces 0 additional flips and the JSON + is byte-equal across the second run. + + The processed counter MUST advance on the idempotent run too — every + task we successfully grade is "processed", flip or not — otherwise + the operator can't tell apart "idempotent / nothing to do" from + "nothing was reached because of skip/error". (CodeRabbit round 2.)""" + p = _seed_result_json( + fake_runs_root, fake_annotations_root, + db="alien", iid="alien_1", + run_id="20260611t1200-claudes-slayer-aaa222", + submitted_sql="SELECT val FROM t", + sol_sql=["SELECT 1.23 AS val"], + n1_original_gold=False, + failure_primary="numerical_precision", + ) + proc1 = _run_regrade() + assert proc1.returncode == 0, proc1.stderr + first = p.read_text() + proc2 = _run_regrade() + assert proc2.returncode == 0, proc2.stderr + second = p.read_text() + assert first == second + output2 = proc2.stdout + proc2.stderr + # Second-run report names 0 flips, but DID process the task and + # counted it as an unchanged regrade. + assert "regraded_flipped=0" in output2 + assert "processed=1" in output2 + assert "regraded_unchanged=1" in output2 + + +def test_regrade_script_skips_state_sensitive_mutation_pred( + fake_benchmark_root: Path, fake_runs_root: Path, + fake_annotations_root: Path, +): + """Submitted SQL containing INSERT/UPDATE/etc is skipped — the + backfill DB is pristine while inline grading saw the post-mutation + state. The JSON is left untouched.""" + p = _seed_result_json( + fake_runs_root, fake_annotations_root, + db="alien", iid="alien_2", + run_id="20260611t1200-claudes-slayer-bbb333", + submitted_sql="INSERT INTO t (val) VALUES (9.99); SELECT val FROM t", + sol_sql=["SELECT val FROM t"], + n1_original_gold=False, + failure_primary="agent_miss", + ) + before = p.read_text() + proc = _run_regrade() + assert proc.returncode == 0, proc.stderr + after = p.read_text() + assert before == after + assert "skipped_state_sensitive=1" in (proc.stdout + proc.stderr) + + +def test_regrade_script_skips_state_sensitive_mutation_gold( + fake_benchmark_root: Path, fake_runs_root: Path, + fake_annotations_root: Path, +): + """Gold SQL containing mutations is also state-sensitive.""" + p = _seed_result_json( + fake_runs_root, fake_annotations_root, + db="alien", iid="alien_3", + run_id="20260611t1200-claudes-slayer-ccc444", + submitted_sql="SELECT val FROM t", + sol_sql=["UPDATE t SET val = 1.23", "SELECT val FROM t"], + n1_original_gold=False, + failure_primary="agent_miss", + ) + before = p.read_text() + proc = _run_regrade() + assert proc.returncode == 0, proc.stderr + assert p.read_text() == before + assert "skipped_state_sensitive=1" in (proc.stdout + proc.stderr) + + +def test_regrade_script_refuses_non_mini_interact_paths( + fake_benchmark_root: Path, fake_runs_root: Path, + fake_annotations_root: Path, tmp_path: Path, +): + """A result JSON under runs/livesqlbench-* is untouched (the script + is mini-interact-only).""" + # Seed a mini-interact result so the script has at least one task. + p_mi = _seed_result_json( + fake_runs_root, fake_annotations_root, + db="alien", iid="alien_4", + run_id="20260611t1200-claudes-slayer-ddd555", + submitted_sql="SELECT val FROM t", + sol_sql=["SELECT val FROM t"], + n1_original_gold=True, + failure_primary="no_fail", + ) + # Drop a sibling fake livesqlbench result that the script must skip. + lsb_path = ( + fake_runs_root / "livesqlbench-base-lite-sqlite" / "alien" / "alien_99" + / "20260611t1200-claudes-slayer-eee666.json" + ) + lsb_path.parent.mkdir(parents=True, exist_ok=True) + lsb_path.write_text("{}") + before = lsb_path.read_text() + proc = _run_regrade() + assert proc.returncode == 0, proc.stderr + assert lsb_path.read_text() == before + + +def test_regrade_script_resolves_sqlite_path_via_benchmark_data_root( + fake_benchmark_root: Path, fake_runs_root: Path, + fake_annotations_root: Path, +): + """Codex finding #4: the SQLite db_path is + `benchmark_data_root("mini-interact") / db / f"{db}.sqlite"`, NOT + `mini_interact_root() / 'databases' / db / f"{db}.sqlite"`. The + fake_benchmark_root fixture lays out the former; the script must + find the DB at that location.""" + _seed_result_json( + fake_runs_root, fake_annotations_root, + db="alien", iid="alien_5", + run_id="20260611t1200-claudes-slayer-fff777", + submitted_sql="SELECT val FROM t", + sol_sql=["SELECT val FROM t"], + n1_original_gold=False, + failure_primary="agent_miss", + ) + proc = _run_regrade() + assert proc.returncode == 0, proc.stderr + # The script did not error with "db not found"; it processed the task. + assert "regraded" in (proc.stdout + proc.stderr).lower() + + +def test_regrade_script_dry_run_does_not_write( + fake_benchmark_root: Path, fake_runs_root: Path, + fake_annotations_root: Path, +): + p = _seed_result_json( + fake_runs_root, fake_annotations_root, + db="alien", iid="alien_6", + run_id="20260611t1200-claudes-slayer-ggg888", + submitted_sql="SELECT val FROM t", + sol_sql=["SELECT 1.23 AS val"], + n1_original_gold=False, + failure_primary="numerical_precision", + ) + before = p.read_text() + proc = _run_regrade("--dry-run") + assert proc.returncode == 0, proc.stderr + assert p.read_text() == before # untouched + # Dry-run reports the would-be flip and the write-counter is zero + # (Codex round-2 finding #13). + output = proc.stdout + proc.stderr + assert "would_flip=1" in output + assert "regraded_flipped=0" in output + + +# --------------------------------------------------------------------------- +# Codex round-2 additions +# --------------------------------------------------------------------------- + + +def test_regrade_script_rewrites_stale_tolerance_booleans( + fake_benchmark_root: Path, fake_runs_root: Path, + fake_annotations_root: Path, +): + """Codex round-2 finding #9: prove the script RECOMPUTES the full + cascade rather than patching the N1 field. Seed a JSON with a + deliberate pred-vs-gold mismatch AND tolerance booleans stamped to + True (which the recompute will overwrite to False, since the rows + truly don't overlap under any relaxed tolerance either). A naive + 'flip N1 only' impl would leave the stamped True values intact.""" + p = _seed_result_json( + fake_runs_root, fake_annotations_root, + db="alien", iid="alien_stale", + run_id="20260611t1200-claudes-slayer-stale01", + # Pred returns 1.5 (CAST of stored 1.2345 to int via something); + # gold returns 99.99 — guaranteed disjoint. Even under case-fold + # / trailing-whitespace / column-order / numeric-epsilon, the + # recompute will yield False. + submitted_sql="SELECT 1.5 AS val", + sol_sql=["SELECT 99.99 AS val"], + n1_original_gold=False, + failure_primary="agent_miss", + ) + # Stamp deliberately stale `True` values for every tolerance boolean. + # If the regrade only patched N1, these would survive verbatim. + payload = json.loads(p.read_text()) + payload["evaluation"]["correct_up_to_tie_order"] = True + payload["evaluation"]["correct_under_numeric_epsilon"] = True + payload["evaluation"]["correct_under_trailing_whitespace"] = True + payload["evaluation"]["correct_under_column_order"] = True + payload["evaluation"]["correct_under_case_fold"] = True + p.write_text(json.dumps(payload, indent=2)) + + proc = _run_regrade() + assert proc.returncode == 0, proc.stderr + after = json.loads(p.read_text()) + # Pred (1.5) and gold (99.99) are disjoint, so the recompute writes + # False everywhere — proving the script's recompute path overwrote + # the stamped stale True values rather than copying them forward. + assert after["evaluation"]["phase1_against_original_gold"] == "fail" + assert after["evaluation"]["correct_up_to_tie_order"] is False + assert after["evaluation"]["correct_under_numeric_epsilon"] is False + assert after["evaluation"]["correct_under_trailing_whitespace"] is False + assert after["evaluation"]["correct_under_column_order"] is False + assert after["evaluation"]["correct_under_case_fold"] is False + + +def test_regrade_script_falls_back_to_jsonl_when_annotation_missing( + fake_benchmark_root: Path, fake_runs_root: Path, + fake_annotations_root: Path, tmp_path: Path, monkeypatch, +): + """Codex round-2 finding #10: when the annotation file is missing, + the script falls back to the canonical `mini_interact.jsonl` row for + sol_sql + conditions. Seed a JSONL on disk, point the script at it + via `BIRD_MINI_INTERACT_DATA_PATH`, omit the annotation file, run + the regrade — must process the task, not skip it as + `skipped_missing_inputs`.""" + iid = "alien_jsonl_only" + db = "alien" + inst_dir = fake_runs_root / "mini-interact" / db / iid + inst_dir.mkdir(parents=True, exist_ok=True) + + from bird_interact_agents.eval.annotation_schema import ( + FailureClassification, SubmissionAnnotation, + SubmissionEvaluation, SubmissionMetadata, + ) + submission = SubmissionMetadata( + cloud_run_id="20260611t1200-claudes-slayer-jsonl", + trajectory_path=f"rows/{iid}/attempt-1.json", + ) + ev = SubmissionEvaluation( + phase1_against_original_gold="fail", + phase1_against_audited_primary="fail", + phase1_against_any_audited_variant="fail", + phase1_against_variants=[], + correct_up_to_tie_order=False, + novel_reading_judgment=None, + correct_under_numeric_epsilon=False, + correct_under_trailing_whitespace=False, + correct_under_column_order=False, + correct_under_case_fold=False, + numeric_epsilon=1e-6, + verdict="agent_miss", + matched_variant_id=None, + rationale="", + miss_diagnostics=None, + ) + fc = FailureClassification( + primary="agent_miss", secondary=[], agent_at_fault=True, + remediation_target="agent", remediation_text="", details="seed", + ) + ann = SubmissionAnnotation( + instance_id=iid, selected_database=db, + task_annotation_ref=f"annotations/mini-interact/{db}/{iid}.task.json", + annotated_by="seed", annotated_at="2026-06-11T00:00:00Z", + submission=submission, evaluation=ev, failure_classification=fc, + submitted_sql="SELECT val FROM t", + ) + result_path = inst_dir / "20260611t1200-claudes-slayer-jsonl.json" + result_path.write_text(json.dumps(ann.model_dump(mode="json"), indent=2)) + + # Write a canonical jsonl carrying sol_sql + conditions for this iid. + jsonl_path = tmp_path / "mini_interact.jsonl" + jsonl_path.write_text(json.dumps({ + "instance_id": iid, + "selected_database": db, + "sol_sql": ["SELECT val FROM t"], + "conditions": None, + }) + "\n") + monkeypatch.setenv("BIRD_MINI_INTERACT_DATA_PATH", str(jsonl_path)) + + proc = _run_regrade() + assert proc.returncode == 0, proc.stderr + output = proc.stdout + proc.stderr + # Should be processed (not skipped_missing_inputs). + assert "skipped_missing_inputs=0" in output + assert "regraded" in output.lower() + + +def test_regrade_script_forwards_task_conditions_to_grade_submission( + fake_benchmark_root: Path, fake_runs_root: Path, + fake_annotations_root: Path, +): + """Round 2 (Codex): the backfill loaded `conditions` from the task + annotation but threw them away before calling `grade_submission`. + Now the conditions kwarg MUST reach `_compute_n1` so ordered tasks + regrade with positional semantics. Verify by patching `grade_submission` + in the script's module and capturing kwargs.""" + iid = "alien_cond" + db = "alien" + p = _seed_result_json( + fake_runs_root, fake_annotations_root, + db=db, iid=iid, + run_id="20260612t1200-claudes-slayer-cond01", + submitted_sql="SELECT val FROM t", + sol_sql=["SELECT val FROM t"], + n1_original_gold=False, + failure_primary="agent_miss", + ) + # Stamp `conditions={"order": True}` onto the task annotation file. + ann_path = ( + fake_annotations_root / "mini-interact" / db / f"{iid}.task.json" + ) + ann_payload = json.loads(ann_path.read_text()) + ann_payload["conditions"] = {"order": True} + ann_path.write_text(json.dumps(ann_payload, indent=2)) + + # Import-as-module so we can patch grade_submission for the script's + # own _process_one path. Wrap the sys.path mutation + module import + # in try/finally so subsequent tests don't see polluted state, and + # reload-if-cached so a previously-imported module doesn't bind to + # a stale `grade_submission` reference (CodeRabbit round 3). + import importlib + import sys + scripts_path = str(Path(__file__).resolve().parents[2] / "scripts") + prior_path = list(sys.path) + prior_mod = sys.modules.get("regrade_n1_ex_base") + if scripts_path in sys.path: + sys.path.remove(scripts_path) + sys.path.insert(0, scripts_path) + try: + if "regrade_n1_ex_base" in sys.modules: + mod = importlib.reload(sys.modules["regrade_n1_ex_base"]) + else: + mod = importlib.import_module("regrade_n1_ex_base") + seen_conditions: list = [] + real_grade = mod.grade_submission + + def spy(**kwargs): + seen_conditions.append(kwargs.get("conditions")) + return real_grade(**kwargs) + + from unittest.mock import patch as _patch + with _patch.object(mod, "grade_submission", side_effect=spy): + mod.regrade(dry_run=False) + + assert seen_conditions, "grade_submission was never called" + assert seen_conditions[0] == {"order": True}, ( + f"Expected conditions={{'order': True}}, got {seen_conditions[0]}" + ) + finally: + sys.path[:] = prior_path + if prior_mod is None: + sys.modules.pop("regrade_n1_ex_base", None) + else: + sys.modules["regrade_n1_ex_base"] = prior_mod + + +def test_regrade_script_skips_when_sqlite_db_missing( + fake_runs_root: Path, fake_annotations_root: Path, tmp_path: Path, + monkeypatch, +): + """Codex round-2 finding #11: when the SQLite DB file is missing + (or unreadable), the script does NOT crash — it logs the task with + a `missing_db` status and leaves the JSON untouched. The other + tasks in the run still get processed.""" + parent = tmp_path / "benchmarks_missing" + (parent / "mini-interact").mkdir(parents=True) + monkeypatch.setenv("BIRD_BENCHMARKS_ROOT", str(parent)) + # benchmark_data_root('mini-interact')//.sqlite is missing + # for db='missing_db'. + + p = _seed_result_json( + fake_runs_root, fake_annotations_root, + db="missing_db", iid="missing_db_1", + run_id="20260611t1200-claudes-slayer-missdb", + submitted_sql="SELECT 1", + sol_sql=["SELECT 1"], + n1_original_gold=False, + failure_primary="agent_miss", + ) + before = p.read_text() + proc = _run_regrade() + assert proc.returncode == 0, proc.stderr + assert p.read_text() == before + assert "skipped_missing_inputs" in (proc.stdout + proc.stderr).lower() or \ + "missing_db" in (proc.stdout + proc.stderr).lower() diff --git a/tests/eval/test_upstream_ex_base.py b/tests/eval/test_upstream_ex_base.py new file mode 100644 index 00000000..c32f1e81 --- /dev/null +++ b/tests/eval/test_upstream_ex_base.py @@ -0,0 +1,1310 @@ +"""Tests for the upstream-ex_base shim that powers N1. + +The shim wraps upstream's `test_case_default` pipeline (remove_comments ++ remove_distinct + remove_round + ex_base) so our cascade tier N1 +matches the upstream harness's grading semantics: + +- 2-dp Decimal/float rounding via `preprocess_results` +- date / datetime normalisation to "YYYY-MM-DD" +- set-dedup (not multiset) equality +- ROUND() / DISTINCT / comment cleanup applied to BOTH SQLs + +One deliberate deviation from upstream: when BOTH preprocessed result +lists are empty, the shim returns True (matches our legacy "both empty += pass" behavior), while upstream returns 0. This is intentional and +pinned by `test_compare_pred_vs_gold_ex_base_both_empty_returns_true`. +""" + +from __future__ import annotations + +import datetime as _dt +import sqlite3 +from decimal import Decimal +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest + + +# --------------------------------------------------------------------------- +# Helpers — synthetic SQLite DB used by the comparison tests +# --------------------------------------------------------------------------- + + +def _make_sqlite_db(path: Path, *, rows: list[tuple]) -> sqlite3.Connection: + """Create a tiny SQLite DB with one table `t(val)` populated with `rows`.""" + conn = sqlite3.connect(str(path)) + cur = conn.cursor() + cur.execute("CREATE TABLE t (val)") + cur.executemany("INSERT INTO t (val) VALUES (?)", [(v,) for v in rows]) + conn.commit() + return conn + + +# --------------------------------------------------------------------------- +# is_mutation_sql +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize("sql", [ + "INSERT INTO t VALUES (1)", + "insert into t values (1)", + " UPDATE t SET x=1 WHERE id=2", + "DELETE FROM t WHERE id=1", + "CREATE TABLE u (x INT)", + "DROP TABLE t", + "ALTER TABLE t ADD COLUMN y INT", + "TRUNCATE TABLE t", + "REPLACE INTO t (id, x) VALUES (1, 2)", +]) +def test_is_mutation_sql_positive(sql: str): + from bird_interact_agents.eval.upstream_ex_base import is_mutation_sql + + assert is_mutation_sql(sql) is True + + +@pytest.mark.parametrize("sql", [ + # Round 5 (Codex): CTE-prefixed mutations. SQLite + Postgres both + # accept these — the mutation verb sits AFTER a `WITH ... AS (...)` + # block, so the statement-start regex alone misses them and the + # dispatcher would have routed straight to upstream's writeable + # exec path, committing the mutation against the per-task DB. + "WITH x AS (SELECT id FROM t WHERE v > 0) DELETE FROM t WHERE id IN (SELECT id FROM x)", + "with cte as (select 1 as v) insert into t (v) select v from cte", + "WITH a AS (SELECT 1 AS x), b AS (SELECT 2 AS y) UPDATE t SET val = (SELECT x FROM a) WHERE val = (SELECT y FROM b)", + "WITH temp_data AS (SELECT * FROM src) CREATE TABLE dest AS SELECT * FROM temp_data", +]) +def test_is_mutation_sql_detects_cte_prefixed_mutations(sql: str): + from bird_interact_agents.eval.upstream_ex_base import is_mutation_sql + + assert is_mutation_sql(sql) is True + + +@pytest.mark.parametrize("sql", [ + # Negative: a SELECT with a `WITH` clause but a SELECT body. The + # verb-target regex must NOT match this. + "WITH x AS (SELECT 1 AS v) SELECT * FROM x", + "WITH a AS (SELECT id, val FROM t) SELECT id, val FROM a WHERE val > 0", +]) +def test_is_mutation_sql_negative_for_cte_select(sql: str): + from bird_interact_agents.eval.upstream_ex_base import is_mutation_sql + + assert is_mutation_sql(sql) is False + + +@pytest.mark.parametrize("sql", [ + # Round 3 (Codex): commented mutations must still be detected. Upstream's + # remove_comments runs before exec, so a commented mutation would + # otherwise sneak past the dispatcher and commit through ex_base. + "-- explanation\nINSERT INTO t VALUES (1)", + "/* multi\nline */ UPDATE t SET x=1", + " -- leading\n DELETE FROM t WHERE id=1", + "/* a */ /* b */ CREATE TABLE u (x INT)", +]) +def test_is_mutation_sql_strips_comments_before_match(sql: str): + from bird_interact_agents.eval.upstream_ex_base import is_mutation_sql + + assert is_mutation_sql(sql) is True + + +@pytest.mark.parametrize("sql", [ + "SELECT 1", + "SELECT * FROM t WHERE val > 0", + "WITH x AS (SELECT 1) SELECT * FROM x", + " select count(*) from t ", + # "REPLACE" inside a string literal must not trip the regex + "SELECT REPLACE(name, 'a', 'b') FROM t", + # Substring of a column name must not trip + "SELECT updated_at FROM t", + "SELECT inserted_at FROM t", + "SELECT created_at FROM t", + # Word-boundary adversarial cases (Codex finding #8) + "SELECT dropoff FROM t", + "SELECT * FROM truncate_value", + "SELECT replaceable FROM t", + # Substring inside identifiers + "SELECT * FROM inserts_log", # 'inserts' shouldn't trip 'INSERT' +]) +def test_is_mutation_sql_negative(sql: str): + from bird_interact_agents.eval.upstream_ex_base import is_mutation_sql + + assert is_mutation_sql(sql) is False + + +# --------------------------------------------------------------------------- +# compare_pred_vs_gold_ex_base — semantic correctness +# --------------------------------------------------------------------------- + + +def test_compare_pred_vs_gold_ex_base_rounds_to_2dp(tmp_path: Path): + """A 4th-decimal float divergence rounds away at 2 dp and the shim + reports True. The legacy `_set_equal` would call this a mismatch.""" + from bird_interact_agents.eval.upstream_ex_base import ( + compare_pred_vs_gold_ex_base, + ) + + pred_db = tmp_path / "p.sqlite" + gold_db = tmp_path / "g.sqlite" + pred_conn = _make_sqlite_db(pred_db, rows=[1.234567]) + gold_conn = _make_sqlite_db(gold_db, rows=[1.23]) + pred_conn.close() + + # Single conn for `ex_base` execution; both SQLs run against `gold_db` + # with their literal rowsets via UNION ALL — keep it simple by + # binding values inline. + conn = sqlite3.connect(":memory:") + conn.execute("CREATE TABLE t (val)") + conn.execute("INSERT INTO t (val) VALUES (1.234567)") + pred_sqls = ["SELECT val FROM t"] + sol_sqls = ["SELECT 1.23 AS val"] + + result = compare_pred_vs_gold_ex_base( + benchmark="mini-interact", + pred_sqls=pred_sqls, sol_sqls=sol_sqls, + db_name=":memory:", conn=conn, + conditions=None, + ) + conn.close() + gold_conn.close() + assert result is True + + +def test_compare_pred_vs_gold_ex_base_set_semantics(tmp_path: Path): + """Predicted has duplicate rows, gold has one. Upstream set-dedup => + True. The legacy `_set_equal` multiset would say False.""" + from bird_interact_agents.eval.upstream_ex_base import ( + compare_pred_vs_gold_ex_base, + ) + + conn = sqlite3.connect(":memory:") + conn.execute("CREATE TABLE t (val INT)") + conn.executemany("INSERT INTO t (val) VALUES (?)", [(1,), (1,), (1,), (2,)]) + pred_sqls = ["SELECT val FROM t"] + sol_sqls = ["SELECT 1 AS val UNION ALL SELECT 2 AS val"] + + result = compare_pred_vs_gold_ex_base( + benchmark="mini-interact", + pred_sqls=pred_sqls, sol_sqls=sol_sqls, + db_name=":memory:", conn=conn, + conditions=None, + ) + conn.close() + assert result is True + + +def test_compare_pred_vs_gold_ex_base_normalizes_dates(tmp_path: Path): + """A `date` column compared against the gold's `YYYY-MM-DD` string + matches after `preprocess_results`'s strftime normalisation.""" + from bird_interact_agents.eval.upstream_ex_base import ( + compare_pred_vs_gold_ex_base, + ) + + conn = sqlite3.connect(":memory:", detect_types=sqlite3.PARSE_DECLTYPES) + conn.execute("CREATE TABLE t (d DATE)") + conn.execute("INSERT INTO t (d) VALUES (?)", (_dt.date(2026, 6, 11),)) + pred_sqls = ["SELECT d FROM t"] + sol_sqls = ["SELECT '2026-06-11' AS d"] + + result = compare_pred_vs_gold_ex_base( + benchmark="mini-interact", + pred_sqls=pred_sqls, sol_sqls=sol_sqls, + db_name=":memory:", conn=conn, + conditions=None, + ) + conn.close() + assert result is True + + +def test_compare_pred_vs_gold_ex_base_strips_distinct_and_round(): + """Codex finding #2: `ex_base` itself does NOT strip ROUND/DISTINCT — + `test_case_default` does. The shim MUST apply the cleanup. A pred SQL + that wraps the gold expression in `ROUND(..., 4)` must still match a + gold computing the same value, because both ROUND calls get stripped.""" + from bird_interact_agents.eval.upstream_ex_base import ( + compare_pred_vs_gold_ex_base, + ) + + conn = sqlite3.connect(":memory:") + conn.execute("CREATE TABLE t (val)") + conn.executemany("INSERT INTO t (val) VALUES (?)", [(1.23,), (1.23,), (4.56,)]) + # Pred uses DISTINCT + ROUND; gold doesn't. After upstream's cleanup + # they're identical SELECTs. + pred_sqls = ["SELECT DISTINCT ROUND(val, 4) FROM t"] + sol_sqls = ["SELECT val FROM t"] + + result = compare_pred_vs_gold_ex_base( + benchmark="mini-interact", + pred_sqls=pred_sqls, sol_sqls=sol_sqls, + db_name=":memory:", conn=conn, + conditions=None, + ) + conn.close() + assert result is True + + +def test_compare_pred_vs_gold_ex_base_both_empty_returns_true(): + """Legacy deviation: both predicted and gold returning zero rows is + a PASS in our shim (matches legacy `_set_equal([], [])`), even though + upstream `ex_base` would return 0. Documented in the spec.""" + from bird_interact_agents.eval.upstream_ex_base import ( + compare_pred_vs_gold_ex_base, + ) + + conn = sqlite3.connect(":memory:") + conn.execute("CREATE TABLE t (val INT)") + # No inserts — both queries return empty. + pred_sqls = ["SELECT val FROM t WHERE val > 1000"] + sol_sqls = ["SELECT val FROM t WHERE val > 1000"] + + result = compare_pred_vs_gold_ex_base( + benchmark="mini-interact", + pred_sqls=pred_sqls, sol_sqls=sol_sqls, + db_name=":memory:", conn=conn, + conditions=None, + ) + conn.close() + assert result is True + + +def test_compare_pred_vs_gold_ex_base_one_side_empty_returns_false(): + """Asymmetric empty: pred returns rows, gold returns nothing (or + vice-versa) — still a real mismatch. Upstream behaviour preserved.""" + from bird_interact_agents.eval.upstream_ex_base import ( + compare_pred_vs_gold_ex_base, + ) + + conn = sqlite3.connect(":memory:") + conn.execute("CREATE TABLE t (val INT)") + conn.execute("INSERT INTO t (val) VALUES (1)") + pred_sqls = ["SELECT val FROM t"] + sol_sqls = ["SELECT val FROM t WHERE val > 1000"] + + result = compare_pred_vs_gold_ex_base( + benchmark="mini-interact", + pred_sqls=pred_sqls, sol_sqls=sol_sqls, + db_name=":memory:", conn=conn, + conditions=None, + ) + conn.close() + assert result is False + + +def test_compare_pred_vs_gold_ex_base_fails_on_real_mismatch(): + from bird_interact_agents.eval.upstream_ex_base import ( + compare_pred_vs_gold_ex_base, + ) + + conn = sqlite3.connect(":memory:") + conn.execute("CREATE TABLE t (val INT)") + conn.executemany("INSERT INTO t (val) VALUES (?)", [(1,), (2,), (3,)]) + pred_sqls = ["SELECT val FROM t"] + sol_sqls = ["SELECT 1 AS val UNION ALL SELECT 2 AS val"] # missing 3 + + result = compare_pred_vs_gold_ex_base( + benchmark="mini-interact", + pred_sqls=pred_sqls, sol_sqls=sol_sqls, + db_name=":memory:", conn=conn, + conditions=None, + ) + conn.close() + assert result is False + + +# --------------------------------------------------------------------------- +# Dispatch correctness +# --------------------------------------------------------------------------- + + +def test_compare_pred_vs_gold_ex_base_dispatches_to_mini_interact(monkeypatch): + """Mini-interact benchmarks invoke the mini-interact upstream module's + `ex_base`, NOT the livesqlbench one.""" + from bird_interact_agents.eval import upstream_ex_base as mod + + mini_called: list[tuple] = [] + lsb_called: list[tuple] = [] + + fake_mini = MagicMock() + fake_mini.ex_base = lambda p, s, db, conn, conditions: ( + mini_called.append((p, s, db, conditions)) or 1 + ) + fake_mini.remove_comments = lambda x: x + fake_mini.remove_distinct = lambda x: x + fake_mini.remove_round = lambda x: x + + fake_lsb = MagicMock() + fake_lsb.ex_base = lambda p, s, db, conn, conditions: ( + lsb_called.append((p, s, db, conditions)) or 1 + ) + fake_lsb.remove_comments = lambda x: x + fake_lsb.remove_distinct = lambda x: x + fake_lsb.remove_round = lambda x: x + + monkeypatch.setattr(mod, "_load_mini_interact_module", lambda: fake_mini) + monkeypatch.setattr(mod, "_load_livesqlbench_module", lambda: fake_lsb) + + mod.compare_pred_vs_gold_ex_base( + benchmark="mini-interact", + pred_sqls=["SELECT 1"], sol_sqls=["SELECT 1"], + db_name="x.sqlite", conn=MagicMock(), + conditions=None, + ) + assert len(mini_called) == 1 + assert len(lsb_called) == 0 + + +def test_compare_pred_vs_gold_ex_base_dispatches_to_livesqlbench(monkeypatch): + from bird_interact_agents.eval import upstream_ex_base as mod + + mini_called: list = [] + lsb_called: list = [] + fake_mini = MagicMock() + fake_mini.ex_base = lambda *a, **kw: mini_called.append(a) or 1 + fake_mini.remove_comments = lambda x: x + fake_mini.remove_distinct = lambda x: x + fake_mini.remove_round = lambda x: x + fake_lsb = MagicMock() + fake_lsb.ex_base = lambda *a, **kw: lsb_called.append(a) or 1 + fake_lsb.remove_comments = lambda x: x + fake_lsb.remove_distinct = lambda x: x + fake_lsb.remove_round = lambda x: x + + monkeypatch.setattr(mod, "_load_mini_interact_module", lambda: fake_mini) + monkeypatch.setattr(mod, "_load_livesqlbench_module", lambda: fake_lsb) + + mod.compare_pred_vs_gold_ex_base( + benchmark="livesqlbench-base-lite", + pred_sqls=["SELECT 1"], sol_sqls=["SELECT 1"], + db_name="alien", conn=MagicMock(), + conditions=None, + ) + assert len(lsb_called) == 1 + assert len(mini_called) == 0 + + +def test_compare_pred_vs_gold_ex_base_unknown_benchmark_raises(monkeypatch): + """Benchmark not in the supported set => ExBaseUnavailableError so + the caller (N1 dispatch) can fall back to legacy `_set_equal`.""" + from bird_interact_agents.eval.upstream_ex_base import ( + ExBaseUnavailableError, + compare_pred_vs_gold_ex_base, + ) + + with pytest.raises(ExBaseUnavailableError): + compare_pred_vs_gold_ex_base( + benchmark="bird-interact-lite-exp", + pred_sqls=["SELECT 1"], sol_sqls=["SELECT 1"], + db_name="x", conn=MagicMock(), + conditions=None, + ) + + +def test_compare_pred_vs_gold_ex_base_pg_rolls_back_on_return(monkeypatch): + """LiveSQLBench Postgres path MUST rollback the conn after grading so + pred-side mutations cannot leak into the next grade on the same conn. + Verified by spying on `conn.rollback`.""" + from bird_interact_agents.eval import upstream_ex_base as mod + + fake_lsb = MagicMock() + fake_lsb.ex_base = lambda *a, **kw: 1 + fake_lsb.remove_comments = lambda x: x + fake_lsb.remove_distinct = lambda x: x + fake_lsb.remove_round = lambda x: x + monkeypatch.setattr(mod, "_load_livesqlbench_module", lambda: fake_lsb) + monkeypatch.setattr(mod, "_load_mini_interact_module", lambda: MagicMock()) + + conn = MagicMock() + mod.compare_pred_vs_gold_ex_base( + benchmark="livesqlbench-base-lite", + pred_sqls=["SELECT 1"], sol_sqls=["SELECT 1"], + db_name="alien", conn=conn, + conditions=None, + ) + conn.rollback.assert_called_once() + + +def test_compare_pred_vs_gold_ex_base_closes_owned_sqlite_conn_on_success( + tmp_path: Path, monkeypatch, +): + """Codex round 4 #1: when the caller passes ``conn=None`` the shim + opens a SQLite conn itself and MUST close it on the success path, + otherwise every N1 comparison leaks a file descriptor.""" + import sqlite3 + from bird_interact_agents.eval import upstream_ex_base as mod + + # Build a real on-disk SQLite DB so the upstream PRAGMAs (which + # need a real file) don't fail. + db_path = tmp_path / "alien.sqlite" + seed = sqlite3.connect(str(db_path)) + seed.execute("CREATE TABLE t (val INT)") + seed.execute("INSERT INTO t (val) VALUES (1)") + seed.commit() + seed.close() + + # Spy on sqlite3.connect to count the owned-conn open/close. + real_connect = sqlite3.connect + opened: list = [] + + class _SpyConn: + def __init__(self, inner): + self._inner = inner + self.closed = False + opened.append(self) + + def __getattr__(self, name): + return getattr(self._inner, name) + + def close(self): + self.closed = True + self._inner.close() + + def spy_connect(*args, **kwargs): + return _SpyConn(real_connect(*args, **kwargs)) + + monkeypatch.setattr(sqlite3, "connect", spy_connect) + + mod.compare_pred_vs_gold_ex_base( + benchmark="mini-interact", + pred_sqls=["SELECT val FROM t"], sol_sqls=["SELECT val FROM t"], + db_name=str(db_path), conn=None, + conditions=None, + ) + assert opened, "shim did not open a SQLite conn even though conn=None" + # Every owned conn we opened was closed. + assert all(c.closed for c in opened), ( + "shim leaked a SQLite conn after the comparison" + ) + + +def test_compare_pred_vs_gold_ex_base_closes_owned_sqlite_conn_on_exception( + tmp_path: Path, monkeypatch, +): + """Even when upstream `ex_base` raises, the owned conn we opened + must close so a flaky upstream call can't exhaust FDs.""" + import sqlite3 + from bird_interact_agents.eval import upstream_ex_base as mod + + db_path = tmp_path / "alien.sqlite" + seed = sqlite3.connect(str(db_path)) + seed.execute("CREATE TABLE t (val INT)") + seed.commit() + seed.close() + + real_connect = sqlite3.connect + opened: list = [] + + class _SpyConn: + def __init__(self, inner): + self._inner = inner + self.closed = False + opened.append(self) + + def __getattr__(self, name): + return getattr(self._inner, name) + + def close(self): + self.closed = True + self._inner.close() + + def spy_connect(*args, **kwargs): + return _SpyConn(real_connect(*args, **kwargs)) + + monkeypatch.setattr(sqlite3, "connect", spy_connect) + + # Force upstream `ex_base` to raise. + fake_mini = MagicMock() + fake_mini.ex_base = lambda *a, **kw: (_ for _ in ()).throw( + RuntimeError("simulated upstream blow-up") + ) + fake_mini.remove_comments = lambda x: x + fake_mini.remove_distinct = lambda x: x + fake_mini.remove_round = lambda x: x + monkeypatch.setattr(mod, "_load_mini_interact_module", lambda: fake_mini) + + with pytest.raises(RuntimeError): + mod.compare_pred_vs_gold_ex_base( + benchmark="mini-interact", + pred_sqls=["SELECT 1"], sol_sqls=["SELECT 1"], + db_name=str(db_path), conn=None, + conditions=None, + ) + assert opened + assert all(c.closed for c in opened), ( + "shim leaked a SQLite conn after upstream raised" + ) + + +def test_compare_pred_vs_gold_ex_base_does_not_close_caller_supplied_conn( + monkeypatch, +): + """When the caller PROVIDES a conn (the cloud SQLite inline path), + the shim must NOT close it — that's the caller's responsibility.""" + from bird_interact_agents.eval import upstream_ex_base as mod + + fake_mini = MagicMock() + fake_mini.ex_base = lambda *a, **kw: 1 + fake_mini.remove_comments = lambda x: x + fake_mini.remove_distinct = lambda x: x + fake_mini.remove_round = lambda x: x + monkeypatch.setattr(mod, "_load_mini_interact_module", lambda: fake_mini) + + caller_conn = MagicMock() + mod.compare_pred_vs_gold_ex_base( + benchmark="mini-interact", + pred_sqls=["SELECT 1"], sol_sqls=["SELECT 1"], + db_name="x.sqlite", conn=caller_conn, + conditions=None, + ) + caller_conn.close.assert_not_called() + + +def test_compare_pred_vs_gold_ex_base_pg_rolls_back_on_exception(monkeypatch): + """The rollback fires even when ex_base raises so a bad conn doesn't + poison the next caller.""" + from bird_interact_agents.eval import upstream_ex_base as mod + + fake_lsb = MagicMock() + + def _boom(*a, **kw): + raise RuntimeError("ex_base went boom") + + fake_lsb.ex_base = _boom + fake_lsb.remove_comments = lambda x: x + fake_lsb.remove_distinct = lambda x: x + fake_lsb.remove_round = lambda x: x + monkeypatch.setattr(mod, "_load_livesqlbench_module", lambda: fake_lsb) + monkeypatch.setattr(mod, "_load_mini_interact_module", lambda: MagicMock()) + + conn = MagicMock() + with pytest.raises(RuntimeError): + mod.compare_pred_vs_gold_ex_base( + benchmark="livesqlbench-base-lite", + pred_sqls=["SELECT 1"], sol_sqls=["SELECT 1"], + db_name="alien", conn=conn, + conditions=None, + ) + # Codex round-2 finding #7: assert "at least once", not "exactly once", + # so a defensively-double-rolling impl doesn't fail this test. + assert conn.rollback.call_count >= 1 + + +# --------------------------------------------------------------------------- +# Codex round-2 additions +# --------------------------------------------------------------------------- + + +def test_compare_pred_vs_gold_ex_base_ordered_comparison_pass(): + """conditions={'order': True} compares result lists positionally (not as + sets). Same rows in matching ORDER must pass.""" + from bird_interact_agents.eval.upstream_ex_base import ( + compare_pred_vs_gold_ex_base, + ) + + conn = sqlite3.connect(":memory:") + conn.execute("CREATE TABLE t (val INT)") + conn.executemany("INSERT INTO t (val) VALUES (?)", [(3,), (1,), (2,)]) + pred_sqls = ["SELECT val FROM t ORDER BY val"] + sol_sqls = ["SELECT 1 AS val UNION ALL SELECT 2 AS val UNION ALL SELECT 3 AS val"] + + result = compare_pred_vs_gold_ex_base( + benchmark="mini-interact", + pred_sqls=pred_sqls, sol_sqls=sol_sqls, + db_name=":memory:", conn=conn, + conditions={"order": True}, + ) + conn.close() + assert result is True + + +def test_compare_pred_vs_gold_ex_base_ordered_comparison_fails_when_order_differs(): + """conditions={'order': True}: same rows in different order must FAIL. + Without `conditions['order']=True`, the set-dedup path would have + accepted them; the conditions arg must reach upstream `ex_base`.""" + from bird_interact_agents.eval.upstream_ex_base import ( + compare_pred_vs_gold_ex_base, + ) + + conn = sqlite3.connect(":memory:") + conn.execute("CREATE TABLE t (val INT)") + conn.executemany("INSERT INTO t (val) VALUES (?)", [(1,), (2,), (3,)]) + pred_sqls = ["SELECT val FROM t"] # natural order 1,2,3 + sol_sqls = ["SELECT 3 AS val UNION ALL SELECT 2 AS val UNION ALL SELECT 1 AS val"] + + result = compare_pred_vs_gold_ex_base( + benchmark="mini-interact", + pred_sqls=pred_sqls, sol_sqls=sol_sqls, + db_name=":memory:", conn=conn, + conditions={"order": True}, + ) + conn.close() + assert result is False + + +def test_compare_pred_vs_gold_ex_base_strips_distinct_only_on_pred_side(): + """Codex round-2 finding #2: prove the cleanup is applied to BOTH + sides, not silently only one side. DISTINCT on pred alone must not + cause a real-mismatch failure.""" + from bird_interact_agents.eval.upstream_ex_base import ( + compare_pred_vs_gold_ex_base, + ) + + conn = sqlite3.connect(":memory:") + conn.execute("CREATE TABLE t (val INT)") + conn.executemany("INSERT INTO t (val) VALUES (?)", [(1,), (1,), (2,)]) + pred_sqls = ["SELECT DISTINCT val FROM t"] + sol_sqls = ["SELECT val FROM t"] # no DISTINCT + + result = compare_pred_vs_gold_ex_base( + benchmark="mini-interact", + pred_sqls=pred_sqls, sol_sqls=sol_sqls, + db_name=":memory:", conn=conn, + conditions=None, + ) + conn.close() + assert result is True + + +def test_compare_pred_vs_gold_ex_base_strips_round_only_on_gold_side(): + """Symmetric: ROUND() on gold alone must not cause a real-mismatch + failure.""" + from bird_interact_agents.eval.upstream_ex_base import ( + compare_pred_vs_gold_ex_base, + ) + + conn = sqlite3.connect(":memory:") + conn.execute("CREATE TABLE t (val REAL)") + conn.execute("INSERT INTO t (val) VALUES (1.5)") + pred_sqls = ["SELECT val FROM t"] + sol_sqls = ["SELECT ROUND(val, 4) FROM t"] + + result = compare_pred_vs_gold_ex_base( + benchmark="mini-interact", + pred_sqls=pred_sqls, sol_sqls=sol_sqls, + db_name=":memory:", conn=conn, + conditions=None, + ) + conn.close() + assert result is True + + +def test_compare_pred_vs_gold_ex_base_strips_comments(): + """Codex round-2 finding #3: upstream's `remove_comments` is part of + the cleanup pipeline. SQL containing comments that would change + parsing behaviour must still execute and grade correctly.""" + from bird_interact_agents.eval.upstream_ex_base import ( + compare_pred_vs_gold_ex_base, + ) + + conn = sqlite3.connect(":memory:") + conn.execute("CREATE TABLE t (val INT)") + conn.execute("INSERT INTO t (val) VALUES (1)") + pred_sqls = ["SELECT val /* trailing comment */ FROM t -- line comment"] + sol_sqls = ["SELECT val FROM t"] + + result = compare_pred_vs_gold_ex_base( + benchmark="mini-interact", + pred_sqls=pred_sqls, sol_sqls=sol_sqls, + db_name=":memory:", conn=conn, + conditions=None, + ) + conn.close() + assert result is True + + +def test_compare_pred_vs_gold_ex_base_loader_import_failure_raises_unavailable( + monkeypatch, +): + """Codex round-2 finding #4: if the lazy loader raises `ImportError` + (upstream tree missing), the public surface raises + `ExBaseUnavailableError`, not the raw ImportError.""" + from bird_interact_agents.eval import upstream_ex_base as mod + + def _raise_import(*a, **kw): + raise ImportError("upstream tree not installed") + + monkeypatch.setattr(mod, "_load_mini_interact_module", _raise_import) + + with pytest.raises(mod.ExBaseUnavailableError): + mod.compare_pred_vs_gold_ex_base( + benchmark="mini-interact", + pred_sqls=["SELECT 1"], sol_sqls=["SELECT 1"], + db_name=":memory:", conn=MagicMock(), + conditions=None, + ) + + +def test_compare_pred_vs_gold_ex_base_loader_filenotfound_raises_unavailable( + monkeypatch, +): + """Same shape, FileNotFoundError (upstream root env var points + nowhere).""" + from bird_interact_agents.eval import upstream_ex_base as mod + + def _raise_fnf(*a, **kw): + raise FileNotFoundError("test_utils.py not found at configured root") + + monkeypatch.setattr(mod, "_load_livesqlbench_module", _raise_fnf) + + with pytest.raises(mod.ExBaseUnavailableError): + mod.compare_pred_vs_gold_ex_base( + benchmark="livesqlbench-base-lite", + pred_sqls=["SELECT 1"], sol_sqls=["SELECT 1"], + db_name="alien", conn=MagicMock(), + conditions=None, + ) + + +def test_load_module_from_file_isolates_db_utils_between_upstream_trees( + tmp_path: Path, +): + """Both upstream trees define their own ``db_utils.py``. Without + cache isolation in ``_load_module_from_file``, Python reuses the + first tree's ``sys.modules['db_utils']`` for the second tree's + bare ``from db_utils import ...`` — leaking mini-interact's sqlite3 + helpers into livesqlbench's psycopg2 module (or vice versa). + Regression test (CodeRabbit round 2): build two minimal trees with + distinct ``db_utils`` marker constants, load each, and assert + the bound helpers are tree-specific.""" + import sys + from bird_interact_agents.eval import upstream_ex_base as mod + + tree_a = tmp_path / "tree_a" + tree_a.mkdir() + (tree_a / "db_utils.py").write_text("ORIGIN = 'tree_a'\n") + (tree_a / "test_utils.py").write_text( + "from db_utils import ORIGIN\nWHICH = 'a:' + ORIGIN\n" + ) + + tree_b = tmp_path / "tree_b" + tree_b.mkdir() + (tree_b / "db_utils.py").write_text("ORIGIN = 'tree_b'\n") + (tree_b / "test_utils.py").write_text( + "from db_utils import ORIGIN\nWHICH = 'b:' + ORIGIN\n" + ) + + # Pre-pollute the cache to simulate a prior load. + prior = sys.modules.pop("db_utils", None) + try: + a = mod._load_module_from_file( + "test_utils_a", tree_a / "test_utils.py", + sys_path_addition=tree_a, + ) + b = mod._load_module_from_file( + "test_utils_b", tree_b / "test_utils.py", + sys_path_addition=tree_b, + ) + # Each module bound the correct sibling at exec time. + assert a.WHICH == "a:tree_a" + assert b.WHICH == "b:tree_b" + finally: + if prior is not None: + sys.modules["db_utils"] = prior + else: + sys.modules.pop("db_utils", None) + sys.modules.pop("test_utils_a", None) + sys.modules.pop("test_utils_b", None) + + +def test_load_module_from_file_reloading_first_tree_still_finds_own_db_utils( + tmp_path: Path, +): + """Codex round 2: ``sys.path.insert(0, ...)`` was conditional on + absence — so after loading tree_a then tree_b, ``sys.path`` looked + like ``[tree_b_dir, tree_a_dir, ...]``. A subsequent reload of + tree_a would re-execute its ``test_utils.py`` but the bare + ``from db_utils import ...`` would walk ``sys.path`` in order and + pick tree_b's sibling first. Regression: each load must re-front + its own dir.""" + import sys + from bird_interact_agents.eval import upstream_ex_base as mod + + tree_a = tmp_path / "tree_a" + tree_a.mkdir() + (tree_a / "db_utils.py").write_text("ORIGIN = 'tree_a'\n") + (tree_a / "test_utils.py").write_text( + "from db_utils import ORIGIN\nWHICH = ORIGIN\n" + ) + + tree_b = tmp_path / "tree_b" + tree_b.mkdir() + (tree_b / "db_utils.py").write_text("ORIGIN = 'tree_b'\n") + (tree_b / "test_utils.py").write_text( + "from db_utils import ORIGIN\nWHICH = ORIGIN\n" + ) + + prior_modules = { + k: sys.modules.get(k) for k in ("db_utils",) + } + prior_path = list(sys.path) + try: + mod._load_module_from_file( + "tu_a_first", tree_a / "test_utils.py", sys_path_addition=tree_a, + ) + mod._load_module_from_file( + "tu_b_after", tree_b / "test_utils.py", sys_path_addition=tree_b, + ) + # Now reload tree_a; without the re-front, this picks tree_b's + # db_utils because tree_b's dir is at position 0 of sys.path. + a_again = mod._load_module_from_file( + "tu_a_reload", tree_a / "test_utils.py", + sys_path_addition=tree_a, + ) + assert a_again.WHICH == "tree_a" + finally: + for k, v in prior_modules.items(): + if v is not None: + sys.modules[k] = v + else: + sys.modules.pop(k, None) + for k in ("tu_a_first", "tu_b_after", "tu_a_reload"): + sys.modules.pop(k, None) + sys.path[:] = prior_path + + +def test_load_module_from_file_restores_prior_db_utils_after_load(tmp_path: Path): + """The cache-isolation snapshot restores whatever ``db_utils`` was + in ``sys.modules`` BEFORE the load, so a third-party caller with + its own ``db_utils`` import doesn't get clobbered.""" + import sys + import types + from bird_interact_agents.eval import upstream_ex_base as mod + + sentinel = types.ModuleType("db_utils") + sentinel.ORIGIN = "caller_sentinel" # type: ignore[attr-defined] + sys.modules["db_utils"] = sentinel + + tree = tmp_path / "tree" + tree.mkdir() + (tree / "db_utils.py").write_text("ORIGIN = 'tree_internal'\n") + (tree / "test_utils.py").write_text( + "from db_utils import ORIGIN\nWHICH = ORIGIN\n" + ) + try: + m = mod._load_module_from_file( + "test_utils_iso", tree / "test_utils.py", + sys_path_addition=tree, + ) + assert m.WHICH == "tree_internal" + # Post-load: the caller's sentinel is restored. + assert sys.modules["db_utils"] is sentinel + finally: + sys.modules.pop("db_utils", None) + sys.modules.pop("test_utils_iso", None) + + +@pytest.mark.parametrize("benchmark", [ + "livesqlbench-base-lite-sqlite", + "livesqlbench-base-lite", + "livesqlbench-base-full", + "livesqlbench-large", +]) +def test_compare_pred_vs_gold_ex_base_dispatches_for_all_livesqlbench_variants( + monkeypatch, benchmark: str, +): + """Codex round-2 finding #5 + #6: every LSB-shape benchmark in the + supported set must dispatch — including the SQLite variant.""" + from bird_interact_agents.eval import upstream_ex_base as mod + + seen: list[str] = [] + fake_mini = MagicMock() + fake_mini.ex_base = lambda *a, **kw: (seen.append("mini") or 1) + fake_mini.remove_comments = lambda x: x + fake_mini.remove_distinct = lambda x: x + fake_mini.remove_round = lambda x: x + fake_lsb = MagicMock() + fake_lsb.ex_base = lambda *a, **kw: (seen.append("lsb") or 1) + fake_lsb.remove_comments = lambda x: x + fake_lsb.remove_distinct = lambda x: x + fake_lsb.remove_round = lambda x: x + monkeypatch.setattr(mod, "_load_mini_interact_module", lambda: fake_mini) + monkeypatch.setattr(mod, "_load_livesqlbench_module", lambda: fake_lsb) + + mod.compare_pred_vs_gold_ex_base( + benchmark=benchmark, + pred_sqls=["SELECT 1"], sol_sqls=["SELECT 1"], + db_name="alien", conn=MagicMock(), + conditions=None, + ) + # The SQLite LSB variant uses upstream livesqlbench's grader (same + # algorithm, sqlite3 driver) — dispatched to LSB module. + assert seen == ["lsb"] + + +# --------------------------------------------------------------------------- +# DEV-1550: upstream-tree root resolution. The cloud actor MUST be able to +# load the upstream grader modules; the prior author-private absolute paths +# (`/home/james/...`) meant the loaders silently `FileNotFoundError`d on every +# other machine, and the N1 dispatch downgraded to legacy `_set_equal` without +# anyone noticing. +# --------------------------------------------------------------------------- + + +def test_cloud_grader_root_constants_are_under_in_image_bake_dir(): + """The in-image bake paths must point under ``/app/upstream_graders/`` + (matched 1:1 by ``Dockerfile.cloud``'s ``COPY --from=...`` lines) — + otherwise the dispatch silently falls back to legacy ``_set_equal`` in + the cloud actor.""" + from bird_interact_agents.eval.upstream_ex_base import ( + _CLOUD_BIRD_INTERACT_ROOT, _CLOUD_LIVESQLBENCH_ROOT, + ) + + assert str(_CLOUD_BIRD_INTERACT_ROOT).startswith("/app/upstream_graders/") + assert str(_CLOUD_LIVESQLBENCH_ROOT).startswith("/app/upstream_graders/") + # No author-private hardcoded paths anywhere in the defaults. + assert not str(_CLOUD_BIRD_INTERACT_ROOT).startswith("/home/") + assert not str(_CLOUD_LIVESQLBENCH_ROOT).startswith("/home/") + + +_MARKER_REL_BIRD_INTERACT = ( + "mini_interact/knowledge_based/mini_interact_conv/evaluation/test_utils.py" +) +_MARKER_REL_LIVESQLBENCH = "evaluation/src/test_utils.py" + + +def _populate_grader_markers(root: Path, marker_rel: str) -> None: + """Drop a `test_utils.py` + `db_utils.py` pair under the eval dir + derived from `marker_rel`, so the round-8 resolver accepts `root`.""" + from bird_interact_agents.eval.upstream_ex_base import ( + REQUIRED_UPSTREAM_GRADER_MARKERS, + ) + + eval_dir = (root / marker_rel).parent + eval_dir.mkdir(parents=True, exist_ok=True) + for marker in REQUIRED_UPSTREAM_GRADER_MARKERS: + (eval_dir / marker).write_text("") + + +def test_resolve_upstream_root_prefers_env_var(monkeypatch, tmp_path: Path): + """Env override wins over both the in-image bake path and the + sibling-of-main-checkout discovery — local devs must be able to point + the loader at an arbitrary checkout. Round 8: override is also + validated, so it must contain the full marker set to be accepted.""" + from bird_interact_agents.eval.upstream_ex_base import ( + _CLOUD_BIRD_INTERACT_ROOT, _resolve_upstream_root, + ) + + override = tmp_path / "my-bird-interact-fork" + _populate_grader_markers(override, _MARKER_REL_BIRD_INTERACT) + monkeypatch.setenv("BIRD_BIRD_INTERACT_ROOT", str(override)) + resolved = _resolve_upstream_root( + "BIRD_BIRD_INTERACT_ROOT", _CLOUD_BIRD_INTERACT_ROOT, "BIRD-Interact", + marker_rel=_MARKER_REL_BIRD_INTERACT, + ) + assert resolved == override + + +def test_resolve_upstream_root_falls_back_to_sibling_of_main_checkout( + monkeypatch, tmp_path: Path, +): + """With no env override and no in-image bake dir, the resolver returns + the sibling-of-main-checkout path produced by + ``paths.bird_interact_upstream_root`` (the common local-dev layout).""" + from bird_interact_agents.eval.upstream_ex_base import _resolve_upstream_root + + monkeypatch.delenv("BIRD_BIRD_INTERACT_ROOT", raising=False) + sentinel = tmp_path / "sibling-bird-interact" + _populate_grader_markers(sentinel, _MARKER_REL_BIRD_INTERACT) + + import bird_interact_agents.paths as paths_mod + monkeypatch.setattr(paths_mod, "bird_interact_upstream_root", lambda: sentinel) + + # Point the cloud path at a directory that does NOT exist, so the + # resolver falls through to the sibling branch. + nonexistent_cloud = tmp_path / "no-such-cloud-bake" + resolved = _resolve_upstream_root( + "BIRD_BIRD_INTERACT_ROOT", nonexistent_cloud, "BIRD-Interact", + marker_rel=_MARKER_REL_BIRD_INTERACT, + ) + assert resolved == sentinel + + +def test_resolve_upstream_root_prefers_cloud_bake_when_marker_present( + monkeypatch, tmp_path: Path, +): + """Inside the cloud actor (where ``/app/upstream_graders/...`` is baked + by ``Dockerfile.cloud``), the resolver picks the in-image path over the + sibling discovery — but only when the deeper ``test_utils.py`` marker + is actually present (Codex round 7: a partial bake that leaves the + cloud root dir alone but drops the inner file must fall through to + the sibling, not silently downgrade).""" + from bird_interact_agents.eval.upstream_ex_base import _resolve_upstream_root + + monkeypatch.delenv("BIRD_LIVESQLBENCH_ROOT", raising=False) + cloud_bake = tmp_path / "app-upstream-graders-livesqlbench" + _populate_grader_markers(cloud_bake, _MARKER_REL_LIVESQLBENCH) + + # If the resolver ignored the present cloud bake and consulted + # `paths.livesqlbench_upstream_root` instead, this raise-on-call + # would trip. + import bird_interact_agents.paths as paths_mod + + def _explode(): + raise AssertionError("sibling discovery used while cloud bake present") + + monkeypatch.setattr(paths_mod, "livesqlbench_upstream_root", _explode) + resolved = _resolve_upstream_root( + "BIRD_LIVESQLBENCH_ROOT", cloud_bake, "livesqlbench", + marker_rel=_MARKER_REL_LIVESQLBENCH, + ) + assert resolved == cloud_bake + + +def test_resolve_upstream_root_falls_through_on_partial_cloud_bake( + monkeypatch, tmp_path: Path, +): + """Codex round 7: a partial bake that leaves the cloud root directory + present BUT drops the deeper ``test_utils.py`` marker must NOT + short-circuit the sibling discovery — otherwise the loader silently + raises FileNotFoundError downstream and N1 falls back to legacy + ``_set_equal`` without any operator-visible signal.""" + from bird_interact_agents.eval.upstream_ex_base import _resolve_upstream_root + + monkeypatch.delenv("BIRD_BIRD_INTERACT_ROOT", raising=False) + + cloud_root_present = tmp_path / "app-upstream-graders-bird-interact" + cloud_root_present.mkdir() # dir exists but marker is absent + + sibling = tmp_path / "sibling-bird-interact-full-bake" + _populate_grader_markers(sibling, _MARKER_REL_BIRD_INTERACT) + + import bird_interact_agents.paths as paths_mod + monkeypatch.setattr( + paths_mod, "bird_interact_upstream_root", lambda: sibling, + ) + + resolved = _resolve_upstream_root( + "BIRD_BIRD_INTERACT_ROOT", cloud_root_present, "BIRD-Interact", + marker_rel=_MARKER_REL_BIRD_INTERACT, + ) + assert resolved == sibling, ( + "Partial cloud bake (dir present, marker missing) short-circuited " + "the sibling fallback — that's the silent-degrade case the round-7 " + "tightening exists to prevent." + ) + + +# --------------------------------------------------------------------------- +# DEV-1550 round 8: every candidate branch (env override, cloud bake, +# sibling discovery) must validate against the COMPLETE marker set. +# Previously the env-override branch returned unconditionally and the +# sibling branch was unvalidated — both routes silently downgraded N1 +# when the named tree was incomplete. +# --------------------------------------------------------------------------- + + +def test_resolve_upstream_root_falls_through_on_partial_env_override( + monkeypatch, tmp_path: Path, +): + """Env override is honoured if it's COMPLETE; an incomplete override + must fall through to the cloud / sibling rather than silently win. + Otherwise a stale `$BIRD_BIRD_INTERACT_ROOT` pointing at a partial + fork masks a fully-baked cloud tree and N1 silently degrades.""" + from bird_interact_agents.eval.upstream_ex_base import _resolve_upstream_root + + partial_override = tmp_path / "stale-bird-interact-fork" + # Only test_utils.py — no db_utils.py. Round-7 build-time guard + # rejected this; round-8 resolver must too. + eval_dir = partial_override / Path(_MARKER_REL_BIRD_INTERACT).parent + eval_dir.mkdir(parents=True) + (eval_dir / "test_utils.py").write_text("") + monkeypatch.setenv("BIRD_BIRD_INTERACT_ROOT", str(partial_override)) + + sibling = tmp_path / "sibling-complete" + _populate_grader_markers(sibling, _MARKER_REL_BIRD_INTERACT) + import bird_interact_agents.paths as paths_mod + monkeypatch.setattr( + paths_mod, "bird_interact_upstream_root", lambda: sibling, + ) + + nonexistent_cloud = tmp_path / "no-such-cloud-bake" + resolved = _resolve_upstream_root( + "BIRD_BIRD_INTERACT_ROOT", nonexistent_cloud, "BIRD-Interact", + marker_rel=_MARKER_REL_BIRD_INTERACT, + ) + assert resolved == sibling, ( + "Incomplete env override masked the complete sibling tree — " + "round-8 resolver must validate every branch's marker set." + ) + + +def test_resolve_upstream_root_falls_through_on_partial_sibling( + monkeypatch, tmp_path: Path, +): + """An incomplete sibling-of-checkout tree must not silently win + either — code paths that don't go through ``build_and_push`` (local + regrade, dev shell, etc.) would otherwise import the upstream + successfully via test_utils.py but crash on db_utils.py and N1 + silently downgrades.""" + from bird_interact_agents.eval.upstream_ex_base import _resolve_upstream_root + + monkeypatch.delenv("BIRD_LIVESQLBENCH_ROOT", raising=False) + nonexistent_cloud = tmp_path / "no-such-cloud-bake" + + partial_sibling = tmp_path / "sibling-partial" + eval_dir = partial_sibling / Path(_MARKER_REL_LIVESQLBENCH).parent + eval_dir.mkdir(parents=True) + (eval_dir / "test_utils.py").write_text("") # MISSING db_utils.py + + import bird_interact_agents.paths as paths_mod + monkeypatch.setattr( + paths_mod, "livesqlbench_upstream_root", lambda: partial_sibling, + ) + + with pytest.raises( + __import__( + "bird_interact_agents.eval.upstream_ex_base", fromlist=["x"], + ).ExBaseUnavailableError, + ) as excinfo: + _resolve_upstream_root( + "BIRD_LIVESQLBENCH_ROOT", nonexistent_cloud, "livesqlbench", + marker_rel=_MARKER_REL_LIVESQLBENCH, + ) + msg = str(excinfo.value) + assert "db_utils.py" in msg + assert "livesqlbench" in msg + assert "BIRD_LIVESQLBENCH_ROOT" in msg, ( + "Failure message must name the env-var remediation." + ) + + +def test_load_module_from_file_rolls_back_state_on_exec_module_failure( + tmp_path: Path, +): + """Codex round 12: an exception during ``exec_module`` MUST leave + ``sys.path`` and ``sys.modules[name]`` exactly as they were before + the load — otherwise a partially initialised upstream module + lingers under that name, and the prepended sys.path entry leaks + into unrelated callers.""" + import sys + from bird_interact_agents.eval import upstream_ex_base as mod + + # Tree that fails mid-exec_module. `raise RuntimeError(...)` at top + # level fires when `exec_module` runs the module body. db_utils is + # present so the import chain reaches the failing line. + tree = tmp_path / "tree_failing" + tree.mkdir() + (tree / "db_utils.py").write_text("ORIGIN = 'fail-tree'\n") + (tree / "test_utils.py").write_text( + "from db_utils import ORIGIN\n" + "raise RuntimeError('synthetic exec failure')\n" + ) + + name = "test_utils_failure_rollback" + # Snapshot the world before the (failing) load. + sys_path_before = list(sys.path) + name_before = sys.modules.get(name) + + with pytest.raises(RuntimeError, match="synthetic exec failure"): + mod._load_module_from_file( + name, tree / "test_utils.py", sys_path_addition=tree, + ) + + assert sys.path == sys_path_before, ( + "sys.path was not restored after exec_module failure — the " + "prepended sys_path_addition leaked into the process." + ) + assert sys.modules.get(name) == name_before, ( + "sys.modules[name] still points at the partially initialised " + "upstream module after exec_module failure. An unrelated " + "importer using this name would see the broken stub." + ) + + +def test_load_module_from_file_is_thread_safe_under_concurrent_loads( + tmp_path: Path, +): + """Codex round 11: ``_load_module_from_file`` mutates process-global + ``sys.path`` + ``sys.modules`` and runs ``exec_module``; two + threads loading DIFFERENT upstream trees can interleave the + re-front / evict / exec steps, and one tree's grader binds the + other tree's ``db_utils``. Without the module-load lock this test + flaps (or wedges) — with the lock, each thread sees its own + consistent sibling.""" + import sys + import threading + from bird_interact_agents.eval import upstream_ex_base as mod + + n_threads = 8 + iterations_per_thread = 6 + + tree_a = tmp_path / "tree_a" + tree_a.mkdir() + (tree_a / "db_utils.py").write_text("ORIGIN = 'tree_a'\n") + (tree_a / "test_utils.py").write_text( + "from db_utils import ORIGIN\nWHICH = ORIGIN\n" + ) + + tree_b = tmp_path / "tree_b" + tree_b.mkdir() + (tree_b / "db_utils.py").write_text("ORIGIN = 'tree_b'\n") + (tree_b / "test_utils.py").write_text( + "from db_utils import ORIGIN\nWHICH = ORIGIN\n" + ) + + barrier = threading.Barrier(n_threads) + mismatches: list[tuple[str, str]] = [] + mismatches_lock = threading.Lock() + + def worker(which_tree: str): + tree = tree_a if which_tree == "a" else tree_b + expected = "tree_a" if which_tree == "a" else "tree_b" + unique_id = threading.get_ident() + barrier.wait() + for i in range(iterations_per_thread): + name = f"test_utils_{which_tree}_{unique_id}_{i}" + loaded = mod._load_module_from_file( + name, tree / "test_utils.py", sys_path_addition=tree, + ) + if loaded.WHICH != expected: + with mismatches_lock: + mismatches.append((expected, loaded.WHICH)) + sys.modules.pop(name, None) + + threads = [ + threading.Thread(target=worker, args=(("a" if i % 2 == 0 else "b"),)) + for i in range(n_threads) + ] + prior = sys.modules.pop("db_utils", None) + try: + for t in threads: + t.start() + for t in threads: + t.join() + finally: + if prior is not None: + sys.modules["db_utils"] = prior + else: + sys.modules.pop("db_utils", None) + + assert not mismatches, ( + f"Cross-tree binding leaked under concurrent loads: {mismatches[:5]} " + f"(showing first 5 of {len(mismatches)})." + ) + + +def test_resolve_upstream_root_raises_actionable_message_when_no_candidate_valid( + monkeypatch, tmp_path: Path, +): + """If env, cloud, and sibling all fail validation, the resolver + raises ExBaseUnavailableError naming every candidate's failure mode + so the operator can fix the right tree.""" + from bird_interact_agents.eval import upstream_ex_base as mod + + monkeypatch.delenv("BIRD_BIRD_INTERACT_ROOT", raising=False) + nonexistent_cloud = tmp_path / "no-cloud" + nonexistent_sibling = tmp_path / "no-sibling" + + monkeypatch.setattr( + __import__("bird_interact_agents.paths", fromlist=["x"]), + "bird_interact_upstream_root", lambda: nonexistent_sibling, + ) + + with pytest.raises(mod.ExBaseUnavailableError) as excinfo: + mod._resolve_upstream_root( + "BIRD_BIRD_INTERACT_ROOT", nonexistent_cloud, "BIRD-Interact", + marker_rel=_MARKER_REL_BIRD_INTERACT, + ) + msg = str(excinfo.value) + assert "in-image bake" in msg + assert "sibling-of-checkout" in msg + assert "BIRD_BIRD_INTERACT_ROOT" in msg + assert "BIRD-Interact" in msg diff --git a/tests/test_autopsy.py b/tests/test_autopsy.py index 260f9b06..51903421 100644 --- a/tests/test_autopsy.py +++ b/tests/test_autopsy.py @@ -1369,6 +1369,158 @@ async def test_run_autopsy_one_shot_validates_and_returns_one_shot_analysis(tmp_ assert result.user_sim_interaction is None +@pytest.mark.asyncio +async def test_run_autopsy_retries_once_on_validation_error_and_recovers(tmp_path): + """Pydantic validation failures get one corrective retry. First call + returns an output missing the required ``pattern`` field (the + archeology_10 production failure); the harness sends back a + ``tool_result`` carrying the validation error with ``is_error=True`` + and the model returns a valid second attempt. The autopsy lands as + a successful analysis, NOT ``validation_error``.""" + from bird_interact_agents.eval.annotation_schema import ( + AutopsyAnalysisOneShot, + AutopsyResult, + ) + from bird_interact_agents.eval.autopsy import run_autopsy + + task_ann = _minimal_task_annotation() + bad_input = { + # ``pattern`` deliberately omitted — the archeology_10 failure shape. + "other_details": None, + "narrative": "Some narrative.", + "remediation": "Some remediation.", + "decision_point_trajectory_index": None, + "decision_point_description": None, + } + good_input = { + "pattern": "wrong_join_path", + "other_details": None, + "narrative": "Agent used the wrong join path.", + "remediation": "Fix host discovery.", + "decision_point_trajectory_index": 4, + "decision_point_description": "Wrong join chosen at step 4.", + } + bad_response = _stub_tool_use(bad_input) + # Give the bad response a tool_use id so the retry can echo it. + bad_response.content[0].id = "tu_bad_1" + good_response = _stub_tool_use(good_input) + good_response.content[0].id = "tu_good_1" + + mock_client = AsyncMock() + mock_client.messages.create = AsyncMock( + side_effect=[bad_response, good_response] + ) + + with patch("anthropic.AsyncAnthropic", return_value=mock_client): + result = await run_autopsy( + task_annotation=task_ann, + trajectory=[], + slayer_storage_dir=str(tmp_path), + miss_diagnostics=None, + model="anthropic/claude-sonnet-4-5", + is_one_shot=True, + ) + + assert isinstance(result, AutopsyResult) + assert result.error is None + assert isinstance(result.analysis, AutopsyAnalysisOneShot) + assert result.analysis.pattern == "wrong_join_path" + # Exactly two LLM calls: the failed attempt plus the corrective retry. + assert mock_client.messages.create.await_count == 2 + # The retry call carried the prior assistant turn + a tool_result with + # is_error=True so the model knew its previous output was rejected. + retry_messages = mock_client.messages.create.await_args_list[1].kwargs["messages"] + assert len(retry_messages) == 3 # user prompt + assistant + tool_result + tool_result_msg = retry_messages[-1] + assert tool_result_msg["role"] == "user" + tr_block = tool_result_msg["content"][0] + assert tr_block["type"] == "tool_result" + assert tr_block["tool_use_id"] == "tu_bad_1" + assert tr_block["is_error"] is True + assert "pattern" in tr_block["content"] # the failing field is named + + +@pytest.mark.asyncio +async def test_run_autopsy_validation_error_persists_after_two_failed_attempts(tmp_path): + """If both the first attempt AND the corrective retry fail validation, + the autopsy surfaces a ``validation_error`` carrying the LAST + exception. Guard against the bug where the loop silently returns the + first attempt's error instead of the retry's.""" + from bird_interact_agents.eval.annotation_schema import AutopsyResult + from bird_interact_agents.eval.autopsy import run_autopsy + + task_ann = _minimal_task_annotation() + bad_input_1 = { # missing pattern + "narrative": "n1", "remediation": "r1", + } + bad_input_2 = { # still missing pattern, plus extra invalid value + "narrative": "n2", "remediation": "r2", + } + r1 = _stub_tool_use(bad_input_1); r1.content[0].id = "tu_1" + r2 = _stub_tool_use(bad_input_2); r2.content[0].id = "tu_2" + + mock_client = AsyncMock() + mock_client.messages.create = AsyncMock(side_effect=[r1, r2]) + + with patch("anthropic.AsyncAnthropic", return_value=mock_client): + result = await run_autopsy( + task_annotation=task_ann, + trajectory=[], + slayer_storage_dir=str(tmp_path), + miss_diagnostics=None, + model="anthropic/claude-sonnet-4-5", + is_one_shot=True, + ) + + assert isinstance(result, AutopsyResult) + assert result.analysis is None + assert result.error is not None + assert result.error.kind == "validation_error" + assert mock_client.messages.create.await_count == 2 + + +def test_autopsy_tool_schema_generated_from_pydantic_model(): + """Tool schema is generated from the Pydantic model, not hand-mirrored. + A new pattern enum value or required field added to AutopsyLLMOutput + must show up in the tool schema automatically.""" + from bird_interact_agents.eval.autopsy import ( + AutopsyLLMOutput, + AutopsyLLMOutputOneShot, + _AUTOPSY_TOOL_SCHEMA, + _AUTOPSY_TOOL_SCHEMA_ONE_SHOT, + ) + + # Every field that's required on the Pydantic model appears in the + # tool schema's ``required`` array. + for model_cls, schema in [ + (AutopsyLLMOutput, _AUTOPSY_TOOL_SCHEMA), + (AutopsyLLMOutputOneShot, _AUTOPSY_TOOL_SCHEMA_ONE_SHOT), + ]: + pydantic_required = { + name for name, field in model_cls.model_fields.items() + if field.is_required() + } + schema_required = set(schema["input_schema"]["required"]) + assert pydantic_required == schema_required, ( + f"{model_cls.__name__}: pydantic={pydantic_required} " + f"schema={schema_required}" + ) + + # The pattern enum on the tool schema matches the Pydantic Literal. + a_pattern_enum = set( + _AUTOPSY_TOOL_SCHEMA["input_schema"]["properties"]["pattern"]["enum"] + ) + o_pattern_enum = set( + _AUTOPSY_TOOL_SCHEMA_ONE_SHOT["input_schema"]["properties"]["pattern"]["enum"] + ) + # One-shot is a strict subset of A-interact patterns. + assert o_pattern_enum.issubset(a_pattern_enum) + # Nested ``key_asks`` schema is inlined (no $ref/$defs left over). + a_schema_str = json.dumps(_AUTOPSY_TOOL_SCHEMA["input_schema"]) + assert "$ref" not in a_schema_str + assert "$defs" not in a_schema_str + + @pytest.mark.asyncio async def test_run_autopsy_validation_error_to_autopsy_error_repro_robot_10(tmp_path): """Robot_10 repro: stub the actual LLM payload shape that broke diff --git a/tests/test_slayer_otf_cache.py b/tests/test_slayer_otf_cache.py index 90e98f60..a1166109 100644 --- a/tests/test_slayer_otf_cache.py +++ b/tests/test_slayer_otf_cache.py @@ -816,3 +816,286 @@ def boom(**_kw): DB, cache_root=cache_root, mini_interact_root=fake_mini_interact_root, ) assert entry.cache_dir == cache_root / DB + + +# --------------------------------------------------------------------------- +# DEV-1557 / Stage-2: cache builder delegates embedding-text truncation to +# SLayer 0.7.4+. Migrated from the deleted test_slayer_otf_cache_embed_truncate +# file with a delegation-style test replacing the per-helper unit tests. +# --------------------------------------------------------------------------- + + +def test_embedding_builder_version_in_cache_fingerprint(monkeypatch): + """Bumping ``_EMBEDDING_BUILDER_VERSION`` MUST change + ``_impl_fingerprint_of(...)`` so already-built caches invalidate + automatically when the embedding-text pipeline changes (e.g. we + delegated truncation to SLayer in version 3; a future change to the + pipeline lifts to version 4 and the bump alone forces a rebuild). + Migrated from test_slayer_otf_cache_embed_truncate.py before that + file was deleted.""" + fp_before = otf_cache._impl_fingerprint_of(None) + monkeypatch.setattr(otf_cache, "_EMBEDDING_BUILDER_VERSION", 999) + fp_after = otf_cache._impl_fingerprint_of(None) + assert fp_before != fp_after + + +def test_embedding_builder_version_is_3_after_stage_2_delegation(): + """Codex /spec review (f): assert the literal version value so a + partial implementation that bumps everything else but forgets the + constant gets caught. The migration contract is encoded here.""" + assert otf_cache._EMBEDDING_BUILDER_VERSION == 3 + + +def test_bird_side_truncation_helpers_were_deleted(): + """Codex /spec review (a) / (f): direct absence assertions on the + symbols the Stage-2 migration deleted. No finite-input + "passes-through-50k-chars" test can prove "no truncation ever"; + only the absence of the helper API gives us that guarantee.""" + assert not hasattr(otf_cache, "_truncate_for_embedding"), ( + "_truncate_for_embedding must be deleted — SLayer 0.7.4+ is the " + "single source of truth for per-text token truncation" + ) + assert not hasattr(otf_cache, "_EMBEDDING_MAX_TOKENS"), ( + "_EMBEDDING_MAX_TOKENS must be deleted with the helper" + ) + assert not hasattr(otf_cache, "_EMBEDDING_FALLBACK_MAX_CHARS"), ( + "_EMBEDDING_FALLBACK_MAX_CHARS must be deleted with the helper" + ) + + +def test_materialise_cache_memories_passes_raw_text_to_embed_batch( + monkeypatch, tmp_path: Path, +): + """DEV-1557 / Stage-2: bird-agents no longer pre-truncates the + rendered memory text before ``embed_batch``. SLayer 0.7.4's own + ``embed_batch`` handles per-text token truncation + per-input retry. + + Verify by: monkeypatching ``embed_batch`` to capture inputs; + forcing one of the rendered memories to be 50k chars long (way + over any plausible cap); asserting the EXACT raw text is what + ``embed_batch`` receives. If a future refactor reintroduces + bird-side truncation, this test fails immediately with a + length mismatch. + + Replaces the prior per-helper unit tests, which were tightly + coupled to the deleted ``_truncate_for_embedding`` and would + otherwise drift into testing SLayer's private helpers.""" + from unittest.mock import MagicMock + + seen_inputs: list[str] = [] + + async def stub_embed_batch(texts, *, model=None): + seen_inputs.extend(texts) + return [[0.1] * 8 for _ in texts] + + monkeypatch.setattr(otf_cache, "embed_batch", stub_embed_batch) + monkeypatch.setattr(otf_cache, "_embeddings_available", lambda: True) + monkeypatch.setattr( + otf_cache, "_embedding_current_model", + lambda: "openai/text-embedding-3-small", + ) + + long_raw = "x " * 25_000 # ~50k chars; well above any prior bird-side cap + short_raw = "short text" + + def fake_render(*, memory): + return long_raw if memory.id == "long" else short_raw + + monkeypatch.setattr(otf_cache, "render_memory_text_for_embedding", fake_render) + + long_mem = MagicMock(); long_mem.id = "long" + short_mem = MagicMock(); short_mem.id = "short" + monkeypatch.setattr( + otf_cache, "encode_kb_as_memories", + lambda *a, **kw: [{"id": "long"}, {"id": "short"}], + ) + monkeypatch.setattr( + otf_cache.Memory, "model_validate", + lambda d: long_mem if d.get("id") == "long" else short_mem, + ) + + build_dir = tmp_path / "build" + build_dir.mkdir() + asyncio.run(otf_cache._materialise_cache_memories( + db="alien", build_dir=build_dir, kb_rows=[{"id": 1}, {"id": 2}], + )) + + # Both texts arrived at embed_batch. + assert len(seen_inputs) == 2 + # The long memory's text arrives VERBATIM — no bird-side truncation. + # If somebody reintroduces a `_truncate_for_embedding` step, the + # length comparison fails immediately. + assert long_raw in seen_inputs, ( + "long memory text was not passed through to embed_batch verbatim — " + "did somebody re-add bird-side truncation? Slayer 0.7.4+ is the " + "single source of truth for per-text token truncation." + ) + assert short_raw in seen_inputs + + +def test_materialise_cache_memories_logs_per_memory_observability( + monkeypatch, tmp_path: Path, caplog, +): + """DEV-1557 / Stage-2: keep per-memory observability after deleting + the truncation helper. SLayer 0.7.4 logs truncation events with a + sha256_prefix but doesn't know our memory id / db; emit an INFO log + near the embed_batch call mapping ``memory.id``, ``db``, + ``len(text)``, and a sha256 prefix so the two log streams can be + correlated.""" + import logging + from unittest.mock import MagicMock + + async def stub_embed_batch(texts, *, model=None): + return [[0.1] * 8 for _ in texts] + + monkeypatch.setattr(otf_cache, "embed_batch", stub_embed_batch) + monkeypatch.setattr(otf_cache, "_embeddings_available", lambda: True) + monkeypatch.setattr( + otf_cache, "_embedding_current_model", + lambda: "openai/text-embedding-3-small", + ) + monkeypatch.setattr( + otf_cache, "render_memory_text_for_embedding", + lambda *, memory: f"text for {memory.id}", + ) + + mem_a = MagicMock(); mem_a.id = "alpha" + mem_b = MagicMock(); mem_b.id = "beta" + monkeypatch.setattr( + otf_cache, "encode_kb_as_memories", + lambda *a, **kw: [{"id": "alpha"}, {"id": "beta"}], + ) + monkeypatch.setattr( + otf_cache.Memory, "model_validate", + lambda d: mem_a if d["id"] == "alpha" else mem_b, + ) + + build_dir = tmp_path / "build" + build_dir.mkdir() + with caplog.at_level(logging.INFO, logger="bird_interact_agents.slayer_otf.cache"): + asyncio.run(otf_cache._materialise_cache_memories( + db="alien_db", build_dir=build_dir, + kb_rows=[{"id": 1}, {"id": 2}], + )) + + import hashlib + + records = [r for r in caplog.records + if "[slayer_otf]" in r.message + and "embedding" in r.message.lower() + and ("alpha" in r.message or "beta" in r.message)] + assert len(records) == 2, ( + "expected one INFO log per memory mapping memory.id + db + chars + sha256 prefix; " + f"got {len(records)} matching records" + ) + # Codex /spec review (b): assert level explicitly. ``caplog.at_level(INFO)`` + # still captures warnings, so without this a regression to a + # warning-shaped log would slip through. + for r in records: + assert r.levelno == logging.INFO, ( + f"per-memory observability log must be INFO, got level={r.levelname}" + ) + # Compute the digest WE expect (from the rendered text) and verify + # the logged sha256 prefix is at least 8 hex chars of that digest — + # ties the log line to the right text without coupling to + # slayer's private formatting. + expected_digests = { + "alpha": hashlib.sha256(b"text for alpha").hexdigest(), + "beta": hashlib.sha256(b"text for beta").hexdigest(), + } + for r in records: + if "alpha" in r.message: + mid = "alpha" + elif "beta" in r.message: + mid = "beta" + else: + continue + assert "alien_db" in r.message, f"db not carried: {r.message}" + assert "chars=" in r.message or "len=" in r.message, ( + f"length not carried: {r.message}" + ) + # Look for a non-trivial prefix of the expected digest in the + # message — at least 8 hex chars proves we hashed THIS text. + digest = expected_digests[mid] + # Try a few common prefix lengths; the impl picks the budget. + assert any(digest[:k] in r.message for k in (8, 12, 16, 24, 32)), ( + f"expected a sha256 prefix of {digest!r} in {r.message!r}" + ) + + +def test_materialise_cache_memories_partial_batch_persists_good_skips_failed( + monkeypatch, tmp_path: Path, caplog, +): + """Codex /spec review (d): bird-side resilience contract — when + `embed_batch` returns `[vec, None]` (good + failed), the cache + builder must persist the good embedding row and skip / warn for the + failed one. This stays independent of SLayer's internal retry + mechanics (we don't assert on BadRequestError flows); we just + contract on the per-input None we receive.""" + import logging + from unittest.mock import MagicMock + + persisted: list = [] + + async def stub_embed_batch(texts, *, model=None): + # First memory got a vector; second came back None (slayer + # exhausted its per-input retry or the input was unrecoverable). + return [[0.1] * 8, None] + + async def stub_save_embeddings(self, rows): + # YAMLStorage.save_embeddings is an async method on the storage + # object (self + rows); just record what bird tries to persist. + persisted.extend(rows) + + monkeypatch.setattr(otf_cache, "embed_batch", stub_embed_batch) + monkeypatch.setattr(otf_cache, "_embeddings_available", lambda: True) + monkeypatch.setattr( + otf_cache, "_embedding_current_model", lambda: "openai/test-model", + ) + monkeypatch.setattr( + otf_cache, "render_memory_text_for_embedding", + lambda *, memory: f"text for {memory.id}", + ) + + mem_good = MagicMock(); mem_good.id = "good_mem" + mem_bad = MagicMock(); mem_bad.id = "bad_mem" + monkeypatch.setattr( + otf_cache, "encode_kb_as_memories", + lambda *a, **kw: [{"id": "good_mem"}, {"id": "bad_mem"}], + ) + monkeypatch.setattr( + otf_cache.Memory, "model_validate", + lambda d: mem_good if d["id"] == "good_mem" else mem_bad, + ) + + # Patch YAMLStorage.save_embeddings to capture, NOT write to disk. + monkeypatch.setattr( + otf_cache.YAMLStorage, "save_embeddings", + stub_save_embeddings, + raising=False, + ) + + build_dir = tmp_path / "build" + build_dir.mkdir() + with caplog.at_level(logging.WARNING, + logger="bird_interact_agents.slayer_otf.cache"): + asyncio.run(otf_cache._materialise_cache_memories( + db="alien", build_dir=build_dir, kb_rows=[{"id": 1}, {"id": 2}], + )) + + # Exactly one row persisted — the good one. + assert len(persisted) == 1, ( + f"expected one persisted embedding (the good memory); " + f"got {len(persisted)}" + ) + persisted_id = persisted[0].canonical_id + assert "good_mem" in persisted_id, ( + f"expected `good_mem` in canonical_id, got {persisted_id!r}" + ) + # The failed memory's id surfaces in a warning. + fail_warns = [r for r in caplog.records + if r.levelno >= logging.WARNING and "bad_mem" in r.message] + assert fail_warns, ( + "expected a WARNING naming the failed memory id so the operator " + "can find which memory slayer couldn't embed" + ) diff --git a/uv.lock b/uv.lock index 57b9e695..ada74db7 100644 --- a/uv.lock +++ b/uv.lock @@ -496,8 +496,8 @@ requires-dist = [ { name = "mcp-agent", marker = "extra == 'mcp-agent'", specifier = ">=0.0.16" }, { name = "mini-interact-agent", marker = "extra == 'all'", git = "https://github.com/MotleyAI/BIRD-Interact.git?subdirectory=mini_interact%2Fknowledge_based%2Fmini_interact_agent&rev=6b897950780d123c8e8057d15f7d3f1b92819962" }, { name = "mini-interact-agent", marker = "extra == 'original'", git = "https://github.com/MotleyAI/BIRD-Interact.git?subdirectory=mini_interact%2Fknowledge_based%2Fmini_interact_agent&rev=6b897950780d123c8e8057d15f7d3f1b92819962" }, - { name = "motley-slayer", extras = ["embedding-search"], marker = "extra == 'all'", specifier = ">=0.7.3" }, - { name = "motley-slayer", extras = ["embedding-search"], marker = "extra == 'slayer'", specifier = ">=0.7.3" }, + { name = "motley-slayer", extras = ["embedding-search"], marker = "extra == 'all'", specifier = ">=0.7.4" }, + { name = "motley-slayer", extras = ["embedding-search"], marker = "extra == 'slayer'", specifier = ">=0.7.4" }, { name = "nbconvert", marker = "extra == 'analysis'", specifier = ">=7.0" }, { name = "nbformat", marker = "extra == 'analysis'", specifier = ">=5.9" }, { name = "openai", marker = "extra == 'dev'" }, @@ -3224,7 +3224,7 @@ wheels = [ [[package]] name = "motley-slayer" -version = "0.7.3" +version = "0.7.5" source = { registry = "https://pypi.org/simple" } dependencies = [ { name = "duckdb" }, @@ -3241,9 +3241,9 @@ dependencies = [ { name = "tantivy" }, { name = "uvicorn" }, ] -sdist = { url = "https://files.pythonhosted.org/packages/d5/df/72f4f0281daba5a6e56f385584f1687d5c43b1df464c0bebd641ec29479e/motley_slayer-0.7.3.tar.gz", hash = "sha256:1d18e6ec7c577b43f4b948df5c7e25044ad01e2acb201dfb847c1a9dcc61669e", size = 476858, upload-time = "2026-06-10T16:11:06.093Z" } +sdist = { url = "https://files.pythonhosted.org/packages/e2/f6/39aa9adfa3971cdc01783c87c4838ffdd2e3647b1e89e43a2bef2d201f30/motley_slayer-0.7.5.tar.gz", hash = "sha256:61c177a3e1ffb209dc9c2d2444194072506c4d4a844312983bff23576c237e93", size = 515891, upload-time = "2026-06-17T19:30:30.513Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/1d/01/0d4a44f3429716aa6fa0e00216327633240a713a00e71c1770116f27cb54/motley_slayer-0.7.3-py3-none-any.whl", hash = "sha256:aff712df50d8c7bb7fe23450c4a300c6adcf4445b599ce1abb6024d44290167b", size = 542651, upload-time = "2026-06-10T16:11:04.046Z" }, + { url = "https://files.pythonhosted.org/packages/39/2f/43141c7d8ca2276f4fb4ebfd575a66144739c7a67e7660d195e0e18c2ad8/motley_slayer-0.7.5-py3-none-any.whl", hash = "sha256:e41afbc74c42f7bb60ad2c3018b15f8c5aefe286ee47317350d120b07f57d1cd", size = 584029, upload-time = "2026-06-17T19:30:29.077Z" }, ] [package.optional-dependencies]