Skip to content

DEV-1581: R2 hard partition via two persistent clients + ask_discovery - #56

Merged
ZmeiGorynych merged 9 commits into
mainfrom
egor/dev-1581-dev-1555-v1-subagent-split-enforce-hard-partition-rewrite
Jun 23, 2026
Merged

DEV-1581: R2 hard partition via two persistent clients + ask_discovery#56
ZmeiGorynych merged 9 commits into
mainfrom
egor/dev-1581-dev-1555-v1-subagent-split-enforce-hard-partition-rewrite

Conversation

@ZmeiGorynych

@ZmeiGorynych ZmeiGorynych commented Jun 22, 2026

Copy link
Copy Markdown
Member

Problem

The DEV-1555 v1 subagent split hurt convergence: on alien_1 it took 3.4× more turns and 3.7× more wall-clock than v0 to reach the same answer. Root cause (verified via SDK spikes): the partition was softallowed_tools is permission-only, so main kept the introspection tool schemas in its context and re-ran discovery's tools by reflex; and the prompt lied about tool availability.

What this does (R2)

Replaces the SDK-subagent split with two persistent ClaudeSDKClients per task in one process:

  • MAIN client: orchestration + encode/query/submit + an in-process ask_discovery(question) tool. No Task, no AgentDefinition.
  • DISCOVERY client: a long-lived warm client holding the schema/KB introspection tools, reached only through ask_discovery. Its context accumulates across calls (no cold re-introspection — the root cause of the blow-up).
  • One shared in-process SLayer engine backs both clients' SLayer natives (no slayer stdio process), so a model main writes is immediately visible to discovery's inspect_model/search.

Because the two clients are separate sessions, main's per-turn tool schema never contains the introspection tools — for slayer and raw. The partition is now a HARD boundary: no disallowed_tools global, no per-call deny hook, no per-agent AgentDefinition.mcpServers.

Changes

  • _query.py — SLayer storage handle + tool-fn caches made task-local (ContextVar-backed _TaskQueryState); attach_storage rebinds (never mutates) so concurrent run_tasks can't clobber each other (Codex R2 Cloud SLayer mode end-to-end (mirror local) + consolidate the OTF artifact lifecycle (DEV-1468) #2, a real pre-existing concurrency bug).
  • claude_sdk/agent.py_ask_discovery_impl + the ask_discovery native; in-process SLayer natives bridged from SLayer's own descriptions/schemas, backed by the shared engine (create_model/edit_model filter-normalized first); resolve_native_tool / build_bird_interact_server.
  • discovery_channel.pyDiscoveryChannel (single-flight lock, call cap = 10, per-stream fresh usage tracker, never-raises) + discovery_session / open_main_and_discovery lifecycle helpers.
  • discovery_runtime.py (new) — run_main_with_discovery: discovery entered first under one AsyncExitStack (→ closes last, so an in-flight ask_discovery during main shutdown never hits a closed client); warm channel published to _ctx["_discovery"]; both clients' usage aggregated into one accumulator; DEV-1561 otf_timer wraps the first SDK subprocess spawn.
  • Four v1 agents — rewrote run_task to build two clients via hermetic_claude_sdk_session (DEV-1579 compliant for both); per-client MAIN_NATIVE_TOOL_NAMES / DISCOVERY_NATIVE_TOOL_NAMES; removed AgentDefinition/Task/partition_deny; threaded pre_encoded write-stripping.
  • partition.pybuild_main_workflow_note reworded for the ask_discovery model (truthful tool availability + anti-thrash injunction after grader misses); dead subagent helpers (make_partition_deny_hook, DISCOVERY_AGENT_NAME) removed.
  • _pre_encoded.pystrip_write_tool_names is now server-prefix-agnostic (tools moved from mcp__slayer__ to mcp__bird-interact-tools__).

Tests

Full non-integration suite green (3341 passed). Four per-agent test files updated to the two-client API; test_dev1555_subagent_options.py deleted (superseded by test_dev1581_agent_wiring.py); test_dev1555_partition_hooks.py trimmed; sdk-enter-timing / pre-encoded / query-wrapper tests updated. New test_dev1581_* files pin the partition contract, the warm-channel bridge, lifecycle teardown ordering, task-local storage isolation, and shared-engine coherence.

Out of scope / follow-up

  • Cloud validation (the alien_1 v0-vs-new-v1 turn-count A/B) is deferred per the implementation handoff — re-ask after merge.

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

  • New Features

    • Added a persistent warm discovery runtime with in-process ask_discovery handoffs.
    • Exposed SLayer MCP tools as in-process native tools with clear main vs discovery routing.
  • Bug Fixes

    • Prevented cross-task interference by making SLayer query/tool caching task-local.
    • Improved pre-encoded write-tool filtering to be MCP server prefix agnostic.
    • Hardened discovery streaming (single-flight), timeouts, call caps, and teardown ordering.
  • Refactor

    • Split execution into persistent discovery + main clients with enforced read-only discovery tool partitioning.
  • Tests

    • Expanded DEV-1581/1586 coverage for tool contracts, discovery behavior, and task-local isolation.

ZmeiGorynych and others added 3 commits June 22, 2026 13:23
…-merge checkpoint)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…5-v1-subagent-split-enforce-hard-partition-rewrite
Replace the DEV-1555 v1 SDK-subagent split (which took 3.4x more turns than
v0) with R2: two persistent ClaudeSDKClients per task — a main loop and a
long-lived warm discovery client reached only through an in-process
ask_discovery tool. Because the two clients are separate sessions, main's
per-turn tool schema never contains the introspection tools, so the
partition is a HARD boundary (no AgentDefinition, no Task, no
partition_deny hook, no slayer stdio process).

