Conversation
* feat: add provider session reuse opt-out * refactor: tighten provider session reuse follow-up
* fix: gate session stats by high concurrency mode * fix: move logs page test to unit tests * test: harden logs page mode assertions * refactor: polish high-concurrency dashboard follow-ups
|
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在 system_settings 表新增布尔标志 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
github-actions
Bot
added
size/XL
![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 introduces a "High-Concurrency Mode" designed to reduce CPU and IO pressure by disabling non-essential Redis debug snapshots and real-time observability writes during high RPM periods. Additionally, it adds a "Skip session reuse" setting for providers, allowing the system to ignore sticky bindings and rerun priority checks—useful for quickly switching back to preferred providers after they recover. The changes span database migrations, proxy handling logic, and UI updates across multiple languages. Review feedback highlights missing fields in the system configuration API, opportunities to refactor verbose repository selection logic, and a suggestion for more consistent session ID validation.
| currencyDisplay: validated.currencyDisplay, | ||
| billingModelSource: validated.billingModelSource, | ||
| codexPriorityBillingSource: validated.codexPriorityBillingSource, |
There was a problem hiding this comment.
The siteTitle and allowGlobalUsageView fields are missing from the update payload, even though they are defined in the UpdateSystemSettingsSchema. This prevents these settings from being updated through this API endpoint. Since you are already expanding the list of updated fields in this PR, please include these as well.
| currencyDisplay: validated.currencyDisplay, | |
| billingModelSource: validated.billingModelSource, | |
| codexPriorityBillingSource: validated.codexPriorityBillingSource, | |
| siteTitle: validated.siteTitle, | |
| allowGlobalUsageView: validated.allowGlobalUsageView, | |
| currencyDisplay: validated.currencyDisplay, | |
| billingModelSource: validated.billingModelSource, | |
| codexPriorityBillingSource: validated.codexPriorityBillingSource, |
| const selectionWithoutHighConcurrencyMode = { | ||
| id: systemSettings.id, | ||
| siteTitle: systemSettings.siteTitle, | ||
| allowGlobalUsageView: systemSettings.allowGlobalUsageView, | ||
| currencyDisplay: systemSettings.currencyDisplay, | ||
| billingModelSource: systemSettings.billingModelSource, | ||
| timezone: systemSettings.timezone, | ||
| enableAutoCleanup: systemSettings.enableAutoCleanup, | ||
| cleanupRetentionDays: systemSettings.cleanupRetentionDays, | ||
| cleanupSchedule: systemSettings.cleanupSchedule, | ||
| cleanupBatchSize: systemSettings.cleanupBatchSize, | ||
| enableClientVersionCheck: systemSettings.enableClientVersionCheck, | ||
| verboseProviderError: systemSettings.verboseProviderError, | ||
| enableHttp2: systemSettings.enableHttp2, | ||
| codexPriorityBillingSource: systemSettings.codexPriorityBillingSource, | ||
| interceptAnthropicWarmupRequests: systemSettings.interceptAnthropicWarmupRequests, | ||
| enableThinkingSignatureRectifier: systemSettings.enableThinkingSignatureRectifier, | ||
| enableThinkingBudgetRectifier: systemSettings.enableThinkingBudgetRectifier, | ||
| enableBillingHeaderRectifier: systemSettings.enableBillingHeaderRectifier, | ||
| enableResponseInputRectifier: systemSettings.enableResponseInputRectifier, | ||
| enableCodexSessionIdCompletion: systemSettings.enableCodexSessionIdCompletion, | ||
| enableClaudeMetadataUserIdInjection: systemSettings.enableClaudeMetadataUserIdInjection, | ||
| enableResponseFixer: systemSettings.enableResponseFixer, | ||
| responseFixerConfig: systemSettings.responseFixerConfig, | ||
| quotaDbRefreshIntervalSeconds: systemSettings.quotaDbRefreshIntervalSeconds, | ||
| quotaLeasePercent5h: systemSettings.quotaLeasePercent5h, | ||
| quotaLeasePercentDaily: systemSettings.quotaLeasePercentDaily, | ||
| quotaLeasePercentWeekly: systemSettings.quotaLeasePercentWeekly, | ||
| quotaLeasePercentMonthly: systemSettings.quotaLeasePercentMonthly, | ||
| quotaLeaseCapUsd: systemSettings.quotaLeaseCapUsd, | ||
| createdAt: systemSettings.createdAt, | ||
| updatedAt: systemSettings.updatedAt, | ||
| }; |
There was a problem hiding this comment.
The selection objects (e.g., selectionWithoutHighConcurrencyMode) are extremely verbose and duplicated across getSystemSettings and updateSystemSettings. This makes the code difficult to maintain and prone to errors when the schema changes. Consider defining a shared constant for the full selection and using a helper to omit specific keys, or at least consolidate these definitions.
| }).catch((error) => { | ||
| logger.error("ProxyForwarder: Failed to update session provider info", { error }); | ||
| }); | ||
| if (session.shouldTrackSessionObservability()) { |
There was a problem hiding this comment.
For consistency and safety, consider checking for session.sessionId before calling SessionManager.updateSessionProvider. While the session ID is typically present at this stage, an explicit check (as seen in other parts of the file like line 357) prevents potential runtime errors if the session was not properly initialized.
| if (session.shouldTrackSessionObservability()) { | |
| if (session.sessionId && session.shouldTrackSessionObservability()) { |
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| try { | ||
| const systemSettings = await getCachedSystemSettings(); | ||
| session.setHighConcurrencyModeEnabled(systemSettings.enableHighConcurrencyMode ?? false); | ||
| } catch (settingsError) { | ||
| logger.warn("[ProxyHandler] Failed to load high concurrency setting, fallback to disabled", { | ||
| error: settingsError, | ||
| }); | ||
| session.setHighConcurrencyModeEnabled(false); | ||
| } |
There was a problem hiding this comment.
setHighConcurrencyModeEnabled call
setHighConcurrencyModeEnabled is called here before the guard pipeline runs, and then called a second time inside ProxySessionGuard.ensure (session-guard.ts line 54) with the same value from the same in-memory cache. The early call in proxy-handler is useful as a safety net (ensures the flag is set even if the guard pipeline errors before reaching the session guard), but the second call is redundant. A comment explaining the intentional early call would make this clearer and prevent future confusion.
| try { | |
| const systemSettings = await getCachedSystemSettings(); | |
| session.setHighConcurrencyModeEnabled(systemSettings.enableHighConcurrencyMode ?? false); | |
| } catch (settingsError) { | |
| logger.warn("[ProxyHandler] Failed to load high concurrency setting, fallback to disabled", { | |
| error: settingsError, | |
| }); | |
| session.setHighConcurrencyModeEnabled(false); | |
| } | |
| try { | |
| const systemSettings = await getCachedSystemSettings(); | |
| // Set early so the flag is respected even if the guard pipeline short-circuits | |
| // before ProxySessionGuard runs. The session guard will re-apply the same value. | |
| session.setHighConcurrencyModeEnabled(systemSettings.enableHighConcurrencyMode ?? false); | |
| } catch (settingsError) { | |
| logger.warn("[ProxyHandler] Failed to load high concurrency setting, fallback to disabled", { | |
| error: settingsError, | |
| }); | |
| session.setHighConcurrencyModeEnabled(false); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy-handler.ts
Line: 21-29
Comment:
**Redundant `setHighConcurrencyModeEnabled` call**
`setHighConcurrencyModeEnabled` is called here before the guard pipeline runs, and then called a second time inside `ProxySessionGuard.ensure` (`session-guard.ts` line 54) with the same value from the same in-memory cache. The early call in proxy-handler is useful as a safety net (ensures the flag is set even if the guard pipeline errors before reaching the session guard), but the second call is redundant. A comment explaining the intentional early call would make this clearer and prevent future confusion.
```suggestion
try {
const systemSettings = await getCachedSystemSettings();
// Set early so the flag is respected even if the guard pipeline short-circuits
// before ProxySessionGuard runs. The session guard will re-apply the same value.
session.setHighConcurrencyModeEnabled(systemSettings.enableHighConcurrencyMode ?? false);
} catch (settingsError) {
logger.warn("[ProxyHandler] Failed to load high concurrency setting, fallback to disabled", {
error: settingsError,
});
session.setHighConcurrencyModeEnabled(false);
}
```
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/app/v1/_lib/proxy/forwarder.ts (1)
357-368: 建议把高并发模式的写入门禁收敛成统一 helper。现在
session.sessionId && session.shouldPersistSessionDebugArtifacts()这套判断分散在很多分支里，后面只要再新增一个写点就很容易漏掉，结果高并发模式下又悄悄恢复 Redis 写入。把这层门禁统一封装后，会更容易审计，也更不容易回归。Also applies to: 1882-1904, 1992-2004, 2029-2054, 2097-2120, 2139-2145, 2232-2238, 4047-4059
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/forwarder.ts` around lines 357 - 368, Create a single helper (e.g., ensurePersistAllowed or guardPersistWrite) that encapsulates the check session.sessionId && session.shouldPersistSessionDebugArtifacts() and performs/guards any write calls (wrapping calls like SessionManager.storeSessionSpecialSettings) so all Redis/write code paths use that helper; update the occurrences in ProxyForwarder (where SessionManager.storeSessionSpecialSettings is called) and the other duplicated regions (around the ranges noted) to call this helper instead of repeating the conditional, and ensure the helper logs or returns a no-op when persistence is not permitted to avoid missed branches.src/app/v1/_lib/proxy/response-handler.ts (1)
1643-1662: 建议让超大响应告警与调试持久化开关联动当前
wasTruncated告警在关闭调试持久化时也可能触发，容易产生“本来就不存储却告警”的噪音。可把告警分支也与shouldPersistSessionDebugArtifacts()绑定。可选修改示例
- } else if (session.sessionId && wasTruncated) { + } else if ( + session.sessionId && + wasTruncated && + session.shouldPersistSessionDebugArtifacts() + ) { logger.warn("[ResponseHandler] Skip storing passthrough response: body too large", { taskId, providerId: provider.id, providerName: provider.name, maxBytes: MAX_STATS_BUFFER_BYTES, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/response-handler.ts` around lines 1643 - 1662, The warn branch currently logs a truncation alert even when debug persistence is disabled; update the condition so the truncation warning only runs when session.sessionId is present, wasTruncated is true, and session.shouldPersistSessionDebugArtifacts() returns true (mirror the persistence check used in the successful store path), so modify the else-if using session.shouldPersistSessionDebugArtifacts() to avoid noisy alerts; ensure you keep the same log payload (taskId, providerId, providerName, MAX_STATS_BUFFER_BYTES) and only call logger.warn when that combined condition is met.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/`[locale]/dashboard/logs/page.tsx:
- Line 4: The page currently imports and calls getSystemSettings directly which
creates a hard dependency that can throw and make the whole logs page 500; swap
that call to the project's cache-backed reader (the system settings cache /
fail-open API) instead — import and call the cache layer (e.g., the cached
getter used elsewhere in the repo) in place of getSystemSettings inside the page
component, ensure the call is non-throwing by catching errors or allowing the
cache to return undefined, and fall back to safe defaults or previous values so
rendering continues; make the same replacement at the other occurrence
referenced (the second use around line 29).
---
Nitpick comments:
In `@src/app/v1/_lib/proxy/forwarder.ts`:
- Around line 357-368: Create a single helper (e.g., ensurePersistAllowed or
guardPersistWrite) that encapsulates the check session.sessionId &&
session.shouldPersistSessionDebugArtifacts() and performs/guards any write calls
(wrapping calls like SessionManager.storeSessionSpecialSettings) so all
Redis/write code paths use that helper; update the occurrences in ProxyForwarder
(where SessionManager.storeSessionSpecialSettings is called) and the other
duplicated regions (around the ranges noted) to call this helper instead of
repeating the conditional, and ensure the helper logs or returns a no-op when
persistence is not permitted to avoid missed branches.
In `@src/app/v1/_lib/proxy/response-handler.ts`:
- Around line 1643-1662: The warn branch currently logs a truncation alert even
when debug persistence is disabled; update the condition so the truncation
warning only runs when session.sessionId is present, wasTruncated is true, and
session.shouldPersistSessionDebugArtifacts() returns true (mirror the
persistence check used in the successful store path), so modify the else-if
using session.shouldPersistSessionDebugArtifacts() to avoid noisy alerts; ensure
you keep the same log payload (taskId, providerId, providerName,
MAX_STATS_BUFFER_BYTES) and only call logger.warn when that combined condition
is met.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 15a980e4-bf45-4c79-addd-64d89c83d85b
📒 Files selected for processing (73)
drizzle/0086_perpetual_runaways.sqldrizzle/0087_nappy_lady_mastermind.sqldrizzle/meta/0086_snapshot.jsondrizzle/meta/0087_snapshot.jsondrizzle/meta/_journal.jsonmessages/en/settings/config.jsonmessages/en/settings/providers/form/sections.jsonmessages/en/settings/providers/guide.jsonmessages/ja/settings/config.jsonmessages/ja/settings/providers/form/sections.jsonmessages/ja/settings/providers/guide.jsonmessages/ru/settings/config.jsonmessages/ru/settings/providers/form/sections.jsonmessages/ru/settings/providers/guide.jsonmessages/zh-CN/settings/config.jsonmessages/zh-CN/settings/providers/form/sections.jsonmessages/zh-CN/settings/providers/guide.jsonmessages/zh-TW/settings/config.jsonmessages/zh-TW/settings/providers/form/sections.jsonmessages/zh-TW/settings/providers/guide.jsonsrc/actions/providers.tssrc/actions/system-config.tssrc/app/[locale]/dashboard/_components/bento/dashboard-bento.tsxsrc/app/[locale]/dashboard/_components/dashboard-bento-sections.tsxsrc/app/[locale]/dashboard/logs/_components/usage-logs-sections.tsxsrc/app/[locale]/dashboard/logs/page.tsxsrc/app/[locale]/settings/config/_components/system-settings-form.tsxsrc/app/[locale]/settings/config/page.tsxsrc/app/[locale]/settings/providers/_components/batch-edit/analyze-batch-settings.tssrc/app/[locale]/settings/providers/_components/batch-edit/build-patch-draft.tssrc/app/[locale]/settings/providers/_components/forms/provider-form/index.tsxsrc/app/[locale]/settings/providers/_components/forms/provider-form/provider-form-context.tsxsrc/app/[locale]/settings/providers/_components/forms/provider-form/provider-form-types.tssrc/app/[locale]/settings/providers/_components/forms/provider-form/sections/options-section.tsxsrc/app/api/admin/system-config/route.tssrc/app/v1/_lib/proxy-handler.tssrc/app/v1/_lib/proxy/forwarder.tssrc/app/v1/_lib/proxy/provider-selector.tssrc/app/v1/_lib/proxy/response-fixer/index.tssrc/app/v1/_lib/proxy/response-fixer/response-fixer.test.tssrc/app/v1/_lib/proxy/response-handler.tssrc/app/v1/_lib/proxy/session-guard.tssrc/app/v1/_lib/proxy/session.tssrc/app/v1/_lib/proxy/warmup-guard.tssrc/drizzle/schema.tssrc/lib/config/system-settings-cache.tssrc/lib/provider-patch-contract.tssrc/lib/validation/schemas.tssrc/repository/_shared/transformers.tssrc/repository/provider.tssrc/repository/system-config.tssrc/types/provider.tssrc/types/system-config.tstests/integration/billing-model-source.test.tstests/unit/actions/system-config-save.test.tstests/unit/batch-edit/analyze-batch-settings.test.tstests/unit/dashboard/dashboard-home-layout.test.tsxtests/unit/dashboard/logs/page.test.tsxtests/unit/lib/config/system-settings-cache.test.tstests/unit/lib/timezone/timezone-resolver.test.tstests/unit/proxy/hedge-error-pipeline.test.tstests/unit/proxy/pricing-no-price.test.tstests/unit/proxy/provider-selector-model-mismatch-binding.test.tstests/unit/proxy/proxy-forwarder-hedge-first-byte.test.tstests/unit/proxy/proxy-forwarder-thinking-signature-rectifier.test.tstests/unit/proxy/proxy-handler-session-id-error.test.tstests/unit/proxy/response-handler-endpoint-circuit-isolation.test.tstests/unit/proxy/session-guard-warmup-intercept.test.tstests/unit/proxy/session.test.tstests/unit/proxy/warmup-guard.test.tstests/unit/repository/system-config-update-missing-columns.test.tstests/unit/settings/providers/build-patch-draft.test.tstests/unit/settings/providers/options-section.test.tsx
| import { Suspense } from "react"; | ||
| import { redirect } from "@/i18n/routing"; | ||
| import { getSession } from "@/lib/auth"; | ||
| import { getSystemSettings } from "@/repository/system-config"; |
There was a problem hiding this comment.
改用缓存层读取系统设置，避免日志页因为配置查询失败而整页 500。
这里直接 getSystemSettings() 把日志页变成了对 system_settings 的硬依赖；一旦这次查询抖动或报错，整个页面都会失败。项目里已经有带 fail-open 语义的系统设置缓存，这里更适合走缓存层，至少还能回退到旧值或默认值继续渲染。
建议修改
-import { getSystemSettings } from "@/repository/system-config";
+import { getCachedSystemSettings } from "@/lib/config";
@@
- const systemSettings = await getSystemSettings();
+ const systemSettings = await getCachedSystemSettings();Also applies to: 29-29
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/`[locale]/dashboard/logs/page.tsx at line 4, The page currently
imports and calls getSystemSettings directly which creates a hard dependency
that can throw and make the whole logs page 500; swap that call to the project's
cache-backed reader (the system settings cache / fail-open API) instead — import
and call the cache layer (e.g., the cached getter used elsewhere in the repo) in
place of getSystemSettings inside the page component, ensure the call is
non-throwing by catching errors or allowing the cache to return undefined, and
fall back to safe defaults or previous values so rendering continues; make the
same replacement at the other occurrence referenced (the second use around line
29).
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
No significant issues identified in this PR.
PR Size: XL
- Lines changed: 9428
- Files changed: 73
- Split suggestions: (1) DB schema + migrations, (2) proxy pipeline high-concurrency gating, (3) provider
disableSessionReuseend-to-end (types/repo/UI/i18n), (4) tests-only adjustments.
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Clean
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Adequate
- Code clarity - Good
Automated review by Codex AI
There was a problem hiding this comment.
- Reviewed PR
#990(“release v0.6.7”) across the 6 perspectives (comments/docs, tests, error handling, types, general correctness/security/perf, simplification), scoped to changed lines only. - PR size computed as
XL(9254 additions + 174 deletions = 9428 lines changed; 73 files) and labelsize/XLapplied. - Posted the required summary review via
gh pr review --comment; no inline review comments were created because no diff-line findings met the ≥80 confidence threshold after validation.
There was a problem hiding this comment.
Code Review Summary
Release v0.6.7 introduces two well-scoped features (high-concurrency mode and provider session-reuse opt-out) with dashboard UI adaptations. The implementation is thorough: both features include multi-layer DB migration fallbacks, per-request gating in the proxy pipeline, comprehensive test coverage across guards/forwarder/response-handler/hedge paths, and complete i18n across all 5 locales. No significant issues were identified.
PR Size: XL
- Lines changed: 9,428 (9,254 additions + 174 deletions)
- Files changed: 73
- Note: Large size is primarily due to Drizzle snapshot JSON files (~8K lines) and i18n locale files (~800 lines). Core logic changes are ~500 lines across ~20 source files.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Observations (Below Threshold)
These observations were evaluated but did not meet the 80% confidence reporting threshold:
-
[PERFORMANCE]
updateSessionBindingSmartnot gated bydisableSessionReuse(Confidence: 70) —forwarder.ts:1169-1178andforwarder.ts:3376-3382. When a provider hasdisableSessionReuse = true, the binding is still written to Redis on success but cleared on the nextfindReusable()call. This is one extra Redis write per request. Functionally correct by design (clear-on-read pattern), but a potential optimization for future consideration. -
[CODE] Redundant
setHighConcurrencyModeEnabledcall (Confidence: 50) —proxy-handler.ts:21-29sets the flag, thensession-guard.ts:54sets it again from the same cached value. The early call inproxy-handler.tsserves as a safety net if the guard pipeline short-circuits, which is intentional.
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Clean
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Comprehensive (15+ test files modified/added)
- Code clarity - Good
- i18n completeness - All 5 locales covered
Automated review by Claude AI
* fix(proxy): preserve hedge model redirect audit per attempt * test(proxy): cover hedge redirect audit follow-ups
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![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: 7ea7ba490f
ℹ️ 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".
| if (session.shouldTrackSessionObservability()) { | ||
| await SessionManager.updateSessionProvider(session.sessionId!, { | ||
| providerId: attempt.provider.id, | ||
| providerName: attempt.provider.name, | ||
| }); |
There was a problem hiding this comment.
This block now gates updateSessionProvider on observability, but the sticky write just above (updateSessionBindingSmart) still runs unconditionally, so providers with disableSessionReuse=true still write a session binding on success and then immediately clear it on the next findReusable call. In workloads that enable this opt-out specifically to reduce sticky behavior (and often Redis churn), that creates an avoidable write/delete cycle per request and undercuts the performance benefit; please guard the binding write with the provider opt-out flag here (and in the non-hedge success path as well).
Useful? React with 👍 / 👎.
* fix: sanitize db errors before toast display Amp-Thread-ID: https://ampcode.com/threads/T-019d57c3-5a72-714b-a247-e56b1a7d62d3 Co-authored-by: Amp <amp@ampcode.com> * fix: localize sanitized toast fallback Amp-Thread-ID: https://ampcode.com/threads/T-019d57c3-5a72-714b-a247-e56b1a7d62d3 Co-authored-by: Amp <amp@ampcode.com> * fix: refresh sanitized toast fallback by locale Amp-Thread-ID: https://ampcode.com/threads/T-019d57c3-5a72-714b-a247-e56b1a7d62d3 Co-authored-by: Amp <amp@ampcode.com> --------- Co-authored-by: Amp <amp@ampcode.com>
🧪 测试结果
总体结果: ✅ 所有测试通过 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/utils/user-visible-error.ts (1)
32-47:stripStructuredDatabasePayload可考虑简化当前实现使用数组过滤单个候选位置，逻辑正确但略显冗余。由于只有
markerMatch?.index一个来源，可以简化为直接判断：可选简化
function stripStructuredDatabasePayload(message: string): string { const markerMatch = message.match(DATABASE_DETAIL_MARKER); - const cutIndexCandidates = [markerMatch?.index].filter( - (value): value is number => typeof value === "number" && value >= 0 - ); - - if (cutIndexCandidates.length === 0) { + const cutIndex = markerMatch?.index; + + if (cutIndex === undefined || cutIndex < 0) { return message; } - const cutIndex = Math.min(...cutIndexCandidates); return message .slice(0, cutIndex) .replace(/[\s:;,-]+$/g, "") .trim(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/utils/user-visible-error.ts` around lines 32 - 47, The function stripStructuredDatabasePayload uses an unnecessary array/filter step to obtain markerMatch?.index; simplify by directly checking markerMatch and its index (from markerMatch.index), return original message if it's undefined or negative, otherwise compute cutIndex = markerMatch.index and proceed with the slice/.replace/.trim logic; refer to DATABASE_DETAIL_MARKER, markerMatch, and stripStructuredDatabasePayload to locate and update the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/utils/user-visible-error.ts`:
- Around line 32-47: The function stripStructuredDatabasePayload uses an
unnecessary array/filter step to obtain markerMatch?.index; simplify by directly
checking markerMatch and its index (from markerMatch.index), return original
message if it's undefined or negative, otherwise compute cutIndex =
markerMatch.index and proceed with the slice/.replace/.trim logic; refer to
DATABASE_DETAIL_MARKER, markerMatch, and stripStructuredDatabasePayload to
locate and update the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 17b7d719-56eb-498a-9cb6-645db04b7e6d
📒 Files selected for processing (4)
src/components/ui/sonner.tsxsrc/lib/hooks/use-server-action.tssrc/lib/utils/user-visible-error.test.tssrc/lib/utils/user-visible-error.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44c6a6eea0
ℹ️ 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".
| if (session.shouldTrackSessionObservability()) { | ||
| void SessionTracker.refreshSession(session.sessionId, key.id, provider.id, user.id).catch( | ||
| (error) => { |
There was a problem hiding this comment.
Gating SessionTracker.refreshSession behind shouldTrackSessionObservability() makes high-concurrency mode stop refreshing sticky-session TTLs, but refreshSession is also what extends session:${sessionId}:provider/session:${sessionId}:key expirations (src/lib/session-tracker.ts, lines 205-207). In normal request flow, updateSessionBindingSmart often takes the SET NX path and does not rewrite an existing binding, so with this guard enabled the binding can expire after ~5 minutes during an active conversation; once that happens findReusable() falls back to fresh provider selection, causing provider churn and lost stickiness/context behavior.
Useful? React with 👍 / 👎.
* feat: support provider model redirect rules * fix: tighten provider redirect rule validation * chore: format code (feat-provider-model-routing-rules-5821b81) * fix: harden provider redirect validation --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
🧪 测试结果
总体结果: ✅ 所有测试通过 |
There was a problem hiding this comment.
🧹 Nitpick comments (9)
src/app/[locale]/settings/providers/_components/model-multi-select.tsx (2)
373-386: CommandItem 和 Checkbox 存在重复事件绑定。
CommandItem.onSelect和Checkbox.onCheckedChange都绑定了handleRemoveSelectedModel(index)。虽然 cmdk 库通常会阻止事件冒泡避免重复触发，但建议移除 Checkbox 上的onCheckedChange，仅依赖CommandItem.onSelect，以明确事件处理责任。建议移除 Checkbox 上的 onCheckedChange
<CommandItem key={`selected:${index}:${model}`} value={model} onSelect={() => handleRemoveSelectedModel(index)} className="cursor-pointer" > <Checkbox checked={true} className="mr-2" - onCheckedChange={() => handleRemoveSelectedModel(index)} /> <span className="font-mono text-sm flex-1">{model}</span> <Check className="h-4 w-4 text-primary" /> </CommandItem>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/model-multi-select.tsx around lines 373 - 386, The CommandItem and Checkbox both call handleRemoveSelectedModel(index), causing duplicate event handling; remove the onCheckedChange prop from the Checkbox and let CommandItem.onSelect handle model removal exclusively. Update the JSX where CommandItem, Checkbox, and handleRemoveSelectedModel are used so only CommandItem.onSelect invokes handleRemoveSelectedModel(index), leaving Checkbox with just checked and className props.
467-483: 冗余的onInput事件处理器。
onChange已经足够处理 React 受控输入的值更新，onInput处理器是多余的，可以移除以简化代码。建议移除冗余处理器
<Input value={editValue} data-model-edit-input={model} onChange={(e) => setEditValue(e.target.value)} - onInput={(e) => setEditValue((e.target as HTMLInputElement).value)} onKeyDown={(e) => { if (e.key === "Enter") { e.preventDefault(); handleSaveEditSelectedModel(index); } else if (e.key === "Escape") { e.preventDefault(); handleCancelEditSelectedModel(); } }} className="font-mono text-sm h-8 flex-1" autoFocus />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/model-multi-select.tsx around lines 467 - 483, Remove the redundant onInput handler from the Input used for inline model editing: the controlled value is already updated by onChange via setEditValue, so delete the onInput={(e) => setEditValue((e.target as HTMLInputElement).value)} handler in the Input component (the one using editValue and setEditValue) and keep the existing onChange, keydown logic (handleSaveEditSelectedModel, handleCancelEditSelectedModel), data-model-edit-input attribute, className and autoFocus as-is.src/types/message.ts (1)
95-99: 建议复用ProviderModelRedirectMatchType类型，避免重复定义。
matchType的联合类型"exact" | "prefix" | "suffix" | "contains" | "regex"与src/types/provider.ts:49-55中定义的ProviderModelRedirectMatchType完全一致。内联重复定义会增加维护负担，且两处定义可能随时间产生偏差。♻️ 建议的重构
首先在文件顶部添加导入：
import type { Numeric } from "decimal.js-light"; import type { CacheTtlApplied } from "./cache"; -import type { ProviderType } from "./provider"; +import type { ProviderModelRedirectMatchType, ProviderType } from "./provider"; import type { SpecialSetting } from "./special-settings";然后修改
matchedRule的类型定义：matchedRule?: { - matchType: "exact" | "prefix" | "suffix" | "contains" | "regex"; + matchType: ProviderModelRedirectMatchType; source: string; target: string; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types/message.ts` around lines 95 - 99, Replace the inline union type on the matchedRule.matchType with the shared ProviderModelRedirectMatchType type: update the matchedRule property (in the interface/type that declares matchedRule) to import and use ProviderModelRedirectMatchType instead of the literal union, ensuring you add an import for ProviderModelRedirectMatchType at the top and keep the matchedRule structure (source and target) unchanged.src/app/[locale]/settings/providers/_components/model-redirect-editor.tsx (2)
265-282: 冗余的事件处理器。同时绑定
onChange和onInput处理相同的状态更新逻辑是冗余的。onChange在大多数场景下已足够，onInput仅在需要实时（每次按键）更新时才需要。如果是为了特定浏览器兼容性，建议添加注释说明原因。
建议移除冗余的 onInput 处理器
<Input value={editRule.source} data-redirect-edit-source={ruleKey} onChange={(e) => setEditRule((current) => ({ ...current, source: e.target.value })) } - onInput={(e) => - setEditRule((current) => ({ - ...current, - source: (e.target as HTMLInputElement).value, - })) - } onKeyDown={(e) => handleEditKeyDown(e, ruleKey)}Also applies to: 290-306
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/model-redirect-editor.tsx around lines 265 - 282, The Input component registers both onChange and onInput updating the same state (setEditRule -> source), which is redundant; remove the onInput handler from the Input that uses editRule.source and setEditRule (and similarly for the other instance around lines 290-306) so only onChange updates state, or if you truly need per-keystroke updates retain onInput but add a comment explaining the browser/behavior reason; keep other props (onKeyDown -> handleEditKeyDown, data-redirect-edit-source -> ruleKey, disabled) unchanged.
447-461: 同样存在冗余的 onInput 处理器。新增规则表单的输入框也同时绑定了
onChange和onInput，建议保持一致性处理。Also applies to: 472-486
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/model-redirect-editor.tsx around lines 447 - 461, The Input for creating new rules binds both onChange and onInput redundantly (see the Input with id "new-source" that updates newRule via setNewRule and the similar Input around lines 472-486); remove one of the handlers to keep a single, consistent update path (prefer using onChange) and ensure both Inputs (source and the other at 472-486) only call setNewRule from that single handler so state updates occur once and consistently; keep handleCreateKeyDown, placeholder, disabled and className as-is.tests/unit/actions/providers-patch-contract.test.ts (1)
137-141: 可选：补充错误码断言以提高回归信号强度当前负例只断言
field。建议再断言code，避免未来因其它错误路径触发同字段时报错而“误通过”。可参考的断言增强
expect(result.ok).toBe(false); if (result.ok) return; + expect(result.error.code).toBe(PROVIDER_PATCH_ERROR_CODES.INVALID_PATCH_SHAPE); expect(result.error.field).toBe("model_redirects");Also applies to: 150-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/actions/providers-patch-contract.test.ts` around lines 137 - 141, The negative-path tests currently only assert result.error.field; update the assertions to also check result.error.code to strengthen regression detection: locate the assertions around expect(result.ok).toBe(false) / expect(result.error.field).toBe("model_redirects") in tests/unit/actions/providers-patch-contract.test.ts (and the similar block later), and add an assertion that result.error.code equals the expected error code string/enum for this validation failure so the test fails if a different error path triggers the same field.src/app/[locale]/settings/providers/_components/forms/provider-form/index.tsx (1)
492-493: 建议把disableSessionReuse纳入 Routing 的“configured”判定当前只有模型白名单/重定向会点亮 routing 状态；如果用户只开启“禁用会话复用”，导航状态仍显示未配置，反馈会偏弱。
可参考的调整
- if (state.routing.allowedModels.length > 0 || state.routing.modelRedirects.length > 0) { + if ( + state.routing.allowedModels.length > 0 || + state.routing.modelRedirects.length > 0 || + state.routing.disableSessionReuse + ) { status.routing = "configured"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/forms/provider-form/index.tsx around lines 492 - 493, 当前的 routing 状态只在 state.routing.allowedModels 或 state.routing.modelRedirects 非空时置为 "configured"，应当把禁用会话复用也纳入判定；在设置 status.routing = "configured" 的条件中加入对 state.routing.disableSessionReuse（或相应字段）的布尔检查（e.g. || state.routing.disableSessionReuse === true），以便只要用户开启 disableSessionReuse 就把 routing 标记为已配置。src/app/api/actions/[...route]/route.ts (1)
38-38: 建议直接复用规则列表 Schema，避免约束漂移当前写法是
z.array(PROVIDER_MODEL_REDIRECT_RULE_SCHEMA)，而重定向规则在公共层已有列表级约束（如数量上限、去重）。这里复用列表 Schema 会更稳，避免文档层与校验层后续出现分叉。可参考的调整
-import { PROVIDER_MODEL_REDIRECT_RULE_SCHEMA } from "@/lib/provider-model-redirect-schema"; +import { PROVIDER_MODEL_REDIRECT_RULE_LIST_SCHEMA } from "@/lib/provider-model-redirect-schema"; ... - modelRedirects: z - .array(PROVIDER_MODEL_REDIRECT_RULE_SCHEMA) - .nullable() - .describe("模型重定向规则列表"), + modelRedirects: PROVIDER_MODEL_REDIRECT_RULE_LIST_SCHEMA + .nullable() + .describe("模型重定向规则列表"),Also applies to: 542-545
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/actions/`[...route]/route.ts at line 38, Replace the per-item schema import/usage with the existing list-level schema to preserve shared constraints: instead of importing and using PROVIDER_MODEL_REDIRECT_RULE_SCHEMA wrapped in z.array(...) in route.ts, import and use the shared list schema (e.g. PROVIDER_MODEL_REDIRECT_RULES_SCHEMA) wherever you currently call z.array(PROVIDER_MODEL_REDIRECT_RULE_SCHEMA); also update the other occurrences noted (around lines 542-545) to use the list schema so count/uniqueness/other list-level constraints are consistently enforced.src/lib/provider-patch-contract.ts (1)
10-10: 请改用@/路径别名导入 schema。这里新增了相对路径导入，和仓库 TS/JS 导入规范不一致，建议统一成别名导入以避免风格漂移。
建议修改
-import { PROVIDER_MODEL_REDIRECT_RULE_LIST_SCHEMA } from "./provider-model-redirect-schema"; +import { PROVIDER_MODEL_REDIRECT_RULE_LIST_SCHEMA } from "@/lib/provider-model-redirect-schema";As per coding guidelines "Use path alias
@/mapped to ./src/ for imports".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/provider-patch-contract.ts` at line 10, 将相对导入替换为仓库约定的别名导入：把当前文件中的相对导入 PROVIDER_MODEL_REDIRECT_RULE_LIST_SCHEMA 从 "./provider-model-redirect-schema" 改为使用 "@/lib/provider-model-redirect-schema"（保持导出符号 PROVIDER_MODEL_REDIRECT_RULE_LIST_SCHEMA 不变），以遵循项目中 "Use path alias `@/` mapped to ./src/" 的导入规范并避免风格漂移。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/app/`[locale]/settings/providers/_components/forms/provider-form/index.tsx:
- Around line 492-493: 当前的 routing 状态只在 state.routing.allowedModels 或
state.routing.modelRedirects 非空时置为 "configured"，应当把禁用会话复用也纳入判定；在设置
status.routing = "configured" 的条件中加入对
state.routing.disableSessionReuse（或相应字段）的布尔检查（e.g. ||
state.routing.disableSessionReuse === true），以便只要用户开启 disableSessionReuse 就把
routing 标记为已配置。
In `@src/app/`[locale]/settings/providers/_components/model-multi-select.tsx:
- Around line 373-386: The CommandItem and Checkbox both call
handleRemoveSelectedModel(index), causing duplicate event handling; remove the
onCheckedChange prop from the Checkbox and let CommandItem.onSelect handle model
removal exclusively. Update the JSX where CommandItem, Checkbox, and
handleRemoveSelectedModel are used so only CommandItem.onSelect invokes
handleRemoveSelectedModel(index), leaving Checkbox with just checked and
className props.
- Around line 467-483: Remove the redundant onInput handler from the Input used
for inline model editing: the controlled value is already updated by onChange
via setEditValue, so delete the onInput={(e) => setEditValue((e.target as
HTMLInputElement).value)} handler in the Input component (the one using
editValue and setEditValue) and keep the existing onChange, keydown logic
(handleSaveEditSelectedModel, handleCancelEditSelectedModel),
data-model-edit-input attribute, className and autoFocus as-is.
In `@src/app/`[locale]/settings/providers/_components/model-redirect-editor.tsx:
- Around line 265-282: The Input component registers both onChange and onInput
updating the same state (setEditRule -> source), which is redundant; remove the
onInput handler from the Input that uses editRule.source and setEditRule (and
similarly for the other instance around lines 290-306) so only onChange updates
state, or if you truly need per-keystroke updates retain onInput but add a
comment explaining the browser/behavior reason; keep other props (onKeyDown ->
handleEditKeyDown, data-redirect-edit-source -> ruleKey, disabled) unchanged.
- Around line 447-461: The Input for creating new rules binds both onChange and
onInput redundantly (see the Input with id "new-source" that updates newRule via
setNewRule and the similar Input around lines 472-486); remove one of the
handlers to keep a single, consistent update path (prefer using onChange) and
ensure both Inputs (source and the other at 472-486) only call setNewRule from
that single handler so state updates occur once and consistently; keep
handleCreateKeyDown, placeholder, disabled and className as-is.
In `@src/app/api/actions/`[...route]/route.ts:
- Line 38: Replace the per-item schema import/usage with the existing list-level
schema to preserve shared constraints: instead of importing and using
PROVIDER_MODEL_REDIRECT_RULE_SCHEMA wrapped in z.array(...) in route.ts, import
and use the shared list schema (e.g. PROVIDER_MODEL_REDIRECT_RULES_SCHEMA)
wherever you currently call z.array(PROVIDER_MODEL_REDIRECT_RULE_SCHEMA); also
update the other occurrences noted (around lines 542-545) to use the list schema
so count/uniqueness/other list-level constraints are consistently enforced.
In `@src/lib/provider-patch-contract.ts`:
- Line 10: 将相对导入替换为仓库约定的别名导入：把当前文件中的相对导入
PROVIDER_MODEL_REDIRECT_RULE_LIST_SCHEMA 从 "./provider-model-redirect-schema"
改为使用 "@/lib/provider-model-redirect-schema"（保持导出符号
PROVIDER_MODEL_REDIRECT_RULE_LIST_SCHEMA 不变），以遵循项目中 "Use path alias `@/` mapped to
./src/" 的导入规范并避免风格漂移。
In `@src/types/message.ts`:
- Around line 95-99: Replace the inline union type on the matchedRule.matchType
with the shared ProviderModelRedirectMatchType type: update the matchedRule
property (in the interface/type that declares matchedRule) to import and use
ProviderModelRedirectMatchType instead of the literal union, ensuring you add an
import for ProviderModelRedirectMatchType at the top and keep the matchedRule
structure (source and target) unchanged.
In `@tests/unit/actions/providers-patch-contract.test.ts`:
- Around line 137-141: The negative-path tests currently only assert
result.error.field; update the assertions to also check result.error.code to
strengthen regression detection: locate the assertions around
expect(result.ok).toBe(false) /
expect(result.error.field).toBe("model_redirects") in
tests/unit/actions/providers-patch-contract.test.ts (and the similar block
later), and add an assertion that result.error.code equals the expected error
code string/enum for this validation failure so the test fails if a different
error path triggers the same field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 39c414a4-55e4-4644-8443-fb7cc0f11387
📒 Files selected for processing (43)
messages/en/settings/providers/form/modelRedirect.jsonmessages/en/settings/providers/form/modelSelect.jsonmessages/ja/settings/providers/form/modelRedirect.jsonmessages/ja/settings/providers/form/modelSelect.jsonmessages/ru/settings/providers/form/modelRedirect.jsonmessages/ru/settings/providers/form/modelSelect.jsonmessages/zh-CN/settings/providers/form/modelRedirect.jsonmessages/zh-CN/settings/providers/form/modelSelect.jsonmessages/zh-TW/settings/providers/form/modelRedirect.jsonmessages/zh-TW/settings/providers/form/modelSelect.jsonsrc/actions/providers.tssrc/app/[locale]/settings/providers/_components/batch-edit/analyze-batch-settings.tssrc/app/[locale]/settings/providers/_components/batch-edit/build-patch-draft.tssrc/app/[locale]/settings/providers/_components/forms/provider-form.legacy.tsxsrc/app/[locale]/settings/providers/_components/forms/provider-form/index.tsxsrc/app/[locale]/settings/providers/_components/forms/provider-form/provider-form-context.tsxsrc/app/[locale]/settings/providers/_components/forms/provider-form/provider-form-types.tssrc/app/[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsxsrc/app/[locale]/settings/providers/_components/model-multi-select.tsxsrc/app/[locale]/settings/providers/_components/model-redirect-editor.tsxsrc/app/api/actions/[...route]/route.tssrc/app/v1/_lib/proxy/model-redirector.tssrc/drizzle/schema.tssrc/lib/provider-model-redirect-schema.tssrc/lib/provider-model-redirects.tssrc/lib/provider-patch-contract.tssrc/lib/validation/schemas.tssrc/repository/_shared/transformers.tssrc/repository/provider.tssrc/types/message.tssrc/types/provider.tstests/api/providers-actions.test.tstests/integration/batch-edit-prefill.test.tstests/unit/actions/providers-batch-field-mapping.test.tstests/unit/actions/providers-patch-contract.test.tstests/unit/batch-edit/analyze-batch-settings.test.tstests/unit/dashboard/provider-form-clone-deep-copy.test.tstests/unit/lib/provider-model-redirects.test.tstests/unit/proxy/proxy-forwarder-hedge-first-byte.test.tstests/unit/settings/providers/build-patch-draft.test.tstests/unit/settings/providers/model-multi-select-custom-models-ui.test.tsxtests/unit/settings/providers/model-redirect-editor.test.tsxtests/unit/settings/providers/provider-form-batch-context.test.ts
✅ Files skipped from review due to trivial changes (11)
- messages/ja/settings/providers/form/modelSelect.json
- messages/en/settings/providers/form/modelRedirect.json
- messages/ja/settings/providers/form/modelRedirect.json
- messages/en/settings/providers/form/modelSelect.json
- messages/zh-CN/settings/providers/form/modelSelect.json
- messages/zh-TW/settings/providers/form/modelSelect.json
- messages/zh-TW/settings/providers/form/modelRedirect.json
- messages/zh-CN/settings/providers/form/modelRedirect.json
- tests/unit/settings/providers/build-patch-draft.test.ts
- messages/ru/settings/providers/form/modelSelect.json
- messages/ru/settings/providers/form/modelRedirect.json
🚧 Files skipped from review as they are similar to previous changes (10)
- src/app/[locale]/settings/providers/_components/batch-edit/build-patch-draft.ts
- tests/unit/batch-edit/analyze-batch-settings.test.ts
- src/app/[locale]/settings/providers/_components/batch-edit/analyze-batch-settings.ts
- src/repository/_shared/transformers.ts
- src/repository/provider.ts
- src/app/[locale]/settings/providers/_components/forms/provider-form/provider-form-types.ts
- src/lib/validation/schemas.ts
- src/drizzle/schema.ts
- src/app/v1/_lib/proxy/model-redirector.ts
- src/types/provider.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd9751c3b1
ℹ️ 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".
| preserve_client_ip: z.boolean().optional().default(false), | ||
| model_redirects: z.record(z.string(), z.string()).nullable().optional(), | ||
| disable_session_reuse: z.boolean().optional().default(false), | ||
| model_redirects: PROVIDER_MODEL_REDIRECT_RULES_SCHEMA, |
There was a problem hiding this comment.
Switching model_redirects to PROVIDER_MODEL_REDIRECT_RULES_SCHEMA here (and in UpdateProviderSchema) removes support for the previously valid object-map payload shape, so existing API clients sending { "model_redirects": { "old": "new" } } will now fail validation before normalization runs. This is a breaking behavior change in the provider create/update APIs; keeping a union schema (legacy map + rule list) and normalizing would avoid regressing existing integrations.
Useful? React with 👍 / 👎.
Amp-Thread-ID: https://ampcode.com/threads/T-019d5928-dd67-740a-86dd-27e8f6eb2f63 Co-authored-by: Amp <amp@ampcode.com>
🧪 测试结果
总体结果: ✅ 所有测试通过 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/`[locale]/settings/providers/_components/model-multi-select.tsx:
- Around line 126-129: The "Select All" handler (selectAll) currently writes
only availableModels back and thus silently drops manually added items; change
selectAll to merge the current selectedModels with the available/filtered list
(or write back the deduplicated displayedModels) so manually-added whitelist
entries are preserved—use displayedModels or Set([...availableModels,
...selectedModels]) to produce the final unique selection and update the state
accordingly (referencing displayedModels, availableDisplayedModels,
availableModels, selectedModels, and selectAll).
- Around line 102-104: The code uses editingIndex (and setEditingIndex) to track
the row being edited which breaks when selectedModels shifts; replace the
index-based state with a stable identifier (e.g., editingModelId or
editingModelValue: string | null) and update all uses (isEditing check, save
logic that currently reads/editValue, delete logic that compared indices) to
compare the stable id against selectedModels items instead of an index, or
alternatively clear editing state whenever selectedModels changes (useEffect
watching selectedModels to call setEditingIndex(null)/setEditValue("") or
setEditingModelId(null)); update setEditingIndex calls to setEditingModelId, and
ensure save/delete paths use the stable id to find the target model before
mutating selectedModels.
- Around line 326-343: The icon-only buttons (e.g., the Refresh button with
<RefreshCw> and other icon Buttons in model-multi-select.tsx) lack accessible
names; add localized aria-label attributes using the existing i18n function (t)
— e.g., aria-label={t("refresh")} — for the Refresh, Add, Edit, Save, Cancel,
and Delete icon Buttons and do not rely on Tooltip text as the accessible name;
also add corresponding i18n keys/translations for these labels in the
localization resources (zh-CN, zh-TW, en, ja, ru) so every user-facing string is
localized.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 665ef952-c47d-495e-ac0b-5f1439b87512
📒 Files selected for processing (7)
messages/en/settings/providers/form/modelSelect.jsonmessages/ja/settings/providers/form/modelSelect.jsonmessages/ru/settings/providers/form/modelSelect.jsonmessages/zh-CN/settings/providers/form/modelSelect.jsonmessages/zh-TW/settings/providers/form/modelSelect.jsonsrc/app/[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsxsrc/app/[locale]/settings/providers/_components/model-multi-select.tsx
✅ Files skipped from review due to trivial changes (5)
- messages/ja/settings/providers/form/modelSelect.json
- messages/zh-TW/settings/providers/form/modelSelect.json
- messages/en/settings/providers/form/modelSelect.json
- messages/ru/settings/providers/form/modelSelect.json
- messages/zh-CN/settings/providers/form/modelSelect.json
| <Button | ||
| size="icon" | ||
| variant="ghost" | ||
| className="h-6 w-6" | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| loadModels(); | ||
| }} | ||
| type="button" | ||
| > | ||
| <RefreshCw className="h-3 w-3" /> | ||
| </Button> | ||
| </TooltipTrigger> | ||
| <TooltipContent side="top"> | ||
| <p className="text-xs">{t("refresh")}</p> | ||
| </TooltipContent> | ||
| </Tooltip> | ||
| </TooltipProvider> |
There was a problem hiding this comment.
图标按钮需要可访问名称。
Line 326、432、483-529 新增的按钮只有图标，没有 aria-label 或可见文本。读屏用户无法分辨“刷新 / 添加 / 编辑 / 保存 / 取消 / 删除”，这会让模型管理流程基本不可用；tooltip 不能替代按钮名称。请补上本地化的可访问名称。
As per coding guidelines "All user-facing strings must use i18n (5 languages supported: zh-CN, zh-TW, en, ja, ru). Never hardcode display text".
Also applies to: 432-439, 483-529
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/`[locale]/settings/providers/_components/model-multi-select.tsx
around lines 326 - 343, The icon-only buttons (e.g., the Refresh button with
<RefreshCw> and other icon Buttons in model-multi-select.tsx) lack accessible
names; add localized aria-label attributes using the existing i18n function (t)
— e.g., aria-label={t("refresh")} — for the Refresh, Add, Edit, Save, Cancel,
and Delete icon Buttons and do not rely on Tooltip text as the accessible name;
also add corresponding i18n keys/translations for these labels in the
localization resources (zh-CN, zh-TW, en, ja, ru) so every user-facing string is
localized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e86cad96f2
ℹ️ 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".
| case "model_redirects": | ||
| return isStringRecord(value); | ||
| return PROVIDER_MODEL_REDIRECT_RULE_LIST_SCHEMA.safeParse(value).success; |
There was a problem hiding this comment.
model_redirects batch patch validation now only accepts the new rule-array shape, so previously valid payloads like { "model_redirects": { "old": "new" } } are rejected before any normalization runs. This is a breaking regression for existing batch-edit/API clients, especially because the codebase still contains a legacy-map normalizer (src/lib/provider-model-redirects.ts) and schema types that support that format; accepting a union (legacy map + rule list) here would keep backward compatibility.
Useful? React with 👍 / 👎.
…on (#996) * feat: add advanced provider model allowlist simulator * fix: use i18n-friendly error codes in dispatch actions * fix: keep dispatch simulator on selected priority tier * chore: format code (feat-provider-whitelist-match-simulator-92f0f2f) * fix: address provider review follow-ups * fix: address provider bot review findings * refactor: remove unused legacy provider form --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
🧪 测试结果
总体结果: ✅ 所有测试通过 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c604df186a
ℹ️ 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".
| (rules) => { | ||
| const keys = new Set<string>(); | ||
| for (const rule of rules) { | ||
| const key = `${rule.matchType}:${rule.source.trim().toLowerCase()}`; |
There was a problem hiding this comment.
PROVIDER_MODEL_REDIRECT_RULE_LIST_SCHEMA lowercases source when building the duplicate key, but runtime rule matching is case-sensitive (matchesPattern in src/lib/model-pattern-matcher.ts). This means valid rules that differ only by case (for example two regex patterns like [A-Z]+ vs [a-z]+, or two exact sources with different casing) are rejected as duplicates during validation even though they match different model names, preventing legitimate redirect configs from being saved.
Useful? React with 👍 / 👎.
| (rules) => { | ||
| const keys = new Set<string>(); | ||
| for (const rule of rules) { | ||
| const key = `${rule.matchType}:${rule.pattern.trim().toLowerCase()}`; |
There was a problem hiding this comment.
The allowed-model schema also lowercases pattern for duplicate detection, while model matching remains case-sensitive at runtime. In practice, case-distinct rules become unsavable (especially regex patterns that intentionally separate uppercase/lowercase matches), so valid allowlist configurations are rejected before they can be applied.
Useful? React with 👍 / 👎.
… feature - Remove one-click upstream model import from AllowedModelRuleEditor (button, handler, 5 props) - Delete dead code model-multi-select.tsx (520 lines, zero importers) and its test - Add badge prop to FieldGroup component for inline rule counts - Replace conditional tester rendering with Collapsible panels (stable layout) - Tighten spacing, add separators, compact client restrictions hint - Polish dispatch simulator: funnel opacity, stronger tier highlight, compact cards - Add testRule i18n key across 5 languages Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
🧪 测试结果
总体结果: ✅ 所有测试通过 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa08ed37a5
ℹ️ 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".
| return (normalizeAllowedModelRules(provider.allowedModels) ?? []) | ||
| .filter((rule) => rule.matchType === "exact") | ||
| .map((rule) => ({ id: rule.pattern })); |
There was a problem hiding this comment.
This early return now drops every non-exact allowed-model rule, so a provider configured with only prefix/contains/regex rules will return an empty model list even when upstream has matching models. Because this branch also skips upstream fetching entirely, /v1/models (and any UI built on it) becomes misleading or unusable for the new rule-based allowlist configs; this should only shortcut when rules are fully enumerable (e.g., all exact), otherwise fetch upstream and filter.
Useful? React with 👍 / 👎.
| case "model_redirects": | ||
| return isStringRecord(value); | ||
| return PROVIDER_MODEL_REDIRECT_RULE_LIST_SCHEMA.safeParse(value).success; |
There was a problem hiding this comment.
This validation only checks safeParse(...).success, but unlike allowed_models, the parsed/trimmed result is never propagated into the normalized patch payload, so model_redirects keeps raw input.set values. In batch patch apply flows this lets whitespace-surrounded patterns persist (for example source: "gpt-4 "), which then changes runtime matching semantics and defeats the normalization guarantees implied by the schema.
Useful? React with 👍 / 👎.
…oup routing - Add DispatchSimulatorDialog entry to /dashboard/providers page - Fix group filter defaulting to "default" instead of bypassing all groups - Pre-select "default" group in simulator UI to match production behavior - Always include "default" in group options list Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e0f55fc28
ℹ️ 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".
| const parsedRedirectRules = PROVIDER_MODEL_REDIRECT_RULE_LIST_SCHEMA.safeParse( | ||
| updates.model_redirects | ||
| ); | ||
| if (!parsedRedirectRules.success) { | ||
| return { ok: false, error: "模型重定向规则格式无效" }; |
There was a problem hiding this comment.
batchUpdateProviders now validates updates.model_redirects only as the new rule-array shape, so existing callers that still send the previously accepted map payload (e.g. { "model_redirects": { "old": "new" } }) now get a hard validation error before normalization. This is a breaking regression on the direct batch-update path and bypasses the legacy compatibility that normalizeProviderModelRedirectRules was added to preserve on read paths.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| function getRuleIdentity(rule: Pick<ProviderModelRedirectRule, "matchType" | "source">): string { | ||
| return `${rule.matchType}:${rule.source.trim().toLowerCase()}`; |
There was a problem hiding this comment.
The editor dedupe key lowercases source for all match types, which makes case-distinct rules (especially regex) unsavable even though runtime matching is case-sensitive (matchesPattern in src/lib/model-pattern-matcher.ts). For example, ^GPT- and ^gpt- are treated as duplicates in the UI despite matching different model names, so valid redirect configurations are blocked before submission.
Useful? React with 👍 / 👎.
* feat: restore provider whitelist model picker * fix: harden provider whitelist model picker * test: align dispatch simulator default group expectations
🧪 测试结果
总体结果: ✅ 所有测试通过 |
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
1 participant
Summary
Release v0.6.7 bundles three features: a high-concurrency mode that disables expensive Redis observability writes, a per-provider session-reuse opt-out toggle, and dashboard UI adaptations for high-concurrency mode.
Included Changes
1. High-Concurrency Mode (#979)
Introduces an
enableHighConcurrencyModesystem setting (defaultfalse). When enabled, non-critical Redis writes (request body snapshots, session info, live chain tracking, provider binding updates, usage tracking) are skipped to reduce CPU/IO pressure under high RPM. Billing, quota enforcement, forwarding, and rectifiers remain fully active.src/app/v1/_lib/proxy/session.ts--setHighConcurrencyModeEnabled(),shouldPersistSessionDebugArtifacts(),shouldTrackSessionObservability()src/app/v1/_lib/proxy/{session-guard,forwarder,response-handler,response-fixer,warmup-guard}.ts-- guard calls gated on the two predicatessrc/repository/system-config.ts-- multi-layer column-fallback for pre-migration DBsdrizzle/0086_perpetual_runaways.sql-- migration addingenable_high_concurrency_modecolumnRelated to #854 -- reducing Redis write pressure addresses a contributing factor to async write queue saturation.
2. Provider Session Reuse Opt-Out (#980)
Adds a per-provider
disable_session_reuseflag (defaultfalse). When enabled for a provider,findReusable()clears the sticky session binding and falls through to normal priority/health-based provider selection instead of reusing the previously bound provider.src/app/v1/_lib/proxy/provider-selector.ts-- opt-out check infindReusable()src/types/provider.ts/src/lib/validation/schemas.ts/src/repository/provider.ts-- full type, validation, and repository wiringsrc/lib/provider-patch-contract.ts-- batch patch supportdrizzle/0087_nappy_lady_mastermind.sql-- migration addingdisable_session_reusecolumn3. Dashboard UI for High-Concurrency Mode (#988)
Adapts dashboard and usage-logs UI when
enableHighConcurrencyModeis active:Follow-up to #979 -- ensures session-dependent UI is not rendered when session observability is intentionally reduced.
Database Migrations
0086_perpetual_runaways.sqlsystem_settingsenable_high_concurrency_mode boolean DEFAULT false NOT NULL0087_nappy_lady_mastermind.sqlprovidersdisable_session_reuse boolean DEFAULT false NOT NULLTesting
Both features include comprehensive unit test coverage across proxy pipeline guards, provider selector, dashboard components, settings forms, batch edit logic, and repository operations. Integration tests verify billing accuracy is unaffected in high-concurrency mode.
Description enhanced by Claude AI
Greptile Summary
This release bundles three features: high-concurrency mode (skips non-critical Redis observability writes to reduce I/O under load), per-provider
disableSessionReuseopt-out, and dashboard UI adaptations. The proxy pipeline gating is applied consistently across all write sites, the multi-layer DB column fallback handles pre-migration deployments correctly, and thedisableSessionReusepath correctly clears the Redis binding before falling through to normal provider selection. Themodel_redirectsfield has been migrated fromRecord<string,string>to a structured rule array; existing DB records are transparently normalized on read, but the API now strictly requires the new format — a deliberate but breaking change for direct API clients.Confidence Score: 5/5
Safe to merge; all findings are P2 style/consistency issues that do not affect runtime correctness
Core logic is well-structured, gating predicates are applied consistently across the entire proxy pipeline, the multi-layer DB fallback chain is correct, and the disableSessionReuse flow properly clears Redis bindings. The only finding is a cosmetic inconsistency in one error response shape in a rarely-triggered validation path.
src/actions/providers.ts — model_redirects validation error response shape
Important Files Changed
Sequence Diagram
sequenceDiagram participant C as Client participant SG as SessionGuard participant PS as ProviderSelector participant MR as ModelRedirector participant PF as ProxyForwarder participant Redis as Redis C->>SG: Request SG->>SG: setHighConcurrencyModeEnabled() alt highConcurrencyMode=false SG->>Redis: storeSessionRequestBody() SG->>Redis: storeSessionInfo() SG->>Redis: trackSession() end SG->>PS: resolve provider PS->>Redis: getSessionProvider() [findReusable] alt disableSessionReuse=true PS->>Redis: clearSessionProvider() PS-->>SG: null (re-select) end PS->>MR: applyRedirect() MR->>MR: setCurrentModelRedirect() MR->>MR: attachCurrentModelRedirectToLastChainItem() PS->>PF: forward request alt highConcurrencyMode=false PF->>Redis: storeSessionSpecialSettings() PF->>Redis: updateSessionProvider() PF->>Redis: storeSessionRequestHeaders() end PF-->>C: Stream response alt highConcurrencyMode=false PF->>Redis: updateSessionUsage() PF->>Redis: storeSessionResponse() PF->>Redis: deleteLiveChain() endComments Outside Diff (1)
src/app/v1/_lib/proxy/forwarder.ts, line 1169-1178 (link)disableSessionReusenot respected at binding creationupdateSessionBindingSmartis called unconditionally on success, so a provider withdisableSessionReuse = truestill writes a new session binding to Redis on every successful request. That binding is then cleared at the top of the next request'sfindReusablecall, making it functionally correct but wasteful (one extra Redis write per request).Skipping the binding for opted-out providers avoids the unnecessary write:
The same guard should be applied at the hedge-winner path around line 3378.
Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (9): Last reviewed commit: "feat: restore provider whitelist model p..." | Re-trigger Greptile