DEV-1609: make claude_sdk_otf_encode a first-class OTF encoder - #64
Merged
ZmeiGorynych merged 12 commits intoJun 29, 2026
Merged
Conversation
Add `claude_sdk_otf_encode` as the default OTF *reference* encoder, reachable from the cloud submit path (previously only the local `scripts/build_otf_references.py --encoder-framework claude_sdk` could drive the claude_sdk encoder; the cloud could only encode with the legacy path). * New build-only `ClaudeSDKOtfEncodeAgent` (agents/claude_sdk_otf_encode/ agent.py): its `run_task` constructs the claude_sdk build-encoder (DEV-1589) and calls `ensure_db_reference` to build the canonical per-DB `slayer_models_otf/<benchmark>/<db>` reference that the cloud merge-back uploads home — behaviourally identical to the local script. No per-task HARD-8 masking, no agentic eval loop. * Register the framework in `run._make_runner` + both `--framework` choice lists (run.py + cloud/cli.py), and in the `--otf-rebuild` force-wipe gate. * Single source of truth `frameworks.is_otf_encode_framework` replaces the five scattered `framework == "pydantic_ai_otf_encode"` literals (driver uploads, ray_app artifacts + seed snapshot, gcs artifact name, upload_back guard). Legacy string stays recognised for back-compat. * Repoint the ~5 "use pydantic_ai_otf_encode" guidance strings + the harness storage-opt-out note + CLAUDE.md smoke recipes to the new default. * Subscription-auth already covers it via the `claude_sdk*` prefix gating. Tests: new framework registration + dispatch, build-only run_task wiring (telemetry from ReferenceEntry, zero-usage reuse path, error-row on failure), guardrails, force-wipe gate, and cloud keying parity (incl. LiveSQLBench root parity + non-encode negatives). Full non-integration suite green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
DEV-1609 Make claude_sdk the default OTF encoder everywhere (wire claude_sdk_otf_encode into run.py + cloud)
GoalMake the Why now (motivating case)We want to OTF-encode the bird-interact-lite-exp databases (Postgres) using the claude_sdk encoder, reusing the encoding work already done for mini-interact (the per-DB KB content is byte-identical modulo line endings — verified across all 18 overlapping DBs). The mini-interact references are NOT directly reusable because they are SQLite-dialect (
So claude_sdk encoding of any Postgres benchmark is currently impossible without local Postgres. This issue removes that gap. Diagnosis — architectural mismatchThe two encoders are shaped differently:
Framework registration gaps
Cloud merge-back is keyed on the LITERAL string
|
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:
📝 WalkthroughWalkthroughAdds ChangesClaude SDK OTF encode rollout
Possibly related PRs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLAUDE.md (1)
196-204: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDrop the retired
--slayer-setupflag from these smoke recipes.
bird-interact-cloud submitnow uses--pre-encoded-modelsand derivesslayer_setup, so the remaining--slayer-setup on-the-flyin both command blocks should be removed; otherwise the recipe will hit an unrecognized-argument error.🤖 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 `@CLAUDE.md` around lines 196 - 204, The smoke recipe still passes the retired --slayer-setup flag in the bird-interact-cloud submit command. Update the command blocks in CLAUDE.md to remove --slayer-setup on-the-fly and rely on the current --pre-encoded-models flow, since submit now derives slayer_setup automatically. Make sure both smoke recipe examples are kept in sync so they do not trigger an unrecognized-argument error.
🤖 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/agents/claude_sdk_otf_encode/agent.py`:
- Around line 125-129: The mode gate in claude_sdk_otf_encode.Agent currently
rejects anything except a-interact, which blocks one-shot benchmark runs coming
from run.py via eval_mode. Update the check in the agent’s encode path to accept
one-shot as well, or remap one-shot to a-interact before this validation so the
encoder can proceed; use the existing eval_mode branch in agent.py as the fix
point.
---
Outside diff comments:
In `@CLAUDE.md`:
- Around line 196-204: The smoke recipe still passes the retired --slayer-setup
flag in the bird-interact-cloud submit command. Update the command blocks in
CLAUDE.md to remove --slayer-setup on-the-fly and rely on the current
--pre-encoded-models flow, since submit now derives slayer_setup automatically.
Make sure both smoke recipe examples are kept in sync so they do not trigger an
unrecognized-argument error.
🪄 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: 1a3887a8-3585-4d39-a7f4-914c260c1493
📒 Files selected for processing (19)
CLAUDE.mdsrc/bird_interact_agents/agents/_pre_encoded.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_v1/agent.pysrc/bird_interact_agents/agents/claude_sdk_otf_encode/__init__.pysrc/bird_interact_agents/agents/claude_sdk_otf_encode/agent.pysrc/bird_interact_agents/agents/claude_sdk_otf_v1/agent.pysrc/bird_interact_agents/cloud/cli.pysrc/bird_interact_agents/cloud/driver.pysrc/bird_interact_agents/cloud/gcs.pysrc/bird_interact_agents/cloud/ray_app.pysrc/bird_interact_agents/cloud/upload_back.pysrc/bird_interact_agents/frameworks.pysrc/bird_interact_agents/harness.pysrc/bird_interact_agents/run.pytests/cloud/test_dev1609_encode_keying.pytests/cloud/test_gcs.pytests/test_dev1609_claude_sdk_otf_encode.py
…(Codex review) * run_task now accepts --mode one-shot in addition to a-interact. One-shot benchmarks (LiveSQLBench + its Postgres variants this issue unblocks) have no a-interact mode, so rejecting one-shot made claude_sdk encoding of them impossible. Mode is irrelevant to what an encode run builds anyway. * Encode telemetry: report the build's `_setup_usage.json` only when THIS task actually built the reference (no pre-existing `_reference_fp.txt` marker). On a marker-based reuse the persisted sidecar would otherwise be re-reported, double-counting tokens across tasks for the same DB. Tests: one-shot acceptance + reuse-zero-usage regressions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… (Codex r2) The marker pre-check couldn't tell a fresh build from a cross-process peer- reuse: when two Ray actors on one VM build the same DB concurrently, the loser reuses the peer's reference under the flock but its pre-check saw no marker, so it re-reported the peer's setup-encode tokens (double-count). Make `ensure_db_reference` return a `ReferenceEntry.built` flag — True only when THIS call ran the encoder, False on every reuse path (top marker check, under-lock double-check, and the under-flock cross-process peer-reuse). The encode agent now reads usage from `_setup_usage.json` only when `entry.built`. `_build_reference[_inside_lock]` now return a ReferenceEntry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…es (CodeRabbit) bird-interact-cloud submit derives slayer_setup from --pre-encoded-models; the retired --slayer-setup on-the-fly would error. on-the-fly is the default (no --pre-encoded-models), so dropping the flag keeps the recipes correct. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…aude_sdk-the-default-otf-encoder-everywhere-wire # Conflicts: # src/bird_interact_agents/cloud/ray_app.py
…ces as an error The maiden cloud on-the-fly claude_sdk_otf_encode run hung in hermetic_claude_sdk_session.__aenter__ (bundled claude CLI + slayer stdio-MCP handshake + get_mcp_status parity probe) — nothing bounded it, so the actor stayed alive with zero output for 2h until the VM self-delete timer fired. The existing per-attempt 300s timeout only covers query/receive, AFTER a successful enter. Add enter_timeout_s (default None = unchanged for every other claude_sdk agent) to hermetic_claude_sdk_session, bounding both __aenter__ and the parity probe; on expiry tear down the half-started subprocess (disconnect/ aexit) and raise SdkSessionEnterError. setup_encoder passes 240s and emits per-KB otf_timing milestone events, so a future cloud hang becomes a per-KB status=error plus a visible log line instead of a silent 2h block. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Local repro proved the claude_sdk_otf_encode path is healthy (cache reuse, SDK enter <1s, KB encoded in 63s) — the cloud hang is environmental and was masked by two gaps this commit closes: 1. Observability: the Ray actor's root log handler defaults to WARNING, so our INFO otf_timing kb.start/kb.done/sdk_client_enter breadcrumbs were dropped — a healthy-but-slow encode looked identical to a hang in ray job logs / the per-task debug log. _ensure_actor_logging() attaches one INFO StreamHandler to the bird_interact_agents package logger in the actor (idempotent). 2. Un-wedgeable timeout: the per-attempt asyncio.wait_for could hang forever when the SDK's anyio-threaded transport read never returns (the cloud failure mode) — wait_for cancels the inner coro and awaits a cancellation that never completes. _drive_with_timeout times the cycle with asyncio.wait (no cancel), then on expiry force-closes the client (disconnect) to break the stuck read, drains the task, and raises TimeoutError → a per-KB error row + an uploaded per-task log instead of a silent 2h block. Together: a future cloud hang self-resolves into error rows whose uploaded per-task log shows exactly which KB/stage stalled. Full suite green (3674). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/test_dev1589_setup_encoder.py (1)
760-768: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the hang test cancellation-resistant.
asyncio.sleep(10)cancels cleanly, so this test does not catch_drive_with_timeouthanging while draining a task that ignores cancellation. Add a regression case wheredrive()stays pending after cancellation until the test releases it.🤖 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_dev1589_setup_encoder.py` around lines 760 - 768, The hang regression test using test_drive_with_timeout_force_closes_on_hang is too cancellation-friendly because asyncio.sleep(10) exits on cancel, so it does not exercise the wedged-drain path in _drive_with_timeout. Update drive() in this test to remain pending after cancellation until the test explicitly releases it, using a named synchronization primitive or event so the task ignores the initial cancel and only finishes when the test allows it. Keep the existing timeout assertion and disconnect check so the test verifies forced close behavior under a truly cancellation-resistant hang.src/bird_interact_agents/cloud/ray_app.py (1)
520-531: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReapply
propagate=Falseon idempotent calls.If another logging configuration leaves the flagged handler installed but flips propagation back on, the next per-task call returns at Line 523 and stops enforcing the duplicate-prevention invariant.
Proposed fix
pkg = logging.getLogger("bird_interact_agents") pkg.setLevel(logging.INFO) + pkg.propagate = False if any(getattr(h, _ACTOR_LOG_HANDLER_FLAG, False) for h in pkg.handlers): return handler = logging.StreamHandler(sys.stdout) handler.setLevel(logging.INFO) handler.setFormatter(logging.Formatter("%(levelname)s:%(name)s:%(message)s")) setattr(handler, _ACTOR_LOG_HANDLER_FLAG, True) pkg.addHandler(handler) - # Don't ALSO propagate to the root WARNING handler — avoids duplicate - # WARNING/ERROR lines; INFO would be dropped there anyway. - pkg.propagate = False🤖 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 520 - 531, The per-task logging setup in the actor logger helper is not fully idempotent because the early return prevents re-enforcing the no-duplicate logging invariant. Update the logger configuration logic around the pkg handler check so that pkg.propagate = False is applied on every call, even when the flagged StreamHandler already exists, while still avoiding re-adding the handler.
🤖 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/agents/claude_sdk_otf_encode/setup_encoder.py`:
- Around line 359-363: The timeout failure path in `_drive_with_timeout` is
still unbounded because `client.disconnect()` and the subsequent task drain can
hang even after force-close. Update the teardown in `setup_encoder.py` around
the `client.disconnect()` and `task.cancel()`/`await task` sequence to apply a
secondary timeout or use a non-blocking force-close mechanism so shutdown cannot
stall indefinitely.
In `@src/bird_interact_agents/agents/claude_sdk/sdk_env.py`:
- Around line 433-436: Handle non-timeout failures during
ClaudeSDKClient.__aenter__() by disconnecting the client even when entered is
still false, so a partially started subprocess does not linger. Update the
BaseException cleanup path in the context manager around client_obj to call
_quiet_disconnect(client_obj) for failures before successful entry, while
preserving the existing _quiet_aexit(client_obj) behavior when entered is true.
In `@tests/cloud/test_ray_app.py`:
- Around line 2488-2526: The actor logging tests are leaking global logger state
because _clear_pkg_handlers only removes handlers and restores propagate, but
does not restore the bird_interact_agents logger level after
ray_app._ensure_actor_logging runs. Update the test setup/cleanup around
test_ensure_actor_logging_attaches_info_handler and
test_ensure_actor_logging_is_idempotent to use _isolated_actor_logging so the
original logger level is preserved, and keep _clear_pkg_handlers focused on
handler cleanup only.
---
Nitpick comments:
In `@src/bird_interact_agents/cloud/ray_app.py`:
- Around line 520-531: The per-task logging setup in the actor logger helper is
not fully idempotent because the early return prevents re-enforcing the
no-duplicate logging invariant. Update the logger configuration logic around the
pkg handler check so that pkg.propagate = False is applied on every call, even
when the flagged StreamHandler already exists, while still avoiding re-adding
the handler.
In `@tests/test_dev1589_setup_encoder.py`:
- Around line 760-768: The hang regression test using
test_drive_with_timeout_force_closes_on_hang is too cancellation-friendly
because asyncio.sleep(10) exits on cancel, so it does not exercise the
wedged-drain path in _drive_with_timeout. Update drive() in this test to remain
pending after cancellation until the test explicitly releases it, using a named
synchronization primitive or event so the task ignores the initial cancel and
only finishes when the test allows it. Keep the existing timeout assertion and
disconnect check so the test verifies forced close behavior under a truly
cancellation-resistant hang.
🪄 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: f23a44d0-2856-43a1-a1f4-6cde845444d9
📒 Files selected for processing (6)
src/bird_interact_agents/agents/claude_sdk/sdk_env.pysrc/bird_interact_agents/agents/claude_sdk_otf_encode/setup_encoder.pysrc/bird_interact_agents/cloud/ray_app.pytests/cloud/test_ray_app.pytests/test_dev1589_setup_encoder.pytests/test_sdk_subprocess_hermetic.py
…p, no logging leak) - setup_encoder._drive_with_timeout: the force-close teardown (disconnect plus drain) was itself unbounded — the SDK disconnect has no internal timeout and a cancelled drive task may ignore cancellation, so the path meant to CONTAIN a hang could hang. Run teardown as a child task capped by TEARDOWN_TIMEOUT_S (asyncio.wait, no cancel); abandon it on stall and raise anyway, so the call always returns within budget. New cancellation-resistant regression test. - sdk_env.hermetic_claude_sdk_session: the non-timeout enter-failure path now quiet-disconnects a half-started client when __aenter__ raised before the entered flag was set, symmetric with the timeout path (no lingering subprocess). - ray_app._ensure_actor_logging: drop the propagate=False line — it globally suppressed propagation for all bird_interact_agents.* loggers, breaking caplog-based tests across the suite. INFO breadcrumbs still surface (handler on the package logger); a rare duplicate WARNING line in cloud is acceptable. - test_ray_app: _isolated_actor_logging CM saves/restores logger level and propagate so the actor-logging tests do not leak global state. Full non-integration suite green (3675 passed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codex flagged that the session-enter timeout path re-introduced the hang class the PR contains: after asyncio.timeout fires, _quiet_aexit/_quiet_disconnect awaited SDK teardown with NO bound, so a wedged __aexit__/disconnect on a half-started CLI/MCP subprocess meant hermetic_claude_sdk_session never raised SdkSessionEnterError — blocking indefinitely. Add _bounded_teardown (asyncio.wait + abandon, NOT wait_for which can itself hang cancelling an uncancellable teardown) capped at _SDK_TEARDOWN_TIMEOUT_S, and route both _quiet_aexit and _quiet_disconnect through it. timeout_s reads the module global at call time (a signature default binds once at import and is unpatchable). Tests: _quiet_aexit/_quiet_disconnect return under the cap when teardown hangs, and an enter timeout whose cleanup also wedges still raises SdkSessionEnterError promptly. Full suite green (3678 passed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…out (Codex) Codex: the timeout teardown awaited client.disconnect() BEFORE task.cancel(), so if disconnect() hung, the drive coroutine was never even cancelled — it stayed alive holding the SDK stream / MCP subprocess and could write into the build dir after _encode_one_kb finalized/purged the KB. Reorder: cancel the drive task first (always requested), then disconnect to unblock the read so the cancel lands, then drain. Also attach a done-callback to the drive task so an abandoned/late task never logs an unretrieved-exception warning. Full suite green (3678 passed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…code mode (Codex) - sdk_env.hermetic_claude_sdk_session: the enter_timeout_s path wrapped __aenter__/parity in asyncio.timeout, which cannot bound a cancellation-resistant CLI/MCP handshake (same class _drive_with_timeout was fixed for) — a wedged enter could hang before cleanup. Run enter+parity as a child task timed by asyncio.wait (no cancel); on expiry force-disconnect (entered-aware: aexit if entered else disconnect, both time-bounded) and raise SdkSessionEnterError. Isolated to the enter_timeout_s-is-set path, so the 8 other claude_sdk agents (which pass None) are byte-for-byte unchanged. Removed the now-unused _maybe_timeout helper. - run._validate_framework_mode: reject claude_sdk_otf_encode with any mode other than a-interact / one-shot at CLI/cloud validation, instead of passing validation and failing per-task after setup (agent.py guardrail). 4 new tests. Full suite green (3682 passed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ion (Codex) Symmetric with the mode guard: the encoder builds the SLayer reference and requires --query-mode slayer (agent.py guardrail), but _validate_slayer_setup returned early for raw, so a raw encode run could start, build/upload setup, and then fail every task. Reject raw for claude_sdk_otf_encode in _validate_slayer_setup before the raw early-return (shared by local run.py and cloud submit). 2 new tests. Full suite green (3684 passed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Goal
Make
claude_sdk_otf_encodethe default OTF reference encoder, reachable from the cloud submit path. Previously the superior claude_sdk encoder (DEV-1589) was only reachable via localscripts/build_otf_references.py --encoder-framework claude_sdk; the cloud submit--frameworkcould only encode with the legacy path. This unblocks claude_sdk encoding of Postgres benchmarks (e.g. bird-interact-lite-exp), where the cloud auto-loads Postgres but the dev box has none.Design — build-only, cloud == local
An OTF-encode run's only durable output is the per-DB
slayer_models_otf/<benchmark>/<db>reference that the cloud merge-back uploads home. The newClaudeSDKOtfEncodeAgent.run_taskdoes exactly what the local script does: construct the claude_sdk build-encoder and callensure_db_reference. No per-task HARD-8 masking, no agentic eval loop — so cloud encoding is behaviourally identical to local encoding (modulo Postgres loading + GCS transport).Changes
ClaudeSDKOtfEncodeAgent(agents/claude_sdk_otf_encode/agent.py), exported from the package.run._make_runner+ both--frameworkchoice lists (run.py+cloud/cli.py), and in the--otf-rebuildforce-wipe gate (_maybe_force_wipe_otf).frameworks.is_otf_encode_frameworkreplaces the 5 scatteredframework == "pydantic_ai_otf_encode"literals: driver uploads, ray_app artifacts + seed snapshot, gcs artifact name, upload_back guard. The legacy string stays recognised for back-compat (existing manifests /resubmit).claude_sdk*prefix gating (run._apply_subscription_auth_env/prereqs._is_claude_sdk_framework) — no prod change, pinned by tests.Telemetry
Encode result-row telemetry is read from the returned
ReferenceEntry(setup_results→kb_encoded, best-effort_setup_usage.json→usage; zero usage on a cache-reuse where no LLM ran) — not from the build-encoder factory.Tests
New: framework registration +
_make_runnerdispatch, build-onlyrun_taskwiring, telemetry-from-ReferenceEntry (+ reuse zero-usage), error-row on build failure, constructor/run_task guardrails, force-wipe gate, subscription-auth explicit-choice, and full cloud keying parity (mini-interact + LiveSQLBench root parity + non-encode negatives). Full non-integration suite green (3557 passed).Out of scope
🤖 Generated with Claude Code
Summary by CodeRabbit
claude_sdk_otf_encodeas a first-class OTF reference encoding framework for cloud submissions.claude_sdk_otf_encode, including rebuild-from-scratch with--otf-rebuild.CLAUDE.mdand submit instructions to default toclaude_sdk_otf_encodeand require explicit--subscription-auth.