Skip to content

DEV-1605: version OTF references by encoder model + stamp consumed version - #61

Open
ZmeiGorynych wants to merge 6 commits into
egor/dev-1589-claude_sdk-otf-reference-encoder-build-pre-encodedfrom
egor/dev-1605-version-otf-references-by-encoder-model-stamp-consumed
Open

DEV-1605: version OTF references by encoder model + stamp consumed version#61
ZmeiGorynych wants to merge 6 commits into
egor/dev-1589-claude_sdk-otf-reference-encoder-build-pre-encodedfrom
egor/dev-1605-version-otf-references-by-encoder-model-stamp-consumed

Conversation

@ZmeiGorynych

@ZmeiGorynych ZmeiGorynych commented Jun 25, 2026

Copy link
Copy Markdown
Member

Sequel to DEV-1589 (#57). Based on the DEV-1589 branch so the diff is DEV-1605-only; retarget to main once #57 merges.

Problem

One slot per (benchmark, db) — re-running the encoder with a different LLM overwrites the prior reference, and a consumer run isn't self-describing about which encoded version it used.

What this does

1. Versioned layout slayer_models_otf/<benchmark>/<version>/<db>/

  • paths.slayer_models_otf_root(benchmark, version=None) — optional version segment above <db> + a label validator. version=None returns the legacy parent, so existing root / db_name call sites are unchanged.
  • model_string.encoder_version_slug — default label from the encoder model (anthropic/claude-opus-4-7→opus-4-7, zai/glm-5.2→glm-5.2, slashes→__).
  • scripts/build_otf_references.py gains --version (default = slug); per-version --force slot.

2. _encoder_meta.json provenance

Rich EncoderMeta (model / framework / version / benchmark / db / fp / built_at / settings) written next to _reference_fp.txt, before the marker. Optional — legacy callers unaffected.

3. Consumer version resolution (legacy flat fallback abolished)

resolve_otf_version: single→use, explicit→must exist, 2+→error, 0→error. Threaded through all 4 claude_sdk_otf* consumers, run.py (--pre-encoded-version), and the cloud.

4. Consumed-version stamp

ConsumedReference on each per-task SubmissionAnnotation (additive optional field — no schema_version bump, old annotations still validate) + a per-db consumed_references list in the run manifest (local + cloud).

5. Full cloud versioning

Submit/resubmit forward --pre-encoded-version/--version; the concrete version is resolved at submit so upload / download / merge agree; actor download, upload-back, and post-run merge are version-aware. GCS prefixes stay <db>-keyed (runs/<run_id>/ already isolates versions across runs).

6. Migration

scripts/migrate_otf_references.py (idempotent, dry-run) relocates legacy flat refs; version derived from _setup_usage.json; unknown + warning when underivable; backfills _encoder_meta.json.

Deviations from the issue (signed off)

  • Layout is <version>/<db> (version above db) — cleaner plumbing than the issue's literal <db>/<version>.
  • Legacy flat fallback abolished (issue assumed it stays) → migration script brought in-scope.

Tests

Mechanical contracts only (no prompt-content tests). 8 new DEV-1605 test files + a cloud test. Full non-integration suite green (3516 passed). Codex reviewed both the plan and the tests-vs-plan; all findings folded in (incl. keeping schema_version=1).

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Added DEV-1605 version-pinned OTF pre-encoded references, including version-scoped storage paths and captured encoder provenance.
    • Introduced --pre-encoded-version and --version options for consistent OTF version labeling across local and cloud runs.
    • Added a one-shot migration to move legacy flat OTF reference layouts into the new versioned directory structure.
    • Result submission annotations now include optional consumed-reference details.
  • Bug Fixes
    • Ensured consumed-reference provenance is written for both success and failure flows, and that cloud/local fetch and warm-cache merge target the correct versioned roots.

…rsion

Adds an encoder-model version axis to the on-the-fly SLayer reference
layout (`slayer_models_otf/<benchmark>/<version>/<db>/`) and records which
encoded version a consumer run used, end to end (local + cloud).

- paths.slayer_models_otf_root gains an optional `version=` segment (above
  <db>) + a label validator; version=None is the legacy parent so existing
  `root / db_name` call sites are unchanged.
- model_string.encoder_version_slug derives the default version label
  (strip provider + leading `claude-`, slashes -> __).
- reference_build writes a self-describing `_encoder_meta.json` (model /
  framework / version / fp / built_at / settings) next to the marker;
  the in-process build lock is keyed by the versioned target dir.
- The legacy flat fallback is ABOLISHED: the consumer resolves a concrete
  version (single -> use, explicit -> must exist, 2+ -> error). Threaded
  through the 4 claude_sdk_otf* agents, run.py (--pre-encoded-version), and
  the cloud (cli/driver/ray_app, concrete version resolved at submit so
  upload/download/merge agree; GCS prefixes stay <db>-keyed).
- The consumed reference is stamped onto each per-task SubmissionAnnotation
  (additive optional field, NO schema_version bump) and aggregated into a
  per-db `consumed_references` list in the run manifest.
- scripts/migrate_otf_references.py relocates legacy flat refs into the
  versioned layout (version derived from _setup_usage.json; `unknown` +
  warning when underivable; idempotent; dry-run).

Tests: mechanical contracts only (no prompt-content tests). Full
non-integration suite green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@linear

linear Bot commented Jun 25, 2026

Copy link
Copy Markdown
DEV-1605 Version OTF references by encoder model + stamp consumed version into run manifest

Version OTF references + record which version a run consumed

Problem

Today there is exactly one slot per (benchmark, db):
slayer_models_otf/<benchmark>/<db>/. Re-running the encoder with different
settings (e.g. a different LLM — opus vs zai/glm-5.2) overwrites the
previous reference; there is no way to keep multiple encoded versions
side-by-side.

Separately, a consumer run is not self-describing about which encoded
reference it used:

  • framework + query-mode are recoverable from the run-id slug;
    agent_model / pre_encoded_source / slayer_setup live in the cloud
    manifest (results/<benchmark>/cloud/<run_id>/manifest.json).
  • The encoder model that BUILT the reference is recorded only inside the
    reference dir (_setup_usage.jsonsetup_encoder::<model>), not in the
    consumer run.
  • The reference content fingerprint (_reference_fp.txt) is uploaded into
    the run's GCS seed prefix (runs/<run_id>/slayer_setup/slayer_models_otf/ <db>/_reference_fp.txt, ray_app.py:977) but is not copied into the
    local per-task runs/.../<iid>.json annotation, and the fingerprint hashes
    output content only — it does not encode who built it.

Net: you can't store two versions, and from the local runs/ artifact alone
you can't tell which encoder build a run consumed.

Proposed change

1. Version segment in the path (default = encoder model slug)

Add a version segment: slayer_models_otf/<benchmark>/<db>/<version>/.

  • <version> defaults to the encoder-model slug (e.g. opus-4-7,
    glm-5.2) derived from the model string the encoder ran with
    (anthropic/claude-opus-4-7opus-4-7, zai/glm-5.2glm-5.2).
    Allow an explicit --version <label> override on
    scripts/build_otf_references.py for the rare case of two builds with the
    same model but different settings.
  • Back-compat: treat a legacy un-versioned <db>/ dir as the implicit
    default version so existing references + cloud upload-back/merge keep
    working until migrated. Resolution prefers an explicit version, then falls
    back to the legacy flat layout (mirrors the DEV-1462 per-benchmark
    paths.slayer_models_otf_root precedent for additive path axes).
  • Plumb the version through paths.slayer_models_otf_root (new optional
    version= kwarg) and the consumer's --pre-encoded-models otf resolution
    (new --pre-encoded-version, default = latest / single).

2. _encoder_meta.json next to the output

The encoder writes {encoder_model, encoder_framework, settings, reference_fp, built_at} beside _reference_fp.txt so a reference is
self-describing about its provenance (today only the usage breakdown name
carries the model, incidentally).

3. Stamp consumed version into the consumer run

Copy {reference_fp, encoder_model, version} of the consumed reference into
the consumer run manifest (and ideally the per-task runs/ annotation) so
runs/ answers "which encoded version did this run use" without reaching
into GCS.

Out of scope

  • Migrating existing flat references to the versioned layout (the legacy
    fallback covers them; a one-shot migration script can be a follow-up).
  • Garbage-collecting old versions.

Tests

  • paths.slayer_models_otf_root(version=...) resolution incl. legacy
    fallback (pin in tests/test_paths.py).
  • Encoder writes _encoder_meta.json with the right model/fp.
  • Consumer manifest carries the consumed {reference_fp, encoder_model, version}.
  • Mechanical contracts only — no prompt-content tests.

Context

Sequel to DEV-1589 (claude_sdk OTF encoder) and DEV-1586 (--pre-encoded-models).

Review in Linear

@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@ZmeiGorynych, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 9 minutes and 3 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

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

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

🚦 How do rate limits work?

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

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

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7e8cbcbc-4247-4216-8b7a-1a34da395a17

📥 Commits

Reviewing files that changed from the base of the PR and between 52be82d and 470e66f.

📒 Files selected for processing (2)
  • src/bird_interact_agents/run.py
  • tests/test_dev1605_review_fixes.py
📝 Walkthrough

Walkthrough

Adds version-scoped OTF reference paths and metadata, migrates legacy flat references into versioned layouts, threads pre_encoded_version through runtime and cloud flows, and records consumed_reference in annotations.

Changes

DEV-1605 versioned OTF reference flow

Layer / File(s) Summary
Version labels and path roots
src/bird_interact_agents/model_string.py, src/bird_interact_agents/paths.py, scripts/build_otf_references.py, tests/test_dev1605_slug.py, tests/test_dev1605_paths_version.py, tests/test_dev1605_build_script.py, tests/test_dev1605_review_fixes.py
encoder_version_slug derives safe version labels, slayer_models_otf_root accepts an optional version segment, and the build script CLI/version parsing is covered by tests.
Versioned reference build and migration
src/bird_interact_agents/slayer_otf/encoder_types.py, src/bird_interact_agents/slayer_otf/reference_build.py, scripts/migrate_otf_references.py, tests/test_dev1605_encoder_meta.py, tests/test_dev1605_migration.py
Reference builds write _encoder_meta.json and use versioned roots and locks, and the migration script relocates flat references into versioned directories with backfilled metadata.
Pre-encoded OTF resolution
src/bird_interact_agents/agents/_pre_encoded.py, tests/test_dev1605_consumer_version.py, tests/test_dev1586_pre_encoded.py, tests/test_dev1605_review_fixes.py
Pre-encoded resolution selects an OTF version, returns PreEncodedResolution with consumed, and the tests cover version lookup, the updated return shape, and malformed metadata handling.
Runner version axis
src/bird_interact_agents/run.py, src/bird_interact_agents/agents/claude_sdk_otf/agent.py, src/bird_interact_agents/agents/claude_sdk_otf_v1/agent.py, src/bird_interact_agents/agents/claude_sdk_otf_ainteract/agent.py, src/bird_interact_agents/agents/claude_sdk_otf_ainteract_v1/agent.py, src/bird_interact_agents/agents/pydantic_ai_otf_encode/agent.py, tests/test_one_shot_run.py, tests/test_pydantic_ai_otf_encode_agent.py, tests/test_dev1605_review_fixes.py
pre_encoded_version and encode_version are threaded through runner factories, OTF agent constructors, and local evaluation/version checks.
Consumed-reference stamping
src/bird_interact_agents/eval/annotation_schema.py, src/bird_interact_agents/eval/grade_in_place.py, src/bird_interact_agents/harness.py, tests/test_dev1605_annotation_stamp.py, src/bird_interact_agents/run.py
consumed_reference is added to submission annotations, grading helpers, result rows, and local failure handling.
Cloud submit and fetch version axis
src/bird_interact_agents/cloud/cli.py, src/bird_interact_agents/cloud/driver.py, src/bird_interact_agents/cloud/ray_app.py, src/bird_interact_agents/cloud/upload_back.py, tests/cloud/test_dev1605_cloud_version.py, tests/cloud/test_driver.py, tests/cloud/test_ray_app.py, tests/cloud/test_upload_back.py, tests/test_cloud_paths_unchanged.py
Cloud submit, worker, fetch, warm-cache merge, and upload paths now carry pre_encoded_version and encode_version, and the tests cover the updated root resolution and job arguments.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • MotleyAI/bird-interact-agents#23: Shares the OTF reference-build path and slayer_models_otf_root usage that this PR extends with version-scoped layout and provenance metadata.
  • MotleyAI/bird-agents#9: This PR extends claude_sdk_otf agent flows with pre_encoded_version and consumed-reference stamping on top of the same agent family.
  • MotleyAI/bird-agents#55: Also changes the pre-encoded OTF reference flow and related agent/cloud plumbing.

Poem

I hopped through versioned paths tonight, 🐇
With slugged-up names and metadata in sight.
I left consumed crumbs at every gate,
So every run knows its proper state.
Hooray for burrows orderly and bright!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 42.47% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly captures the main change: versioning OTF references by encoder model and stamping the consumed version.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/bird_interact_agents/run.py (1)

323-357: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Reject pre_encoded_version unless the source is OTF.

pre_encoded_version is now exposed on the public APIs and CLI, but nothing validates that it is only used with pre_encoded_source="otf". In the supplied resolver, version is ignored for source="custom" and never consulted for on-the-fly runs, so a misconfigured call silently drops the version pin instead of failing fast.

Suggested guard
 def _validate_slayer_setup(
     *, slayer_setup: str, framework: str, query_mode: str, mode: str,
     pre_encoded_source: str | None = None,
+    pre_encoded_version: str | None = None,
 ) -> None:
+    if pre_encoded_version is not None and pre_encoded_source != "otf":
+        raise ValueError(
+            "--pre-encoded-version requires --pre-encoded-models otf; "
+            f"got {pre_encoded_source!r}."
+        )
+
     validate_pre_encoded_source(pre_encoded_source)

Then thread pre_encoded_version through the existing _validate_slayer_setup(...) call sites.

Also applies to: 1028-1074, 1172-1255, 1993-2004, 2088-2112

🤖 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/run.py` around lines 323 - 357,
`pre_encoded_version` is accepted by the public runner APIs but is not being
validated against `pre_encoded_source`, so misconfigured calls can silently
ignore the version pin. Add a fast-fail guard in the runner path around
`_make_runner`/`_validate_slayer_setup` to reject any non-OTF use of
`pre_encoded_version`, and thread that argument through the existing
`_validate_slayer_setup(...)` call sites so the same rule is enforced
consistently wherever the runner is built.
🧹 Nitpick comments (3)
tests/test_dev1605_migration.py (1)

57-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add regression cases for malformed usage JSON and dry-run collisions.

Please cover wrong-shaped _setup_usage.json and pre-existing destination paths so the migration fallback and dry-run contracts don’t regress.

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

329-345: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the encode-version fallback into one shared resolver.

This branch now duplicates the same encode_version or encoder_version_slug(agent_model) rule that src/bird_interact_agents/cloud/upload_back.py applies before walking the post-run reference root. If those rules ever drift, the actor can download/build one versioned root and upload back from another. A shared helper would keep worker setup and upload-back locked to the same version contract.

🤖 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/ray_app.py` around lines 329 - 345, The
encode-version fallback is duplicated and can drift between _otf_version_for in
ray_app.py and the upload-back logic in upload_back.py. Extract the shared
“encode_version or encoder_version_slug(agent_model)” resolution into one helper
and have both call sites use it so worker setup and post-run upload-back resolve
the same version contract.
tests/cloud/test_dev1605_cloud_version.py (1)

67-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Persist the derived encode version in the manifest instead of asserting None.

Locking the default contract to m["encode_version"] is None means worker setup, upload-back, and fetch/merge all have to re-derive the slug from agent_model independently. Given the version axis is supposed to be concrete by submit time, I'd rather have build_manifest() store the resolved slug and assert that here.

🤖 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/cloud/test_dev1605_cloud_version.py` around lines 67 - 81, The manifest
version test is still asserting that encode_version stays None, but
build_manifest() should persist the resolved encode slug instead. Update the
test around driver.build_manifest to expect the derived encode_version value
based on agent_model rather than None, and keep pre_encoded_version aligned with
the existing contract. Use the build_manifest function and the manifest keys
encode_version and pre_encoded_version to locate the change.
🤖 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 `@scripts/build_otf_references.py`:
- Around line 49-56: The version resolution in resolve_build_version currently
treats an explicit empty version the same as an omitted flag because it relies
on truthiness. Update the check to distinguish None from an empty string so an
explicit --version "" is preserved as user-supplied and does not fall back to
encoder_version_slug(agent_model). Keep the returned provenance flag accurate in
resolve_build_version so callers can tell when the version was explicitly
provided, even if empty.

In `@scripts/migrate_otf_references.py`:
- Around line 66-76: The `_setup_usage.json` parsing in the migration helper can
crash on valid JSON with the wrong shape because `_get_encoder_version` assumes
`json.loads()` returns a dict and that each `breakdown` entry is a dict. Update
the parsing logic to defensively validate `data` and each `row` before calling
`.get()` in `_get_encoder_version` (and the same related parsing path referenced
by the follow-up range), and fall back to `_UNKNOWN, False` whenever the
structure is not the expected dict/list-of-dicts shape.
- Around line 143-155: The dry-run path in migrate_otf_references.py is
reporting moves without checking whether the destination already exists, so it
can disagree with a real run. Update the logic around the destination handling
in the move/report flow so the same collision check used after computing dest is
applied before adding or keeping a move in the report; if dest exists, have
dry_run skip or remove that move the same way the non-dry-run path does. Use the
existing migrate_otf_references flow and its report.moves handling to keep
dry-run and real-run behavior aligned.

In `@src/bird_interact_agents/agents/_pre_encoded.py`:
- Around line 188-197: The `_pre_encoded.py` metadata handling in the
`meta_fp.is_file()` block only treats read/JSON failures as best-effort, but it
still assumes the parsed value is a mapping with a valid string `encoder_model`.
Update the `ConsumedReference` path so malformed `_encoder_meta.json` values are
also tolerated: verify the decoded JSON is an object/dict, safely extract
`encoder_model`, and fall back to `"unknown"` when it is missing, null, or not a
string before constructing `ConsumedReference`.

In `@src/bird_interact_agents/cloud/cli.py`:
- Around line 371-387: The `--pre-encoded-version` handling in `cloud/cli.py` is
overwriting a user-supplied version with `None` when `_dbs_for_instances(...)`
returns no DBs. Update the version-selection logic in this CLI path so
`ns.pre_encoded_version` is only replaced when `_resolved` is non-empty, and
otherwise preserve the explicit value already present on
`ns.pre_encoded_version`; use the existing `_dbs_for_instances`,
`resolve_otf_version`, and `ns.pre_encoded_version` flow as the place to fix it.
- Around line 360-364: `encoder_version_slug()` in `submit` can raise
`ValueError` before the existing error handling, so invalid derived encode
versions currently escape as a traceback. Move this derivation into the same
`try/except` path used by the CLI parsing logic in `cli.py`, and convert the
failure into an `argparse` error for `ns.encode_version` / `ns.agent_model` so
malformed model strings exit cleanly with a normal CLI message.

In `@src/bird_interact_agents/model_string.py`:
- Around line 64-69: The encoder_version_slug helper currently only rejects an
empty slug, so it can still return dot-segment values like "." and ".." for
inputs such as anthropic/. and anthropic/.., which breaks the “safe single path
segment” contract. Update encoder_version_slug in model_string.py to explicitly
reject "." and ".." (along with the empty string) before returning the slug, and
keep the ValueError message aligned with the existing explicit-version guidance
so callers fail early and consistently.

---

Outside diff comments:
In `@src/bird_interact_agents/run.py`:
- Around line 323-357: `pre_encoded_version` is accepted by the public runner
APIs but is not being validated against `pre_encoded_source`, so misconfigured
calls can silently ignore the version pin. Add a fast-fail guard in the runner
path around `_make_runner`/`_validate_slayer_setup` to reject any non-OTF use of
`pre_encoded_version`, and thread that argument through the existing
`_validate_slayer_setup(...)` call sites so the same rule is enforced
consistently wherever the runner is built.

---

Nitpick comments:
In `@src/bird_interact_agents/cloud/ray_app.py`:
- Around line 329-345: The encode-version fallback is duplicated and can drift
between _otf_version_for in ray_app.py and the upload-back logic in
upload_back.py. Extract the shared “encode_version or
encoder_version_slug(agent_model)” resolution into one helper and have both call
sites use it so worker setup and post-run upload-back resolve the same version
contract.

In `@tests/cloud/test_dev1605_cloud_version.py`:
- Around line 67-81: The manifest version test is still asserting that
encode_version stays None, but build_manifest() should persist the resolved
encode slug instead. Update the test around driver.build_manifest to expect the
derived encode_version value based on agent_model rather than None, and keep
pre_encoded_version aligned with the existing contract. Use the build_manifest
function and the manifest keys encode_version and pre_encoded_version to locate
the change.
🪄 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: 0f0186d0-21ac-48df-a402-0f71e05abc5a

📥 Commits

Reviewing files that changed from the base of the PR and between 39cdd28 and 4d17d31.

📒 Files selected for processing (32)
  • scripts/build_otf_references.py
  • scripts/migrate_otf_references.py
  • src/bird_interact_agents/agents/_pre_encoded.py
  • src/bird_interact_agents/agents/claude_sdk_otf/agent.py
  • src/bird_interact_agents/agents/claude_sdk_otf_ainteract/agent.py
  • src/bird_interact_agents/agents/claude_sdk_otf_ainteract_v1/agent.py
  • src/bird_interact_agents/agents/claude_sdk_otf_v1/agent.py
  • src/bird_interact_agents/cloud/cli.py
  • src/bird_interact_agents/cloud/driver.py
  • src/bird_interact_agents/cloud/ray_app.py
  • src/bird_interact_agents/cloud/upload_back.py
  • src/bird_interact_agents/eval/annotation_schema.py
  • src/bird_interact_agents/eval/grade_in_place.py
  • src/bird_interact_agents/harness.py
  • src/bird_interact_agents/model_string.py
  • src/bird_interact_agents/paths.py
  • src/bird_interact_agents/run.py
  • src/bird_interact_agents/slayer_otf/encoder_types.py
  • src/bird_interact_agents/slayer_otf/reference_build.py
  • tests/cloud/test_dev1605_cloud_version.py
  • tests/cloud/test_driver.py
  • tests/cloud/test_ray_app.py
  • tests/cloud/test_upload_back.py
  • tests/test_cloud_paths_unchanged.py
  • tests/test_dev1586_pre_encoded.py
  • tests/test_dev1605_annotation_stamp.py
  • tests/test_dev1605_build_script.py
  • tests/test_dev1605_consumer_version.py
  • tests/test_dev1605_encoder_meta.py
  • tests/test_dev1605_migration.py
  • tests/test_dev1605_paths_version.py
  • tests/test_dev1605_slug.py

Comment thread scripts/build_otf_references.py
Comment thread scripts/migrate_otf_references.py
Comment thread scripts/migrate_otf_references.py Outdated
Comment thread src/bird_interact_agents/agents/_pre_encoded.py
Comment thread src/bird_interact_agents/cloud/cli.py Outdated
Comment thread src/bird_interact_agents/cloud/cli.py Outdated
Comment thread src/bird_interact_agents/model_string.py
- Codex (major): the in-cloud `pydantic_ai_otf_encode` encoder now builds
  into the versioned root `slayer_models_otf/<benchmark>/<version>/<db>` and
  stamps `_encoder_meta.json`, so its cloud-built references are found by the
  versioned upload-back / merge instead of being orphaned. `encode_version`
  threads run.py → ray_app → agent.
- Centralise the `encode_version or slug(agent_model)` rule in one shared
  `model_string.resolve_encode_version` used by the agent, ray_app,
  upload_back, and driver (no drift between build/download/upload/merge).
- Validation hardening: `encoder_version_slug` rejects `.`/`..`/`\` slugs;
  `resolve_build_version` treats explicit `--version ""` as operator-supplied
  (not omitted); cloud submit routes a bad encode model through `p.error`;
  preserve an explicit `--pre-encoded-version` when no DBs resolve; reject
  `--pre-encoded-version` unless `--pre-encoded-models otf` (via
  `_validate_slayer_setup`).
- Defensive JSON parsing: malformed `_setup_usage.json` / `_encoder_meta.json`
  fall back to `unknown` instead of crashing; migration dry-run honours
  destination collisions.
- Regression tests for all of the above. Full non-integration suite green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Caution

Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted.

Error details
{}

- Local pre-encoded otf runs now enforce the SAME "one run consumes one
  encoder version" cross-DB guard the cloud submit applies: run_evaluation
  resolves the version across all selected DBs once and errors on a mix,
  so local and cloud behave identically.
- resolve_encode_version distinguishes an explicit value by `is not None`
  (not truthiness), so a deliberate `--version ""` is preserved as
  operator-supplied and fails loudly at the label validator — matching
  resolve_build_version. Cloud submit validates the resolved label inside
  p.error so a bad/empty --version exits cleanly, not with a traceback.
- Regression tests for both. Full non-integration suite green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/bird_interact_agents/cloud/ray_app.py (1)

336-339: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Fail fast when an OTF consumer has no resolved version.

For slayer_setup="pre-encoded" + pre_encoded_source="otf", returning None here can send artifact resolution into the legacy version=None layout instead of enforcing the new version-pinned flow. Please reject missing/empty pre_encoded_version before returning.

Suggested guard
     if cfg.get("slayer_setup") == "pre-encoded" and (
         cfg.get("pre_encoded_source") == "otf"
     ):
-        return cfg.get("pre_encoded_version")
+        version = cfg.get("pre_encoded_version")
+        if not version:
+            raise ValueError(
+                "pre_encoded_version is required for pre-encoded OTF runs"
+            )
+        return str(version)
🤖 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/ray_app.py` around lines 336 - 339, The
version resolution in ray_app.py should fail fast for the OTF pre-encoded path
instead of returning a missing value. In the pre-encoded branch of the version
lookup logic, add a guard for pre_encoded_version being absent or empty when
slayer_setup is "pre-encoded" and pre_encoded_source is "otf", and raise an
error before any return. Update the helper that handles cfg lookups in the same
flow so only a valid resolved version is returned for the version-pinned
artifact path.
🤖 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 `@src/bird_interact_agents/cloud/ray_app.py`:
- Around line 336-339: The version resolution in ray_app.py should fail fast for
the OTF pre-encoded path instead of returning a missing value. In the
pre-encoded branch of the version lookup logic, add a guard for
pre_encoded_version being absent or empty when slayer_setup is "pre-encoded" and
pre_encoded_source is "otf", and raise an error before any return. Update the
helper that handles cfg lookups in the same flow so only a valid resolved
version is returned for the version-pinned artifact path.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: e2e2a291-c4df-4691-8add-0c641c36d0fd

📥 Commits

Reviewing files that changed from the base of the PR and between 4d17d31 and 5fba574.

📒 Files selected for processing (15)
  • scripts/build_otf_references.py
  • scripts/migrate_otf_references.py
  • src/bird_interact_agents/agents/_pre_encoded.py
  • src/bird_interact_agents/agents/pydantic_ai_otf_encode/agent.py
  • src/bird_interact_agents/cloud/cli.py
  • src/bird_interact_agents/cloud/driver.py
  • src/bird_interact_agents/cloud/ray_app.py
  • src/bird_interact_agents/cloud/upload_back.py
  • src/bird_interact_agents/model_string.py
  • src/bird_interact_agents/run.py
  • tests/test_dev1605_build_script.py
  • tests/test_dev1605_migration.py
  • tests/test_dev1605_review_fixes.py
  • tests/test_one_shot_run.py
  • tests/test_pydantic_ai_otf_encode_agent.py
🚧 Files skipped from review as they are similar to previous changes (7)
  • tests/test_dev1605_build_script.py
  • src/bird_interact_agents/cloud/upload_back.py
  • scripts/build_otf_references.py
  • src/bird_interact_agents/cloud/cli.py
  • scripts/migrate_otf_references.py
  • src/bird_interact_agents/cloud/driver.py
  • src/bird_interact_agents/agents/_pre_encoded.py

ZmeiGorynych and others added 3 commits June 26, 2026 10:26
- migrate_otf_references: `_derive_version` / `_backfill_encoder_meta` now
  require a non-empty STRING `model` in the `setup_encoder` breakdown row, so
  a malformed-but-valid `{"model": 123}` falls back to 'unknown' instead of
  crashing `encoder_version_slug`'s `.partition()` and aborting the migration.
- Regression test for the non-string-model case. Full suite green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Local `run.py` now exposes the encode-side `--version` flag (mirror of the
  build-script / cloud-submit `--version`), threaded args.encode_version →
  run_evaluation → _make_runner → PydanticAIOtfEncodeAgent. Local on-the-fly
  encode runs can now build into separate version slots instead of only the
  default agent-model slug — closing the last local/cloud versioning-surface
  gap.
- Regression test that _make_runner threads encode_version to the agent.
  Full suite green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Local CLI now fail-fast validates the encode `--version` label (resolve +
  paths._validate_otf_version) inside the argparse validation try/except, so a
  bad label like `--version ""` / `--version "a/b"` exits with a clean CLI
  error before any task runs — mirroring cloud submit, instead of surfacing as
  per-task failed result rows inside the adapter's broad except.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant