Add fallback models support and capabilities-gated UI features#2850
Add fallback models support and capabilities-gated UI features#2850tim-inkeep merged 22 commits intomainfrom
Conversation
Introduce first-class `fallbackModels` on ModelSettings so each model slot (base, structuredOutput, summarizer) can declare an ordered list of fallback models. At runtime ModelFactory translates the array into `providerOptions.gateway.models` when AI_GATEWAY_API_KEY is set; otherwise the field is silently ignored. Extend the /capabilities endpoint with two feature-oriented flags: - `modelFallback.enabled` – gates the fallback-model UI - `costTracking.enabled` – gates the Cost nav in the sidebar UI changes: - ModelConfiguration renders a "Fallback Models" section (list of ModelSelector instances) when modelFallback is enabled and a primary model is selected. Fallback selectors are filtered to gateway-routable providers (anthropic, openai, google) via a new `gatewayOnly` prop on ModelSelector. - Sidebar Cost nav items (org + project level) are conditionally rendered based on costTracking.enabled. Cost columns in traces already self-hide when costUsd is null, so no changes needed there. - Serialization, deserialization, form validation, and the UI-local ModelSettings type all carry fallbackModels through. - Fallback models inherit through the existing project → agent → sub-agent chain. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
🦋 Changeset detectedLatest commit: ee38236 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
TL;DR — Adds gateway-backed fallback model support so agents can automatically retry on alternate models when the primary fails, and gates the Cost nav item and fallback-models UI behind runtime capability flags derived from Key changes
Summary | 20 files | 1 commit | base: Capabilities endpoint:
|
| Component | Change |
|---|---|
ModelConfiguration |
Renders ordered fallback list, gated on capability |
ModelSelector |
New gatewayOnly prop filters providers |
ModelSection / ProjectModelsSection |
Pass fallbackModels + callbacks for base, structuredOutput, summarizer |
AppSidebar |
Cost link conditional on costTracking.enabled |
serialize.ts / deserialize.ts |
Round-trip fallbackModels through form state |
agents-manage-ui/src/components/shared/model-configuration.tsx · agents-manage-ui/src/components/agent/sidepane/nodes/model-selector.tsx · agents-manage-ui/src/components/sidebar-nav/app-sidebar.tsx · agents-manage-ui/src/features/agent/domain/serialize.ts
Claude Opus | 𝕏
There was a problem hiding this comment.
Clean, well-structured PR. Schema addition is additive, runtime gating on AI_GATEWAY_API_KEY is correct, UI feature-flags via /capabilities is a good pattern. Two actionable issues below — one is a data integrity bug (empty-string fallback models reaching the gateway), the other is a minor DRY improvement.
Claude Opus | 𝕏
| .optional() | ||
| .describe('The provider options to use for the project.'), | ||
| fallbackModels: z | ||
| .array(z.string()) |
There was a problem hiding this comment.
Empty strings pass validation here — z.array(z.string()) accepts ['']. Downstream, ModelFactory.prepareGenerationConfig checks fallbackModels?.length which is truthy for [''], so an empty string will be sent to the gateway as a fallback model name.
Filter or validate elements:
z.array(z.string().min(1))Alternatively, filter at the runtime layer in prepareGenerationConfig with .filter(Boolean). Either way, at least one layer should guard against it.
| .array(z.string()) | |
| fallbackModels: z | |
| .array(z.string().min(1)) | |
| .optional() | |
| .describe( | |
| 'Ordered list of fallback models if the primary fails. Requires AI Gateway. Format: provider/model (e.g. "openai/gpt-5.2").' | |
| ), |
| size="sm" | ||
| className="w-full" | ||
| onClick={() => { | ||
| onFallbackModelsChange([...(fallbackModels ?? []), '']); |
There was a problem hiding this comment.
This appends an empty string '' as a placeholder for a new fallback. Since the schema currently accepts it and serialization doesn't filter empty strings, saving the form at this point would persist [''] to the API. Even with the z.string().min(1) fix on the schema, the UI should filter empties before calling the parent callback — or the serialization layer should strip them.
Consider filtering in the serializer (serialize.ts) as a safety net:
fallbackModels: modelsData.base.fallbackModels.filter(Boolean),| tooltip="Ordered list of models to try if the primary model fails. Requires AI Gateway." | ||
| /> | ||
| {(fallbackModels ?? inheritedFallbackModels ?? []).map((model, index) => ( | ||
| <div key={`${editorNamePrefix}-fallback-${index}`} className="flex items-center gap-2"> |
There was a problem hiding this comment.
Using array index as key (key={\${editorNamePrefix}-fallback-${index}`}) will cause React reconciliation issues when items are reordered or removed from the middle — the wrong ModelSelectorinstance will retain state from the previous item at that index. Since fallback models are strings, consider using the model value + index as key, e.g.key={`${editorNamePrefix}-fallback-${model}-${index}`}`, or better yet assign stable IDs.
|
TL;DR — Adds gateway-backed fallback model support and provider restriction so agents can automatically retry on alternate models when the primary fails and limit which providers serve requests. Introduces Key changes
Summary | 35 files | 9 commits | base: Capabilities endpoint:
|
| Change | Detail |
|---|---|
usage-cost-middleware.ts |
Renamed to setGatewayAttributesOnSpan; reads routing.finalProvider / routing.resolvedProvider from gateway metadata; adds overrideModelId hook and exported normalizeModelId |
otel-attributes.ts |
Two new span keys: GEN_AI_REQUEST_PROVIDER, GEN_AI_RESPONSE_PROVIDER |
signoz-stats.ts |
getUsageCostSummary accepts 'provider' as groupBy; event mapping prefers GEN_AI_RESPONSE_PROVIDER over AI_MODEL_PROVIDER |
cost-dashboard.tsx |
Third breakdown table ("Cost by Provider"), provider column in events table, 3-column grid on xl |
usage-cost-middleware.ts · otel-attributes.ts · cost-dashboard.tsx · signoz-stats.ts
UI: Capabilities-gated fallback model editor and allowed providers
Before: The model configuration component had no fallback or provider-restriction support; the Cost nav link was always visible.
After: A numbered fallback-model list with add/remove appears whenmodelFallback.enabledis true; an allowed-providers section with drag-to-reorder and all/specific radio toggle appears alongside it; the Cost sidebar link and cost pages render only whencostTracking.enabledis true.
| Component | Change |
|---|---|
AllowedProvidersSection |
New component with all/specific radio toggle, draggable ordered list from AVAILABLE_PROVIDERS (6 providers including Bedrock, Azure, Vertex), add/remove controls, and inheritance |
FallbackModelsSection |
Pending-selector pattern with onClose cleanup — only commits to array on selection |
ModelConfiguration |
Renders both allowed-providers and fallback sections gated on modelFallback.enabled and gateway-routability of the selected model |
ModelSelector |
gatewayOnly, defaultOpen, onClose props; filters providers via GATEWAY_ROUTABLE_PROVIDERS_SET, auto-opens popover |
ModelSection |
Sub-agent panel wires fallbackModels + allowedProviders + callbacks for all three model slots with inheritance from agent and project |
MetadataEditor |
Agent panel wires fallbackModels + allowedProviders for all three model slots with project-level inheritance |
ProjectModelsSection |
Project form passes both fields for all model slots |
AppSidebar |
Cost link conditional on costTracking.enabled |
| Cost pages | Both tenant and project cost pages call notFound() when costTracking is disabled |
serialize.ts / validation.ts |
Round-trip allowedProviders and fallbackModels through form state with .filter(Boolean) sanitization |
model-configuration.tsx · model-selector.tsx · model-section.tsx · metadata-editor.tsx
Documentation: AI Gateway section in models docs
Before: Gateway usage was documented inline with a
providerOptions.gateway.modelscode example.
After: A dedicated "AI Gateway" section documents fallback models, allowed providers, combined usage, and manual gateway provider options. Sub-agents docs updated with new configurable fields.
The old inline gateway model routing example was replaced with a brief mention linking to the new section. The new docs cover inheritance behavior (project → agent → sub-agent), provider/model format requirements, and interaction semantics when both features are combined.
Claude Opus | 𝕏
There was a problem hiding this comment.
PR Review Summary
(6) Total Issues | Risk: Medium
🟠⚠️ Major (2) 🟠⚠️
Inline Comments:
- 🟠 Major:
serialize.ts:573-594Agent-level models serialization missing fallbackModels field - 🟠 Major:
model-configuration.tsx:250-258Empty string added to fallbackModels array on "Add fallback model"
🟡 Minor (3) 🟡
Inline Comments:
- 🟡 Minor:
model-configuration.tsx:235-240Missing accessible name on remove button - 🟡 Minor:
model-configuration.tsx:214-215Array index used as key can cause state corruption - 🟡 Minor:
constants/models.ts:92GATEWAY_ROUTABLE_PROVIDERS duplicated between constants and model-factory
💭 Consider (1) 💭
💭 1) model-factory.test.ts Missing test for undefined fallbackModels
Issue: Tests cover empty array [] but not undefined for fallbackModels. The implementation checks modelSettings?.fallbackModels?.length which handles undefined differently.
Why: If a future refactor changes the truthiness check, the bug would slip through. Models saved without fallbackModels field should not inject gateway options.
Fix: Add test case:
test('should not inject when fallbackModels is undefined', () => {
process.env.AI_GATEWAY_API_KEY = 'test-key';
const config: ModelSettings = { model: 'anthropic/claude-sonnet-4-5' };
const result = ModelFactory.prepareGenerationConfig(config);
expect(result.providerOptions?.gateway).toBeUndefined();
});Refs: model-factory.ts:359
🚫 REQUEST CHANGES
Summary: The implementation is well-structured overall, but the agent-level models serialization bug will cause fallbackModels to be silently dropped when saving agents (as opposed to sub-agents where it works correctly). This is a data loss issue that should be fixed before merge. The empty string injection to fallbackModels is also worth addressing to prevent gateway errors. The minor issues (accessibility, key prop, duplicated constant) can be addressed in this PR or as follow-up.
Discarded (10)
| Location | Issue | Reason Discarded |
|---|---|---|
app-sidebar.tsx:69 |
Conditional spread creates unstable array reference | Minor perf concern, nav doesn't re-render frequently |
app-sidebar.tsx:53 |
Capabilities query in sidebar causes re-renders | React Query deduplication handles this; acceptable pattern |
model-selector.tsx:67 |
Missing setHasOpenModelConfig in useEffect deps | Pre-existing issue, zustand setters are stable |
model-section.tsx:34-71 |
Repeated inheritance computation logic | Pre-existing code duplication, not introduced by this PR |
capabilities.ts:39 |
OpenAPI schema name uses 'Schema' suffix | 50/50 convention - both patterns exist in codebase |
app-sidebar.tsx:54 |
costTrackingEnabled vs isWorkAppsEnabled naming | Minor naming inconsistency, not worth blocking |
model-configuration.tsx:207 |
Flash when capabilities load | Minor CLS, acceptable given React Query caching |
serialize.test.ts |
No tests for fallbackModels serialization | Valid concern but Consider-level, not blocking |
deserialize.test.ts |
No round-trip test for fallbackModels | Valid concern but Consider-level, not blocking |
model-configuration.tsx |
No React component tests for fallback UI | Manual testing noted in PR, acceptable |
Reviewers (4)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
2 | 0 | 0 | 0 | 2 | 0 | 0 |
pr-review-frontend |
8 | 0 | 0 | 0 | 3 | 0 | 5 |
pr-review-tests |
4 | 0 | 1 | 0 | 0 | 0 | 3 |
pr-review-consistency |
3 | 0 | 0 | 0 | 1 | 0 | 2 |
| Total | 17 | 0 | 1 | 0 | 6 | 0 | 10 |
| <Button | ||
| variant="outline" | ||
| size="sm" | ||
| className="w-full" | ||
| onClick={() => { | ||
| onFallbackModelsChange([...(fallbackModels ?? []), '']); | ||
| }} | ||
| disabled={disabled} | ||
| > |
There was a problem hiding this comment.
🟠 MAJOR: Empty string added to fallbackModels array
Issue: Clicking "Add fallback model" appends an empty string to the fallbackModels array. If the user doesn't select a model before saving, this empty string will be sent to the AI Gateway as part of gateway.models, which may cause gateway errors.
Why: Empty model strings in the fallback list will be passed through ModelFactory.prepareGenerationConfig() to the gateway, potentially causing API errors when the gateway attempts to route to an empty model identifier.
Fix: Filter empty/falsy values before calling onFallbackModelsChange, or add defensive filtering in ModelFactory.prepareGenerationConfig():
| <Button | |
| variant="outline" | |
| size="sm" | |
| className="w-full" | |
| onClick={() => { | |
| onFallbackModelsChange([...(fallbackModels ?? []), '']); | |
| }} | |
| disabled={disabled} | |
| > | |
| <Button | |
| variant="outline" | |
| size="sm" | |
| className="w-full" | |
| onClick={() => { | |
| onFallbackModelsChange([...(fallbackModels ?? []), '']); | |
| }} | |
| disabled={disabled} | |
| > |
Or add this filter in model-factory.ts at line 365:
models: modelSettings.fallbackModels.filter(m => m && m.trim())Refs:
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| className="h-8 w-8 shrink-0" | ||
| onClick={() => { | ||
| const models = [...(fallbackModels ?? [])]; |
There was a problem hiding this comment.
🟡 Minor: Missing accessible name on remove button
Issue: The remove button contains only an X icon without an accessible label. Screen reader users won't understand what this button does or which fallback model it removes.
Why: Violates WCAG 2.1 SC 4.1.2 (Name, Role, Value). Users relying on assistive technology cannot determine the button's purpose.
Fix:
| <Button | |
| variant="ghost" | |
| size="icon" | |
| className="h-8 w-8 shrink-0" | |
| onClick={() => { | |
| const models = [...(fallbackModels ?? [])]; | |
| <Button | |
| variant="ghost" | |
| size="icon" | |
| className="h-8 w-8 shrink-0" | |
| aria-label={`Remove fallback model ${index + 1}`} | |
| onClick={() => { |
Refs:
| {(fallbackModels ?? inheritedFallbackModels ?? []).map((model, index) => ( | ||
| <div key={`${editorNamePrefix}-fallback-${index}`} className="flex items-center gap-2"> |
There was a problem hiding this comment.
🟡 Minor: Array index used as key can cause state corruption
Issue: Using key={\${editorNamePrefix}-fallback-${index}`}with array index means that when items are removed from the middle of the list, React will reuse DOM nodes incorrectly. TheModelSelector` components have internal state (popover open state, custom input mode), which will get mismatched to the wrong items.
Why: When a user removes fallback model #2 from a list of 3, React will match the old component for index 2 to the new item at index 2 (which was previously index 3), potentially preserving stale UI state.
Fix: Consider using the model value in the key. Since models can be empty strings or duplicates, a combination approach works:
| {(fallbackModels ?? inheritedFallbackModels ?? []).map((model, index) => ( | |
| <div key={`${editorNamePrefix}-fallback-${index}`} className="flex items-center gap-2"> | |
| {(fallbackModels ?? inheritedFallbackModels ?? []).map((model, index) => ( | |
| <div key={`${editorNamePrefix}-fallback-${model || 'empty'}-${index}`} className="flex items-center gap-2"> |
For a more robust solution, consider storing fallback models as objects with stable IDs.
Refs:
Use a pending selector pattern: clicking "Add fallback model" shows a local-only ModelSelector row with defaultOpen. The save callback only fires when the user actually picks a model, preventing premature auto-save with empty values. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
PR Review Summary
(3) Total Issues | Risk: Low
Re-review scope: This review covers only the delta since the last automated review (1 commit, 2 files). See previous review for the full initial assessment.
✅ Addressed from Previous Review
The empty string injection bug (previously Major) is fixed by the new FallbackModelsSection component. The showPendingSelector state pattern correctly defers adding to the fallbackModels array until an actual model is selected — clicking "Add fallback model" no longer appends an empty string.
🟡 Minor (3) 🟡
Inline Comments:
- 🟡 Minor:
model-configuration.tsx:38Array index as key can cause state corruption - 🟡 Minor:
model-configuration.tsx:58-68Remove button missing accessible name - 🟡 Minor:
model-configuration.tsx:93-98Cancel button missing accessible name
💭 Consider (1) 💭
Inline Comments:
- 💭 Consider:
model-selector.tsx:59-63Redundant useEffect for defaultOpen synchronization
🕐 Pending Recommendations (3)
These issues were raised in the previous review and remain unresolved:
- 🟠
serialize.ts:573-594Agent-level models serialization missing fallbackModels field — this is a data loss issue where fallbackModels won't persist for root agents - 🟡
constants/models.ts:92GATEWAY_ROUTABLE_PROVIDERS duplicated between constants and model-factory - 💭
model-factory.test.tsMissing test for undefined fallbackModels
💡 APPROVE WITH SUGGESTIONS
Summary: The delta successfully fixes the empty string injection bug with a clean deferred-selector pattern. The remaining issues are minor accessibility improvements (add aria-label to icon buttons) and a key prop optimization. The pending serialization bug from the prior review (serialize.ts missing fallbackModels for agent-level models) should still be addressed before merge to prevent data loss.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-frontend |
4 | 0 | 0 | 0 | 4 | 0 | 0 |
| Total | 4 | 0 | 0 | 0 | 4 | 0 | 0 |
| tooltip="Ordered list of models to try if the primary model fails. Requires AI Gateway." | ||
| /> | ||
| {savedModels.map((model, index) => ( | ||
| <div key={`${editorNamePrefix}-fallback-${index}`} className="flex items-center gap-2"> |
There was a problem hiding this comment.
🟡 Minor: Array index as key can cause state corruption
Issue: Using index as the key (key={\${editorNamePrefix}-fallback-${index}`}`) will cause React to incorrectly preserve component state when items are removed from the middle of the list.
Why: When a fallback model is removed via splice(index, 1), React sees keys shift (e.g., removing index 1 causes item at index 2 to become index 1). React reuses DOM/state from the old index position, which can cause ModelSelector internal state to show stale data.
Fix: Use the model value as the key:
| <div key={`${editorNamePrefix}-fallback-${index}`} className="flex items-center gap-2"> | |
| {savedModels.map((model, index) => ( | |
| <div key={model || `${editorNamePrefix}-fallback-${index}`} className="flex items-center gap-2"> |
Refs:
| useEffect(() => { | ||
| if (defaultOpen) { | ||
| setOpen(true); | ||
| } | ||
| }, [defaultOpen]); |
There was a problem hiding this comment.
💭 Consider: Redundant useEffect for defaultOpen synchronization
Issue: This useEffect may be redundant since useState(defaultOpen) already sets the initial value. The effect only sets open = true when defaultOpen becomes true, but doesn't handle the reverse case (closing when defaultOpen becomes false).
Why: The current usage works because the pending selector unmounts/remounts, but this creates fragile coupling. If ModelSelector is reused in a context where defaultOpen toggles without remounting, the popover won't close.
Fix: Either remove the effect entirely (if re-mounting is the intended pattern) or make it bidirectional:
useEffect(() => {
setOpen(defaultOpen);
}, [defaultOpen]);For this specific use case where re-mounting is intentional, removing the effect entirely would be cleaner.
Refs:
| .optional() | ||
| .describe('The provider options to use for the project.'), | ||
| fallbackModels: z | ||
| .array(z.string().min(1)) |
There was a problem hiding this comment.
| .array(z.string().min(1)) | |
| .array(z.string().nonempty()) |
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: Medium
Re-review scope: This review covers only the delta since the previous review (1 commit, 4 files).
✅ Addressed from Previous Reviews
The delta (41f3c2377) successfully addresses 3 issues from prior reviews:
| Issue | Fix Applied |
|---|---|
| Empty strings pass fallbackModels validation | z.string().min(1) in schemas.ts:225 |
GATEWAY_ROUTABLE_PROVIDERS duplicated between constants and model-factory |
Removed local const, imports from constants/models.js |
| Array index as key causes React state issues | Changed to ${model}-${index} composite key in model-configuration.tsx:39 |
Additionally, .filter(Boolean) was added to all three model slots in serialize.ts (processModels function) as a defense-in-depth measure for filtering empty strings before persistence.
🕐 Pending Recommendations (1)
These issues were raised in prior reviews and remain unresolved:
- 🟠
serialize.ts:573-594Agent-level models serialization missing fallbackModels field — theprocessModelsfunction correctly includes fallbackModels with.filter(Boolean)for sub-agents, but the agent-level models (lines 573-594) still serializes withoutfallbackModels. This means root agent fallback models will be silently dropped on save.
🟡 Minor Items from Prior Reviews (not blocking)
These accessibility issues from prior reviews remain relevant but are not blocking:
- Missing
aria-labelon remove button (model-configuration.tsx:61-73) - Missing
aria-labelon cancel button (model-configuration.tsx:96-103)
🚫 REQUEST CHANGES
Summary: The delta makes good progress — the validation and deduplication fixes are solid. However, the agent-level serialization bug (serialize.ts:573-594) from the initial review still needs to be addressed. Without it, fallbackModels configured at the root agent level will be lost when saving. The fix is straightforward: apply the same pattern used in processModels (lines 78-80, 87-89, 96-98) to the agent-level serialization block.
Discarded (0)
No findings discarded in this delta review.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Orchestrator analysis only | — | 0 | 0 | 0 | 0 | 1 | 0 |
Note: Sub-reviewers were not dispatched for this delta review due to the small scope (4 files, ~15 LOC changed). Findings are based on orchestrator analysis of the diff against prior review context.
Ito Test Report ❌19 test cases ran. 5 failed, 14 passed. Across 19 test cases, 14 passed and 5 failed, with core gateway capability behavior, cost-navigation gating/routing, agent fallback persistence, unsaved-change handling, save-race stability, and security checks (unauthorized API access, XSS, malformed/oversized IDs, and capability-fetch failure handling) all working as expected. The most important confirmed defects are in fallback-model UX and persistence: project settings drops models.*.fallbackModels during save/reload (including duplicate and concurrent-edit scenarios, causing silent data loss despite success feedback), inherited sub-agent fallback rows can be edited before explicit override, and pressing Escape in the selector leaves an orphan pending fallback row. ❌ Failed (5)🟠 Inherited fallback settings unlock before explicit override
Relevant code:
const savedModels = fallbackModels ?? inheritedFallbackModels ?? [];
const isInherited = !fallbackModels && !!inheritedFallbackModels;
{!showPendingSelector && (
<Button
variant="outline"
size="sm"
className="w-full"
onClick={() => setShowPendingSelector(true)}
disabled={disabled}
>
fallbackModels={models?.structuredOutput?.fallbackModels}
inheritedFallbackModels={structuredOutputInheritance.fallbackModels}
onFallbackModelsChange={(models) =>
updatePath('models.structuredOutput.fallbackModels', models.length ? models : undefined)
}🟠 Duplicate fallback entries and ordering semantics
Relevant code:
base: {
model: data.models.base.model,
providerOptions: cleanProviderOptions(data.models.base.providerOptions),
},
structuredOutput: data.models?.structuredOutput?.model
? {
model: data.models.structuredOutput.model,
providerOptions: cleanProviderOptions(data.models.structuredOutput.providerOptions),
}
: undefined,
summarizer: data.models?.summarizer?.model
? {
model: data.models.summarizer.model,
providerOptions: cleanProviderOptions(data.models.summarizer.providerOptions),
}
: undefined,
{capabilities?.modelFallback?.enabled && effectiveModel && onFallbackModelsChange && (
<FallbackModelsSection
editorNamePrefix={editorNamePrefix}
fallbackModels={fallbackModels}
inheritedFallbackModels={inheritedFallbackModels}
onFallbackModelsChange={onFallbackModelsChange}
disabled={disabled}
/>
)}
|
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Re-review scope: This review covers the delta since the previous automated approval (commit
630df345f→786ef5ea3). The delta consists entirely of a merge frommainwith no changes to PR-specific code.
✅ Delta Analysis
The commit (786ef5ea3 — "updated ui") is a merge from main with no changes to the PR's fallback models feature code. Changes brought in from main:
| Component | Change | Relevance to PR |
|---|---|---|
| Org Entitlement System (#2845) | Adds seat limits, billing UI, entitlement middleware | ❌ Unrelated — separate feature |
| Security Fixes (#2922, #2924, #2927) | Anonymous session JWT claims hardening | ❌ Unrelated — auth layer |
| CI Fixes (#2921) | Preview environment teardown | ❌ Unrelated — infrastructure |
Sidebar (app-sidebar.tsx) |
Adds Billing nav item with entitlement gating | ❌ Unrelated — the Cost nav gating logic remains intact |
| Member Components | Entitlement error handling and seat limit UX | ❌ Unrelated — org member management |
The PR-specific files have no changes:
model-configuration.tsx— unchangedmodel-selector.tsx— unchangedmodel-factory.ts— unchangedcapabilities.ts— unchangedschemas.ts— unchanged
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
model-selector.tsx:61-65SimplifyuseEffectwith direct Popover props (per @dimaMachina's feedback)
🕐 Pending Recommendations (1)
Human reviewer feedback that remains unaddressed:
- 🟡
model-selector.tsx:125@dimaMachina: ReplaceuseEffectwith direct Popover props — The current pattern usesuseState(defaultOpen)+useEffectto sync thedefaultOpenprop. The cleaner approach is to passdefaultOpen={defaultOpen} open={open}directly to the Popover component, aligning with Radix's controlled API. This is a minor code quality improvement that can be addressed in this PR or as follow-up.
✅ APPROVE
Summary: The delta contains only a merge from main with no changes to PR-specific code. All prior fallback models functionality has been implemented correctly and reviewed in previous cycles. The outstanding item is @dimaMachina's feedback about simplifying the useEffect pattern in model-selector.tsx — this is a minor code quality improvement that can be addressed in this PR or as a follow-up. The feature is solid and ready to ship! 🚀
Discarded (0)
No findings discarded in this review.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Orchestrator analysis only | — | 0 | 0 | 0 | 1 | 1 | 0 |
Note: Sub-reviewers were not dispatched for this delta review as the PR-specific code has no changes since the last approval. The delta consists entirely of a merge from main.
| if (defaultOpen) { | ||
| setOpen(true); | ||
| } | ||
| }, [defaultOpen]); |
There was a problem hiding this comment.
🟡 Minor: Simplify useEffect with direct Popover props
Issue: Per @dimaMachina's feedback, this useEffect can be simplified by passing defaultOpen directly to the Popover component instead of syncing via state.
Why: The current pattern creates unnecessary indirection. Radix's Popover supports controlled usage where you pass both defaultOpen and open together.
Fix:
// Remove this useEffect block (lines 61-65)
// Then change the Popover props from:
<Popover open={open} onOpenChange={...}>
// To:
<Popover defaultOpen={defaultOpen} open={open} onOpenChange={...}>Refs:
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Re-review scope: This review covers the delta since the previous automated approval (commit
786ef5ea3→d2b86a73). The delta is a single commit addressing human reviewer feedback.
✅ Delta Analysis
The commit (d2b86a734 — "updated with dima feedback") successfully addresses @dimaMachina's feedback:
| Change | Assessment |
|---|---|
Removed useEffect block (previously lines 61-65) |
✅ Correct — eliminates redundant state sync |
Added defaultOpen={defaultOpen} prop to Popover (line 119) |
✅ Correct — uses Radix's native API for controlled/uncontrolled state |
Code quality improvement:
The previous pattern used useState(defaultOpen) + useEffect to sync the defaultOpen prop to internal state. The new pattern passes defaultOpen directly to the Radix Popover component alongside open, which:
- Eliminates unnecessary re-renders from state synchronization
- Aligns with Radix Popover's documented API
- Is cleaner and more maintainable
✅ All Prior Issues Addressed
| Issue | Status |
|---|---|
Empty string validation for fallbackModels |
✅ Fixed with z.string().nonempty() |
| Project-level models serialization | ✅ Fixed in project-form.tsx |
| Agent-level models serialization | ✅ Fixed in serialize.ts |
| Accessibility labels on buttons | ✅ Added aria-label attributes |
| Escape key orphan pending row | ✅ Fixed with onClose callback |
| Inherited fallback settings protection | ✅ Fixed with disabled={disabled || isInherited} |
GATEWAY_ROUTABLE_PROVIDERS deduplication |
✅ Consolidated in constants/models.ts |
| Cost route deep-link gating | ✅ Added notFound() when capability disabled |
| Model ID normalization in telemetry | ✅ Added normalizeModelId() |
useEffect simplification (human reviewer) |
✅ Fixed in this commit |
✅ APPROVE
Summary: The delta successfully implements @dimaMachina's feedback by replacing the useEffect state sync pattern with direct defaultOpen prop passing to the Radix Popover component. This is a clean, focused code quality improvement. All prior review issues have been addressed. Ship it! 🚀
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Orchestrator analysis only | — | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Sub-reviewers were not dispatched for this delta review as the change is a minimal code quality fix (~6 lines) directly addressing human reviewer feedback.
| .filter( | ||
| ([provider]) => | ||
| !gatewayOnly || | ||
| (GATEWAY_ROUTABLE_PROVIDERS as readonly string[]).includes(provider) |
There was a problem hiding this comment.
@claude can you ensure there are no .includes? .includes is usually a bug
Ito Test Report ❌18 test cases ran. 2 failed, 16 passed. Overall, 16 of 18 test cases passed, confirming that capability-gated behavior, API contracts, sidebar visibility, deep-link blocking when disabled, enabled cost-page rendering paths, project model-configuration logic, persistence, inheritance/serialization, and adversarial security checks all behaved as expected without mocked route behavior. The key issue was 2 high-severity failures reproducing the same real regression introduced by this PR: tenant/project cost routes can incorrectly hit 404 in enabled environments during fresh load/refresh because the guard calls notFound() while capabilities are still unresolved (null initial query data). ❌ Failed (2)
|
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Re-review scope: This review covers the delta since the previous automated approval (commit
d2b86a734→8d67eb26b). The delta consists of 4 commits including a main merge and feature enhancements.
✅ Addressing @tim-inkeep's .includes Concern
The delta already fixes the .includes → Set.has() pattern throughout the codebase:
| File | Fix Applied |
|---|---|
model-selector.tsx:233 |
GATEWAY_ROUTABLE_PROVIDERS.includes(provider) → GATEWAY_ROUTABLE_PROVIDERS_SET.has(provider) |
model-configuration.tsx:46 |
effectiveProviders.includes(p.value) → effectiveSet.has(p.value) |
model-factory.ts:209 |
GATEWAY_ROUTABLE_PROVIDERS.includes(provider) → GATEWAY_ROUTABLE_PROVIDERS_SET.has(provider) |
usage-cost-middleware.ts:51 |
GATEWAY_ROUTABLE_PROVIDERS.includes(prefix) → GATEWAY_ROUTABLE_PROVIDERS_SET.has(prefix) |
The remaining .includes('/') on line 204 of model-selector.tsx is a string method (checking if a model value contains a / character), NOT an array membership check — this is correct usage.
✅ Delta Analysis
The delta includes well-implemented changes:
| File | Change | Assessment |
|---|---|---|
model-resolver.ts |
New inheritGatewayFields helper and resolveModelConfig function for gateway field inheritance |
✅ Clean implementation with proper parent → child inheritance for fallbackModels and allowedProviders |
model-resolver.test.ts |
150+ lines of comprehensive test coverage | ✅ Excellent coverage including inheritance, edge cases, and error handling |
metadata-editor.tsx |
Wires fallbackModels and allowedProviders to agent-level model configuration |
✅ Proper form integration with inherited values |
model-configuration.tsx |
Adds isGatewayRoutable check to conditionally show Allowed Providers and Fallback Models sections |
✅ Correct gating — only gateway-routable models (anthropic, openai, google, gateway) show these features |
models.mdx (docs) |
Major documentation update with new "AI Gateway" section covering fallback models, allowed providers, and inheritance | ✅ Clear, comprehensive documentation with code examples |
sub-agents.mdx (docs) |
Minor doc update referencing gateway features | ✅ Consistent |
constants/models.ts |
Exports GATEWAY_ROUTABLE_PROVIDERS_SET as a ReadonlySet<string> |
✅ O(1) lookup vs O(n) for array .includes |
Code Quality Highlights
-
resolveModelConfiginheritance logic — Clean implementation that inheritsfallbackModelsandallowedProvidersfrom parent configurations (project → agent → sub-agent) even when the child overrides the base model. This enables DRY configuration. -
Set-based membership testing — Converting from array
.includes()toSet.has()provides O(1) lookup performance, which matters for frequently-called code paths like model routing. -
Documentation — The new "AI Gateway" section in
models.mdxis comprehensive and well-structured, covering:- Fallback models with inheritance semantics
- Allowed providers with priority ordering
- Combined usage patterns
- Advanced manual gateway options
🧹 While You're Here (1)
🧹 1) model-factory.ts:287,291 Pre-existing .includes on BUILT_IN_PROVIDERS array
Issue: The parseModelString method still uses .includes() on the BUILT_IN_PROVIDERS array for provider validation. This is pre-existing code (not introduced by this PR) but follows the same pattern that was fixed elsewhere.
Why: While BUILT_IN_PROVIDERS is small (9 items), consistency with the rest of the codebase suggests using a Set for O(1) lookup.
Fix: Consider adding a BUILT_IN_PROVIDERS_SET alongside the existing constant for membership checks in a follow-up PR.
Refs: model-factory.ts:268-278
✅ APPROVE
Summary: The delta successfully addresses @tim-inkeep's concern about .includes usage — all gateway provider membership checks now use Set.has() for O(1) performance. The new resolveModelConfig module with its inheritance logic is well-tested and cleanly implemented. Documentation is comprehensive. The feature is ready to ship! 🚀
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Orchestrator analysis only | — | 0 | 0 | 1 | 0 | 0 | 0 |
Note: Sub-reviewers were not dispatched for this delta review as the changes are focused and follow established patterns. The delta primarily adds gateway field inheritance and converts .includes to Set.has().
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Re-review scope: This review covers the delta since the previous automated approval (commit
8d67eb26b→ee38236a6). The delta is a single commit improving documentation wording.
✅ Delta Analysis
The commit (ee38236a6 — "updated docs") is a minor documentation improvement:
| File | Change | Assessment |
|---|---|---|
agents-docs/content/typescript-sdk/models.mdx |
Updated the "Combining Both" section description from "This tries claude-sonnet-4-5 via Bedrock, then Anthropic. If both fail, it falls through to the fallback models in order:" to "With this configuration, allowedProviders restricts and prioritizes which providers can serve the primary model, while fallbackModels defines alternative models to try if the primary model is unavailable:" | ✅ Clearer explanation of the interaction between allowedProviders and fallbackModels |
This is a pure documentation clarification with no code changes.
✅ All Prior Issues Addressed
All issues from prior reviews have been resolved:
| Issue | Status |
|---|---|
Empty string validation for fallbackModels |
✅ Fixed with z.string().nonempty() |
| Project-level models serialization | ✅ Fixed in project-form.tsx |
| Agent-level models serialization | ✅ Fixed in serialize.ts |
| Accessibility labels on buttons | ✅ Added aria-label attributes |
| Escape key orphan pending row | ✅ Fixed with onClose callback |
| Inherited fallback settings protection | ✅ Fixed with disabled={disabled || isInherited} |
GATEWAY_ROUTABLE_PROVIDERS deduplication |
✅ Consolidated in constants/models.ts |
| Cost route deep-link gating | ✅ Added notFound() when capability disabled |
| Model ID normalization in telemetry | ✅ Added normalizeModelId() |
useEffect simplification (human reviewer) |
✅ Fixed per @dimaMachina's feedback |
.includes → Set.has() (human reviewer) |
✅ Fixed per @tim-inkeep's feedback |
💭 Consider (1) 💭
💭 1) [tenantId]/cost/page.tsx:27-29 and [projectId]/cost/page.tsx:26-28 Cost route capability guard race condition
Issue: The Ito test report identified a potential race condition where cost pages can show 404 during initial capability loading. The guard pattern:
if (!capabilitiesLoading && !capabilities?.costTracking?.enabled) {
notFound();
}can trigger 404 if capabilities is null (initial state from useCapabilitiesQuery) before the fetch completes.
Why: The capabilities query uses initialData: null with initialDataUpdatedAt: 0. During the brief window between hook initialization and query execution, isLoading could be false while data is still null, causing an incorrect 404.
Fix (optional): Add an explicit loading state or check for capabilities !== null:
// Option A: Show nothing while loading
if (capabilitiesLoading) return null;
if (!capabilities?.costTracking?.enabled) notFound();
// Option B: Only check when capabilities are resolved
if (capabilities !== null && !capabilities.costTracking?.enabled) notFound();Refs:
- capabilities.ts:24 —
initialData: null - Ito Test Report — "Cost pages can render 404 before capabilities resolve"
✅ APPROVE
Summary: The delta is a documentation-only change that improves the explanation of how allowedProviders and fallbackModels interact. All code issues from prior reviews have been addressed. The Ito-identified race condition in cost page capability gating is noted as a "Consider" item — it's an edge case during initial load that could be addressed as a follow-up if it causes user friction. The PR is ready to ship! 🚀
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Orchestrator analysis only | — | 0 | 1 | 0 | 0 | 0 | 0 |
Note: Sub-reviewers were not dispatched for this delta review as the change is a minimal documentation update (~2 lines changed).
|
🔎💬 Inkeep AI search and chat service is syncing content for source 'Inkeep Agent Framework Docs' |
Ito Test Report ❌18 test cases ran. 1 failed, 17 passed. Across 18 test cases, 17 passed and 1 failed, indicating generally strong stability with capability/auth, navigation gating, cost dashboard rendering, model configuration flows, and advanced settings behaviors all largely validated in local verification. The single confirmed defect is a medium-severity issue in Project Settings where switching from a gateway-routable base model to a non-gateway model hides gateway-only controls but still submits stale fallbackModels and allowedProviders, while all other critical checks (including 401 protection on unauthenticated capabilities access, correct AI_GATEWAY_API_KEY-driven flag behavior, deep-link blocking when costTracking is disabled, and safe handling of malicious model text) behaved as expected. ❌ Failed (1)
🟠 Gateway-only fields persist after switching to non-gateway model
Relevant code:
const handleModelChange = (modelValue: string) => {
const previousEffectiveModel = value || inheritedValue;
const newModel = modelValue || undefined;
const wasInherited = !value && !!inheritedValue;
const isNowExplicit = !!newModel;
if (previousEffectiveModel !== newModel || (wasInherited && isNowExplicit)) {
setInternalProviderOptions(undefined);
onProviderOptionsChange?.('');
}
onModelChange?.(newModel || '');
};
{capabilities?.modelFallback?.enabled &&
effectiveModel &&
isGatewayRoutable &&
onAllowedProvidersChange && (
<AllowedProvidersSection
allowedProviders={allowedProviders}
inheritedAllowedProviders={inheritedAllowedProviders}
onAllowedProvidersChange={onAllowedProvidersChange}
disabled={disabled}
/>
)}
{capabilities?.modelFallback?.enabled &&
effectiveModel &&
isGatewayRoutable &&
onFallbackModelsChange && (
<FallbackModelsSection
editorNamePrefix={editorNamePrefix}
fallbackModels={fallbackModels}
inheritedFallbackModels={inheritedFallbackModels}
onFallbackModelsChange={onFallbackModelsChange}
disabled={disabled}
/>
)}
base: {
model: data.models.base.model,
providerOptions: cleanProviderOptions(data.models.base.providerOptions),
...(data.models.base.fallbackModels?.length && {
fallbackModels: data.models.base.fallbackModels.filter(Boolean),
}),
...(data.models.base.allowedProviders?.length && {
allowedProviders: data.models.base.allowedProviders.filter(Boolean),
}),
},✅ Passed (17)Commit: Tell us how we did: Give Ito Feedback |
Summary
fallbackModelsfield toModelSettingsschema — each model slot (base, structuredOutput, summarizer) can declare an ordered list of fallback models that the AI Gateway tries in sequence when the primary model fails/capabilitiesendpoint withmodelFallback.enabledandcostTracking.enabledflags (both derived fromAI_GATEWAY_API_KEYpresence) — feature-oriented naming so consumers don't need to know about gateway internalsModelConfiguration— reuses existingModelSelectorwith a newgatewayOnlyprop that filters to gateway-routable providers (anthropic, openai, google)costTracking.enabled— cost in traces already self-hides viacostUsd != nullchecksDetails
Schema (
agents-core):ModelSettingsSchemagainsfallbackModels: z.array(z.string()).optional(). SinceModelSchemaandProjectModelSchemacomposeModelSettingsSchema, all three model slots get fallback support automatically.GATEWAY_ROUTABLE_PROVIDERSexported fromconstants/models.tsfor UI consumption.Runtime (
agents-core):ModelFactory.prepareGenerationConfig()translatesfallbackModelsintoproviderOptions.gateway.modelswhenAI_GATEWAY_API_KEYis set. When gateway isn't configured, fallbacks are silently ignored. Existing gateway provider options (e.g.gateway.order) are preserved during merge.API (
agents-api):/capabilitiesresponse extended withmodelFallbackandcostTrackingobjects. Handler readsAI_GATEWAY_API_KEYfromprocess.envdirectly (same pattern asModelFactory). OpenAPI snapshot updated.UI (
agents-manage-ui):ModelConfigurationrenders a "Fallback Models" section below provider options whencapabilities.modelFallback.enabledis true — each fallback is aModelSelectorinstance with remove button, plus an "Add fallback model" buttonModelSelectorgainsgatewayOnlyprop — filters dropdown to only anthropic/openai/google and hides Custom/LLM Gateway sectionsModelSectionandProjectModelsSectionwirefallbackModelsprops through for all three slots with inheritance supportcapabilities.costTracking.enabledModelSettingstype all updatedTest plan
model-factory.test.ts— 4 new tests covering fallback translation with/without gateway key, empty array, and merge with existing gateway optionscapabilities.test.ts— 3 tests updated to verifymodelFallbackandcostTrackingfields in both configured and unconfigured statescapabilities-query.test.tsx— test data updated with new capability fieldsAI_GATEWAY_API_KEYis set🤖 Generated with Claude Code