Conversation
Keep the configured primary and drop later chain elements when the request needs a feature that adapter is known not to serve: tools, images, structured output, explicit reasoning effort, or a [1m]/[1M] context hint. A 1M hint currently excludes every fallback. Exclusions are logged and counted as capability_excluded; they are not remembered as best failures.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements capability-aware fallback filtering for heterogeneous upstream chains, skipping fallback adapters that do not support requested features like tools, images, structured output, reasoning effort, or 1M context hints. The changes include a new capability module, failover integration, metrics, updated multi-language documentation, and integration tests. The review feedback suggests refactoring the manual index loop in filter_fallbacks to use Rust's idiomatic drain method, which improves readability and avoids the performance overhead of repeated remove operations.
| let mut index = 1; | ||
| while index < routes.len() { | ||
| let reasons = requirements.incompatibilities(&routes[index].adapter); | ||
| if reasons.is_empty() { | ||
| index += 1; | ||
| continue; | ||
| } | ||
| let route = routes.remove(index); | ||
| tracing::warn!( | ||
| provider = %route.provider, | ||
| model = %route.upstream_model, | ||
| reasons = %reasons.join(","), | ||
| "fallback excluded by request capabilities" | ||
| ); | ||
| crate::metrics::record_failover(&route.provider, "capability_excluded"); | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Refactor manual index loop to use idiomatic vector draining
Symptom: The filter_fallbacks function uses a manual while loop with index manipulation and routes.remove(index) to filter incompatible fallback routes in-place.
Source: McConnell — Code Complete (Ch. 12: General Loop Guidelines - simplifying loop controls and avoiding manual index manipulation).
Consequence: Manual index tracking and in-place vector shifting via remove increases cognitive load, is more prone to off-by-one errors during future maintenance, and incurs unnecessary O(N) element shifting overhead on each removal.
Remedy: Use routes.drain(1..).collect() to cleanly separate the fallbacks from the primary route, then iterate over them and push the compatible ones back. This is more idiomatic, safer, and more efficient.
let fallbacks: Vec<Route> = routes.drain(1..).collect();
for route in fallbacks {
let reasons = requirements.incompatibilities(&route.adapter);
if reasons.is_empty() {
routes.push(route);
} else {
tracing::warn!(
provider = %route.provider,
model = %route.upstream_model,
reasons = %reasons.join(","),
"fallback excluded by request capabilities"
);
crate::metrics::record_failover(&route.provider, "capability_excluded");
}
}
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if self.explicit_effort { | ||
| reasons.push("reasoning-effort"); |
There was a problem hiding this comment.
Native Antigravity routes use AdapterKind::Gemini, but they read output_config.effort and translate it into the model tier and thinkingLevel. This blanket check therefore removes a compatible Antigravity fallback whenever explicit effort is requested. If the primary fails, Shunt skips that viable route and may return an avoidable failure.
Knowledge Base Used: Protocol and model translation
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/proxy/capability.rs
Line: 94-95
Comment:
**Compatible Antigravity fallback excluded**
Native Antigravity routes use `AdapterKind::Gemini`, but they read `output_config.effort` and translate it into the model tier and `thinkingLevel`. This blanket check therefore removes a compatible Antigravity fallback whenever explicit effort is requested. If the primary fails, Shunt skips that viable route and may return an avoidable failure.
**Knowledge Base Used:** [Protocol and model translation](https://app.greptile.com/passionfactory/-/custom-context/knowledge-base/pleaseai/shunt/-/docs/protocol-translation.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 14 files
Architecture diagram
sequenceDiagram
participant Client as API Client
participant Proxy as Proxy Layer
participant Cap as Capability Filter
participant Route as Route Resolution
participant Metrics as Metrics Service
participant Adapter as Adapter Layer
participant Upstream as Upstream Provider
Note over Client,Upstream: Request Flow with Capability-Aware Failover
Client->>Proxy: POST /v1/messages (model, tools, images, output_config)
Proxy->>Route: Resolve ordered chain (primary + fallbacks)
Route-->>Proxy: Routes list + requested_model
Proxy->>Proxy: Parse request body
Proxy->>Cap: filter_fallbacks(routes, body, requested_model)
Note over Cap: Extract requirement flags from request
Cap->>Cap: Check for tools, base64/url images, structured output, effort, [1m]/[1M] hint
alt Request has capability requirements
Cap->>Cap: Evaluate each fallback adapter against requirement matrix
loop Each fallback in chain (index >= 1)
alt Adapter incompatible with requirement
Cap->>Metrics: record_failover(provider, "capability_excluded")
Cap->>Cap: Remove route from chain
Note over Cap: Log warning with provider, model, reasons
else Adapter compatible
Cap->>Cap: Keep route in chain
end
end
end
Note over Cap: Primary route is always preserved even if incompatible
Cap-->>Proxy: Filtered routes list
loop Each remaining route in chain order
Proxy->>Adapter: Dispatch request to adapter
Adapter->>Upstream: Forward request
Upstream-->>Adapter: Response (status + body)
alt Success (2xx)
Adapter-->>Proxy: Return response
Proxy-->>Client: Stream response
else Failure (4xx/5xx)
alt More routes remaining
Adapter-->>Proxy: Error status
Proxy->>Proxy: Advance to next route
else Chain exhausted
Adapter-->>Proxy: Best failure relayed
Proxy-->>Client: Return best error response
end
end
end
Note over Client,Upstream: [1m]/[1M] hint excludes all fallbacks since no adapter advertises verified 1M path
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dffd16e63d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if self.explicit_effort { | ||
| reasons.push("reasoning-effort"); |
There was a problem hiding this comment.
When a later kind = "antigravity" upstream is configured, routing maps it to AdapterKind::Gemini, so this branch excludes it whenever Claude Code sends output_config.effort. The native Antigravity path explicitly consumes that field in antigravity_upstream_model_with (src/adapters/gemini/mod.rs:218-228), meaning a compatible fallback is skipped and the primary failure is returned instead. Distinguish native Antigravity from ordinary Gemini through adapter-owned capability logic.
AGENTS.md reference: src/AGENTS.md:L31-L31
Useful? React with 👍 / 👎.
| AdapterKind::Gemini | AdapterKind::Cursor => { | ||
| if self.url_images { | ||
| reasons.push("url-images"); | ||
| } |
There was a problem hiding this comment.
For a request containing a base64 image nested under tool_result.content, extraction sets base64_images, but the Gemini branch checks only url_images and leaves the fallback eligible. extract_tool_result_content then rejects that request as unsupported rich media (src/model/gemini_request.rs:291-294); because this is a gateway-local 400, the failover loop returns immediately and never reaches any later compatible Anthropic fallback. Track nested rich media separately and exclude Gemini for that case.
AGENTS.md reference: src/AGENTS.md:L31-L31
Useful? React with 👍 / 👎.
| response: Box::new(error.into_response()), | ||
| } | ||
| })?; | ||
| super::capability::filter_fallbacks(&mut routes, body.json(), &requested_model); |
There was a problem hiding this comment.
When /v1/messages/count_tokens carries tools or images and the configured model has an incompatible later route, this call logs and increments capability_excluded before lines 74-79 truncate the chain to its primary. Count-token requests never attempt fallbacks, so repeated token estimation can produce false failover telemetry and warnings for routes that were never candidates; skip capability filtering on this endpoint or run it only after the count-token early path.
Useful? React with 👍 / 👎.
…ter tool result images
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/proxy/capability.rs">
<violation number="1" location="src/proxy/capability.rs:137">
P2: Custom provider names are allowed, but `Route` erases whether `AdapterKind::Gemini` came from `ProviderKind::Antigravity`; this name-prefix check can drop valid custom native-Antigravity fallbacks for explicit-effort requests and retain ordinary Gemini fallbacks with an `antigravity-` name. Carry the provider kind or auth identity into this check instead of inferring it from `route.provider`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
| } | ||
|
|
||
| fn is_native_antigravity(route: &Route) -> bool { |
There was a problem hiding this comment.
P2: Custom provider names are allowed, but Route erases whether AdapterKind::Gemini came from ProviderKind::Antigravity; this name-prefix check can drop valid custom native-Antigravity fallbacks for explicit-effort requests and retain ordinary Gemini fallbacks with an antigravity- name. Carry the provider kind or auth identity into this check instead of inferring it from route.provider.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/proxy/capability.rs, line 137:
<comment>Custom provider names are allowed, but `Route` erases whether `AdapterKind::Gemini` came from `ProviderKind::Antigravity`; this name-prefix check can drop valid custom native-Antigravity fallbacks for explicit-effort requests and retain ordinary Gemini fallbacks with an `antigravity-` name. Carry the provider kind or auth identity into this check instead of inferring it from `route.provider`.</comment>
<file context>
@@ -114,26 +134,30 @@ impl Requirements {
}
}
+fn is_native_antigravity(route: &Route) -> bool {
+ route.adapter == AdapterKind::Gemini
+ && (route.provider == "antigravity" || route.provider.starts_with("antigravity-"))
</file context>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39514753e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| route.adapter == AdapterKind::Gemini | ||
| && (route.provider == "antigravity" || route.provider.starts_with("antigravity-")) |
There was a problem hiding this comment.
With a valid [[upstreams]] entry such as name = "backup" and kind = "antigravity", routing collapses the kind to AdapterKind::Gemini, so this name heuristic returns false and removes the otherwise compatible fallback whenever output_config.effort is present. The chain contract explicitly permits same-kind upstreams under arbitrary distinct names (docs/upstreams-failover.md:106-107). Fresh evidence after the earlier review is that the new helper bases the exception solely on reserved-looking provider names; retain the configured kind on Route or query the provider config instead.
AGENTS.md reference: AGENTS.md:L43-L43
Useful? React with 👍 / 👎.
| if self.structured_output { | ||
| reasons.push("structured-output"); | ||
| } | ||
| if self.explicit_effort && !is_native_antigravity(route) { |
There was a problem hiding this comment.
When a native Antigravity fallback maps to a supported non-Gemini ID such as claude-sonnet-4-6 or gpt-oss-120b-medium, this blanket exception keeps it for an explicit-effort request, but antigravity_upstream_model_with documents that those IDs are returned untouched (src/model/antigravity_request.rs:209-214), and the Gemini request translator does not map output_config.effort; a successful response therefore silently ignores the requested effort and prevents a later Responses-compatible fallback from being tried. Fresh evidence after the earlier review is that the newly added exception applies to every native Antigravity model rather than only the gemini-* IDs whose tier is actually resolved.
AGENTS.md reference: src/AGENTS.md:L31-L31
Useful? React with 👍 / 👎.
amondnet
left a comment
amondnet
left a comment
There was a problem hiding this comment.
Thanks for this — the shape of the change is right, and I want to say what I think it gets correct before the two things I'd like changed.
What holds up well. Filtering before dispatch (rather than mid-loop) keeps this entirely clear of the post-2xx no-replay boundary in §3 of docs/upstreams-failover.md: excluded routes never enter the attempt loop, so the claim that capability_excluded is "not remembered as best failures" is true by construction rather than by discipline. The is_count_tokens guard is the right carve-out. And the AntigravityCli + tools exclusion is genuinely valuable — reject_caller_tools returns a hard 400, so this converts a guaranteed failure into a skip. I spot-checked the Cursor + url-images claim too and it's accurate (src/adapters/cursor/request.rs:57: "Only base64 source images are included. URL images are skipped."). Docs are updated across README + locales, docs/, and the site, and the branch merges cleanly with main.
Two changes requested.
1. The [1m]/[1M] rule excludes every fallback on a hint that never reaches an upstream
Requirements::extract sets million_context from the requested model id, and incompatibilities early-returns vec!["1m-context-unknown"] before the adapter match — so it fires for every adapter, including Anthropic, which the matrix otherwise treats as excluding nothing.
I don't think the premise holds. Per docs/running.md:1012-1016, [1m] is a client-side lever that raises Claude Code's own context-window / auto-compact threshold, and shunt "strips a trailing [1m] from the model id before route matching and before forwarding upstream (routing.rs), so … the provider never sees the suffix." It isn't a request feature an upstream has to serve — it's a display/compaction setting on the client.
Three consequences:
- It silently disables failover for the project's canonical setup. An
anthropic→anthropicpool chain (the M8 multi-account case) loses its fallback entirely the moment a user types[1m]. The new testcapability_filter_keeps_primary_but_suppresses_fallback_for_1m_requirementbakes that in as a500. - It hits ids this repo's own docs recommend.
kimi-k3[1m]appears atdocs/running.md:237,:298, and:306as the documented Kimi model id. - The protection is incoherent even on its own terms. The primary is always kept, so a small-window primary is fine while a small-window fallback is not — for a suffix that changes nothing about what either one receives.
The reason string "1m-context-unknown" is, I think, an honest admission of the problem: it fires on absence of knowledge, which contradicts the module's own stated scope — "only for request features whose current adapter behavior is already known." Every other rule in the matrix mirrors a verified adapter behavior; this one mirrors a gap.
My suggestion is to drop million_context entirely for now. If there's a real truncation concern behind it, it wants a context-window field on the route (and would apply to the primary too), not a suffix gate — and that's a bigger change than this PR is scoped for.
2. The AntigravityCli tools rule doesn't mirror reject_caller_tools
eligibility_matrix_matches_existing_adapter_fidelity names the contract, but tool_choice appears nowhere in capability.rs, while reject_caller_tools (src/adapters/antigravity/mod.rs:632-648) keys off both. That splits two ways:
toolspresent andtool_choice: {"type": "none"}→ the adapter explicitly accepts (it returnsOk(())before even looking attools), but the filter excludes. A viable fallback is dropped.- no
toolsbuttool_choice: {"type": "any"}or{"type": "tool"}→ the adapter rejects with a 400, but the filter keeps it. This is the case the gate most wants to catch, and it misses it.
Mirroring it is small — read tool_choice.type, return early on none, and treat any/tool as requiring tools:
let choice_type = request.pointer("/tool_choice/type").and_then(Value::as_str);
let tools = choice_type != Some("none")
&& (matches!(choice_type, Some("any" | "tool"))
|| request.get("tools").and_then(Value::as_array).is_some_and(|t| !t.is_empty()));Worth a test where tool_choice alone drives the verdict in each direction — otherwise the mirror can drift again without anything going red.
Neither of these touches the architecture, which I think is sound. Fix these two and I'm happy to land it.
2 participants
Summary
Independent of #509/#510. After the ordered chain is resolved, later fallbacks are dropped when the request needs a feature that adapter is known not to serve:
[1m]/[1M]context hintThe configured primary is always kept. A
[1m]/[1M]hint currently excludes every fallback because no adapter advertises a verified 1M path. Exclusions are logged and counted ascapability_excluded; they are not remembered as best failures.This is a small, known-behavior matrix — not a general capability registry.
Test plan
cargo fmt --all --checkRUSTFLAGS=-D warnings cargo clippy --all-targets -- -D warningscargo test --lib proxy::capabilitycargo test --test failover -- capability_filter(
--all-featuresclippy needs the admin SPA bundle from #503; default features pass.)Summary by cubic
Filters incompatible fallbacks before dispatch so requests only reach upstreams that can serve the requested features. Previously all fallbacks were attempted regardless of capability; now later chain elements are dropped when the request needs tools, images, structured output, explicit reasoning effort, or a
[1m]/[1M]context hint, while the configured primary is always kept. Native antigravity fallbacks stay in the chain when the request asks for explicit reasoning effort.Notes
capability_excluded; they are not remembered as best failures.[1m]/[1M]hint currently excludes every fallback because no adapter advertises a verified 1M path.Written for commit 3951475. Summary will update on new commits.