- _query.py: SLayer storage + tool-fn cache made task-local (ContextVar)
  so concurrent run_tasks can't clobber each other (Codex R2 #2).
- claude_sdk/agent.py: _ask_discovery_impl + ask_discovery native; in-process
  SLayer natives bridged from SLayer's own schemas, backed by the shared
  task-local engine (create/edit filter-normalized); resolve_native_tool /
  build_bird_interact_server.
- discovery_runtime.py (new): run_main_with_discovery — discovery entered
  first (closes last), warm DiscoveryChannel in _ctx, usage aggregated,
  DEV-1561 enter-timing on the first SDK spawn.
- four v1 agents: rewrote run_task to the two-client model; per-client
  MAIN_/DISCOVERY_NATIVE_TOOL_NAMES; removed AgentDefinition/Task/
  partition_deny/slayer-stdio.
- partition.py: build_main_workflow_note reworded for ask_discovery (+
  anti-thrash after grader misses); dead subagent helpers removed.
- _pre_encoded.py: strip_write_tool_names now server-prefix-agnostic.

Tests: four per-agent test files updated to the two-client API;
test_dev1555_subagent_options deleted (superseded by test_dev1581_agent_wiring);
test_dev1555_partition_hooks trimmed. Full non-integration suite green.

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

linear Bot commented Jun 22, 2026

Copy link
Copy Markdown
DEV-1581 DEV-1555 v1 subagent split: enforce hard partition + rewrite main prompt so the discovery handoff is actually trusted

Problem

The DEV-1555 v1 agents (claude_sdk_otf_v1 + claude_sdk_otf_ainteract_v1) introduced a discovery/main subagent partition with the hypothesis that a smaller per-turn context for the main agent → better convergence. On the canonical smoke task (alien_1 mini-interact slayer a-interact opus-4-7) the opposite happened: v1 took 3.4× more turns and 3.7× more wall-clock than v0 to reach the same correct answer.

Metric v0 (single-agent) v1 (subagent split) Ratio
n_agent_turns 69 239 3.46×
duration_s 379 1395 3.68×
usage.cost_usd $2.07 $5.84 2.82×
cache_read_tokens 2.02M 6.25M 3.09×
MAIN query calls 13 30 2.3×
MAIN submit_query calls 6 14 2.3×
MAIN ask_user calls 3 7 2.3×
Task (subagent) calls 0 5
Sum of subagent duration_ms 367s (26% of total)

Both runs eventually submitted the same SNQI-over-weathprofile interpretation; v1 found nothing v0 didn't.

Root cause (two-part)

(1) The partition is SOFT — main can still call discovery's tools

Main's allowed_tools includes inspect_model, models_summary, search — the very tools the workflow note tells main not to use. Per the SDK semantics, allowed_tools is permission-only; tool schemas are still in main's context every turn.

Concrete evidence from the alien_1 v1 trajectory (/home/james/Dropbox/SLayer/bird-agents/results/mini-interact/cloud/20260620t2027-claudes-slayer-9699f0/rows/alien_1/attempt-1.json):

(2) The MAIN_WORKFLOW_NOTE prompt LIES about tool availability

src/bird_interact_agents/agents/claude_sdk/partition.py:138 currently appends to main's system prompt:

"Schema/data introspection tools are NOT available to you directly — they live in the 'discovery' subagent."

But the introspection tools ARE in main's allowed_tools and ARE in main's tool list. The model sees a system instruction claiming the tools don't exist AND the tools' schemas in the same context window. Cognitive dissonance → the model picks the "must be a stale instruction" reading and uses the tools directly. The current wording is the worst of both worlds.

Additional prompt gaps that compound the problem:

Proposed fix

Code: hard partition via disallowed_tools (small)

In all four v1 agents, build a disallowed_tools list that includes every discovery-only tool name (mcp__slayer__inspect_model, mcp__slayer__models_summary, mcp__slayer__search, mcp__slayer__list_datasources, mcp__bird-interact-tools__get_all_external_knowledge_names, mcp__bird-interact-tools__get_knowledge_definition, mcp__bird-interact-tools__get_all_knowledge_definitions) and pass it to ClaudeAgentOptions(disallowed_tools=...). Per the SDK semantics, disallowed_tools is the only knob that actually removes tool schemas from the model's context (related: DEV-1579 hermetic-env work). Effect: main physically cannot call discovery's tools — the prompt finally tells the truth.

Cap Task() at 1 by default: after the first Task() in a run, register a PreToolUse hook that denies further Task() calls unless the main agent first emits a free-text "what discovery missed" justification. Converts the soft "please prefer Task" into a hard "Task() is a one-shot at run start; everything else is on you". Surfaces real handoff gaps as visible data instead of silent thrash.

Prompt: rewrite MAIN_WORKFLOW_NOTE (the bigger lever)

Update src/bird_interact_agents/agents/claude_sdk/partition.py::MAIN_WORKFLOW_NOTE (and the mode-aware build_main_workflow_note from DEV-1555 round 5):

  1. Truthful tool availability. Replace the false claim with the actual contract: "Discovery's tools (inspect_model, models_summary, search, list_datasources, knowledge natives) are removed from your tool surface — you cannot call them. To get more schema information, spawn '{DISCOVERY_AGENT_NAME}' via Task()." (Becomes true after the code-side fix above.)
  2. Pin the handoff format. Mandate the discovery subagent return a report with named sections — e.g. ## Schema, ## Joins, ## KB, ## Sample values, ## Open questions. Tell main: "When you need information, FIRST check whether discovery's last report has it under a named section; cite the section name in your reasoning if you're going to use it. Do NOT spawn additional Task() calls for information present in an existing section."
  3. Anti-thrash injunction after grader misses. "After a failed submit_query (any non-pass status), do NOT spawn additional Task() calls. The new evidence is in the grader's miss_diagnostics, not in re-introspecting the schema. Re-query (query tool) against your candidate, or pivot your operationalisation, or ask_user. Task() is for schema gaps you discovered while reading discovery's report, not for grader misses."
  4. (Optional, harder) Discovery-side counterpart. Update the discovery agent's prompt to emit the structured handoff format above, with a fixed top-of-report summary the main agent can scan in one turn instead of reading the full prose report.

Validation plan

  1. Pre-fix baseline: re-run alien_1 mini-interact slayer a-interact opus-4-7 on cloud with current claude_sdk_v1 (3 trials to control for stochasticity). Record n_agent_turns, duration_s, cost_usd, # Task() calls, # main inspect_model calls.
  2. Apply hard partition + prompt rewrite.
  3. Re-run same 3 trials. Target: median turn count and cost within ±20% of the v0 baseline (~69 turns / $2 / 6 min) while still passing phase1 at ≥ v0 rate.
  4. If the gain is real, run on a 20-task mini-interact subset (opus v0 vs new v1) to confirm the win generalises beyond alien_1.

Out of scope

  • The discovery/main split itself. The split is intentional and earned per DEV-1555 stage 1 — the bug is that it's currently soft and the prompt undermines it. This issue is about hardening the existing split, not reverting it.
  • Hermetic Claude SDK subprocess env: separately tracked as DEV-1579. The two issues share the underlying SDK semantics that allowed_tools is permission-only and disallowed_tools is the only schema-stripping knob; resolving both means the same plumbing pattern gets used twice.

References

  • DEV-1555 (PR DEV-1555 stage 1: discovery/main subagent split + context guards + autopsy squeeze #47): the v1 subagent split landed here. The MAIN_WORKFLOW_NOTE is at src/bird_interact_agents/agents/claude_sdk/partition.py:138 and was mode-parameterised in DEV-1555 review round 5 (build_main_workflow_note(query_mode=...)).
  • DEV-1579: hermetic Claude SDK subprocess env. Sibling issue — same disallowed_tools plumbing.
  • Trajectories for the alien_1 v0/v1 comparison:
    • v0: /home/james/Dropbox/SLayer/bird-agents/results/mini-interact/cloud/20260620t1724-claudes-slayer-9f44c2/rows/alien_1/attempt-1.json (69 turns, $2.07, 6.3 min, phase1_passed)
    • v1: /home/james/Dropbox/SLayer/bird-agents/results/mini-interact/cloud/20260620t2027-claudes-slayer-9699f0/rows/alien_1/attempt-1.json (239 turns, $5.84, 23.3 min, phase1_passed — same final answer as v0)
  • Subagent autopsy report dispatched 2026-06-21 (in the DEV-1555 PR-review conversation transcript).

DEV-1555 Use open weight models in benchmark runs

Spec — Open-weight model backends for claude_sdk_otf* benchmark runs

0. Problem statement (measured)

Target: run the four claude_sdk_otf* agents (slayer + raw, one-shot + a-interact) on open-weight models served via Doubleword (e.g. Kimi K2.6), whose usable context is ≤ ~260K tokens.

Measured on 614 existing claude_sdk_otf slayer sessions (Opus 4.7 agent), peak context read from each session's ResultMessage.usage (exact, not estimated):

metric peak context (tokens)
median 84K
p90 156K
p95 173K
p99 221K
max 262K

Sessions exceeding a given window: >260K: 0.2%, >200K: 2.0%, >160K: 7.7%, >131K: 22.1%. Raw-SQL mode sessions peak at 7–40K (no inherent problem, but uniformity was chosen). Weaker open-weight models will burn more turns on the same tasks, fattening the tail. The dominant context consumers in big sessions are introspection-phase tool outputs (mcp__slayer__search, query/query_nested results, models_summary, inspect_model, help).

The autopsy prompt is worse: it embeds the full trajectory JSON, which duplicates every tool result (the content block the model saw + a larger tool_use_result SDK echo), making autopsy prompts ~2x the agent session (largest ≈ 1.1M chars ≈ 280–350K tokens). run_autopsy is also hardcoded to the Anthropic SDK client with model=self.model (the agent model) and would crash outright with a non-Anthropic agent model.

All three LLM roles move to open weights: task agent, user-sim, autopsy — the expensive long sessions are exactly the migration target.

1. Delivery in two stages

Stage 1 — model-agnostic harness refactor. Subagent split + context guards + autopsy prompt hygiene. Runs entirely on Claude. Goes through the full TDD + /process-reviews loop, lands, and is re-baselined (gate: the 15 households instances, Opus agent / Sonnet sim, patience 500, audited gold, compared instance-by-instance against the latest claudes-slayer runs). Only after this converges does Stage 2 begin.

Stage 2 — open-weight wiring. Provider registry, env plumbing, LiteLLM sidecar, autopsy routing + fallback, CLI/cloud support, smoke vs Doubleword.

2. Stage 1 — harness refactor (all 4 agents, canonical, no flag)

2.1 Discovery/main subagent split

Mechanism: Claude Agent SDK AgentDefinition subagent named discovery, declared in ClaudeAgentOptions(agents={...}), model="inherit". The main session's allowed_tools excludes discovery-only tools (enforced partition) and includes Task. The main system prompt instructs: start by delegating discovery; re-spawn discovery for follow-up introspection or user questions; the discovery prompt defines a structured handoff report (relevant models/tables with entity refs, columns with descriptions + sampled values, join paths, verbatim KB definitions deemed relevant, user clarifications as verbatim Q→A pairs, open ambiguities). Report format lives in prompts only — no prompt-content tests.

Built-ins (Codex r1 #1): the OTF agents currently pass tools=[], which drops EVERY built-in including Task. Stage 1 changes this to tools=["Task"] — re-enabling exactly the one built-in the split needs while keeping Bash/Edit/WebFetch/ToolSearch suppressed (ToolSearch removal is load-bearing: it keeps MCP tools exposed directly instead of deferred). A test asserts both options.tools == ["Task"] and the allowed_tools list per agent.

Hook propagation (Codex r1 #2): "hooks fire inside subagent sessions and subagent turns count toward max_turns" is an SDK behavior assumption, not a fact. Stage 1 includes a verification test (SDK-level fake or integration) that a Task subagent's tool calls hit the PreToolUse/PostToolUse hooks and consume turn budget. If the SDK does NOT propagate, the same hooks/caps are attached to the AgentDefinition equivalently.

ask-gate semantics (Codex r1 #3, light remedy): ask_user stays available in both contexts; the a-interact ask-before-submit gate continues to count any ask (including discovery's). The discovery prompt requires verbatim Q→A pairs in the handoff so answers reach the main agent. A regression test documents the gate semantics (discovery-side ask satisfies the gate).

Tool partitions (exact tool names; ask_user rows apply to a-interact flavors only):

discovery subagent main agent
slayer mcp__slayer__search, mcp__slayer__models_summary, mcp__slayer__inspect_model, mcp__slayer__list_datasources, KB natives (mcp__bird-interact-tools__get_all_external_knowledge_names / get_knowledge_definition / get_all_knowledge_definitions), ask_user Task, mcp__slayer__help, mcp__bird-interact-tools__query / query_nested, mcp__slayer__create_model / edit_model / validate_models / save_memory, KB natives, submit_query, ask_user
raw get_schema, get_all_column_meanings, get_column_meaning, KB tools, execute_sql (profiling), ask_user Task, execute_sql, submit_sql, ask_user

KB natives intentionally in both (slayer): exact formulas must not pass through a lossy summary before encoding. (list_datasources placed discovery-only.)

Unchanged: the coin/budget gate and n_asks accounting share ctx state across both agents; _MAX_TURNS = 120 stays and now covers both contexts' tool calls combined. SDK auto-compaction stays at its default (enabled) as last resort.

2.2 Context-budget hook

The stream consumer records state["context_tokens"] = input + cache_read + cache_creation from each AssistantMessage.usage (the live SDK provides this per turn). A PostToolUse hook reads that shared state and, when it crosses 80% of the model's context window, injects a one-shot [CONTEXT BUDGET] additionalContext warning (mirroring the [TURN BUDGET] wording) telling the agent to submit its best candidate now; a second, final warning at 90%. Window resolution via a new context_window_for(model: str) -> int helper — Stage 1: known Anthropic windows, default 200K; Stage 2 wires the registry into it.

2.3 Autopsy prompt hygiene (model-agnostic part)

  • _compress_trajectory_for_autopsy additionally replaces every tool_use_result value with "[tool_use_result: N chars]" (echo strip — always, ~halves the prompt at zero information loss).
  • Progressive squeeze: estimate prompt tokens (chars/3.5); budget = context_window_for(autopsy_model) × 0.75 minus output reserve. While over budget, elide tool-result block bodies oldest-first ("[tool result elided: N chars]"), never touching assistant text, tool inputs, or the last 20 trajectory items; if still over, hard-truncate the middle with a marker. Deterministic, pure function.
  • Scope (Codex r1 DEV-1478: generous SLayer MCP startup timeout #7): hygiene applies to dict-shaped trajectory items only (the SLayer agents' structured capture). The raw agents capture legacy str(msg)[:500] string items — those pass through unchanged (they are already tiny). No raw-capture normalization.

2.4 Stage-1 tests (written first, TDD)

Mechanical contracts only: partition constants ⇔ AgentDefinition tool lists ⇔ main allowed_tools for all 4 agents (no leak of discovery-only tools into main; Task present; options.tools == ["Task"]); hook-propagation verification for subagent tool calls (Codex r1 #2); ask-gate regression (discovery-side ask satisfies gate, Codex r1 #3); echo-strip and squeeze behavior on synthetic trajectories (size cap met, newest-K preserved, determinism, string items untouched); context-hook firing (once per threshold) from injected state; subagent-tagged AssistantMessage usage still accumulated; full non-integration suite green.

3. Stage 2 — open-weight wiring

3.1 Provider registry

New module (Pydantic BaseModel entries, no dataclasses): provider key → {base_url, api_format: "anthropic"|"openai", auth_env, default_context_window} + per-model window overrides. Initial entry: doublewordhttps://api.doubleword.ai/v1, openai, DOUBLEWORD_API_KEY, Kimi K2.6 window 256K (value confirmed at Stage 2 start). The 4 agents' is_anthropic hard-gate relaxes to anthropic-or-registry; unknown providers still raise with a clear message.

Single source of required env vars (Codex r1 #5): the registry feeds ALL key-handling sites — prereqs._required_api_keys, driver secret collection/forwarding to actors, acompletion_tracked user-sim kwargs injection, autopsy client construction, and sidecar config rendering.

3.2 SDK session env wiring

Per-run (not process-global) env on ClaudeAgentOptions: ANTHROPIC_BASE_URL = registry base (anthropic-format) or local sidecar URL (openai-format); auth token from the provider's env var. Credential hygiene (Codex r1 #6): for open-weight runs, CLAUDE_CODE_OAUTH_TOKEN, ANTHROPIC_API_KEY, and ANTHROPIC_AUTH_TOKEN are explicitly stripped from the SDK session env AND the local/Ray actor env (the existing cleanup only fires on the OAuth path). Tests cover local and Ray actor env. options.model = provider-native model id.

3.3 LiteLLM sidecar (openai-format providers, default for Doubleword)

One proxy per worker VM (postgres-style _ensure_* + lock pattern; local runs launch it on demand), exposing Anthropic /v1/messages, config rendered from the registry (model_listopenai/<id> + api_base + key). Pinned litellm version with the proxy extra added to pyproject. If the Doubleword deployment turns out to speak Anthropic natively: flip api_format in the registry, sidecar is skipped — no code change. (Public Doubleword docs only show OpenAI-compatible endpoints; no documented Anthropic /v1/messages support or Kimi K2.6 hosting as of 2026-06.)

3.4 user-sim and autopsy on open weights

  • user-sim: already litellm-routed; add registry-aware kwargs injection (api_base + key) in acompletion_tracked for registry providers. Unpriced models fall back to cost 0 with the existing warning.
  • autopsy: _build_anthropic_client becomes registry-aware — AsyncAnthropic(base_url=…, api_key=…) pointing at the anthropic-format endpoint or sidecar; model stays self.model (agent model). Squeeze budget uses the registry window.
  • Text-JSON fallback (deterministic, Codex r1 Explicit benchmark, relative-path sqlite fix, formula-aware encode ordering #8): if the response has no tool_use block — prefer a fenced ```json block; else the first balanced {…} object in the concatenated text; validate exactly once against the selected schema. Validation failure → `AutopsyError(kind="validation_error")` with text excerpt; no JSON candidate at all → `kind="missing_tool_use"`. Tests: multiple braces, malformed JSON, valid text JSON.

3.5 CLI / cloud

--agent-model doubleword/<id> (and user-sim equivalent) validated against the registry at submit; provider key forwarded to actors alongside the existing Anthropic-secret path; submit-time presence check for the required key. Auth flag (Codex r1 #4): --subscription-auth/--no-subscription-auth is currently required=True — it becomes required only for Anthropic agent models and is rejected with a clear error for registry providers; CLI tests cover doubleword/... submit without either flag. No image data-path changes.

3.6 Stage-2 tests + smoke

Unit: registry resolution, env construction (both formats), gate relaxation errors, autopsy client routing + JSON fallback paths (multi-brace/malformed/valid), user-sim kwargs injection, sidecar config rendering (no live proxy), prereqs/driver key forwarding from registry, credential stripping (local + Ray), CLI auth-flag conditionality. Smoke: 2-task cloud run (one a-interact, one one-shot) on Kimi K2.6 via Doubleword, then a fuller comparison batch against the Stage-1 Claude baseline.

Recorded risks

  • Task-tool/subagent quality on open-weight models (smoke validates).
  • LiteLLM /v1/messages translation fidelity (tool use; harness must tolerate missing thinking blocks).
  • Forced tool_choice honor on third-party endpoints (text-JSON fallback specced).
  • Kimi K2.6 availability/window on the Doubleword deployment — confirm at Stage 2 start.
  • Re-baseline deltas: the subagent harness may shift P1 on Claude itself; that's the point of the Stage-1 gate.

Codex plan review r1 — dispositions

All 8 findings folded: #1 tools=["Task"] + test; #2 hook-propagation verification test (fallback: hooks on AgentDefinition); #3 light remedy — gate unchanged, verbatim Q→A in handoff + gate-semantics regression test; #4 conditional --subscription-auth; #5 registry as single source of required env vars; #6 credential stripping for open-weight runs (local + Ray); #7 autopsy hygiene scoped to dict-shaped trajectories; #8 deterministic JSON extractor.

Review in Linear

@coderabbitai

coderabbitai Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR implements DEV-1581 R2 by replacing the SDK subagent split with two persistent in-process ClaudeSDKClient sessions (main + discovery) connected via an in-process ask_discovery tool backed by DiscoveryChannel. SLayer query storage becomes task-local via contextvars to prevent cross-task cache leakage. All four claude_sdk_otf*_v1 agents are rewired to use the new two-client run_main_with_discovery orchestration runtime.

Changes

DEV-1581 R2: Two-Client Discovery/Main Architecture

Layer / File(s) Summary
Task-local SLayer query storage and prefix-agnostic write filter
src/bird_interact_agents/agents/_query.py, src/bird_interact_agents/agents/_pre_encoded.py, tests/test_dev1581_query_storage_task_local.py, tests/test_dev1581_shared_engine_and_ctx.py, tests/test_dev1534_query_wrapper.py
Introduces _TaskQueryState + ContextVar (_query_state) replacing module-level globals; attach_storage() and _get_slayer_tool_fn() operate on per-task state. strip_write_tool_names strips any mcp__*__ prefix instead of matching only mcp__slayer__. Concurrency regression tests verify task-local isolation and shared-engine coherence.
DiscoveryChannel: warm bridge with single-flight, call cap, and lifecycle
src/bird_interact_agents/agents/claude_sdk/discovery_channel.py, tests/test_dev1581_discovery_channel.py, tests/test_dev1581_discovery_lifecycle.py
New DiscoveryChannel enforces single-flight via asyncio.Lock, caps calls returning DISCOVERY_CALL_CAP_MESSAGE, creates/finalizes a fresh UsageTracker per stream, converts stream errors to strings, implements timeout via asyncio.wait_for, and idempotent aclose(). discovery_session() and open_main_and_discovery() context managers guarantee teardown ordering (main closes before discovery).
In-process ask_discovery native and SLayer tool wrapper registry
src/bird_interact_agents/agents/claude_sdk/agent.py, tests/test_dev1581_ask_discovery_native.py
Adds _ask_discovery_impl (reads DiscoveryChannel from per-task _ctx, safe fallback when absent) and the ask_discovery MCP tool. Adds _ensure_slayer_storage_attached(), _make_slayer_native() factory (optionally normalizes write payloads, delegates to _get_slayer_tool_fn), and build_bird_interact_server() / native_tool_full_name() / resolve_native_tool() to build per-client bird-interact-tools MCP servers.
run_main_with_discovery orchestration runtime
src/bird_interact_agents/agents/claude_sdk/discovery_runtime.py, tests/test_dev1581_integration.py
New module opens discovery hermetic session first under AsyncExitStack (closes last), publishes DiscoveryChannel into _ctx["_discovery"], enters main session, primes with initial_query, runs the main receive loop updating trajectory, usage_tracker, and context_state per message with optional on_main_message callback.
Partition docs and main workflow note
src/bird_interact_agents/agents/claude_sdk/partition.py, tests/test_dev1581_main_workflow_note.py, tests/test_dev1555_partition_hooks.py
Expands partition module docstring for the R2 two-client enforcement model. build_main_workflow_note() replaces the "Subagent workflow" block with a "Discovery workflow (mandatory)" block directing the agent to use ask_discovery exclusively and not re-introspect after failed submits. Adds mechanical tests for tool-name substitution and placeholder validation.
All four claude_sdk_otf*_v1 agents rewired
src/bird_interact_agents/agents/claude_sdk_otf_v1/agent.py, src/bird_interact_agents/agents/claude_sdk_otf_raw_v1/agent.py, src/bird_interact_agents/agents/claude_sdk_otf_ainteract_v1/agent.py, src/bird_interact_agents/agents/claude_sdk_otf_ainteract_raw_v1/agent.py
Removes AgentDefinition subagent embedding, partition-deny hooks, and single-session loops. Each agent now defines MAIN_NATIVE_TOOL_NAMES/DISCOVERY_NATIVE_TOOL_NAMES, builds two MCP server dicts via build_bird_interact_server(), splits options into _build_main_options/_build_discovery_options, adds "_discovery": None to ctx_dict, and calls run_main_with_discovery().
Agent test suite updates
tests/test_claude_sdk_otf_v1_agent.py, tests/test_claude_sdk_otf_raw_v1_agent.py, tests/test_claude_sdk_otf_ainteract_v1_agent.py, tests/test_claude_sdk_otf_ainteract_raw_v1_agent.py, tests/test_dev1586_pre_encoded.py
Updates all four agent test suites to match the new two-client model: introspection tools absent from main, write tools via bird-interact-tools wrappers in main allowed_tools, ask_user on both partitions where applicable, PreToolUse hook counts reduced, opts.tools empty (no built-ins), and pre-encoded mode correctly strips write tools.
New partition-contract test suite
tests/test_dev1581_agent_wiring.py
Adds comprehensive two-client tool-partition contract validation across all four agents: introspection tools excluded from main, write tools excluded from discovery, ask_discovery required on main, no AgentDefinition references, and pre-encoded mode correctly strips write tools. Prior tests/test_dev1555_subagent_options.py (subagent-split test coverage) is removed.

Sequence Diagram(s)

sequenceDiagram
  rect rgba(70, 130, 180, 0.5)
    note over Agent,run_main_with_discovery: Orchestration setup
    Agent->>run_main_with_discovery: await run_main_with_discovery(main_opts, discovery_opts, initial_query)
    run_main_with_discovery->>hermetic_discovery: enter first (closes last)
    run_main_with_discovery->>DiscoveryChannel: new(discovery_client, accum)
    run_main_with_discovery->>_ctx: _ctx["_discovery"] = DiscoveryChannel
    run_main_with_discovery->>hermetic_main: enter second (closes first)
    run_main_with_discovery->>MainClient: query(initial_query)
  end
  rect rgba(100, 160, 100, 0.5)
    note over MainClient,DiscoveryChannel: Main message receive loop
    loop receive_response()
      MainClient-->>run_main_with_discovery: msg
      run_main_with_discovery->>trajectory: append {type, data}
      run_main_with_discovery->>usage_tracker: observe(msg)
      run_main_with_discovery->>context_state: update_context_tokens(msg)
    end
  end
  rect rgba(200, 120, 60, 0.5)
    note over MainClient,DiscoveryChannel: ask_discovery tool use (in-flight)
    MainClient->>ask_discovery_tool: ask_discovery(question)
    ask_discovery_tool->>_ctx: get _ctx["_discovery"]
    _ctx-->>ask_discovery_tool: DiscoveryChannel
    ask_discovery_tool->>DiscoveryChannel: ask(question) [single-flight lock]
    DiscoveryChannel->>DiscoveryClient: query/receive_response stream
    DiscoveryClient-->>DiscoveryChannel: text blocks
    DiscoveryChannel-->>ask_discovery_tool: answer string
    ask_discovery_tool-->>MainClient: result
  end
  run_main_with_discovery->>AsyncExitStack: exit
  AsyncExitStack->>hermetic_main: aclose() first
  AsyncExitStack->>hermetic_discovery: aclose() second
Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

  • MotleyAI/bird-agents#38: Introduced the generalized SLayer query wrappers and storage-based caching for extracted .fn handles in _query.py — the exact module this PR converts from module-level globals to per-task ContextVar state.
  • MotleyAI/bird-interact-agents#1: Both PRs modify src/bird_interact_agents/agents/claude_sdk/agent.py to change how SLayer-mode tools are exposed/wired (native tool list/wrappers vs MCP server delegation), so the main PR's SLayer tool-surface changes conflict at the same integration points.

Poem

🐇 Hop, hop! Two clients now run side by side,
Discovery warms up while Main takes a ride,
No more shared globals to trip on the trail,
Each task holds its storage behind its own veil,
ask_discovery bridges the gap with a thread,
The subagent is gone — long live async instead! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 32.37% 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 PR title clearly and concisely summarizes the main architectural change: replacing the DEV-1555 subagent split with a DEV-1581 R2 hard-partition design using two persistent clients and an in-process ask_discovery tool.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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: 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/agents/claude_sdk/discovery_runtime.py (1)

89-118: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Restore _ctx["_discovery"] on exit to avoid stale channel leakage.

Line 95 overwrites task context but never restores prior state. If this function exits early (or callers reuse the same context), later ask_discovery can hit a closed/wrong channel.

🔧 Proposed fix
@@
         channel = DiscoveryChannel(
             client=discovery_client,
             usage_accum=accum,
             model=model,
             max_calls=max_discovery_calls,
         )
-        _ctx["_discovery"] = channel
-
-        main_client = await stack.enter_async_context(
-            hermetic_claude_sdk_session(
-                model,
-                mcp_servers=main_mcp_servers,
-                build_options=build_main_options,
-            )
-        )
-
-        await main_client.query(initial_query)
-        seq = 0
-        async for msg in main_client.receive_response():
-            seq += 1
-            if on_main_message is not None:
-                on_main_message(msg, seq)
-            try:
-                _data: object = dataclasses.asdict(msg)
-            except Exception:  # noqa: BLE001
-                _data = str(msg)
-            trajectory.append({"type": str(type(msg).__name__), "data": _data})
-            usage_tracker.observe(msg)
-            update_context_tokens(context_state, msg)
+        prev_discovery = _ctx.get("_discovery")
+        _ctx["_discovery"] = channel
+        try:
+            main_client = await stack.enter_async_context(
+                hermetic_claude_sdk_session(
+                    model,
+                    mcp_servers=main_mcp_servers,
+                    build_options=build_main_options,
+                )
+            )
+
+            await main_client.query(initial_query)
+            seq = 0
+            async for msg in main_client.receive_response():
+                seq += 1
+                if on_main_message is not None:
+                    on_main_message(msg, seq)
+                try:
+                    _data: object = dataclasses.asdict(msg)
+                except Exception:  # noqa: BLE001
+                    _data = str(msg)
+                trajectory.append({"type": str(type(msg).__name__), "data": _data})
+                usage_tracker.observe(msg)
+                update_context_tokens(context_state, msg)
+        finally:
+            _ctx["_discovery"] = prev_discovery
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/bird_interact_agents/agents/claude_sdk/discovery_runtime.py` around lines
89 - 118, The code assigns the DiscoveryChannel to _ctx["_discovery"] without
saving or restoring the prior state, which can cause stale channel references if
the function exits early or the context is reused. Save the original value of
_ctx["_discovery"] before the assignment, then restore it using a try/finally
block (or similar cleanup mechanism) to ensure the prior state is restored when
the function exits. This should wrap the entire block starting from where the
channel is assigned through the main_client interaction and response handling.
🤖 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/discovery_channel.py`:
- Around line 102-133: The ask_discovery method holds the single-flight lock
while awaiting the client query and receiving the response stream without any
timeout protection, which can cause indefinite blocking if the stream stalls.
Wrap the self._client.query() call and the async for loop that iterates over the
response (the agen) with a timeout mechanism to ensure the lock is released
within a reasonable time frame, even if the external client I/O hangs
indefinitely.

In `@tests/test_dev1581_main_workflow_note.py`:
- Around line 41-43: The assertion in the cross-mode finalization check is
verifying against the wrong tool name. When query_mode equals "slayer", the
other_submit variable should be set to the OTHER mode's finalization tool (which
is "submit_sql"), not "execute_sql" which is a verify tool. Update the ternary
condition that sets other_submit to correctly map "slayer" mode to "submit_sql"
(and the alternative mode to the appropriate finalization tool), ensuring the
assertion properly detects any cross-mode bleed of finalization tools.

---

Outside diff comments:
In `@src/bird_interact_agents/agents/claude_sdk/discovery_runtime.py`:
- Around line 89-118: The code assigns the DiscoveryChannel to
_ctx["_discovery"] without saving or restoring the prior state, which can cause
stale channel references if the function exits early or the context is reused.
Save the original value of _ctx["_discovery"] before the assignment, then
restore it using a try/finally block (or similar cleanup mechanism) to ensure
the prior state is restored when the function exits. This should wrap the entire
block starting from where the channel is assigned through the main_client
interaction and response handling.
🪄 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: 310964ff-938a-4cfb-a415-fd70b41ab5d4

📥 Commits

Reviewing files that changed from the base of the PR and between 82a5651 and 79fa269.

📒 Files selected for processing (26)
  • src/bird_interact_agents/agents/_pre_encoded.py
  • src/bird_interact_agents/agents/_query.py
  • src/bird_interact_agents/agents/claude_sdk/agent.py
  • src/bird_interact_agents/agents/claude_sdk/discovery_channel.py
  • src/bird_interact_agents/agents/claude_sdk/discovery_runtime.py
  • src/bird_interact_agents/agents/claude_sdk/partition.py
  • src/bird_interact_agents/agents/claude_sdk_otf_ainteract_raw_v1/agent.py
  • src/bird_interact_agents/agents/claude_sdk_otf_ainteract_v1/agent.py
  • src/bird_interact_agents/agents/claude_sdk_otf_raw_v1/agent.py
  • src/bird_interact_agents/agents/claude_sdk_otf_v1/agent.py
  • tests/test_claude_sdk_otf_ainteract_raw_v1_agent.py
  • tests/test_claude_sdk_otf_ainteract_v1_agent.py
  • tests/test_claude_sdk_otf_raw_v1_agent.py
  • tests/test_claude_sdk_otf_v1_agent.py
  • tests/test_dev1534_query_wrapper.py
  • tests/test_dev1555_partition_hooks.py
  • tests/test_dev1555_subagent_options.py
  • tests/test_dev1581_agent_wiring.py
  • tests/test_dev1581_ask_discovery_native.py
  • tests/test_dev1581_discovery_channel.py
  • tests/test_dev1581_discovery_lifecycle.py
  • tests/test_dev1581_integration.py
  • tests/test_dev1581_main_workflow_note.py
  • tests/test_dev1581_query_storage_task_local.py
  • tests/test_dev1581_shared_engine_and_ctx.py
  • tests/test_dev1586_pre_encoded.py
💤 Files with no reviewable changes (1)
  • tests/test_dev1555_subagent_options.py

Comment thread src/bird_interact_agents/agents/claude_sdk/discovery_channel.py
Comment thread tests/test_dev1581_main_workflow_note.py Outdated
ZmeiGorynych and others added 6 commits June 22, 2026 18:51
…x restore)

- ainteract agents (slayer + raw): register the shared post_ask_counter on
  the DISCOVERY client so a discovery-side ask_user satisfies the main
  submit gate — restores the origin-agnostic ask-user contract the
  subagent split had (Codex).
- DiscoveryChannel.ask: wrap query+drain in asyncio.wait_for (default 600s)
  so a stalled discovery stream can't hold the single-flight lock forever
  (CodeRabbit).
- run_main_with_discovery: save/restore _ctx["_discovery"] around the main
  loop so a closed channel never lingers in a reused context (CodeRabbit).
- test_dev1581_main_workflow_note: the slayer cross-mode assertion checked
  execute_sql (the other mode's VERIFY tool) instead of submit_sql (its
  finalization tool); fixed. Did not also assert the other verify tool is
  absent — raw's verify tool `query` appears as a common word in the prose
  (CodeRabbit, partial).

Codex's discovery-usage-double-count finding was verified INVALID via a live
2-query spike: ResultMessage.usage (tokens) is per-query, not cumulative
(only total_cost_usd/num_turns are session-level, and SdkUsageTracker uses
neither). Full non-integration suite green.

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

run_main_with_discovery threaded the DEV-1561 otf_timer enter_cm_factory into
the discovery session only, leaving the main client's __aenter__ untimed —
reintroducing the silent main-client startup hang the instrumentation exists
to catch. Apply it to both hermetic sessions so each CLI subprocess spawn
emits its own run_task.sdk_client_enter span. The existing timing tests still
pass: success asserts a non-empty .done set (now 2), and the enter-failure
case fails on the first (discovery) spawn so it still emits exactly one
.start + .error.

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

The R2 discovery client is a separate session, so it no longer inherits
main's hooks the way the old subagent did — its tool calls ran outside the
per-task wall-clock guardrails, and while main is blocked inside
ask_discovery its own wall-clock hook can't fire, so discovery could overrun
the task budget (Codex PR #56). Register the shared wall_clock_deny
(PreToolUse) + wall_clock_warning (PostToolUse) on all four agents' discovery
clients, against the SAME context_state as main so the budget is global to
the task. Context-budget stays main-only (discovery's context is bounded by
DISCOVERY_MAX_TURNS per ask + the DiscoveryChannel call cap). Full suite green.

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

run_main_with_discovery owns the main query()/receive loop, so the a-interact
agents had hard-coded run_task.sdk_first_message elapsed_s="0.000" and lost
the run_task.sdk_first_query span — regressing the DEV-1561 SDK
startup/first-response latency diagnostics. The runtime now captures the
monotonic time right after query() returns and passes it to on_main_message
(3rd arg) so the callback reports the true first-response latency, and wraps
the query() call in an optional query_cm_factory (the agents pass
otf_timer("run_task.sdk_first_query")). Full suite green.

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

The R2 main loop no longer holds the introspection tools (slayer
search/inspect_model/models_summary/list_datasources; raw get_schema/
get_all_column_meanings) — they moved to the discovery client — but the base
agent prompts still told main to call them directly, recreating the
prompt/tool-surface dissonance DEV-1581 set out to remove.

Scoped to the two-stage (v1) flavors only:
- build_main_workflow_note (v1-only) gains a mode-aware OVERRIDE clause:
  wherever any guidance names an introspection tool, ask ask_discovery for
  that information instead (KB definitions stay on main via
  get_knowledge_definition). This single clause also covers the introspection
  references baked into the SHARED fragments (host-discovery playbook,
  decompose discipline) and the shared pre-encoded prompt WITHOUT editing
  them — so the v0 single-agent flavors and pydantic adapters, where direct
  introspection IS correct, are untouched.
- The four v1 prompts' primary tool instructions now route schema/sample-
  value/entity discovery through ask_discovery natively (slayer keeps
  create/edit/query/help + KB; raw keeps execute_sql/get_column_meaning/KB).
- Re-baselined the SLAYER_OTF_ONE_SHOT / SLAYER_OTF_AINTERACT golden hashes.

Full suite green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Discovery prompt had a leftover contradiction from the R2 reword: it said
  "for narrow follow-ups, answer just what was asked" then immediately
  "Produce a handoff report with EXACTLY these sections", which would push
  every warm ask_discovery reply back into the verbose full-report shape.
  Scope the EXACTLY-these-sections mandate to the FIRST broad request and tell
  follow-ups to answer narrowly (citing a section by name).
- The slayer bridge clause listed `list_datasources` among the
  discovery-owned tools, but it was retired (one datasource per task) and is
  registered on neither client. Drop it from the named list; the general
  "introspection is NOT on your tool surface — ask ask_discovery" statement
  still reroutes any stale reference (e.g. in the shared pre-encoded prompt).

Both are minor prompt-wording fixes; full suite green.

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