claude_sdk_otf: Claude SDK agent that encodes KB on-the-fly (DEV-1505) - #9
Conversation
…505) New Claude Agent SDK framework that encodes the relevant KB items into the per-task SLayer store off the deterministic OTF cache (KB pre-loaded as memories), then builds the final query off the named columns/measures it created instead of inlining everything. slayer-query-mode only; eval modes a-interact + one-shot; always --slayer-setup on-the-fly. - Reuses claude_sdk's contextvar plumbing + native user-sim/submit/knowledge tools; adds the SLayer MCP write tools (create_model/edit_model/save_memory/ query_nested/validate_models). One-shot drops ask_user. - Lifts a shared cache-only resolver (resolve_otf_task_storage_dir) into slayer_otf.runtime so the Claude-SDK-only adapter doesn't import the pydantic_ai adapter packages. - run.py: --framework choices, _make_runner branch, _validate_slayer_setup (on-the-fly required), _validate_one_shot_framework, _maybe_force_wipe_otf, Anthropic-lock help text. Cloud is cache-only by construction (no edits). - Save-time SLayer-write validator deferred to DEV-1506. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughThis PR adds a new ChangesClaude SDK OTF Agent Framework
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
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)
1023-1030:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRefresh the CLI help for
--slayer-setup.Exposing
claude_sdk_otfhere makes--helpmisleading unless the--slayer-setuphelp text is updated too. It still says on-the-fly is only valid forpydantic_ai_recursiveina-interact, while_validate_slayer_setup()now acceptspydantic_ai_otf_encode,claude_sdk_otf, andone-shot.🤖 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 1023 - 1030, Update the CLI help text for the "--slayer-setup" option to reflect the expanded valid values that _validate_slayer_setup() now accepts (pydantic_ai_otf_encode, claude_sdk_otf, and one-shot) instead of only mentioning on-the-fly for pydantic_ai_recursive; find the help string that documents "--slayer-setup" in the CLI parser and revise its description to list or describe the new valid frameworks (including claude_sdk_otf and pydantic_ai_otf_encode) and any behavior differences so the --help output matches the validation logic in _validate_slayer_setup().
🤖 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/slayer_otf/runtime.py`:
- Around line 45-58: _otf_work_dir currently returns a stable path per
instance_id which causes concurrent runs to clash (see prepare_task_storage
deleting <work_dir>/<db>); change _otf_work_dir to create a unique
per-invocation scratch directory (e.g., append a short UUID or timestamp+PID or
use tempfile.mkdtemp/TemporaryDirectory) so each call returns a distinct path,
and update callers that assume a persistent path accordingly (refer to function
_otf_work_dir and prepare_task_storage to locate the usage).
In `@tests/test_claude_sdk_otf_run_wiring.py`:
- Line 76: Replace hardcoded "/tmp" test paths with the pytest tmp_path fixture:
find the call sites that pass data_path="/tmp/x.jsonl" and data_dir="/tmp" (and
the similar occurrences on the other two lines) and change them to use tmp_path
(e.g., data_path should be the stringified tmp_path / "x.jsonl" and data_dir the
stringified tmp_path); ensure the test function signature includes the tmp_path
parameter if it doesn't already so the fixture is available.
---
Outside diff comments:
In `@src/bird_interact_agents/run.py`:
- Around line 1023-1030: Update the CLI help text for the "--slayer-setup"
option to reflect the expanded valid values that _validate_slayer_setup() now
accepts (pydantic_ai_otf_encode, claude_sdk_otf, and one-shot) instead of only
mentioning on-the-fly for pydantic_ai_recursive; find the help string that
documents "--slayer-setup" in the CLI parser and revise its description to list
or describe the new valid frameworks (including claude_sdk_otf and
pydantic_ai_otf_encode) and any behavior differences so the --help output
matches the validation logic in _validate_slayer_setup().
🪄 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: 52331b57-a8b5-4c4d-acbd-8c11d92a3f76
📒 Files selected for processing (9)
README.mdsrc/bird_interact_agents/agents/claude_sdk_otf/__init__.pysrc/bird_interact_agents/agents/claude_sdk_otf/agent.pysrc/bird_interact_agents/agents/claude_sdk_otf/prompts.pysrc/bird_interact_agents/run.pysrc/bird_interact_agents/slayer_otf/__init__.pysrc/bird_interact_agents/slayer_otf/runtime.pytests/test_claude_sdk_otf_agent.pytests/test_claude_sdk_otf_run_wiring.py
|
@CodeRabbit why does it say "review skipped" and yet there are review comments? |
|
Tip For best results, initiate chat on the files or code changes.
So in short: the comments were posted during a previous review run, and the "review skipped" message only means that the latest push didn't trigger a fresh review — not that the old comments were removed or invalidated. If you'd like me to do a fresh full review of the current state of the PR, just say the word! |
…view fixes (DEV-1505) Addresses the Codex + CodeRabbit review on PR #9: - Pass model=native_model_id(model) to ClaudeAgentOptions so --agent-model actually takes effect instead of the claude CLI's configured default (Codex, pre-existing gap shared with claude_sdk). - Add --reasoning-effort {low,medium,high,max} mapped to ClaudeAgentOptions.effort, threaded through run.py (CLI + run_evaluation + run_one_task + make_runner) and the cloud path (cli/driver/ray_app, incl. manifest + resubmit). Default unset = SDK default. - Make the on-the-fly scratch dir unique per invocation (uuid suffix) so concurrent same-task / cross-framework runs can't rmtree each other's live per-task SLayer store (CodeRabbit). - Refresh the --slayer-setup help text to list the new on-the-fly frameworks (CodeRabbit). - Tests use tmp_path instead of hardcoded /tmp (CodeRabbit); add tests for model pinning, effort passthrough + validation, and resubmit preserving --reasoning-effort. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…submit nudge (DEV-1505) Root-caused from the PR #9 households_7 smoke session log: the agent encoded the KB correctly and called submit_query on its FINAL turn, but the answer was dropped. Three fixes: - The full Claude Code built-in toolset was present and our MCP tools were DEFERRED behind ToolSearch, so ~5 turns/run were wasted re-discovering our own tools. Set tools=[] + setting_sources=[] so only the ~16 MCP tools are exposed, directly (no ToolSearch, no built-ins, no CLAUDE.md bleed-through). - The manual MAX_MODEL_TURNS break severed the final turn's submit_query before it executed. Use the SDK's native max_turns (= 2x base = 120) instead, which lets the final turn's tool run before the run stops; drop the manual break. - Add a PostToolUse hook that injects a "submit now" nudge when within 3 turns of the cap. Tests: tools/setting restriction + max_turns + hook wiring; turn-budget hook. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…sage) (DEV-1505) The Claude Agent SDK delivers message.usage as a DICT, but both claude_sdk and claude_sdk_otf read it via getattr(...) (attribute access) — so every agent turn recorded 0 tokens and agent_cost_usd was $0 (only the user-sim's litellm cost showed up). Surfaced when an Opus high-effort run reported $0.035 while the SDK's ResultMessage.total_cost_usd was $1.02. - Add a shared accumulate_assistant_usage() helper in claude_sdk.agent that reads usage from either a dict (live SDK) or an attribute object (mocks), includes cache_write, and gates on AssistantMessage so the cumulative ResultMessage.usage is not double-counted. - Use it from both claude_sdk and claude_sdk_otf so SDK-backed agents report cost consistently with the litellm-tracked frameworks. - Regression test feeds a dict-shaped usage + a ResultMessage and asserts the tokens are captured once and agent_cost_usd > 0. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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 `@tests/test_claude_sdk_otf_agent.py`:
- Around line 314-355: The test
test_accumulate_assistant_usage_dict_shaped_and_skips_result should monkeypatch
usage_mod._cost_per_token to return a deterministic non-zero per-token cost
before calling accumulate_assistant_usage so agent_cost_usd doesn't depend on
LiteLLM's pricing; specifically, in the test (which constructs accum =
usage_mod.TokenUsage() and calls accumulate_assistant_usage), set
usage_mod._cost_per_token = lambda model: 0.0001 (or similar) prior to the calls
and restore if needed so accumulate_assistant_usage → usage._safe_cost() →
usage._cost_per_token() produces a stable >0 agent_cost_usd for the final
assertion.
🪄 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: 8bca782d-86d5-436a-9544-6b90de2b3f13
📒 Files selected for processing (3)
src/bird_interact_agents/agents/claude_sdk/agent.pysrc/bird_interact_agents/agents/claude_sdk_otf/agent.pytests/test_claude_sdk_otf_agent.py
…act-cloud submit (DEV-1505) Surfaced after a smoke run was kicked off against mini-interact when the user had asked for livesqlbench: --dataset defaulted silently to mini_interact and --mode a-interact happened to be consistent with that default, so the validator stack saw no inconsistency and the wrong benchmark ran. Drop the implicit default and require --dataset on every local and cloud CLI call. The mode/framework validators still catch inconsistent combos; this closes the silent-correct-default trap. - run.py + cloud/cli.py: --dataset is required=True (no default), help text updated. - Tests: the two test helpers (_argv_base in test_slayer_setup_flag.py, _slayer_argv in cloud/test_cli.py) now inject --dataset; inline argv blocks in cloud/test_cli.py, test_pydantic_ai_otf_encode_run_wiring.py, and test_claude_sdk_otf_run_wiring.py also do. - The two tests that verified the old default (test_dataset_defaults_to_mini_interact, test_dataset_flag_default_is_mini_interact) are rewritten to assert that omitting --dataset is rejected. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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 `@tests/cloud/test_cli.py`:
- Around line 68-82: Several negative submit tests are now passing for the wrong
reason because argparse rejects the missing required --dataset before the
intended logic runs; update the test argv lists for
test_require_audited_gold_default_on,
test_detach_and_allow_dirty_mutually_exclusive,
test_empty_instance_ids_string_rejected, and
test_empty_instance_ids_file_rejected to include the dataset flag (use
"--dataset", "mini_interact") so that cli.parse_args in those tests reaches the
intended validation paths rather than failing on the required-argument check.
🪄 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: 1b797fbf-6a9c-4584-a802-cba20dea498d
📒 Files selected for processing (7)
src/bird_interact_agents/cloud/cli.pysrc/bird_interact_agents/run.pytests/cloud/test_cli.pytests/test_claude_sdk_otf_run_wiring.pytests/test_one_shot_mode.pytests/test_pydantic_ai_otf_encode_run_wiring.pytests/test_slayer_setup_flag.py
…505) After making --dataset mandatory on the local and cloud CLIs, the existing README examples and the helper scripts continued to omit it and would exit during argument parsing. Add --dataset mini_interact to every bird-interact / bird-interact-cloud invocation in: - README.md: oracle, claude_sdk raw a-interact, claude_sdk slayer a-interact, claude_sdk_otf local mini-interact, claude_sdk_otf cloud (mini-interact). - scripts/run_haiku_slayer.sh - scripts/run_three_way.sh The README livesqlbench/one-shot examples already carry --dataset livesqlbench. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
CodeRabbit on PR #9: after making --dataset required, several test_cli.py negative tests still omitted --dataset and were therefore passing for the wrong reason — argparse rejected the missing required arg before the test's intended guard fired. Add --dataset mini_interact to the 5 16-space-indent argv blocks that test specific guards (audited-gold rule, --detach/--allow-dirty exclusivity, two empty --instance-ids paths, and the unknown-mode test which had the same gap). The test_dataset_is_required block stays untouched — it asserts that omitting --dataset is rejected. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
CodeRabbit on PR #9: the dict-shaped usage helper test asserts `accum.agent_cost_usd > 0`, which depended on litellm's cost-map being loaded for `anthropic/claude-opus-4-7`. Monkeypatch `usage_mod._cost_per_token` to a deterministic non-zero value (matches the pattern other usage tests already use), so the test is robust to litellm pricing-map state. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Summary
Adds
--framework claude_sdk_otf: a single Claude Agent SDK agent that encodes the relevant knowledge-base (KB) items into the per-task SLayer store on the fly — off the deterministic OTF cache (base models + KB pre-loaded as memories) — and then builds the final query off the named columns/measures it created instead of inlining everything. No forced stages or recursion (unlikepydantic_ai_otf_encode).--slayer-setup on-the-fly.claude_sdk.agent's contextvar plumbing + native user-sim/submit/knowledge tools; adds the SLayer MCP write tools (create_model/edit_model/save_memory/query_nested/validate_models). One-shot dropsask_user(decides autonomously).pydantic_ai_otf_encode's_STYLE_GUIDE(string-norm, cross-model access via declared joins, no invented joins, host choice,[kb=N]self-annotation) +claude_sdk's decompose/ask-user discipline — synthetic examples only.resolve_otf_task_storage_dirintoslayer_otf/runtime.pyso this Claude-SDK-only adapter doesn't import the pydantic_ai adapter packages. Recursive adapter left untouched (its tests pin its private copy).claude_sdk_otfalready falls into the cache-only branches ofgcs.slayer_artifact_name/driver._slayer_uploads_for/ray_app._slayer_artifacts_for; the reference upload-back/merge gates correctly skip it. No cloud source edits.Files
agents/claude_sdk_otf/{__init__,agent,prompts}.pyrun.py(choices,_make_runner,_validate_slayer_setup,_validate_one_shot_framework,_maybe_force_wipe_otf, help text),slayer_otf/runtime.py+__init__.py(shared resolver),README.mdtests/test_claude_sdk_otf_agent.py,tests/test_claude_sdk_otf_run_wiring.pyTest plan
1491 passed, 94 skipped)resolve_task_storage_dir);allowed_toolsinclude write tools; one-shot omitsask_userslayer_otf_cacheonly (no reference / upload-back)bird-interact-cloud submit --framework claude_sdk_otf ...) — run manually🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
CLI / Cloud
Documentation
Tests