fix(batch): gate real Batch API on declared batch_endpoint_supported - #1021
fix(batch): gate real Batch API on declared batch_endpoint_supported#1021seonghobae wants to merge 7 commits into
Conversation
TaskOrchestrator.batch_route() selects a worker agent for each prompt via
_select_agent(), which filters only on chat-capability tags -- it can return
any configured chat-capable provider (Anthropic-shaped, NVIDIA NIM,
OpenRouter, a self-hosted OpenAI-compatible gateway, ...), many of which do
not implement OpenAI's real async Batch API (/files, /batches,
/files/{id}/content) at all. ModelClient.batch_chat()'s only gate before
routing into that real endpoint was is_chat_compatible_model_id(), a model-id
shape heuristic that says nothing about provider Batch API support -- so an
unsupported provider's real HTTP call 404d and failed the entire batch group,
with no fallback.
Add ModelAgent.batch_endpoint_supported: bool | None (default None), mirroring
the existing reasoning_effort_supported fail-closed tri-state field exactly:
same __post_init__ validation, to_config/from_dict round-trip, and agent_pool
SQLite persistence/migration. batch_chat() now only takes the real Batch API
path when batch_endpoint_supported is True; every other remote agent falls
back to the same per-item emulation _local_batch_chat already performs for
local providers, aggregating into the identical result shape instead of
either misrouting or hard-failing.
pg-llm-batch (the real Batch API client contextual-orchestrator vendors) only
validates the URL *path* submitted inside an already-created job -- it has no
per-provider capability registry and, per its own module docstring, expects
the orchestrator to know a target supports the Batch API before calling it.
This closes that gap the same way this repo already closes it for
reasoning_effort support, rather than inventing a new mechanism.
Tests extend tests/test_batch_api.py's real-HTTP fake Batch/chat provider:
- a batch_endpoint_supported=True agent still takes the real Batch API path
- an unproven agent (None or False) falls back to emulation and returns a
correctly aggregated result, via an actual per-request HTTP round trip
through /chat/completions (not a stubbed return value) -- proving the real
Batch API was never touched (zero /batches posts, no /files upload)
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 37 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (10)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…pability-gate Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
Rebased onto current
|
…agents batch_chat() now falls closed to per-item emulation unless ModelAgent.batch_endpoint_supported is explicitly True. Two boundary tests constructed remote HTTPS agents to exercise the real Batch API path directly and broke under the new fail-closed default; declare batch_endpoint_supported=True on those agents to keep exercising the real-Batch-API code path they were written for. Verified: full suite green for this change — 3350 passed, 2 skipped, 3 failed (test_admin_contract.py::test_model_group_mutations_refresh_audit_events, test_psychometric_routing.py::test_fast_mlsirm_fit_uses_judge_acceptance_item_for_context_score, test_spend_analytics.py::test_exact_output_without_prompt_usage_is_explicitly_unavailable), all three pre-existing and identical on origin/main, untouched by this branch's diff. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
Fixed: two tests broke under the fail-closed default
Fix ( Verification evidenceFull suite, run twice (once via a stray background process from an earlier turn that I killed as stale, once as a fresh blocking run against the pushed head) in the PR worktree at Targeted re-run of every file touched by this PR or by the test fix, in isolation: The 3 failures in the full run are pre-existing and out of scope — confirmed by
Pushed to 🤖 Generated with Claude Code https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4 Generated by Claude Code |
Addresses Devin review findings on the item-16/17 scheduler target-list drift entry and the item-32 batch-endpoint entry: - "Noema evidence changes ownership": the new "scheduler target-list drift" section was inserted in the middle of the pre-existing "Noema single-request model-control ownership -- PR #1672" entry -- between its "900-second clarification" paragraph and its own "Evidence / acceptance" closing paragraph -- so the closing paragraph (about Noema's retry/deadline fix) ended up trailing the unrelated scheduler section, reading as if it were that section's acceptance evidence. Moved the scheduler section to come after the Noema section's actual closing paragraph, restoring both entries' internal coherence and making the scheduler section the file's true final entry again. - "Structural closure precedes artifacts": the "Structural fix" paragraph described .github#1747 (the mirror file + contract test + doctoring record) in the past tense, as delivered repository state, but that PR is open and unmerged and none of those files exist on this branch or main. Reworded to say so explicitly, in the same "pending merge" framing this entry's own item-32 write-up already uses correctly. - "Cross-repository references are not linkable": bare `#1021` / path-style `contextual-orchestrator/pull/1021` references -> the binding-convention form `ContextualWisdomLab/contextual-orchestrator#1021`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
…n criteria Devin Review findings on PR #1730: - Finding A (comment 3913954439, item 30 polling head-change guard): Devin was wrong -- verified the in-loop guard at opencode-review.yml:467-470 already exits 0, pinned by tests/test_opencode_poll_self_retirement.py. No doc/workflow change made. - Finding B (comment 3913954605, cross-repo #1021 refs): already fixed by 3476a56 before this comment landed -- all four #1021 refs are already fully qualified ContextualWisdomLab/contextual-orchestrator#1021. No change made. - Finding C1 (comment 3914267880, multimodal free-classification mechanism): real gap, fixed. The doc's Correction attributed the exclusion to #933's serving gate, but re-traced `_unit_prices_are_free` to 51fc34b (not ba5e00c) and confirmed live + against PR #1028's own head (aabd69a) that all 8 named models fail earlier, at `_row_is_free` (no unit_pricing/is_free key on OpenRouter rows), making #1028's general_free_serving_candidates exemption inert for every model it targets. - Finding C2 (comment 3914325501, re-open criteria too narrow): real gap, fixed. Added re-open trigger (c) for the fix failing to actually admit the named models -- the exact silently-inactive-gate failure mode Finding C1 found, which the prior two triggers (text-only regression, unreliable tool-call signal) could not have caught. Both mechanism claims independently re-verified against the vendored contextual-orchestrator checkout (git log -S, PR #1028 REST status) before editing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Autonomous loop note: Generated by Claude Code |
Preserve the fail-closed Batch endpoint capability contract while merging the latest protected main. Clarify that unproven providers use explicit per-item chat emulation and ordinary chat stays synchronous. Signed-off-by: Seongho Bae <me@seonghobae.me>
목표 #32 exact-head 재검증
근본 원인은 chat 호환 모델 식별자를 provider Batch API 지원 증거로 오인한 것이었습니다. 이 PR은 OpenAI 공식 계약도 Batch를 모델명 기반 자동 전환으로 정의하지 않습니다.
검증:
README도 실제 Batch와 명시적 emulation 경계를 정확히 설명하도록 맞췄습니다. 제품 PR이므로 admin bypass 없이 기존 auto-merge와 보호 규칙을 따릅니다. @coderabbitai review |
|
|
Root cause
TaskOrchestrator.batch_route()selects a worker agent per prompt via_select_agent(), whichfilters only on chat-capability tags (
_is_general_chat_agent()+ callerrequired_tags/prefer_tags, neither of whichbatch_routesupplies) — it can return any configuredchat-capable agent: Anthropic-shaped, NVIDIA NIM, OpenRouter (proxying dozens of providers), Azure,
or a self-hosted OpenAI-compatible gateway. Many of these do not implement OpenAI's real async Batch
API (
/files,/batches,/files/{id}/content) at all.ModelClient.batch_chat()'s only pre-flight gate before routing straight into that real endpoint wasis_chat_compatible_model_id(agent.model)— confirmed (chat_capability.py) to be a pure model-idshape heuristic (excludes obviously-non-chat ids like embedding/vision/audio models); it says
nothing about whether the provider implements a Batch API. Any agent that was neither
mock://nora recognized local provider URL fell into
_batch_run()'s real HTTP upload → create-batch → poll →download flow unconditionally. On a 404 (provider has no Batch API), the whole prompt group failed
hard —
_local_batch_chat()already implements exactly the batch-emulation fallback needed (loop eachrequest through
chat(), aggregate), but it was wired only to the local-provider branch, never as afallback for an unproven remote provider.
Grepped the codebase for any existing "does this model/provider support Batch API" signal
(
supports_batch,batch_capable,batch_endpoint, etc.) — zero hits.pg-llm-batch(the realBatch API client this repo vendors) only validates the URL path inside an already-created job; it
has no per-provider capability registry either, and its own module docstring assigns that
responsibility to "the orchestrator, not the caller."
Fix
Add
ModelAgent.batch_endpoint_supported: bool | None = None, mirroring the existingreasoning_effort_supportedtri-state field exactly (this repo's own established pattern for"provider support is unproven, fail closed unless explicitly declared" protocol-capability gaps) —
same
__post_init__validation,to_config/from_dictround-trip, andagent_poolSQLitepersistence/migration (
ALTER TABLE ... ADD COLUMN ... CHECK (... IS NULL OR ... IN (0,1)), copiedverbatim from the
reasoning_effort_supportedmigration).ModelClient.batch_chat()now only takes the real Batch API path whenagent.batch_endpoint_supported is True. Every other remote agent (None/False, the default) fallsback to the same per-item emulation
_local_batch_chat()already performs for local providers,aggregating into the identical
{custom_id: {"content", "usage"}}result shape — no misrouting into a404, no hard failure of the whole batch, and no new heuristic or timeout introduced.
_batch_run(),is_chat_compatible_model_id(), andbatch_route()'s group-by-agent persistencelogic are untouched — the fix is entirely which branch
batch_chat()selects.Tests
Extended
tests/test_batch_api.py's real-HTTP fake provider (already used for_batch_run'smultipart-upload/poll/parse flow) with a
/chat/completionshandler, then added:test_batch_chat_routes_batch_capable_agent_to_real_batch_endpoint—batch_endpoint_supported=Truestill takes the real Batch API path (one
/batchesPOST, zero/chat/completionscalls).test_batch_chat_falls_back_to_emulation_when_batch_endpoint_unproven— for bothNoneandFalse,batch_chat()returns a correctly aggregated result via an actual per-request HTTP roundtrip through
/chat/completions(not a stubbed return value — asserts real per-item call count),and proves the real Batch API was never touched (zero
/batchesPOSTs, no/filesupload).🤖 Generated with Claude Code