DEV-1510: audit livesqlbench museum gold + wire single_file overlay - #14
Merged
ZmeiGorynych merged 4 commits intoMay 30, 2026
Conversation
…y (DEV-1510)
Audits the 10 livesqlbench museum SELECT tasks against `museum_kb.jsonl`,
`museum_column_meaning_base.json`, and `museum_schema.txt`. Two locked
findings:
* museum_7 (`edited`): KB 16 says "SESR<4 OR at least three of [4 condition
flags]" but gold treats each flag as independently sufficient. Rewrites to
a NULL-safe `CASE WHEN ... THEN 1 ELSE 0 END + ... >= 3` predicate.
* museum_9 (`clean`): KB 8 (LER) doesn't pin the host table for light
readings, but `column_meaning:ConditionAssessments|LightReadRefObserved`
+ the schema's single-hop declared FK do. Gold's choice IS justified;
the audit ships AS-IS with the column-meaning citation in
`reasoning_summary`.
Wires the audited-gold overlay through to livesqlbench (previously
short-circuited via `not b.gold_required`):
* `Benchmark.audited_gold_layout` ∈ {`per_db`, `single_file`}. mini-interact
stays `per_db` (one sidecar per DB); livesqlbench is `single_file`
(`audited_gold/livesqlbench_audited.jsonl`, `selected_database` per row).
* `apply_audited_gold_overlay` gains a `benchmark` kwarg and dispatches
per layout. Defensive cross-benchmark guard on single_file row's
`selected_database` mismatch.
* `run.run_evaluation` + `cloud/ray_app._load_task_data` drop the
`not gold_required` gate, pass `benchmark=b`.
* `cloud/cli.py` drops the same gate on `--require-audited-gold`.
`missing_audited_gold_ids` dispatches per layout.
* `paths.audited_gold_file(benchmark)` resolves the single_file path
(raises for per_db).
Skill split:
* New `.claude/skills/audit-gold-sql-livesqlbench/` (livesqlbench-shaped
recipe; cites `external_knowledge:<id>` for KB ids in the task's anchor
list, drops `labeled_ambiguity:`/`knowledge_ambiguity:` — no source in
livesqlbench, adds schema FKs as first-class evidence).
* New `.claude/skills/_shared/audit-gold-sql.contract.md` — schema,
citation tokens, audit-status semantics, classification recipe,
anti-patterns. Both skills reference it.
Tests: 22 new tests covering the schema contract, status-claim
consistency, museum_7 + museum_9 pinning, citation resolvability,
original-gold equality with the gated sidecar, overlay dispatch, cloud
wiring, submit-time guard, and cloud-image baking. 31 integration tests
for the audit-execute path.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
More reviews will be available in 35 minutes and 51 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR extends audited-gold SQL handling to support a single consolidated JSONL file layout for livesqlbench benchmarks, complementing the existing per-database sidecar layout. It adds a benchmark-configurable dispatch mechanism throughout the codebase, updates core overlay and validation logic, wires the new ChangesAudited-Gold Single-File Layout for LiveSQLBench
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…ivesqlbench-museum-gold-against-kb-kb-contradicts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/bird_interact_agents/paths.py (1)
175-177: ⚡ Quick winRemove the redundant lazy import.
get_benchmarkis already imported at the module level (line 26), so the lazy import here is unnecessary. Additionally, there is no circular import betweenpaths.pyandbenchmark.py(benchmark.py only imports fromtypingandpydantic), so the comment about avoiding an import cycle is misleading.♻️ Remove the redundant import
def audited_gold_file(*, benchmark: str) -> Path: """DEV-1510: resolve the audited-gold JSONL for a `single_file` benchmark. Mini-interact uses ``per_db`` (each DB gets its own ``<audited_root>/<db>/<db>_audited.jsonl``) and has no single-file path — call ``audited_gold_root()`` directly there. Livesqlbench uses ``single_file`` because its DB names collide with mini-interact's (alien, museum, …); the file is one consolidated ``audited_gold/livesqlbench_audited.jsonl`` keyed by ``instance_id`` with ``selected_database`` as the per-DB discriminator on each row. Raises ``ValueError`` for ``per_db`` benchmarks or unknown tokens — same posture as ``slayer_otf_cache_root`` so a forgotten / typo'd benchmark cannot silently land at a wrong path. """ _validate_benchmark(benchmark) - # Lazy import to avoid a top-level import cycle with `benchmark.py` - # (which imports from `paths` indirectly through other modules). - from bird_interact_agents.benchmark import get_benchmark - b = get_benchmark(benchmark)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bird_interact_agents/paths.py` around lines 175 - 177, Remove the redundant lazy import and misleading comment: delete the comment block about a top-level import cycle and the line "from bird_interact_agents.benchmark import get_benchmark" in paths.py, since get_benchmark is already imported at module scope and there is no circular import; ensure any references to get_benchmark continue to use the existing module-level import.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/bird_interact_agents/paths.py`:
- Around line 175-177: Remove the redundant lazy import and misleading comment:
delete the comment block about a top-level import cycle and the line "from
bird_interact_agents.benchmark import get_benchmark" in paths.py, since
get_benchmark is already imported at module scope and there is no circular
import; ensure any references to get_benchmark continue to use the existing
module-level import.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6e51cf76-b64a-46a4-a3e7-f15e59c71fe7
📒 Files selected for processing (20)
.claude/skills/_shared/audit-gold-sql.contract.md.claude/skills/audit-gold-sql-livesqlbench/SKILL.md.claude/skills/audit-gold-sql/SKILL.mdsrc/bird_interact_agents/benchmark.pysrc/bird_interact_agents/cloud/_audited_gold_check.pysrc/bird_interact_agents/cloud/cli.pysrc/bird_interact_agents/cloud/ray_app.pysrc/bird_interact_agents/harness.pysrc/bird_interact_agents/paths.pysrc/bird_interact_agents/run.pytests/cloud/test_audited_gold_check.pytests/cloud/test_cli.pytests/cloud/test_image.pytests/cloud/test_ray_app.pytests/integration/test_livesqlbench_audited_gold_execute.pytests/test_audited_gold_overlay_wiring.pytests/test_benchmark.pytests/test_dual_eval.pytests/test_livesqlbench_audited_gold.pytests/test_paths.py
…g selected_database (DEV-1510) Codex review of the DEV-1510 PR flagged that both the overlay (`apply_audited_gold_overlay`) and the submit-time guard (`missing_audited_gold_ids`) only rejected MISMATCHING `selected_database` values, not MISSING ones. A row with no `selected_database` field would slip past both checks and the audit could be applied based on `instance_id` alone — defeating the cross-benchmark collision protection that motivated the single_file layout in the first place. Both checks now reject missing AND mismatching `selected_database`: * `harness.apply_audited_gold_overlay`: split into two guards (missing vs mismatching) so the warning log is actionable. * `cloud/_audited_gold_check.missing_audited_gold_ids`: mirror the overlay's logic so a submit-time pass doesn't disagree with the overlay's mid-run rejection (which would silently fall back to the original gold). Tests: 4 new — overlay+guard each get a "no selected_database" case and an "empty selected_database" case. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… cleanup (DEV-1510)
Codex's second-pass review flagged that the cross-benchmark guard only
verifies `selected_database`, not the row's `benchmark` field. Because
DB names overlap across benchmarks BY DESIGN (alien, museum, … exist
in both mini-interact and livesqlbench — that's why single_file exists),
a misfiled row with the right (instance_id, selected_database) but
the wrong `benchmark` would still slip through.
Both layers now check the `benchmark` field too:
* `harness.apply_audited_gold_overlay` (single_file branch): after the
`selected_database` check, verify `row.get("benchmark") == benchmark.name`.
Missing or mismatching → log + missing-row.
* `cloud/_audited_gold_check._load_single_file_audit_index`: capture
the row's `benchmark` field into the per-row tuple.
* `cloud/_audited_gold_check.missing_audited_gold_ids` (single_file
branch): mirror the overlay's benchmark-field check.
Also: paths.py:175-177 had a redundant lazy import of `get_benchmark`
(already imported at module level, line 26) with a misleading comment
about an import cycle that doesn't exist. CodeRabbit nitpick — deleted.
Tests:
* 4 new — overlay+guard each get a "wrong benchmark" and "missing
benchmark" case.
* `_write_lsb_audited` helper defaults to injecting
`benchmark: "livesqlbench"` for existing rows so the older test cases
(which only set `selected_database`) keep passing under the new guard.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 31, 2026
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
museum_kb.jsonl+museum_column_meaning_base.json+museum_schema.txt. The deliverable isaudited_gold/livesqlbench_audited.jsonl(gitignored — rides into the cloud image via the BuildKit `audited-gold=` context, not git).Acceptance criteria status
Follow-up
A child Linear issue will be filed after merge: `claude_sdk_otf: consult column meanings + schema FKs when KB underspecifies the join graph`. museum_9 is the worked example — the agent's `UsageRecords→Showcases→EnvironmentalReadingsCore→LightAndRadiationReadings` 3-hop chain is KB-faithful but column-meaning-blind; the canonical answer requires reading the schema's declared FK.
Pre-existing test failures (out of scope)
Two tests in `tests/test_db_isolation_pre_copy.py` fail on `origin/main` (commit `9d4975f` / DEV-1509, merged via PR #13, before this branch existed). Per the user's instruction these are NOT fixed here.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests