DEV-1525: clean up handling of multiple benchmarks - #26
Conversation
…nto egor/dev-1525-clean-up-handling-of-multiple-benchmarks
Five discrepancies vs. human-authored annotations: 1. provenance.task_jsonl_path was guessed from DB name → harness now overwrites from _benchmark_task_jsonl_name(benchmark). 2. external_knowledge was left empty → harness now copies from task_data verbatim. 3. masked_terms (is_mask=True) were left empty → harness merges critical_ambiguity entries for mini_interact without duplicating agent-supplied is_mask=False entries. 4. evidence_sources_consulted was left empty → prompt now explicitly instructs the agent to list every source it reads. 5. gold_variants was left empty despite non-empty audited_gold_variants → submit_annotation now enforces: if audited variants are submitted and original_gold_is_correct=False, gold_variants must reference every variant. Also requires at least one primary=True in audited_gold_variants. New harness function _fill_deterministic_fields() handles (1-3); runs after _fill_audited_gold_ref_files() in run_task(). prompts.py gains a field-population section for the judgment fields (4-5). Seven new tests cover all changes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…p-handling-of-multiple-benchmarks
…nto egor/dev-1525-clean-up-handling-of-multiple-benchmarks
…dation - _fill_deterministic_fields: wrap string metadata_evidence in a 1-element list instead of silently dropping it (was: `else []`) - submit_annotation: reject audited_gold_variants rows where primary is a non-boolean truthy (e.g. integer 1) — must be JSON true/false - Tests for both behaviors Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
If the agent submitted an is_mask=False schema-linking entry with the same surface term as a critical_ambiguity item, the previous dedup set included it and silently blocked the authoritative is_mask=True entry from being added. Fix by deduplicating only against existing is_mask=True entries. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Renames all benchmarks to official hyphenated names, unifies the CLI
to a single `claude_sdk` framework with automatic dispatch, and makes
all per-benchmark artifact paths uniform with no hard-wired defaults.
Benchmark renames:
- mini_interact → mini-interact
- livesqlbench → livesqlbench-base-lite-sqlite
- livesqlbench_postgres → livesqlbench-base-lite
- mini_interact_postgres → bird-interact-lite-exp
Path helpers:
- slayer_otf_cache/<benchmark>/ and slayer_models_otf/<benchmark>/
(single BIRD_OTF_CACHE_ROOT / BIRD_SLAYER_MODELS_OTF_ROOT env var
controls parent dir; benchmark name appended as subdir)
- New gated_gold_root(*, benchmark) replaces Benchmark.gold_root_env
- Removed shims: mini_interact_root(), mini_interact_data_file(),
livesqlbench_root(), livesqlbench_data_file()
CLI / agent dispatch:
- --framework accepts only `claude_sdk`; _make_runner dispatches to
the right OTF agent flavor based on (benchmark.one_shot, query_mode)
- Removed _validate_framework_dataset_mode, _validate_one_shot_framework,
_FRAMEWORK_DATASET_MODE_BINDING
- make_runner() and run_one_task() require dataset: str (no default)
No silent defaults:
- All `task_data.get("dataset") or "mini_interact"` fallbacks replaced
with hard ValueError raises
- ray_app.py --dataset is now required (no default)
Migration script:
- scripts/migrate_benchmark_paths.py for renaming existing artifact dirs
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughCanonicalize benchmark identifiers to hyphenated forms, require explicit dataset threading, add per-benchmark paths including gated-gold, restrict CLI/framework to claude_sdk, thread dataset through run/cloud flows, strengthen annotator validation/prompts/tools, and update scripts/tests to match. ChangesCanonical benchmark naming and claude_sdk-only execution
Sequence Diagram(s)(Skipped — changes are broad plumbing and validation updates.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
…h new benchmark and framework names - scripts/migrate_benchmark_paths.py: introduce _mv_into_subdir() helper that moves a dir into its own subdirectory via a sibling temp name, avoiding the shutil.move-into-self failure for slayer_otf_cache → slayer_otf_cache/mini-interact and slayer_models_otf → slayer_models_otf/mini-interact - README.md: update all --dataset and --framework CLI examples to the new canonical hyphenated benchmark names (mini-interact, livesqlbench-base-lite-sqlite) and the unified claude_sdk framework name; update OTF artifact roots table to the new nested layout; drop obsolete per-benchmark env var names; consolidate claude_sdk_otf / claude_sdk_otf_ainteract section headers under single claude_sdk - scripts/run_haiku_slayer.sh: --dataset mini_interact → mini-interact, --framework pydantic_ai → claude_sdk - scripts/run_three_way.sh: FRAMEWORK default pydantic_ai → claude_sdk, --dataset mini_interact → mini-interact, update usage help text Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…t tasks require evaluator_prompt - submit_annotation: reject original_gold_is_correct=False + empty variants when verdict is 'sufficient' or 'ambiguous' (task is solvable, correction needed) - submit_annotation: require non-empty evaluator_prompt when verdict='insufficient' so the LLM judge can score responses on underspecified tasks - prompt: add EVALUATOR PROMPT instruction for insufficient tasks - Tests for all three new paths Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…by benchmark flavor - New get_column_sample_values tool returns the N most frequent values for a column (GROUP BY / ORDER BY freq DESC), so the agent can see actual stored values instead of relying on sparse EX. examples in column meanings - Prompt restructured into shared fragments (_DATA_BUNDLE_ITEMS_1_5, _SUFFICIENCY_VERDICTS, _AUDIT_CHECKLIST_TAIL, _SAMPLE_VALUES_INSTRUCTION) composed into benchmark-specific bodies: _INTERACT_BODY includes the critical/knowledge-ambiguity resolutions (items 6-7) and the one-shot body omits them; sufficiency definition reflects whether clarification is possible - Agent is instructed to fetch up to 50 sampled values per relevant column Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…y Codex - run.py: _maybe_force_wipe_otf now checks for "claude_sdk" (the only CLI framework) instead of the old internal names; --otf-rebuild was silently a no-op for every user after the framework rename - agents/annotator/agent.py: _AUDITED_GOLD_FILE values updated to the new canonical filenames (mini-interact_audited.jsonl, livesqlbench-base-lite-sqlite_audited.jsonl); old underscored/shortened names no longer exist after the migration - Update all tests that passed old framework names to _maybe_force_wipe_otf (claude_sdk_otf, claude_sdk_otf_ainteract, pydantic_ai_recursive, pydantic_ai_otf_encode) to use the single public name "claude_sdk" - Update annotator test assertions to expect the new filenames Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…cripts - scripts/migrate_benchmark_paths.py: _mv_into_subdir now checks for benchmark-named subdirs inside src before moving; aborts with a clear error if new-code runs already created the nested layout (e.g. slayer_otf_cache/livesqlbench-base-lite-sqlite/ exists inside the flat slayer_otf_cache/ dir), preventing data corruption on partially-migrated trees - scripts/run_haiku_slayer.sh: replace --slayer-storage-root (pre-encoded flag) with --slayer-setup on-the-fly; _validate_slayer_setup now rejects claude_sdk + slayer mode without this flag - scripts/run_three_way.sh: add --slayer-setup on-the-fly to the slayer leg of run_ours(); raw leg is unaffected Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- annotation_io._canonical_benchmark: remove underscore normalization
so mini-interact (hyphenated) lands in annotations/mini-interact/
- post_run_merge._normalise_benchmark: same removal
- annotate.py: remove inline replace('-','_') when building task_annotation_ref
- cloud/cli.py: --framework choices=["claude_sdk"] only
- integration tests: replace removed path shims (mini_interact_root,
livesqlbench_root) with benchmark_data_root("...")
- All test assertions updated for new hyphenated annotation paths and
single-framework CLI restriction
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
oracle runs go directly to run_oracle_task, bypassing the agent dispatch entirely — so the claude_sdk framework constraint does not apply. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…gold filename; update README examples - cloud/cli.py: add _validate_framework_mode call alongside existing _validate_dataset_mode and _validate_slayer_setup so unsupported mode combinations fail fast at submit time - scripts/verify_audited_gold.py: update consolidated filename from mini_interact_audited.jsonl to mini-interact_audited.jsonl - README.md: add missing --framework claude_sdk to oracle example; add --slayer-setup on-the-fly to slayer example - tests/cloud/test_cli.py: replace c-interact with a-interact in tests that expect success (c-interact is now correctly rejected for mini-interact + claude_sdk) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Move dst.parent.mkdir inside the `if not dry` block to match _mv_into_subdir. Previously dry-run could create new directories even though it promised not to mutate the filesystem. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
annotate.py, grade_in_place.py, regrade.py all had leftover
benchmark.replace('-', '_') calls from before the canonical names
switched to hyphenated form. These caused:
- annotate CLI to fail for mini-interact (KeyError in get_benchmark)
- load_audited_gold_rows_for to silently return [] (all N2/N3 tiers wrong)
- auto-generated submission annotations to write old underscore paths
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…utodiscovery - benchmark_data.py: extract _hash_dir_into; ensure_uploaded now includes gated_gold_root(benchmark) content in the hash and uploads it to _gated_gold/ inside the prefix, so gold files there land in-cluster - driver.py: _validate_gold_under_data_root accepts gold under either benchmark_data_root OR gated_gold_root; _in_cluster_gold_file maps gated gold paths to container_data_dir/_gated_gold/<relpath> - ray_app_annotator.py: replace hard-coded livesqlbench filename in gold autodiscovery with a *.jsonl glob (forward-compatible for new benchmarks) - tests: isolation monkeypatches for gated_gold_root; new tests for gated gold upload behavior, validation acceptance, and in-cluster translation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (4)
tests/test_paths.py (1)
527-527: 💤 Low valueOptional: Consider prefixing unused variables with underscore.
Static analysis flags that the unpacked
mainvariable is unused in these test setup calls. While not a functional issue, prefixing with_mainwould silence the warning.Suggested change
For line 527:
- main, _wt = _setup_main_and_worktree(tmp_path, monkeypatch) + _main, _wt = _setup_main_and_worktree(tmp_path, monkeypatch)For line 584:
- main, _wt = _setup_main_and_worktree(tmp_path, monkeypatch) + _main, _wt = _setup_main_and_worktree(tmp_path, monkeypatch)Also applies to: 584-584
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_paths.py` at line 527, The test unpacks main and _wt from _setup_main_and_worktree but static analysis reports main is unused; rename the variable to _main (or use _ if preferred) in the unpack at the call to _setup_main_and_worktree to silence the warning, and make the same change for the other occurrence referenced (the similar unpack at the second call around line 584) so both unused unpacked values are prefixed with an underscore.src/bird_interact_agents/cloud/post_run_merge.py (1)
600-601: 💤 Low valueConsider removing the now-redundant
_normalise_benchmarkhelper.Since the function now returns its input unchanged (no normalization), it serves no purpose and could be removed. Direct usage of the
benchmarkparameter inmerge_task_annotations()(line 616) andmerge_audited_gold_variants()(line 662) would simplify the code without changing behavior.♻️ Proposed simplification
Remove the function:
-def _normalise_benchmark(benchmark: str) -> str: - return benchmark - -And replace its two call sites:
def merge_task_annotations( *, downloaded_run_dir: Path, benchmark: str, annotations_root: Path, ) -> TaskAnnotationMergeReport: - bm = _normalise_benchmark(benchmark) + bm = benchmark report = TaskAnnotationMergeReport()def merge_audited_gold_variants( *, downloaded_run_dir: Path, benchmark: str, audited_gold_root: Path, override: bool = False, ) -> AuditedGoldVariantsMergeReport: - bm = _normalise_benchmark(benchmark) + bm = benchmark report = AuditedGoldVariantsMergeReport()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bird_interact_agents/cloud/post_run_merge.py` around lines 600 - 601, The helper _normalise_benchmark is redundant because it returns its input unchanged; remove the _normalise_benchmark function and replace its call sites in merge_task_annotations and merge_audited_gold_variants to pass the benchmark parameter directly (i.e., use the benchmark variable instead of calling _normalise_benchmark(benchmark)); ensure any imports or tests referencing _normalise_benchmark are updated/removed and run tests to verify no behavior change.tests/test_claude_sdk_otf_ainteract_run_wiring.py (1)
208-208: 💤 Low valueConsider using ASCII
xinstead of×in docstring.Static analysis flags the multiplication sign character in the docstring. For consistency and to avoid encoding issues, consider using a regular ASCII
xor the word "and".📝 Optional style fix
- """Dataset×mode gate: livesqlbench doesn't support a-interact.""" + """Dataset x mode gate: livesqlbench doesn't support a-interact."""🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_claude_sdk_otf_ainteract_run_wiring.py` at line 208, The module/test docstring contains a multiplication sign '×' which static analysis flags; update the string in tests/test_claude_sdk_otf_ainteract_run_wiring.py (the docstring around the dataset×mode gate) to use an ASCII 'x' or the word "and" (e.g., "Dataset x mode gate: livesqlbench doesn't support a-interact.") to avoid encoding issues and ensure consistency.src/bird_interact_agents/agents/annotator/agent.py (1)
454-503: 💤 Low valueConsider using iterable unpacking for list extension.
At line 494-501, the code concatenates lists using
+. This could be simplified using tuple unpacking in the list constructor.♻️ Suggested refactoring
- updated.masked_terms = list(updated.masked_terms) + [ - MaskedTerm( - term=term, - type=item.get("type", "knowledge_linking_ambiguity"), - is_mask=True, - metadata_evidence=evidence, - ) - ] + updated.masked_terms = [ + *updated.masked_terms, + MaskedTerm( + term=term, + type=item.get("type", "knowledge_linking_ambiguity"), + is_mask=True, + metadata_evidence=evidence, + ), + ]Based on learnings: Static analysis tools flagged this pattern (RUF005).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bird_interact_agents/agents/annotator/agent.py` around lines 454 - 503, In _fill_deterministic_fields, avoid concatenating lists with + when appending the new MaskedTerm to updated.masked_terms; instead construct the new list using iterable unpacking or .extend to preserve type and avoid RUF005 static-analysis warnings (e.g., replace updated.masked_terms = list(updated.masked_terms) + [MaskedTerm(...)] with updated.masked_terms = [*updated.masked_terms, MaskedTerm(...)] or call updated.masked_terms.extend([MaskedTerm(...)]) inside the function where MaskedTerm is created).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bird_interact_agents/cloud/benchmark_data.py`:
- Around line 120-125: The gated-gold directory upload currently calls
gcs.upload_dir_prefix(gated_gold, f"{prefix.rstrip('/')}/{GATED_GOLD_SUBDIR}",
client=client) without excluding VCS metadata, causing .git and similar dirs to
be uploaded even though _hash_dir_into() ignores them; update the call to pass
exclude=_is_vcs_path (or the existing VCS filter function used by
_hash_dir_into) so gcs.upload_dir_prefix(..., exclude=_is_vcs_path,
client=client) omits VCS files and keeps the uploaded file set consistent with
the hash.
In `@src/bird_interact_agents/cloud/upload_back.py`:
- Around line 240-243: The current code silently returns when dataset is missing
(cfg.get("dataset")), which hides misconfigured actors; change this to surface
the error by raising an exception (e.g., ValueError or RuntimeError) with a
clear message that includes the cfg or relevant keys so the missing `dataset`
invariant fails fast instead of quietly skipping. Locate the block that calls
cfg.get("dataset") and currently returns, and replace the silent return with a
raise that mentions the missing dataset and includes context (for example
referencing `cfg` and the attempted `get_benchmark(dataset)` usage) so callers
and the outer except can log/handle it properly.
In `@src/bird_interact_agents/eval/annotation_io.py`:
- Around line 40-43: Update the module-level docstring in
src/bird_interact_agents/eval/annotation_io.py to reflect the current behavior:
canonical benchmark names are hyphenated (e.g. "mini-interact"), no
normalization is applied (benchmark names are returned as-is), and there is no
shared dash/underscore tree or underscore-to-dash normalization; remove the
outdated mention of underscore normalization and shared trees and optionally
reference DEV-1525 for provenance.
In `@tests/cloud/test_annotator_gcs.py`:
- Around line 71-74: The test currently compares identical calls so it no longer
validates normalization; update the test for stable_task_annotation_blob to
assert the intended behavior: either replace the assertion with a context that
expects a ValueError when using an underscore token (call
gcs.stable_task_annotation_blob("mini_interact", "shop", "shop_1") and assert it
raises ValueError), or remove the test entirely if underscore/dash normalization
is no longer supported; locate the test function
test_stable_task_annotation_blob_normalises_dash_benchmark and change its body
to perform the appropriate raise-or-remove change for
gcs.stable_task_annotation_blob.
- Around line 83-86: The test currently compares two identical calls and no
longer verifies normalization; update
test_stable_audited_gold_variants_blob_normalises_dash_benchmark to assert that
gcs.stable_audited_gold_variants_blob rejects underscores: call
gcs.stable_audited_gold_variants_blob with a repo name containing an underscore
(e.g., "mini_interact") and wrap it in a pytest.raises(ValueError) to confirm it
raises, referencing the function name stable_audited_gold_variants_blob from
bird_interact_agents.cloud.gcs; alternatively, if you prefer removal, delete the
redundant test entirely.
In `@tests/cloud/test_annotator_merge.py`:
- Around line 217-223: Tests in the override-path cases still use the old
canonical name "mini_interact" and file "mini_interact_audited.jsonl"; update
those references to the new canonical names used earlier: change any
benchmark="mini_interact" to benchmark="mini-interact" and any filenames like
"mini_interact_audited.jsonl" to "mini-interact_audited.jsonl" (look for
occurrences in test functions and variables in
tests/cloud/test_annotator_merge.py such as the override-path tests that
construct downloaded_run_dir/audited_gold_root and consolidate into
mini_interact_audited.jsonl).
In `@tests/cloud/test_driver.py`:
- Around line 1766-1769: The assertion is too loose because 'or "/data/" in
result' allows unrelated in-cluster paths; update the test that calls
driver._in_cluster_gold_file(ns) to assert the result points exactly into the
benchmark's container directory by checking for the benchmark-specific path plus
the gold filename. Instruct the test to compute the expected fragment using the
benchmark name ("livesqlbench-base-lite-sqlite") and the basename of
ns.gold_file (or gold.name) and assert that expected_fragment in result (or that
result.startswith(f"/livesqlbench-base-lite-sqlite/{basename}")), removing the
generic '"/data/"' alternative so the assertion is precise.
In `@tests/test_annotator_agent.py`:
- Around line 643-658: The test uses a non-canonical benchmark id; update the
benchmark argument in test_fill_deterministic_fields_does_not_mutate_original to
use the hyphenated canonical name "mini-interact" instead of "mini_interact"
when calling _fill_deterministic_fields (reference the test function name and
the _fill_deterministic_fields call to locate the change).
- Around line 564-641: Replace non-canonical benchmark identifiers passed to
_fill_deterministic_fields: in
test_fill_deterministic_fields_no_duplicate_masked_terms change
benchmark="mini_interact" to benchmark="mini-interact", and in
test_fill_deterministic_fields_livesqlbench_skips_masked_terms change
benchmark="livesqlbench" to the canonical hyphenated identifier (e.g.,
"live-sql-bench"); update those string literals where _fill_deterministic_fields
is invoked so all benchmark names use hyphens consistently.
In `@tests/test_livesqlbench_loader.py`:
- Around line 122-125: The inline comment accompanying the assertion that checks
dataset values is stale; update the comment text to reflect the new canonical
dataset name used in the assertion. Locate the assertion that reads assert
all(r.get("dataset") == "livesqlbench-base-lite-sqlite" for r in rows) and
change the trailing comment which currently references
task['dataset']='livesqlbench' so it instead refers to
task['dataset']='livesqlbench-base-lite-sqlite' (or equivalent wording) to match
the actual checked value.
In `@tests/test_one_shot_mode.py`:
- Line 159: The guard test's argv for the one-shot/livesqlbench branch omits the
now-required --dataset flag so argparse fails before the
must_contain=["one-shot","livesqlbench"] assertion is reached; update the argv
array used in the test (the argv tuple/list near the "--framework", "claude_sdk"
entry in tests/test_one_shot_mode.py) to include an explicit non-livesqlbench
dataset, e.g. add "--dataset", "mini-interact", so the parser succeeds and the
one-shot/livesqlbench guard assertion is exercised.
In `@tests/test_pydantic_ai_otf_encode_run_wiring.py`:
- Around line 50-52: The test suite was partially migrated to the public
framework path but still calls run_evaluation(...
framework="pydantic_ai_otf_encode") and monkeypatches PydanticAIOtfEncodeAgent,
causing branch-selection mismatch; update the tests (the calls to run_evaluation
and any monkeypatch targets) to use the public framework string "claude_sdk" and
the corresponding agent/class path used for that public implementation so the
branch-selection assertions (e.g., choices = _framework_choices_from_parser())
and the constructor-path tests remain consistent with the rest of the file.
In `@tests/test_run_local_inline_grader.py`:
- Around line 196-199: Update the stale comment above the assertion that checks
benchmark values: change the comment that currently reads about the "canonical
underscore form" to reflect the actual expected hyphenated form
("mini-interact"). Locate the assertion referencing calls and the string
"mini-interact" (the line containing assert all(c["benchmark"] ==
"mini-interact" for c in calls)) and modify the preceding comment to accurately
state that the benchmark token is expected in hyphenated form.
---
Nitpick comments:
In `@src/bird_interact_agents/agents/annotator/agent.py`:
- Around line 454-503: In _fill_deterministic_fields, avoid concatenating lists
with + when appending the new MaskedTerm to updated.masked_terms; instead
construct the new list using iterable unpacking or .extend to preserve type and
avoid RUF005 static-analysis warnings (e.g., replace updated.masked_terms =
list(updated.masked_terms) + [MaskedTerm(...)] with updated.masked_terms =
[*updated.masked_terms, MaskedTerm(...)] or call
updated.masked_terms.extend([MaskedTerm(...)]) inside the function where
MaskedTerm is created).
In `@src/bird_interact_agents/cloud/post_run_merge.py`:
- Around line 600-601: The helper _normalise_benchmark is redundant because it
returns its input unchanged; remove the _normalise_benchmark function and
replace its call sites in merge_task_annotations and merge_audited_gold_variants
to pass the benchmark parameter directly (i.e., use the benchmark variable
instead of calling _normalise_benchmark(benchmark)); ensure any imports or tests
referencing _normalise_benchmark are updated/removed and run tests to verify no
behavior change.
In `@tests/test_claude_sdk_otf_ainteract_run_wiring.py`:
- Line 208: The module/test docstring contains a multiplication sign '×' which
static analysis flags; update the string in
tests/test_claude_sdk_otf_ainteract_run_wiring.py (the docstring around the
dataset×mode gate) to use an ASCII 'x' or the word "and" (e.g., "Dataset x mode
gate: livesqlbench doesn't support a-interact.") to avoid encoding issues and
ensure consistency.
In `@tests/test_paths.py`:
- Line 527: The test unpacks main and _wt from _setup_main_and_worktree but
static analysis reports main is unused; rename the variable to _main (or use _
if preferred) in the unpack at the call to _setup_main_and_worktree to silence
the warning, and make the same change for the other occurrence referenced (the
similar unpack at the second call around line 584) so both unused unpacked
values are prefixed with an underscore.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e5cf50ff-2007-4349-9ca1-50cd1bd4d603
📒 Files selected for processing (115)
README.mdscripts/append_deferred_stubs.pyscripts/build_w4d_instructions.pyscripts/compare_sar_vs_inhouse.pyscripts/dev1515_households_14_multivariant.pyscripts/export_slayer_models.pyscripts/kb_op_audit_classify.pyscripts/kb_op_audit_extract.pyscripts/migrate_benchmark_paths.pyscripts/multi_kb_audit.pyscripts/prepare_livesqlbench.pyscripts/redirect_kb_duplicates.pyscripts/refresh_kb_annotations.pyscripts/run_haiku_slayer.shscripts/run_three_way.shscripts/verify_audited_gold.pyscripts/verify_kb_coverage.pysrc/bird_interact_agents/agents/annotator/agent.pysrc/bird_interact_agents/agents/annotator/prompts.pysrc/bird_interact_agents/agents/claude_sdk_otf/agent.pysrc/bird_interact_agents/agents/claude_sdk_otf_ainteract/agent.pysrc/bird_interact_agents/agents/claude_sdk_otf_ainteract_raw/agent.pysrc/bird_interact_agents/agents/claude_sdk_otf_raw/agent.pysrc/bird_interact_agents/agents/pydantic_ai_otf_encode/agent.pysrc/bird_interact_agents/agents/pydantic_ai_recursive/agent.pysrc/bird_interact_agents/benchmark.pysrc/bird_interact_agents/cloud/_audited_gold_check.pysrc/bird_interact_agents/cloud/benchmark_data.pysrc/bird_interact_agents/cloud/cli.pysrc/bird_interact_agents/cloud/driver.pysrc/bird_interact_agents/cloud/post_run_merge.pysrc/bird_interact_agents/cloud/ray_app.pysrc/bird_interact_agents/cloud/ray_app_annotator.pysrc/bird_interact_agents/cloud/upload_back.pysrc/bird_interact_agents/config.pysrc/bird_interact_agents/eval/annotate.pysrc/bird_interact_agents/eval/annotation_io.pysrc/bird_interact_agents/eval/grade_in_place.pysrc/bird_interact_agents/eval/implicit_annotation.pysrc/bird_interact_agents/eval/regrade.pysrc/bird_interact_agents/harness.pysrc/bird_interact_agents/paths.pysrc/bird_interact_agents/run.pysrc/bird_interact_agents/sar_audit/driver.pysrc/bird_interact_agents/slayer_pipeline/orchestrator.pytests/cloud/test_annotator_gcs.pytests/cloud/test_annotator_merge.pytests/cloud/test_annotator_ray_app.pytests/cloud/test_audited_gold_check.pytests/cloud/test_benchmark_data.pytests/cloud/test_cli.pytests/cloud/test_driver.pytests/cloud/test_fetch_annotation_merge.pytests/cloud/test_inline_grader.pytests/cloud/test_ray_app.pytests/cloud/test_run_one_task.pytests/cloud/test_upload_back.pytests/conftest.pytests/integration/test_livesqlbench_audited_gold_execute.pytests/sar_audit/test_cli_exit_code.pytests/sar_audit/test_verifier.pytests/test_annotator_agent.pytests/test_annotator_tools.pytests/test_audited_gold_overlay_wiring.pytests/test_autopsy.pytests/test_benchmark.pytests/test_benchmark_naming.pytests/test_benchmark_postgres.pytests/test_claude_sdk_otf_agent.pytests/test_claude_sdk_otf_ainteract_agent.pytests/test_claude_sdk_otf_ainteract_raw_agent.pytests/test_claude_sdk_otf_ainteract_run_wiring.pytests/test_claude_sdk_otf_raw_agent.pytests/test_claude_sdk_otf_raw_run_wiring.pytests/test_claude_sdk_otf_run_wiring.pytests/test_cloud_paths_unchanged.pytests/test_cloud_paths_uniform.pytests/test_db_connection.pytests/test_db_isolation.pytests/test_db_isolation_pre_copy.pytests/test_dual_eval.pytests/test_eval_annotate_cli.pytests/test_eval_annotation_schema.pytests/test_harness_postgres_dispatch.pytests/test_kb_memory_encoder.pytests/test_livesqlbench_audited_gold.pytests/test_livesqlbench_loader.pytests/test_miss_diagnostics.pytests/test_one_shot_mode.pytests/test_one_shot_otf_encode_factories.pytests/test_one_shot_recursive_factories.pytests/test_one_shot_run.pytests/test_otf_encode_reference_root.pytests/test_otf_postgres.pytests/test_otf_rebuild_per_benchmark.pytests/test_otf_rebuild_wiring.pytests/test_paths.pytests/test_paths_annotations.pytests/test_paths_uniform_otf.pytests/test_prepare_livesqlbench.pytests/test_projection_resolver.pytests/test_pydantic_ai_otf_encode_agent.pytests/test_pydantic_ai_otf_encode_run_wiring.pytests/test_pydantic_ai_recursive_trajectory.pytests/test_pydantic_ai_recursive_usage.pytests/test_recursive_runtime_db_root.pytests/test_regrade_cli.pytests/test_run_framework_dispatch.pytests/test_run_local_inline_grader.pytests/test_slayer_otf_smoke.pytests/test_slayer_setup_flag.pytests/test_submit_postgres.pytests/test_tolerant_grader_multi_sql_conn.pytests/test_tolerant_grader_orchestration.pytests/test_tolerant_grader_postgres.py
…mple-values tool - annotation_schema: add EVIDENCE_SOURCE_PREFIXES constant and field_validator that rejects any evidence_sources_consulted entry without a canonical prefix (kb:, column:, schema:, sample_values:, sql:, critical_ambiguity:, critical_ambiguity_evidence:, knowledge_ambiguity:) - agent: add get_column_sample_values tool (top-N frequent values via GROUP BY / ORDER BY freq DESC); enforce evidence_sources_consulted is non-empty at submit_annotation time - prompts: restructure into interactive vs one-shot bodies sharing common string fragments; document the complete information bundle available in each benchmark flavor; add per-prefix evidence-source table and gold-variants field-population instructions - tests: update evidence fixtures to canonical prefix format; add tests for empty-evidence rejection, invalid-prefix rejection, get_column_sample_values SQL shape, and museum_1 coverage exclusion Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…te, n-clamping, dup-variant-id - submit_annotation: inject provenance.task_jsonl_path / task_jsonl_instance_id into ta_dict BEFORE model_validate so agents following the "don't populate harness-filled fields" prompt instruction no longer hit a validation loop - _fill_deterministic_fields: harness-authoritative critical_ambiguity entries now overwrite any stale agent-submitted is_mask=True entry with the same term instead of silently skipping the update - get_column_sample_values: validate n with try/except and clamp to 1..50; return a clear error for bad or out-of-range values - submit_annotation: reject audited_gold_variants with duplicate variant_id before the cross-check (downstream merge is keyed on variant_id) - Import EVIDENCE_SOURCE_PREFIXES constant and use it in the empty-evidence error message instead of a hardcoded copy Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- upload_back.py: raise ValueError when cfg['dataset'] is missing instead of silently returning - ray_app_annotator.py: remove benchmark_data_root from gold autodiscovery (prevents tasks file being picked up as gold) - annotation_io.py: update docstring — benchmark names are now hyphenated - test_annotator_gcs.py: fix trivial identity tests to actually test hyphen→underscore normalisation - test_annotator_merge.py: update mini_interact → mini-interact and filename accordingly - test_driver.py: tighten _in_cluster_gold_file assertion to exact expected path + add gated_gold_root monkeypatch - test_annotator_agent.py: update benchmark names to canonical hyphenated form - test_one_shot_mode.py: add --dataset mini-interact so the one-shot guard is actually reached - test_pydantic_ai_otf_encode_run_wiring.py: update to use claude_sdk + ClaudeSDKOtfAInteractAgent - test_livesqlbench_loader.py: fix stale comment mentioning old benchmark name - test_run_local_inline_grader.py: fix stale comment about underscore form - audited_gold/livesqlbench_audited.jsonl: add missing museum_1 audit row Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nto egor/dev-1525-clean-up-handling-of-multiple-benchmarks
Update all remaining old benchmark names in test_annotator_agent.py:
- benchmark="mini_interact" → "mini-interact"
- benchmark="livesqlbench" → "livesqlbench-base-lite-sqlite"
- _benchmark_task_jsonl_name("livesqlbench") → ("livesqlbench-base-lite-sqlite")
Three tests added by dev-1518 were failing because they used the pre-rename
canonical names that _fill_deterministic_fields no longer recognises.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Line 180: Replace underscore benchmark names with canonical hyphenated names:
change occurrences of "mini_interact" to "mini-interact" in the README examples
(specifically the CLI flag "--benchmark mini_interact") and any audited-gold
filename references so the CLI and file paths match the canonical benchmark
naming and won't be rejected.
In `@src/bird_interact_agents/agents/annotator/agent.py`:
- Around line 157-163: The prefill logic assumes ta_dict is a dict and its
"provenance" field is a dict which can raise on malformed JSON; guard by
checking that ta_dict (from task_annotation_json) is a mapping before calling
setdefault and if it's not, replace or coerce it to an empty dict, then ensure
provenance is a dict (if ta_dict.get("provenance") is not a dict, assign an
empty dict) before setting task_jsonl_path and task_jsonl_instance_id; update
the block around ta_dict, _prov, _benchmark_task_jsonl_name and task_data to
perform these type checks and safe assignments so TaskAnnotation.model_validate
runs and returns validation errors rather than raising.
In `@tests/test_annotator_tools.py`:
- Around line 1160-1168: The new test fixtures (e.g. variable
variant_int_primary and the other fixture blocks around the same change) use
benchmark: "mini_interact" but _setup_ctx now expects "mini-interact", causing
spurious benchmark-mismatch failures; update the benchmark token in those JSON
fixtures from "mini_interact" to "mini-interact" so the tests exercise the
intended primary-typing/duplicate-variant_id logic rather than failing early on
benchmark mismatch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 372dd501-c7c5-4f81-8315-8b34a0615d86
📒 Files selected for processing (18)
README.mdsrc/bird_interact_agents/agents/annotator/agent.pysrc/bird_interact_agents/agents/annotator/prompts.pysrc/bird_interact_agents/cloud/benchmark_data.pysrc/bird_interact_agents/cloud/ray_app_annotator.pysrc/bird_interact_agents/cloud/upload_back.pysrc/bird_interact_agents/eval/annotation_io.pysrc/bird_interact_agents/eval/annotation_schema.pytests/cloud/test_annotator_gcs.pytests/cloud/test_annotator_merge.pytests/cloud/test_driver.pytests/test_annotator_agent.pytests/test_annotator_tools.pytests/test_eval_annotation_schema.pytests/test_livesqlbench_loader.pytests/test_one_shot_mode.pytests/test_pydantic_ai_otf_encode_run_wiring.pytests/test_run_local_inline_grader.py
✅ Files skipped from review due to trivial changes (2)
- tests/test_run_local_inline_grader.py
- tests/test_livesqlbench_loader.py
🚧 Files skipped from review as they are similar to previous changes (6)
- src/bird_interact_agents/agents/annotator/prompts.py
- src/bird_interact_agents/eval/annotation_io.py
- tests/test_annotator_agent.py
- tests/cloud/test_driver.py
- tests/test_one_shot_mode.py
- src/bird_interact_agents/cloud/upload_back.py
- driver.py: update 3 function-default benchmark strings from 'mini_interact' to 'mini-interact' - annotation_schema.py: fix docstring example filename - regrade.py: update stale comment (benchmark names + gold path) - agent.py: guard submit_annotation against non-dict ta_dict / non-dict provenance before setdefault calls - README.md: fix --benchmark and audited-gold filename in annotator cloud-smoke example - test_annotator_tools.py: update all 'mini_interact' / 'livesqlbench' tokens to canonical hyphenated names - test_eval_annotation_schema.py: fix audited-gold filename in test fixture - test_paths.py: rename unused 'main' bindings to '_main' Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
mini-interact,livesqlbench-base-lite-sqlite,livesqlbench-base-lite,bird-interact-lite-exp). Old underscore tokens are rejected with a clear error.slayer_otf_cache/<benchmark>/andslayer_models_otf/<benchmark>/— a single env var per helper controls the parent dir; benchmark name is the subdir.gated_gold_root(*, benchmark)path helper replaces the removedBenchmark.gold_root_envfield.--framework claude_sdkis the only public option;_make_runnerdispatches to the right internal OTF agent based on(benchmark.one_shot, query_mode). Removed_validate_framework_dataset_mode,_validate_one_shot_framework,_FRAMEWORK_DATASET_MODE_BINDING.or "mini_interact"/or "livesqlbench"fallbacks removed — hardValueErrorraises instead.ray_app.py --datasetis now required.mini_interact_root(),mini_interact_data_file(),livesqlbench_root(),livesqlbench_data_file()deleted; callers updated tobenchmark_data_root("mini-interact")etc.scripts/migrate_benchmark_paths.pyrenames existing artifact dirs (OTF cache/models, audited gold, annotations) to match the new layout.Test plan
uv run python -c "from bird_interact_agents import paths; print(paths.slayer_otf_cache_root(benchmark='mini-interact'))"→…/slayer_otf_cache/mini-interactuv run python -c "from bird_interact_agents.benchmark import get_benchmark; print(get_benchmark('mini-interact').name)"→mini-interactget_benchmark('mini_interact')→ValueError: unknown benchmark token🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Chores
User-facing changes
Cloud