feat(routing): add stateless candidate controls - #983
Conversation
|
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:
📝 WalkthroughWalkthrough요청별 Changes요청별 후보 라우팅
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🟠 High · up to The PR adds caller-controlled candidate pinning, exclusions, and routing provenance to public request paths. At the current head, valid requests with more than 32 exclusions can still be rejected, ambiguous execution history can produce incorrect serving identity, and a write-enabled workflow can mutate and push source after review, creating contract failures, misleading routing evidence, and unreviewed behavior changes; the PR is not ready to merge. Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant TaskOrchestrator
participant CostRoutingCoordinator
participant Provider
Client->>Server: routing controls 포함 요청
Server->>TaskOrchestrator: 후보 정책 검증
Server->>CostRoutingCoordinator: 검증된 routing 전달
CostRoutingCoordinator->>TaskOrchestrator: 후보 정책 적용
TaskOrchestrator->>Provider: 허용된 후보 호출
Provider-->>TaskOrchestrator: 응답과 trace 반환
TaskOrchestrator-->>Server: routing evidence 반환
Server-->>Client: orchestration.routing 포함 응답
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 44.44% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 99 functions across 11 files. (3 skipped: 2 unsupported, 1 too large.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
|
Exact-head remediation evidence for
Hosted exact-head gates are pending. |
|
Exact environment classification for the two local full-suite import failures at
Conclusion: this is local ambient dependency drift/namespace contamination, not a PR-scoped or current-main source defect. No dependency declaration, lock, test suppression, or source was changed. Reproduce with the repository contract ( |
Signed-off-by: Seongho Bae <me@seonghobae.me>
Signed-off-by: Seongho Bae <me@seonghobae.me>
Signed-off-by: Seongho Bae <me@seonghobae.me>
Signed-off-by: Seongho Bae <me@seonghobae.me>
Signed-off-by: Seongho Bae <me@seonghobae.me>
|
Cross-PR integration contract: routing identity is provider-neutral |
|
@coderabbitai review |
|
…tration fields Two more fixes to the candidate routing evidence work (#983), both from a fresh Devin review round on round 9's push: 1. "Provider fields forge routing evidence" -- CostRoutingCoordinator. complete()'s provider_request passthrough branch unconditionally republished a `_candidate_routing` field popped off the raw provider response as gateway-computed `orchestration.routing` evidence. TaskOrchestrator.proxy_completion() only ever sets that key itself when an active candidate control made _candidate_routing_evidence return non-None -- so with no active control, observing that key can only mean it arrived already-present on the provider's own (untrusted) response body. A coincidentally- or adversarially-named provider field could therefore forge fake served_candidate_id/attempted_candidate_ids into an ordinary response, violating the documented "present only when the request supplied routing controls" contract. 2. "Provider metadata crashes tool responses" -- server.py's single-agent tool-loop passthrough had the identical trust gap, plus a second bug: `result.setdefault("orchestration", {})["routing"] = evidence` assumes a pre-existing "orchestration" field (if the provider happened to return one) is a mapping. A provider response with a non-dict "orchestration" field (a string, list, ...) crashed this line with `TypeError: 'str' object does not support item assignment` *after* a successful inference call, turning a working response into a 500. Both call sites shared the same underlying question -- "was a candidate control genuinely active for this request" -- so this adds TaskOrchestrator._has_active_candidate_controls(routing) as the single source of truth (matching candidate_routing_policy's own no-op condition: key presence for candidate_id, presence-and-non-empty for exclude_candidate_ids) and uses it in three places: - cost_router.py's has_candidate_controls (refactored to call it, replacing the equivalent inline logic added in round 9) - cost_router.py's provider_request branch, gating the `_candidate_routing` republish - server.py's proxy_tool_request, gating the republish AND checking isinstance(orchestration, dict) before merging into it (falling back to a fresh dict, matching the OpenAPI schema's own "orchestration is an object" contract, when the provider's field isn't one) Two new regression tests (one coordinator-level, one full HTTP tool-loop request) inject a forged `_candidate_routing` field and a non-mapping `orchestration` field via a custom test client and assert: no crash, and the forged evidence never reaches the response. Verified via git stash A/B: both fail against the pre-fix code (the forged evidence leaks through; the HTTP request 500s with the exact TypeError Devin predicted) and pass with the fix. Pulled in four concurrent commits from a separate, already in-flight automated repair (`source-fix-983-no-heuristic-candidate-controls`, addressing distinct Devin findings about a hardcoded 32-ID exclusion cap and served_candidate_id's text-match fallback) via a clean fast-forward merge -- entirely disjoint files from this change, no conflicts. That repair's own code-changing commit had not landed yet as of this push, so its regression file (tests/test_candidate_routing_no_heuristic_limits.py) still has 2 known, pre-existing, not-yet-fixed failures unrelated to this PR; not this change's concern to fix (a separate automated workflow owns that repair). Full suite: 3349 passed, 5 known pre-existing failures (unchanged from round 9), 2 skipped -- run before the fast-forward merge landed, so it does not yet include the 3 not-yet-fixed no-heuristics regression tests (2 fail, 1 passes) tracked separately above. interrogate: 100% docstring coverage. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
Round 10: never trust provider-supplied
|
Applies scripts/source_fix_983_no_heuristic_candidate_controls.py's documented repair directly -- its GitHub Actions workflow had been queued 4.5+ hours with zero progress due to severe org-wide Actions capacity congestion. Removes the unsupported 32-ID exclude_candidate_ids cardinality ceiling (normal authenticated request-size controls remain the resource boundary) and the output-equality/trace-position fallback for resolving served_candidate_id (neither had RouteLLM, FrugalGPT, API-standard, or measured-deployment support). Serving identity is now reported only from an exact answering_step_id row or explicit served_agent_id provenance; historical records with neither remain attempt provenance but omit served_candidate_id rather than guessing. The pre-written script's two orchestrator.py replace_once calls had gone ambiguous (matched 2 occurrences instead of 1) because an earlier round on this same branch had independently added a second, differently-conditioned raise using identical wording -- applied both by hand instead, anchoring on full surrounding context. Extending the fix to route_once()/stream_route()'s own trace-step construction was necessary to avoid a regression: those single-worker paths never set served_agent_id (only route_once's cross-endpoint failover case did), so the stricter evidence resolution would have silently stopped reporting served_candidate_id for the overwhelmingly common single-candidate route request. Both now record served_agent_id explicitly (equal to agent_id when there was no internal failover), which is a real fact being recorded, not an inference -- so the evidence function never needs to fall back to bare agent_id/trace position. Verified against tests/test_candidate_routing_no_heuristic_limits.py, tests/test_candidate_routing_controls.py (54 total), and tests/test_api_contract.py. Removes the now-completed one-shot repair machinery per convention. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
|
Completed this PR's stuck Applied
Verified: Generated by Claude Code |
… tracking
A prior no-heuristics repair for candidate-routing evidence made route_once
and stream_route stamp served_agent_id on every trace row unconditionally,
so _candidate_routing_evidence would have an explicit serving fact even
when the serving agent was unchanged. That broke a separate, pre-existing
regression guard in test_provider_reliability.py and
test_tool_execution_fallback.py: the ordinary, no-candidate-policy path
must never carry served_agent_id/failover metadata for an unchanged
serving agent ("the default mock path must behave exactly as before").
Root-caused by diffing a clean origin/main worktree (passing) against this
branch's head (failing) to rule out full-suite test pollution before
concluding it was a real regression from the served_agent_id change.
Fix: only stamp served_agent_id unconditionally while request-local
candidate-attempt tracking is actually active (inside a
candidate_routing_policy scope, via
_REQUEST_ATTEMPTED_CANDIDATE_IDS.get() is not None). The ordinary path's
trace-row shape is unchanged; failover still always stamps it regardless
of policy state.
Full local suite: 3355 passed, 2 pre-existing sandbox-only failures
(fast_mlsirm unavailable in this sandbox's proxy policy; a known local
tokenizer-usage-source artifact in test_spend_analytics that passes on
real CI) — both unrelated and pre-existing.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
Follow-up: fixed a regression introduced by the no-heuristics repair (commit
|
|
@jules Fresh exact-head verification found a remaining source/contract contradiction on Current RED/GREEN acceptance:
Do not mark this GREEN until the live exact head itself, not a bot-plan or resolved-review annotation, contains the schema correction and regression. |
The runtime's own repository-authored 32-ID exclusion-count cutoff was already removed as unsupported (no identified mathematical, standards, experimental, or research basis for that specific number), but the published OpenAPI schema for CandidateRoutingControls.exclude_candidate_ids still declared maxItems: 32. Generated/OpenAPI clients therefore still rejected exclusion lists the runtime intentionally accepts, making the schema false at source -- the PR body, CHANGELOG, ADR direction, and a prior Devin review resolution all claimed the cutoff was gone, but the schema, a second independent publication surface for the same invariant, still enforced it. Removes maxItems: 32 with no replacement cardinality heuristic; uniqueItems, lexical ID constraints, and normal authenticated request-size bounds are unchanged. Adds a RED-before/GREEN-after regression validating a 64-ID exclusion list against the schema (confirmed it fails against the old schema, passes against the corrected one). Verified the runtime validator (server.py's _validate_routing) has no other hidden count-based cutoff on this field. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
|
Repaired in
Full relevant suite ( Generated by Claude Code |
…ate-controls Bring PR #983 (stateless candidate routing controls) up to date with main, which had advanced past the PR's stale base sha. Resolved one real conflict in fuzz/targets.py: both this PR and main (#917, governed rater observation) independently added a new "11." item to the module docstring's numbered list of fuzzed surfaces (this PR's server._validate_routing, main's rater_observation.RaterInvocation). Kept both, renumbered sequentially as 11 and 12 -- purely additive documentation, no logic conflict. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
|
Rebased this PR onto current CI status before merge: all checks green (CodeRabbit, Devin Review completed). Merge:
All other files ( Verification (Python 3.12 venv,
Pushed directly to Generated by Claude Code |
| elif tracked_attempts != []: | ||
| explicit_served = [ | ||
| value | ||
| for row in rows | ||
| if isinstance(row, Mapping) | ||
| for value in [row.get("served_agent_id")] | ||
| if isinstance(value, str) and value | ||
| ] | ||
| distinct_served = tuple(dict.fromkeys(explicit_served)) | ||
| if len(distinct_served) == 1: | ||
| served = distinct_served[0] |
There was a problem hiding this comment.
🟡 Cache hits misreport serving candidate
When auto triage runs before a response-cache hit, _candidate_routing_evidence reuses the cached trace's serving identity. The response falsely attributes cached output to a provider.
Prompt for agents
Prevent candidate routing evidence from deriving served_candidate_id from a cached trace. In contextual_orchestrator/orchestrator.py, _candidate_routing_evidence currently suppresses explicit serving identity only when tracked_attempts equals an empty list. An auto-mode request can make a fresh triage provider call, leaving tracked_attempts nonempty, and then hit a durable or still-live response cache whose trace belongs to the original request. Use the result's cache_status to ensure cache hits never publish served_candidate_id from historical trace rows, while retaining current attempted_candidate_ids such as a real triage call. Add coverage where the response cache is warm but the triage cache is cold or cleared.
Was this helpful? React with 👍 or 👎 to provide feedback.
…zz target Devin's review found that fuzz/targets.py's exercise_request_body still asserts `len(excluded) <= 32` on the real server._validate_routing() output, even though this PR's earlier "no-heuristics correction" removed that exact cardinality ceiling from both the OpenAPI schema and the runtime Python validator (there is now no repository-authored candidate-count cutoff, only the normal authenticated request-size boundary). The fuzz target was left asserting an invariant the validator it drives no longer enforces, so a legitimately valid >32-item exclusion list would report as a fuzzing false positive. Also updated the module docstring's target #11 summary, which still said "bounded", to match. RED confirmed: reverted the fuzz/targets.py change and reran the new regression -- AssertionError at the removed `assert len(excluded) <= 32` line, exactly as Devin described. GREEN: new deterministic regression (40 unique exclude_candidate_ids) passes; full tests/fuzz/test_fuzz_properties.py suite -- 20 passed. interrogate on fuzz/targets.py: 100%. git diff --check: clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
Fixed one confirmed-stale invariant in the fuzz target; two other open findings verified as already-moot or too delicate to touch blindTriaged this PR's ~62 review threads. Most substantive findings were already resolved through this session's earlier rounds. Checked the remaining open ones: Fixed (commit
Verified already moot: the Left open, not attempted: "Cache hits misreport serving candidate" (Devin, current head, Generated by Claude Code |
…ndidate Merged current main to pick up main's test_admin_contract.py `import json` fix (PR #1035) that this PR's stale base predated -- clean, no conflicts. Hosted CI's "Full unit and contract suite" (run 33692781067) then showed two of this PR's own new tests failing with an extra `worker_only` call in client.calls: test_http_auto_preflight_accepts_worker_only_pin_when_free_model_ always_routes and test_coordinator_auto_route_only_pin_succeeds_for_free_model (tests/test_candidate_routing_controls.py). Neither failed in any earlier local round because this sandbox's blocked fast-mlsirm GitHub-archive download always short-circuits _model_judge_verification to its fail-closed return before a judge is ever selected -- masking a real, pre-existing (present unchanged at merge-base 212ff43, predates #983) selection bug that only a hosted run with fast-mlsirm actually importable can exercise. Root cause: _ranked_agents deliberately still returns role-ineligible members (appended after every eligible one, per its own docstring), so a caller wanting only role-eligible candidates must re-apply `role not in agent.provider_exclusions` itself, exactly as _plan_generated/_parse_workflow_plan already do. _model_judge_verification's judge-selection next(...) was missing that filter, so with a single-candidate pool excluded from "verifier" (#983's own new orchestrator/free worker-only provable-route fixture), it picked that ineligible agent as judge anyway instead of failing closed --an extra, unrequested live call. _invoke's own failover path already enforces this same exclusion for a *backup* judge (test_fast_mlsirm_judge_failover_honors_verifier_exclusions); this closes the identical gap for the *primary* selection. Fix: add `if "verifier" not in agent.provider_exclusions` to the judge-selection generator in _model_judge_verification. Verification: - RED-before/GREEN-after: new regression test_model_judge_never_selects_a_verifier_excluded_sole_candidate (tests/test_model_judge.py) fails on the pre-fix code (records the excluded worker_only agent as judge) and passes after (next(...) raises StopIteration, caught by the existing fail-closed handler, judge never constructed). - test_model_judge.py + test_candidate_routing_controls.py + test_candidate_routing_no_heuristic_limits.py + test_api_contract.py + test_admin_contract.py: 98 passed. - Full local suite (Python 3.12, matching CI's `uv run` toolchain): 3441 passed, 2 pre-existing sandbox-only failures unrelated to this change and already documented in docs/product-technical-gap-baseline.md's 2026-09-02 entry (fast_mlsirm unavailable; test_spend_analytics's local-tokenizer artifact -- same missing-fast-mlsirm mechanism). - interrogate on orchestrator.py: 100%. - git diff --check: clean. CHANGELOG.md and docs/product-technical-gap-baseline.md updated with a dated entry per this PR's established practice. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
Merged stale
|
_compute_triage_verdict's empty free-only ranking fallback was gated behind `not _REQUEST_ZDR_ONLY.get()`, so a zdr_only request that pins a paid ZDR-eligible candidate via routing.candidate_id always hit an empty free-only pool (the pin restricts every candidate list to that one agent, and a paid agent never satisfies free_only) and then skipped the fallback entirely just because ZDR was active -- silently returning False (route, not conduct) with zero live triage call and zero routing evidence. The fallback's own per-agent filter already enforces ZDR eligibility (_zdr_agent_allowed) and the active pin/exclusion (_request_candidate_allowed), so gating the whole fallback build on "not zdr_only" was redundant, not protective. Removing that gate lets the fallback run whenever the free-only pool is empty; it narrows itself to ZDR-eligible agents (the ZDR-eligible pinned one, in this shape) with zero risk of contacting a non-ZDR provider. New regression test in tests/test_candidate_routing_controls.py posts an auto-mode request with zdr_only=true and a paid ZDR candidate pin, confirming triage genuinely calls the pinned candidate and the resulting conduct verdict is honored (a full multi-step workflow runs instead of the one-call route path). Verified RED against the pre-fix code (git stash A/B) and GREEN with the fix. Devin Review, PR #983: "ZDR pins skip workflow triage". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
Fixed: "ZDR pins skip workflow triage" — confirmed real, fixed in
|
|
Adjudication evidence (host 1 session, 2026-09-06 KST; full report with commands in #1080). Nothing here closes, flips, or retargets anything — the decision is the opener's. Semantic duplicate mechanism with #1032. This PR adds |
Summary
routing.candidate_idandrouting.exclude_candidate_idscontrols for virtual chat and Responses requests/v1/modelsContract
candidate_idis an exact private agent IDexclude_candidate_idscontains unique non-empty private agent IDs; there is no repository-authored candidate-count cutoffinvalid_routinganswering_step_idor explicitserved_agent_idprovenance; missing identity fails closed instead of being inferred from output equality or trace orderNo-heuristics correction — 2026-09-02
Fresh RCA found two decision-affecting rules that were not established by RouteLLM, FrugalGPT, an API standard, or measured deployment evidence: the fixed 32-ID exclusion ceiling and the historical serving-candidate fallback that guessed identity from matching output text / trace position. Both violate the repository's no-heuristics contract.
RED regression commit
ef5ae0143a2b19eb32ad3e826a6c21755ba7451dproves that more than 32 exclusions must remain admissible under the normal authenticated request-size boundary and that a historical trace lacking explicit serving identity must omitserved_candidate_id. Exact-guarded one-shot repair machinery was added in3d045c6ee416821e23c6f27963921e5d591e136dand6d93c16369b967d559e556f1170a1acd561ce1e5; canonical trigger head is5166349dea1be54d53edc3e33ce185a90b159163. Source-fix run33585788325is queued and is not GREEN evidence.The repair removes the numeric exclusion limit from both HTTP and Python-API validation; normal authenticated request-body controls remain the resource boundary. It also removes output-equality and first/last-row serving inference. Exact
answering_step_idremains authoritative for multi-step workflows, explicitserved_agent_idremains authoritative for provider-shaped paths, and records with neither identity remain attempt provenance only. ADR 0032,docs/product-technical-gap-baseline.md, README, CHANGELOG, and executable regressions are updated by the same verified one-shot.Verification
Previous focused verification remains historical only after the new head. Hosted exact-head checks and the source-fix RED→GREEN run are authoritative. The one-shot removes itself only after the focused candidate-control/API regression suite is GREEN and then publishes by ordinary non-force push; branch movement causes a fail-closed stop rather than an overwrite.
Research / architecture
ADR 0032 retains its RouteLLM/FrugalGPT routing basis. Candidate controls are operational evidence inputs, not a learned-routing claim. The 2026-09-02 amendment makes the previously implicit distinction executable: caller-provided exact membership is accepted without an arbitrary cardinality ranking rule, while serving identity is emitted only when provenance identifies it exactly.
Summary by CodeRabbit
새로운 기능
orchestration.routing에서 요청된 후보, 제외 후보, 시도된 후보와 실제 제공 후보를 확인할 수 있습니다.문서
테스트