Complete the run-artefact logging (close 8 gaps, fix 2 latent bugs) - #40
Merged
Conversation
…annotation The annotation file (DEV-1515) is the authoritative grading source — gold_variants, evaluator_prompt, masked_terms, original_gold_is_correct all live there. Audited-gold is derivative: a task only has an audited row when its annotation says the original gold is wrong. The old audited-gold guard therefore mis-fires on the (now common) case where the annotation says the original IS correct (48/76 of last week's mini-interact retry batch), forcing operators to pass --no-require-audited-gold just to proceed. Flip the gate to annotation-existence and decouple from --use-audited-gold-sql — the annotation is needed for grading regardless of whether the overlay is applied. Drops the now-obsolete _audited_gold_check module + tests; the harness-side overlay still pins the primary-first contract for multi-variant audited rows. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…y fall-back When the operator passes neither --subscription-auth nor --no-subscription-auth (previously the default), the submit/annotate parser now rejects. When --subscription-auth IS chosen but CLAUDE_CODE_OAUTH_TOKEN is absent or doesn't have the sk-ant-oat01- prefix, parse-time fail too — the OAuth path no longer silently falls through to the API-key path. Failure mode this closes: the OAuth token lives in an env file (.env.ubuntu here) that interactive login shells source but non-interactive bird-interact-cloud invocations don't. The pre-fix code saw no token, silently picked the API-key path, and the 76-task mini-interact retry batch burned through the API account's credit balance partway, leaving 20 tasks with billing_error: "Credit balance is too low" instead of a real verdict. Forcing an explicit choice + a token check at parse + mirror checks in driver/prereqs (defence-in-depth for resubmit, which bypasses the CLI) all stop that class of failure cold. Internal `no_subscription_auth` field name preserved for back-compat with the manifest schema and downstream callers (driver, prereqs, ray_app); the CLI just mirrors the negation into the same attribute. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reading 53 agent_miss trajectories from this week's mini-interact retry batches revealed 17/53 (32%) match a single signature the existing classifier didn't have a label for: SLayer's translator always wraps a dimension-only SELECT in a top-level GROUP BY over every projected column, with no aggregates. When the gold query expects raw rows (one per source record) this silently dedups — the agent's predicted row count comes in well below gold's, and the rowset is a strict subset. Symptoms in `miss_diagnostics`: `agent_has_group_by=True` while `best_variant_has_group_by=False`, `rowset_relation_to_best=disjoint` or `subset_of`. The classifier was previously mis-labelling these as `user_sim_misleading` / `wrong_join_path` / `exhausted_budget_guessing` because none of its existing categories fit and the LLM picked the nearest-neighbour label. Adds the new pattern to both flavor taxonomies and prompts; the remediation hint steers the agent toward `mcp__slayer__query_nested` (or the dedup-off path) when raw rows are wanted. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…fields
Two follow-ups to the DEV-1535 subscription-auth refactor:
1. The autopsy stage built `anthropic.AsyncAnthropic()` with no args,
so the SDK walked `ANTHROPIC_API_KEY` → `ANTHROPIC_AUTH_TOKEN`. On
OAuth workers `ANTHROPIC_API_KEY` is deliberately removed (see
`ray_app._apply_actor_env_local`), so the autopsy crashed with
`TypeError("Could not resolve authentication method...")` — 18/28
missing autopsies in the last mini-interact run. The 3 residual
`BadRequestError: "Credit balance is too low"` are the same class:
autopsy fell through to a still-set API key that had no credit.
`_build_anthropic_client()` resolves auth in order: OAuth subscription
(`auth_token=...` → Bearer), API-key fallback, else a clear
`RuntimeError` so the outer `except Exception` records a meaningful
FQN instead of the SDK's cryptic `TypeError`.
2. The a-interact tool schema marks `key_asks`, `disclosed_resolutions`,
`undisclosed_resolutions` as required, BUT the prompt was silent on
them — the LLM omitted them, pydantic rejected the call, and 7/28
autopsies died with `Field required`. Align UP (not relax DOWN, per
PR feedback): add an explicit instructional block to the a-interact
prompt naming the three fields with `[]`-when-empty guidance.
One-shot stays unchanged (the four ask-shaped fields are not in its
schema; naming them even to say "skip" would invite latching).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5/53 misses in the latest mini-interact retry batches were the
`output_schema_misread` pattern — agent computed the right values but
the column ORDER / column COUNT / value TYPE didn't match gold. The
`_COLUMN_NAMES_DONT_AFFECT_GRADING` block already told the agent that
names don't matter; it was silent on what TO do when stuck.
Adds two paragraphs (shared by all 4 OTF flavors, since the pattern
hits both one-shot livesqlbench and a-interact mini-interact):
1. Re-read the question before submit for explicit ordering language
("list X, then Y"), bare-quoted IDs (string vs int), and boolean
predicates (raw BOOLEAN vs CAST INT).
2. When the grader returns an opaque "ex_base returned 0 but
expected 1" and the values look right: try shape permutations
FIRST (column order, bare types, column-count drop/add) BEFORE
adding logic, changing formulas, or asking the user.
Snapshot hashes for SLAYER_OTF_ONE_SHOT / SLAYER_OTF_AINTERACT
rebaselined; no prompt-content tests added (per
feedback_no_prompt_content_tests in memory).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… cap Sweep of 76 mini-interact retries showed every `correct` verdict landed under 891s / 175 trajectory steps and every `valid_interpretation` under 659s / 202 steps; 27/53 `agent_miss` runs burned past 900s thrashing on a wrong baseline they couldn't recover from. The previous 500-step patience was never close to binding (max overall was 295) — historical paranoia from DEV-1478. Drop to 250 and add a real wall-clock cap. Implementation: `asyncio.wait_for` in `run_one_task_with_runner`, gated by `BIRD_INTERACT_PER_TASK_TIMEOUT_S` (default 900, set 0 to disable). On timeout, raise `TimeoutError` with a clear message so the per-task error row records what actually happened rather than the empty `asyncio.TimeoutError`. Same try/except path → no other plumbing changes; the cap is a hook the existing error machinery catches. Expected impact on the studied retry batch: 0% false-negative on correct/valid, kills 27 (51%) of agent_miss runs early, ~30% sum wallclock reduction. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The most-recent-attempt sweep over runs/mini-interact/ (671 attempts) showed n_agent_turns=None on every claude_sdk* row — only the pydantic_ai* adapters set the field explicitly. The CascadingReport column, the post-hoc budget-sizing analyses, and the cloud cascading metrics all had to fall back on len(trajectory) as a turn-count proxy. Backfill in `finalize_result_row` (the single point every adapter funnels through) by counting AssistantMessage entries in the trajectory when the adapter didn't set the field explicitly. Pre-set values are preserved so the pydantic_ai* path that computes its own count from `run.all_messages()` still wins. Defensive against the claude_sdk fallback that stuffs `str(msg)` into the trajectory when dataclasses.asdict raises — non-dict entries are ignored. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two latent bugs since DEV-1515 left `submission.cost_usd_agent` and
`submission.cost_usd_user_sim` at None on every cloud and local
annotation:
- The cloud actor (`ray_app._grade_one_submission`) read the costs
under the WRONG key names — `cost_usd_agent` / `cost_usd_user_sim`
— while `TokenUsage.model_dump()` emits `agent_cost_usd` /
`user_sim_cost_usd`. The keys never resolved, so the field passed
to the annotation writer was always None.
- The local writer (`run._grade_local_row`) never extracted the cost
fields at all — only `n_agent_turns` / `n_ask_user_calls`.
Adds the shared `extract_usage_costs(usage_blob) -> (agent, sim)`
helper in `eval/grade_in_place.py` next to `decode_result_json`. Both
the cloud and local writer route through it so the wrong-key drift
can't recur. The cloud fall-through path (write_failed_submission_annotation
on grader-exception) also receives the costs now.
Also surface `usage.n_ask_user_calls` symmetrically across all four
`claude_sdk_otf*` flavors. Pre-fix only the a-interact variants
copied `ctx_dict.asks_used` into the result row's usage dict; one-shot
flavors silently dropped it. Writing 0 rather than absent keeps the
usage shape consistent (one-shot SLAYER excludes `ask_user` so the
value is always 0, but field-presence consistency lets downstream
joins / cost queries work uniformly).
`finalize_result_row` gains two more chokepoint backfills:
- `usage.n_ask_user_calls` defaults to 0 if absent (defensive
backstop for adapters that forget the convention).
- `predicted_row_count` is backfilled from `predicted_result_json`'s
`row_count` field when the adapter hasn't set it (handles both raw
dict and JSON-encoded shapes; no-op on malformed snapshots or
error-snapshots without `row_count`).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…runs Two adapter-side signals the claude_sdk family was throwing away: 1. tool_call_stats — per-tool n_calls / n_errors / error_samples in the SAME shape as `_run_capture._extract_tool_stats` (the pydantic_ai sibling). The new `extract_tool_stats_from_claude_sdk_trajectory` walker reads AssistantMessage.tool_use blocks (counts calls + builds the tool_use_id → name map) then UserMessage.tool_result blocks (counts errors; resolves names via the map). Cap at 10 samples / 400 chars each, matching the sibling. 2. ResultMessage metadata — num_turns, stop_reason, duration_ms, duration_api_ms from the SDK's final stream message. Pre-fix `accumulate_assistant_usage` consumed these messages for token counting and dropped the lifecycle fields. `stop_reason` is the canonical "why did the agent stop" signal (turn budget vs end_turn vs error) and was previously invisible. Folded under `usage.sdk_*` rather than a new top-level dict so results.db's `usage_json` carries them for free — no _persist or downstream parser changes. Both backfills land in `finalize_result_row` so zero per-adapter diff is needed; the shape discriminator (`_looks_like_claude_sdk_trajectory`) keeps pydantic_ai trajectories on their existing path. Adapter-explicit values (rare — multi-phase runs that pre-compute these) are preserved via `setdefault` / `is None` guards. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…adata
Cost-by-mode / failure-mode-by-effort analyses previously had to parse
the cloud run-id substring (`-slayer-` vs `-raw-`) to recover the
framework / mode / query_mode of a row. The config was in the manifest
but never landed on the per-task artefact or the results.db sink.
Closes that gap two ways at once (per design choice — user explicitly
asked for both):
- `SubmissionMetadata.config: Optional[SubmissionConfig]` — a new
pydantic class carrying framework, mode, query_mode, agent_model,
user_sim_model, slayer_setup, reasoning_effort, patience,
max_depth, dataset, strict, use_audited_gold_sql, prompt_cache.
Threaded through `_build_submission_annotation`,
`_write_harness_confirmed_annotation`,
`write_failed_submission_annotation`, `grade_and_write`, and
`grade_one_submission` (all Optional with None defaults — older
callers / pre-DEV-1535 annotations parse cleanly).
- `results_db.run_metadata` table extended with 9 new nullable
columns (additive ALTER via the same `_DIAGNOSTIC_COLUMNS`
pattern used for task_results). Existing result DBs gain the
columns on next `open_db`. `insert_run_metadata` accepts the new
fields keyword-only with None defaults so legacy callers
(existing tests, older invocations) work unchanged. Bool fields
coerced via int() — SQLite has no native bool — with None
preserved so "never set" stays distinguishable from "explicitly
False".
Both the local runner (`run.py`) and the cloud actor / collator
(`ray_app.py`, `collation.py`) build the same SubmissionConfig +
extended run_metadata block from the same source — args namespace
for local, manifest fields for cloud. Same shape, same downstream
joins.
After this lands the cost-by-mode summary becomes a single SQL JOIN
on (run_id, framework, mode) — no more substring parsing — and
annotations are individually self-describing for grep / ad-hoc
inspection.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces audited-gold submit checks with annotation-file guards, enforces Claude OAuth prereqs, extracts tool/result telemetry from Claude SDK trajectories into finalize_result_row, threads SubmissionConfig and canonical usage-costs through grading and persistence, adds per-task timeouts, updates autopsy, and expands tests. ChangesCore Infrastructure: Trajectory extraction and harness backfills
Agents & prompts
Submission validation, CLI, driver, and prereqs
Configuration snapshots, costs, and persistence
Advanced features and autopsy
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/test_autopsy.py (1)
1304-1310: ⚡ Quick winUnusual default parameter pattern may cause confusion.
The function signature captures
_ONE_SHOT_PATTERNSas a default parameter value, shadowing the module-level variable. This freezes the list at function definition time rather than reading it at test execution time.If this is intentional (e.g., to guard against runtime mutations of the module variable), consider adding a comment explaining why. Otherwise, remove the parameter and reference the module variable directly in the test body.
♻️ Proposed simplification
-def test_tool_schema_one_shot_pattern_enum_matches(_ONE_SHOT_PATTERNS=_ONE_SHOT_PATTERNS): +def test_tool_schema_one_shot_pattern_enum_matches(): from bird_interact_agents.eval.autopsy import _AUTOPSY_TOOL_SCHEMA_ONE_SHOT pattern_enum = set( _AUTOPSY_TOOL_SCHEMA_ONE_SHOT["input_schema"]["properties"]["pattern"]["enum"] ) assert pattern_enum == set(_ONE_SHOT_PATTERNS)🤖 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_autopsy.py` around lines 1304 - 1310, The test function test_tool_schema_one_shot_pattern_enum_matches currently uses a default parameter `_ONE_SHOT_PATTERNS=_ONE_SHOT_PATTERNS` which shadows and freezes the module-level `_ONE_SHOT_PATTERNS`; remove the default parameter so the test body references the module-level `_ONE_SHOT_PATTERNS` directly (or if freezing was intentional, add an explicit comment above the function explaining why the default is required to guard against runtime mutations). Ensure the assertion still builds `pattern_enum` from `_AUTOPSY_TOOL_SCHEMA_ONE_SHOT["input_schema"]["properties"]["pattern"]["enum"]` and compares it to the module `_ONE_SHOT_PATTERNS`.
🤖 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/_annotation_check.py`:
- Around line 90-91: The current check uses path.exists() which returns true for
directories and can let malformed checkouts pass; change the guard to require a
real file by replacing the exists() check with path.is_file() for the
constructed path variable (the one built from ann_root / bench.name / db /
f"{iid}.task.json") so the code enforces the docstring/CLI contract and fails
early if the annotation file is missing or not a regular file.
In `@src/bird_interact_agents/cloud/driver.py`:
- Around line 166-167: The branch incorrectly calls
prereqs._is_claude_sdk_framework(...) which can be fooled if the prereqs module
is monkeypatched; instead import the predicate function by name (e.g.
_is_claude_sdk_framework) and call it directly, then keep the existing behavior
of checking not no_subscription_auth and reading CLAUDE_CODE_OAUTH_TOKEN into
token; update the top-level imports to import _is_claude_sdk_framework from the
prereqs module and replace prereqs._is_claude_sdk_framework(framework) with a
direct call to _is_claude_sdk_framework(framework).
In `@src/bird_interact_agents/harness.py`:
- Around line 1103-1105: The telemetry backfill only runs when row.get("usage")
is a dict, so create/initialize row["usage"] to an empty dict before applying
defaults: ensure the code in harness.py that sets _usage = row.get("usage")
instead sets row["usage"] = row.get("usage") or {} (and then uses _usage =
row["usage"]) so n_ask_user_calls is added for rows that previously had no
usage; apply the same initialization for the other backfill block referenced
(the one around lines 1147-1155) so sdk_* fields and other defaults are always
populated even when usage is missing.
---
Nitpick comments:
In `@tests/test_autopsy.py`:
- Around line 1304-1310: The test function
test_tool_schema_one_shot_pattern_enum_matches currently uses a default
parameter `_ONE_SHOT_PATTERNS=_ONE_SHOT_PATTERNS` which shadows and freezes the
module-level `_ONE_SHOT_PATTERNS`; remove the default parameter so the test body
references the module-level `_ONE_SHOT_PATTERNS` directly (or if freezing was
intentional, add an explicit comment above the function explaining why the
default is required to guard against runtime mutations). Ensure the assertion
still builds `pattern_enum` from
`_AUTOPSY_TOOL_SCHEMA_ONE_SHOT["input_schema"]["properties"]["pattern"]["enum"]`
and compares it to the module `_ONE_SHOT_PATTERNS`.
🪄 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: 9fecc227-552e-4ddc-8506-90b7890af599
📒 Files selected for processing (32)
src/bird_interact_agents/agents/_run_capture.pysrc/bird_interact_agents/agents/_shared_otf_prompts.pysrc/bird_interact_agents/agents/claude_sdk_otf/agent.pysrc/bird_interact_agents/agents/claude_sdk_otf_raw/agent.pysrc/bird_interact_agents/cloud/_annotation_check.pysrc/bird_interact_agents/cloud/_audited_gold_check.pysrc/bird_interact_agents/cloud/cli.pysrc/bird_interact_agents/cloud/collation.pysrc/bird_interact_agents/cloud/driver.pysrc/bird_interact_agents/cloud/prereqs.pysrc/bird_interact_agents/cloud/ray_app.pysrc/bird_interact_agents/eval/annotation_schema.pysrc/bird_interact_agents/eval/autopsy.pysrc/bird_interact_agents/eval/grade_in_place.pysrc/bird_interact_agents/harness.pysrc/bird_interact_agents/results_db.pysrc/bird_interact_agents/run.pytests/cloud/test_annotation_check.pytests/cloud/test_audited_gold_check.pytests/cloud/test_cli.pytests/cloud/test_driver.pytests/cloud/test_prereqs.pytests/cloud/test_run_one_task.pytests/eval/test_extract_usage_costs.pytests/test_autopsy.pytests/test_dual_eval.pytests/test_extract_claude_sdk_result_metadata.pytests/test_extract_tool_stats_claude_sdk.pytests/test_finalize_result_row.pytests/test_results_db.pytests/test_shared_otf_prompts.pytests/test_submission_config.py
💤 Files with no reviewable changes (2)
- src/bird_interact_agents/cloud/_audited_gold_check.py
- tests/cloud/test_audited_gold_check.py
…dk paths
Three review findings, all in the same area, all small but compound:
1. (Codex) `harness.finalize_result_row` backfilled `n_agent_turns`
only to the top-level `row["n_agent_turns"]`. Every annotation
writer (local `run.py::_grade_local_row`, cloud
`ray_app._grade_one_submission`) reads
`usage_blob.get("n_agent_turns")`, so the backfill was dead for
the exact runs that needed it. Mirror the backfilled value into
`usage["n_agent_turns"]` so writers actually see it.
2. (CodeRabbit, major) The `usage`-side backfills (`n_ask_user_calls`,
`sdk_*`) only fired when `row["usage"]` was already a dict. The
early-error claude_sdk paths
(`agents/claude_sdk_otf/agent.py:393-406`, the analogous early
returns in claude_sdk_otf_raw) build rows with NO `usage` field
at all. Hoist `row["usage"] = {}` initialisation to the top of
`finalize_result_row` so every backfill below has a target.
3. (Codex) `_run_capture._looks_like_claude_sdk_trajectory` only
checked `"data" in item`. The `claude_sdk_*_raw` adapters
serialize each trajectory item as
`{"type": ..., "data": str(msg)[:500]}` — `data` is a string,
not a dict. Pre-fix the discriminator accepted these and the
walker returned an empty stats dict, falsely indicating
"0 tool calls / 0 errors" instead of "not extractable". Tighten
to require `isinstance(item["data"], dict)`.
Tests extended to cover all three:
- `test_initialises_usage_when_row_has_no_usage_field` +
`test_replaces_non_dict_usage_with_initialised_dict` (replace
the prior tests that asserted skip-when-missing — opposite of
the new intent).
- `test_sdk_metadata_lands_even_when_starting_usage_was_not_a_dict`
(the early-error claude_sdk paths are now analyzable).
- `test_n_agent_turns_backfill_mirrors_into_usage` +
`test_n_agent_turns_mirror_does_not_clobber_explicit_usage_value`.
- `test_returns_none_on_claude_sdk_raw_string_data_shape` +
`test_returns_none_when_any_item_has_non_dict_data` for both
walkers (tool stats + result metadata).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-arg trick) Three review nits, no behaviour change in production code paths but each shores up an edge case: 1. (CodeRabbit) `cloud/_annotation_check.py` used `path.exists()`, which returns True for directories too. A malformed checkout where the annotation slot was accidentally created as a directory would silently pass the submit-time guard and then fail later when the harness tried to read the file. `is_file()` matches the docstring / CLI help "file" contract. 2. (CodeRabbit) `cloud/driver.py` called the predicate via `prereqs._is_claude_sdk_framework(framework)` (module-attribute access). Some tests monkeypatch `driver.prereqs` to a generic MagicMock; the predicate then becomes a truthy mock and the OAuth path fires for every framework, masking real test failures. The current driver test fixture works around this with a `side_effect` wire-through, but a future test could trip on it. Importing `_is_claude_sdk_framework` by name removes the indirection. 3. (CodeRabbit nit) `tests/test_autopsy.py::test_tool_schema_one_shot_pattern_enum_matches` had an unusual `_ONE_SHOT_PATTERNS=_ONE_SHOT_PATTERNS` default parameter trick. Dropped — the module-level variable is referenced directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…predicted_row_count
Two Codex findings on the post-Group-1/2 push:
1. (Codex, minor) Cloud actor at `ray_app.py:822` hardcoded
`predicted_row_count=None`, ignoring the new backfill in
`finalize_result_row`. Cloud-side annotations lost the row-count
evidence the new `slayer_overaggregation` autopsy pattern relies
on. Forward `row.get("predicted_row_count")` instead.
2. (Codex, major) The `--require-annotation` swap (commit `da857dd`,
per user request) dropped the audited-gold presence check
entirely. When an annotation has `original_gold_is_correct=False`
AND `--use-audited-gold-sql` is on AND the audited_gold sidecar
is missing a row for that iid, `load_audited_gold_rows_for`
returns [] and the tolerant grader silently falls back to the
(annotated-as-wrong) original gold. The primary annotation guard
doesn't catch this — both annotation AND audited_gold files
exist, but they're out of sync.
Layered guard `annotations_requiring_audited_gold_without_rows`:
only fires when both `--require-annotation` and
`--use-audited-gold-sql` are on (matches the intent of the user's
original gate-swap — annotation is still the primary gate; this
plugs the sync gap without reintroducing the audited-gold
requirement for the 48 `original_gold_is_correct=True` instances
that legitimately have no audited row). Bypassable via the same
`--no-require-annotation` opt-out.
Includes 4 new tests covering: original-correct (no check), wrong
gold + missing audited row (caught), wrong gold + matching row
(passes), id-without-annotation (skipped — primary guard's job).
Also tightens the existing annotation guard's `path.exists()` →
`path.is_file()` regression test added so directories at the
annotation path can't bypass either check.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/_annotation_check.py`:
- Around line 154-156: Currently load_audited_gold_rows_for(...) is being called
inside the instance_ids loop which reparses the audited-gold file for every iid;
instead, call load_audited_gold_rows_for once before iterating instance_ids to
build a presence index (e.g., a set of instance ids or a dict keyed by instance
id) and then consult that in the loop; update the code in
src/bird_interact_agents/cloud/_annotation_check.py to call
load_audited_gold_rows_for(benchmark=bench.name) once, create a fast lookup like
audited_ids = {row['instance_id'] for row in rows} (or keep rows indexed by
instance_id), and replace the per-iteration load_audited_gold_rows_for(...) call
with a membership/lookup check against audited_ids using the iid variable.
🪄 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: 5b8172d7-e4bb-4032-8587-aeffe3956b26
📒 Files selected for processing (4)
src/bird_interact_agents/cloud/_annotation_check.pysrc/bird_interact_agents/cloud/cli.pysrc/bird_interact_agents/cloud/ray_app.pytests/cloud/test_annotation_check.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/bird_interact_agents/cloud/cli.py
Four findings from Codex + CodeRabbit on the round-2 push:
1. (Codex, major) The DEV-1535 per-task wall-clock cap was wrapped
ONLY around `run_one_task_with_runner` (the `cached_runner` path
used for raw query_mode). SLayer / non-raw runs fall through to
`run_one_task` — pre-fix that path ran uncapped, defeating the
cap on exactly the runs most likely to thrash. Mirror the
`asyncio.wait_for` wrapper into `run_one_task` so both entry
points respect `BIRD_INTERACT_PER_TASK_TIMEOUT_S`.
2. (Codex, minor) `eval/annotate.py` and `eval/regrade.py` still
read costs under the WRONG key names (`cost_usd_agent` /
`cost_usd_user_sim`) — the same bug we fixed for the cloud +
local grader-write paths, repeated in the regenerate-annotation
and regrade CLIs. Route both through `extract_usage_costs` so
the canonical TokenUsage keys (`agent_cost_usd` /
`user_sim_cost_usd`) resolve. Fixture in
`tests/test_eval_annotate_cli.py` updated to the canonical
shape; the prior test was asserting the (broken) old behaviour.
3. (Codex, major) The layered audited-gold guard silently swallowed
parse / schema errors on annotation files. The submit guard
exists to surface these BEFORE cluster startup; swallowing them
recreates the exact delayed-failure mode the guard prevents.
Malformed annotations are now REPORTED in the missing-list.
4. (CodeRabbit, major perf) The layered guard re-scanned the full
audited-gold consolidated JSONL once per iid — O(N_ids ×
file_size). For a 76-iid retry batch that's 76 full passes.
New `_build_audited_gold_presence_index` reads it once into a
set; two-pass loop avoids building the index at all when no
annotation actually requires it (`original_gold_is_correct=True`
everywhere). Spy-based tests pin the call counts at 1 (with
requirement) and 0 (without).
Existing CLI test
(`test_require_annotation_passes_for_livesqlbench_when_file_present`)
updated to pass `--no-use-audited-gold-sql` so it tests the primary
guard in isolation — the layered guard now correctly rejects its
bare `{}` stub as malformed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_eval_annotate_cli.py (1)
173-174:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUpdate test fixture to use canonical usage-cost key names.
The inline fixture still uses the old wrong key names (
cost_usd_agent,cost_usd_user_sim), while line 51's_write_attempt_jsonwas correctly updated to the canonical names (agent_cost_usd,user_sim_cost_usd). Whengenerate_submission_annotationcallsextract_usage_costs(usage), it will not find these old keys and will return(None, None), so this test is inadvertently validating the None-cost fallback path instead of actual cost extraction.🔧 Proposed fix to align with canonical key names
- "usage": {"cost_usd_agent": 0.01, "cost_usd_user_sim": 0.0, + "usage": {"agent_cost_usd": 0.01, "user_sim_cost_usd": 0.0, "n_agent_turns": 1, "n_ask_user_calls": 0},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_eval_annotate_cli.py` around lines 173 - 174, The test fixture in tests/test_eval_annotate_cli.py is using old key names ("cost_usd_agent", "cost_usd_user_sim") so extract_usage_costs returns (None, None); update the inline usage dict to use the canonical keys ("agent_cost_usd" and "user_sim_cost_usd") to match the behavior of _write_attempt_json and ensure generate_submission_annotation / extract_usage_costs read real values; locate the usage dict in the failing test and rename those two keys accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/test_eval_annotate_cli.py`:
- Around line 173-174: The test fixture in tests/test_eval_annotate_cli.py is
using old key names ("cost_usd_agent", "cost_usd_user_sim") so
extract_usage_costs returns (None, None); update the inline usage dict to use
the canonical keys ("agent_cost_usd" and "user_sim_cost_usd") to match the
behavior of _write_attempt_json and ensure generate_submission_annotation /
extract_usage_costs read real values; locate the usage dict in the failing test
and rename those two keys accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b625031f-bc31-420d-ad26-126fccc03695
📒 Files selected for processing (8)
src/bird_interact_agents/cloud/_annotation_check.pysrc/bird_interact_agents/eval/annotate.pysrc/bird_interact_agents/eval/regrade.pysrc/bird_interact_agents/run.pytests/cloud/test_annotation_check.pytests/cloud/test_cli.pytests/cloud/test_run_one_task.pytests/test_eval_annotate_cli.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/cloud/test_cli.py
…submit auth
Two Codex findings + one missed CodeRabbit fixture from round 3:
1. (Codex, major) Resubmitting pre-DEV-1535 cloud runs hard-failed
when CLAUDE_CODE_OAUTH_TOKEN wasn't set in the env. Legacy
manifests don't carry the `no_subscription_auth` field at all,
so `manifest.get("no_subscription_auth", False)` defaulted to
"subscription required" and the strict guard in
`read_api_keys_from_local_env` raised. Pre-DEV-1535 those runs
silently used the API-key path. Flip the resubmit default to
`True` (legacy API key) when the field is ABSENT, and log a note
explaining why. New manifests carry the field explicitly so the
strict-submit contract for fresh runs is unchanged.
2. (Codex, major) The primary submit guard
(`missing_annotation_ids`) only checked `path.is_file()` —
schema validation only fired inside the layered audited-gold
check, which is gated on `--use-audited-gold-sql`. With
`--no-use-audited-gold-sql` and default `--require-annotation`,
a malformed `.task.json` passed the submit and failed mid-run
when `read_task_annotation` raised. Schema-validate in the
primary guard too — a present-but-malformed file is just as
bad as a missing one for the submit-time-fail-fast contract.
3. (CodeRabbit, missed in round 3) `tests/test_eval_annotate_cli.py`
had a SECOND inline `usage` dict with the legacy wrong-key
names (`cost_usd_agent` / `cost_usd_user_sim`). My round-3
string-replace only caught the first one because the values
differed. Updated to the canonical TokenUsage keys.
Internal test helper `_write_annotation` and the
`test_require_annotation_passes_for_livesqlbench_when_file_present`
fixture write proper TaskAnnotation pydantic models now — the
bare `{}` stub was exploiting the gap Codex flagged.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…loop
Two valid Codex findings (the third — `annotate --subscription-auth`
not using OAuth — is incorrect: `_is_claude_sdk_framework("annotator")`
returns True, so the OAuth path does apply to annotator):
1. (major) The DEV-1535-r3 audited-gold presence index only checked
`instance_id in set` — way too lax. A sidecar row with just
`{"instance_id": "X"}` and no `audit_status` or `audited_sol_sql`
would have passed the layered guard but failed the runtime
overlay (which the original `cloud._audited_gold_check` module —
replaced in this PR — used to validate via the same rules).
Harden the index to enforce the same contract
`apply_audited_gold_overlay` does at runtime:
- `audit_status` ∈ {clean, original} passes regardless of
`audited_sol_sql` (original IS the audited gold).
- `audit_status` ∈ {edited, unrecoverable} requires
non-empty `audited_sol_sql` list (the overlay's
`isinstance(audited, list) and bool(audited)` check).
- Cross-benchmark collision guard: row's `benchmark` field,
if present, must match the benchmark we're checking (DB
names overlap across benchmarks by design).
- Cross-database collision guard: row's `selected_database`,
if present, must match the dataset map's mapping for the
id (defence-in-depth).
- Grouped format: validate the primary variant; fall back
to the first variant if no primary is marked.
4 new tests pin the validation rules (missing-sql, empty-sql,
cross-benchmark, clean-row-passes-empty-sql).
2. (major) The per-task wall-clock cap (`BIRD_INTERACT_PER_TASK_TIMEOUT_S`)
was wrapped in `run_one_task_with_runner` (cloud actor) and
`run_one_task` (round-3 fix), but the LOCAL `run_evaluation`
loop awaited `runner(...)` directly — so the `bird-interact` CLI
path ran uncapped. Mirror the `asyncio.wait_for` wrapper into
`run_evaluation._run_with_sem` so the cap binds at every entry
point now.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…base fields Round 5 hardened the audited-gold presence index to reject malformed rows (missing audit_status, empty audited_sol_sql), but the cross-benchmark / cross-database checks were "reject when present and wrong" — absent/null fields passed through. The runtime overlay in `harness.apply_audited_gold_overlay` treats absent `benchmark`/`selected_database` as missing-row (silent fallback to original gold). The submit-time index must mirror that contract or the layered guard reopens the silent-fallback gap. Switch to "require present and matching": reject rows whose `benchmark` is not a string equal to the benchmark we're checking, and reject rows whose `selected_database` is not a non-empty string. 2 new regression tests (missing `benchmark` field, missing `selected_database` field). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Jun 22, 2026
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
Audit-driven sweep that closes every gap between what the agents collect during a run and what reaches the on-disk artefacts under
runs/<benchmark>/<db>/<iid>/<run-id>.{json,trajectory.json}+results.db. Two of the gaps were silent bugs that have been writing `None` since DEV-1515.Three commits, each independently testable:
`91999f2` — writer-side plumbing + fix two latent cost-key bugs
`bec60a3` — capture `tool_call_stats` + `ResultMessage` metadata on claude_sdk runs
`3c7c180` — snapshot agent config into every annotation + `run_metadata`
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores