fix: align newapi cookie checkin and downstream key defaults#550
Open
roiding wants to merge 7 commits into
Open
fix: align newapi cookie checkin and downstream key defaults#550roiding wants to merge 7 commits into
roiding wants to merge 7 commits into
Conversation
- New modelContextLengthCache service for in-memory model→context_length mapping - Platform adapters (newApi, standardApiProvider) extract context_length from upstream /v1/models - modelsSurface injects context_length into both OpenAI and Claude response formats - Default 1,000,000 when upstream does not provide context_length - Supports field names: context_length, contextLength, max_context_length, contextWindow, etc.
- Backend: DELETE /api/accounts/:id/models/manual endpoint - Only deletes models where isManual=true (safe against auto-discovered models) - Frontend: AccountModelsModal shows '✕ 删除' button next to each manual model - Frontend: api.removeAccountManualModels() function - Accounts.tsx: wires up delete handler with toast feedback
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds scoped model context-length caching and extraction across adapters and discovery, surfaces per-model context_length in model listings, implements account manual-model deletion (service/route/UI/tests), and changes downstream API key empty-selection default and UI behavior. ChangesModel Context Caching, Removal, and Routing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
github-actions
Bot
added
area: server
![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: a9a059a8f0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+87
to
+89
| if (nextScopeCache.size === 0) { | ||
| cache.delete(scopeKey); | ||
| return; |
There was a problem hiding this comment.
setModelContextLengths currently replaces the entire scope and deletes it when a payload has no context_length values, but refreshModelsForAccount calls adapter.getModels(..., modelContextScope) repeatedly for the same account across API token/access token/default tokens. In multi-credential accounts, a later discovery response without context metadata will erase lengths learned from earlier responses, so /v1/models falls back to 1_000_000 even for models that were previously resolved with real context windows.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/web/pages/accounts/AccountModelsModal.tsx (1)
187-198: ⚡ Quick winHide delete action when no handler is provided.
This currently renders a clickable delete control even when
onRemoveManualModelis undefined.Proposed change
- {model.isManual ? ( + {model.isManual && onRemoveManualModel ? ( <button onClick={(e) => { e.preventDefault(); e.stopPropagation(); - void onRemoveManualModel?.(model.name); + void onRemoveManualModel(model.name); }} className="btn btn-ghost btn-xs" style={{ fontSize: 10, padding: '2px 6px', color: 'var(--color-error)', flexShrink: 0 }} title="删除手动添加的模型" > ✕ 删除 </button> ) : null}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/web/pages/accounts/AccountModelsModal.tsx` around lines 187 - 198, The delete button is shown even when no handler is provided; update the JSX in AccountModelsModal so the delete control is only rendered when onRemoveManualModel is defined (e.g., conditionally render the button based on onRemoveManualModel), preserving the existing onClick logic (preventDefault/stopPropagation and calling onRemoveManualModel(model.name)), className, style and title so nothing else changes when the handler exists.src/server/services/accountManualModelService.ts (1)
30-30: ⚡ Quick winAvoid rebuilding routes when nothing was removed.
Line 30 rebuilds routes even when
deletedCountis0, which adds unnecessary rebuild work on no-op deletes.Proposed change
- await rebuildRoutesBestEffort(); + if (deletedCount > 0) { + await rebuildRoutesBestEffort(); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/services/accountManualModelService.ts` at line 30, The code always calls rebuildRoutesBestEffort() after a delete operation even when deletedCount is 0; update the delete flow in accountManualModelService (the block that inspects deletedCount) to only call rebuildRoutesBestEffort() when deletedCount > 0 so no-op deletes skip the rebuild; locate the variable deletedCount and the unconditional call to rebuildRoutesBestEffort() and wrap or gate that call with a simple conditional check (deletedCount > 0) to avoid unnecessary work.src/server/services/platforms/standardApiProvider.ts (1)
83-88: Note: Malformed responses clear cached context lengths.The cache update happens before validation at line 91. If the upstream returns a malformed payload,
extractContextLengthsFromPayloadreturns an empty map, which triggers scope deletion (lines 87-89 inmodelContextLengthCache.ts). This clears all cached context lengths for that scope until the next successful fetch.This behavior is intentional per the comment on lines 72-73 about preventing stale values, and the fallback to 1M tokens is conservative. Just be aware that transient upstream errors will temporarily reset context lengths to defaults for that scope.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/services/platforms/standardApiProvider.ts` around lines 83 - 88, The code updates the model context-length cache before validating the upstream payload, which allows malformed responses to clear the cache; change the flow so caching only occurs after successful validation (or when a non-empty contextLengths map is returned). Specifically, in standardApiProvider.ts defer calling setModelContextLengths (and the scope computed by buildEndpointModelContextLengthScope) until after the payload has been validated, or at minimum guard the call to setModelContextLengths to run only when extractContextLengthsFromPayload(payload) returns a non-empty result.src/server/services/modelContextLengthCache.ts (2)
34-36: 💤 Low valueOptional: Remove redundant wrapper function.
getNormalizedModelKeysimply forwards tonormalizeKeywithout adding any logic. You can inline thenormalizeKeycall directly at line 43.♻️ Simplify by removing wrapper
-function getNormalizedModelKey(modelName: string): string { - return normalizeKey(modelName); -} - function isValidContextLength(value: number): boolean { return Number.isFinite(value) && value > 0; } function buildScopedEntryKey(sourceScope: string | undefined, modelName: string): [string, string] | null { - const normalizedModelName = getNormalizedModelKey(modelName); + const normalizedModelName = normalizeKey(modelName); if (!normalizedModelName) return null; return [normalizeSourceScope(sourceScope), normalizedModelName]; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/services/modelContextLengthCache.ts` around lines 34 - 36, Remove the redundant wrapper getNormalizedModelKey which only calls normalizeKey; replace all uses of getNormalizedModelKey with direct calls to normalizeKey and delete the getNormalizedModelKey function definition. Ensure any exports or references (e.g., calls in this module or other modules) are updated to reference normalizeKey and that TypeScript types/imports remain correct after removal.
1-183: Consider adding cache eviction policy for long-running deployments.The in-memory cache grows unbounded as accounts and endpoints are added. In production with many accounts or frequently changing endpoint URLs, this could accumulate stale entries over time. Consider adding:
- A TTL (time-to-live) per scope to expire old entries
- A maximum cache size with LRU eviction
- Periodic cleanup of unused scopes
For most deployments this won't be an issue, but it's worth monitoring memory usage in high-scale scenarios.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/services/modelContextLengthCache.ts` around lines 1 - 183, The cache is unbounded; add eviction by tracking per-scope metadata and applying TTL and max-size/LRU eviction. Introduce a scopeMeta Map alongside cache to store lastAccess and created timestamps; update lastAccess in getOrCreateScopeCache, getModelContextLength, hasModelContextLength and getAllModelContextLengths; add configurable constants (e.g., DEFAULT_SCOPE_TTL_MS, MAX_SCOPES) and a periodic cleanup function evictExpiredScopes that removes scopes older than TTL and, when cache.size > MAX_SCOPES, evicts least-recently-used scopes based on lastAccess; call eviction after setModelContextLength/setModelContextLengths and expose a clearModelContextLengthCache to also remove meta entries. Ensure buildScopedEntryKey, getOrCreateScopeCache, setModelContextLength, setModelContextLengths, getModelContextLength, hasModelContextLength and clearModelContextLengthCache are updated to maintain/clean scopeMeta consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/routes/api/accounts.ts`:
- Around line 1960-1964: The route handler is performing a persistence check by
calling db.select(...).where(...).get() (creating `account`) — move that logic
out of the route adapter into the service layer (e.g., into an AccountsService
method such as getAccountById or ensureAccountExists) so the route only parses
request/params and delegates; replace the direct DB call in the route (the
`account` lookup around eq(schema.accounts.id, accountId) and the similar call
at the later occurrence) with a call to the service method and surface
errors/responses from that service instead.
In `@src/server/services/platforms/newApi.test.ts`:
- Line 42: The test constant AUTH_TOKEN_SIGNIN_ONLY_TOKEN is currently set to
"session=..." which prevents exercising the auth_token= cookie flow; update
AUTH_TOKEN_SIGNIN_ONLY_TOKEN to either the raw token string
(COOKIE_GOB_USER_TOKEN) or an explicit "auth_token=..." value so the test
triggers the auth_token branch in the code, then adjust the assertions in the
related test case(s) to expect the auth_token cookie handling (check for
"auth_token" parsing/lookup instead of "session"); repeat this fix for the other
occurrences referenced (around the 740-767 test block) to ensure all
auth_token-style cases are actually validated.
---
Nitpick comments:
In `@src/server/services/accountManualModelService.ts`:
- Line 30: The code always calls rebuildRoutesBestEffort() after a delete
operation even when deletedCount is 0; update the delete flow in
accountManualModelService (the block that inspects deletedCount) to only call
rebuildRoutesBestEffort() when deletedCount > 0 so no-op deletes skip the
rebuild; locate the variable deletedCount and the unconditional call to
rebuildRoutesBestEffort() and wrap or gate that call with a simple conditional
check (deletedCount > 0) to avoid unnecessary work.
In `@src/server/services/modelContextLengthCache.ts`:
- Around line 34-36: Remove the redundant wrapper getNormalizedModelKey which
only calls normalizeKey; replace all uses of getNormalizedModelKey with direct
calls to normalizeKey and delete the getNormalizedModelKey function definition.
Ensure any exports or references (e.g., calls in this module or other modules)
are updated to reference normalizeKey and that TypeScript types/imports remain
correct after removal.
- Around line 1-183: The cache is unbounded; add eviction by tracking per-scope
metadata and applying TTL and max-size/LRU eviction. Introduce a scopeMeta Map
alongside cache to store lastAccess and created timestamps; update lastAccess in
getOrCreateScopeCache, getModelContextLength, hasModelContextLength and
getAllModelContextLengths; add configurable constants (e.g.,
DEFAULT_SCOPE_TTL_MS, MAX_SCOPES) and a periodic cleanup function
evictExpiredScopes that removes scopes older than TTL and, when cache.size >
MAX_SCOPES, evicts least-recently-used scopes based on lastAccess; call eviction
after setModelContextLength/setModelContextLengths and expose a
clearModelContextLengthCache to also remove meta entries. Ensure
buildScopedEntryKey, getOrCreateScopeCache, setModelContextLength,
setModelContextLengths, getModelContextLength, hasModelContextLength and
clearModelContextLengthCache are updated to maintain/clean scopeMeta
consistently.
In `@src/server/services/platforms/standardApiProvider.ts`:
- Around line 83-88: The code updates the model context-length cache before
validating the upstream payload, which allows malformed responses to clear the
cache; change the flow so caching only occurs after successful validation (or
when a non-empty contextLengths map is returned). Specifically, in
standardApiProvider.ts defer calling setModelContextLengths (and the scope
computed by buildEndpointModelContextLengthScope) until after the payload has
been validated, or at minimum guard the call to setModelContextLengths to run
only when extractContextLengthsFromPayload(payload) returns a non-empty result.
In `@src/web/pages/accounts/AccountModelsModal.tsx`:
- Around line 187-198: The delete button is shown even when no handler is
provided; update the JSX in AccountModelsModal so the delete control is only
rendered when onRemoveManualModel is defined (e.g., conditionally render the
button based on onRemoveManualModel), preserving the existing onClick logic
(preventDefault/stopPropagation and calling onRemoveManualModel(model.name)),
className, style and title so nothing else changes when the handler exists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8d06d828-870b-4aba-b5b8-5091cdc29800
📒 Files selected for processing (27)
src/server/proxy-core/surfaces/modelsSurface.test.tssrc/server/proxy-core/surfaces/modelsSurface.tssrc/server/routes/api/accounts.manual-models.test.tssrc/server/routes/api/accounts.tssrc/server/routes/proxy/models.test.tssrc/server/services/accountManualModelService.tssrc/server/services/downstreamApiKeyService.test.tssrc/server/services/downstreamApiKeyService.tssrc/server/services/modelContextLengthCache.test.tssrc/server/services/modelContextLengthCache.tssrc/server/services/modelService.tssrc/server/services/platforms/base.tssrc/server/services/platforms/claude.tssrc/server/services/platforms/cliproxyapi.tssrc/server/services/platforms/gemini.tssrc/server/services/platforms/newApi.test.tssrc/server/services/platforms/newApi.tssrc/server/services/platforms/oneApi.tssrc/server/services/platforms/oneHub.tssrc/server/services/platforms/openai.tssrc/server/services/platforms/standardApiProvider.tssrc/server/services/platforms/veloera.tssrc/web/api.tssrc/web/pages/Accounts.tsxsrc/web/pages/DownstreamKeys.test.tsxsrc/web/pages/DownstreamKeys.tsxsrc/web/pages/accounts/AccountModelsModal.tsx
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/services/accountManualModelService.ts`:
- Around line 35-52: Move the account existence check into the same transaction
and run it before returning early: call
ensureManualModelAccountExists(accountId) inside the db.transaction callback
(before checking normalizedModelNames.length and before performing the delete)
so a missing account triggers the 404 and the transaction scope covers the
check; then keep the existing early-return of deletedCount: 0 when
normalizedModelNames is empty, and proceed to the tx.delete(...) on
schema.modelAvailability only after the existence check passes.
In `@src/server/services/modelService.discovery.test.ts`:
- Around line 212-261: The test's concurrency relies on two microtask ticks (the
two await Promise.resolve()) which is brittle; instead, after starting both
refreshModelsForAccount(account.id) calls, wait until the mocked adapter has
observed two callers by polling seenScopes.length (or awaiting a small promise
loop) before calling releaseGate, so that getModelsMock (the mock implementation
that pushes into seenScopes and awaits gate) is guaranteed to have been entered
twice; update the test to remove the two Promise.resolve() lines and replace
them with a short loop that awaits until seenScopes.length === 2 (with a
sensible timeout) and then invokes releaseGate.
In `@src/server/services/modelService.ts`:
- Line 1040: The call to setModelContextLengths(discoveredContextLengths,
modelContextScope) should be skipped when discoveredContextLengths is empty
because the function treats an empty map as a signal to delete the entire scope
cache; update the caller to guard against this by only invoking
setModelContextLengths when discoveredContextLengths is non-empty (e.g., check
discoveredContextLengths.size > 0 or equivalent), ensuring you still call it for
valid discoveries but avoid passing an empty map that would wipe the cached
context lengths for modelContextScope.
In `@src/server/services/platforms/newApi.test.ts`:
- Around line 190-199: Duplicate authorization check: remove the redundant
conditional block that repeats the same check for req.headers.authorization ===
`Bearer auth_token=${AUTH_TOKEN_SIGNIN_ONLY_TOKEN}` and the identical response
(res.writeHead + res.end), leaving a single guard that returns 401; update any
nearby comments if needed and ensure only the first occurrence of the check
involving AUTH_TOKEN_SIGNIN_ONLY_TOKEN, res.writeHead, and res.end remains.
- Around line 525-547: There are two identical conditional blocks checking for
auth_token=${AUTH_TOKEN_SIGNIN_ONLY_TOKEN} and validating
req.headers['new-api-user'] === '5566' (producing the same JSON responses);
remove the duplicate block so only one handler for AUTH_TOKEN_SIGNIN_ONLY_TOKEN
remains (locate the blocks referencing AUTH_TOKEN_SIGNIN_ONLY_TOKEN and
req.headers['new-api-user'] and delete the redundant copy), ensuring the single
remaining branch preserves the existing responses and return flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: af8e4217-fa4d-45e3-bd3b-251b33202c2e
📒 Files selected for processing (6)
src/server/routes/api/accounts.tssrc/server/services/accountManualModelService.tssrc/server/services/modelService.discovery.test.tssrc/server/services/modelService.tssrc/server/services/platforms/newApi.test.tssrc/server/services/platforms/newApi.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/server/routes/api/accounts.ts
- src/server/services/platforms/newApi.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8afc7e1c2a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+1040
to
+1042
| if (discoveredContextLengths.size > 0) { | ||
| setModelContextLengths(discoveredContextLengths, modelContextScope); | ||
| } |
There was a problem hiding this comment.
On a successful refresh where every credential scan returns models without context_length, discoveredContextLengths stays empty and this guard skips updating the account scope. That leaves the previous account cache intact, so /v1/models can keep serving stale context_length values from an earlier sync instead of falling back to the default when metadata disappears. This is reproducible after an upstream stops emitting context metadata but still returns model IDs.
Useful? React with 👍 / 👎.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Owner
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
修复 newapi 在 session=... cookie 形式下的签到失败问题
保持 cookie 头原样透传，避免把已有 cookie 字符串错误重组
为 cookie 签到路径补齐兼容用户 ID 头
将下游密钥空模型/空群组限制改为“运行时允许全部可路由范围”
避免把创建当时的聚合模型列表固化成过期白名单
补充后端与前端回归测试
Summary by CodeRabbit
New Features
Bug Fixes
Tests