Conversation
…ctions batch edit 1. Add /v1/responses/compact to STANDARD_ENDPOINTS in forwarder to prevent misclassification as MCP request and bypassing endpoint pool selection. 2. Invalidate Redis circuit breaker config cache and in-memory config cache after batch patch apply and undo when CB fields are changed, matching the single-provider editProvider behavior. 3. Add missing allowedClients/blockedClients handlers in buildPatchDraftFromFormState and isValidSetValue validation in provider-patch-contract, fixing silent drops in batch edit pipeline.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro Cache: Disabled due to Reviews > Disable Cache setting 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough新增对 routing.allowedClients / routing.blockedClients 的补丁草稿与验证支持，并在提供商补丁应用/撤销路径中新增对电路断路器字段变更时的 Redis 缓存清理；同时将 "/v1/responses/compact" 纳入标准代理端点列表并补充对应测试。 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Summary of ChangesHello @ding113, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces critical fixes across several areas to enhance system reliability and functionality. It ensures proper routing for a specific API endpoint, maintains data consistency for circuit breaker configurations during batch updates, and corrects the handling of client restriction fields in the batch editing interface. These changes collectively improve API request processing, prevent stale circuit breaker data, and ensure accurate application of client-specific access controls. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
github-actions
bot
added
size/M
![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
The pull request effectively addresses the three identified fixes: correctly handling the /v1/responses/compact endpoint, ensuring proper circuit breaker cache invalidation during batch operations, and integrating client restrictions (allowedClients, blockedClients) into the batch edit pipeline. The changes are well-implemented, following existing patterns, and are supported by comprehensive unit tests, which is excellent. No critical, high, or medium severity issues were found during the review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/app/[locale]/settings/providers/_components/batch-edit/build-patch-draft.ts (1)
61-74: 数组字段 clear/set 分支实现正确，建议后续抽取复用 helper。Line 61-74 与
allowedModels逻辑一致，行为正确。可后续把“空数组 clear / 非空 set”抽成小函数，减少重复分支。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/batch-edit/build-patch-draft.ts around lines 61 - 74, The array-field branches for routing.allowedClients and routing.blockedClients duplicate the same "empty -> clear / non-empty -> set" logic already used for allowedModels; extract that pattern into a small helper (e.g., applyArrayPatch(draftKey: string, sourceArray: any[])) and replace the two blocks that reference dirtyFields.has("routing.allowedClients") / dirtyFields.has("routing.blockedClients") and assign draft.allowed_clients / draft.blocked_clients with calls to that helper using state.routing.allowedClients and state.routing.blockedClients respectively so the clear/set decision is centralized and reused.src/actions/providers.ts (1)
2045-2065: 考虑将CB_PROVIDER_KEYS提取为模块级常量。当前在函数内部定义 Set，每次调用都会创建新实例。建议参照
CLAUDE_ONLY_FIELDS、CODEX_ONLY_FIELDS等的模式，将其提取为模块级常量以提高效率。建议的修改
在文件顶部（约 1578 行附近，与其他类似常量一起）添加：
const CB_PROVIDER_KEYS: ReadonlySet<string> = new Set([ "circuitBreakerFailureThreshold", "circuitBreakerOpenDuration", "circuitBreakerHalfOpenSuccessThreshold", ]);然后在
undoProviderPatch中直接使用该常量：- const CB_PROVIDER_KEYS = new Set([ - "circuitBreakerFailureThreshold", - "circuitBreakerOpenDuration", - "circuitBreakerHalfOpenSuccessThreshold", - ]); const hasCbRevert = Object.values(snapshot.preimage).some((fields) => Object.keys(fields).some((k) => CB_PROVIDER_KEYS.has(k)) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/providers.ts` around lines 2045 - 2065, The Set CB_PROVIDER_KEYS is being recreated on each call inside undoProviderPatch; move it to module-level alongside CLAUDE_ONLY_FIELDS and CODEX_ONLY_FIELDS to avoid repeated allocations. Create a ReadonlySet<string> named CB_PROVIDER_KEYS at the top of the file and replace the in-function Set creation with a reference to that constant, leaving the logic that checks snapshot.preimage and the loop that calls deleteProviderCircuitConfig/clearConfigCache unchanged.
🤖 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/actions/providers.ts`:
- Around line 2045-2065: The Set CB_PROVIDER_KEYS is being recreated on each
call inside undoProviderPatch; move it to module-level alongside
CLAUDE_ONLY_FIELDS and CODEX_ONLY_FIELDS to avoid repeated allocations. Create a
ReadonlySet<string> named CB_PROVIDER_KEYS at the top of the file and replace
the in-function Set creation with a reference to that constant, leaving the
logic that checks snapshot.preimage and the loop that calls
deleteProviderCircuitConfig/clearConfigCache unchanged.
In
`@src/app/`[locale]/settings/providers/_components/batch-edit/build-patch-draft.ts:
- Around line 61-74: The array-field branches for routing.allowedClients and
routing.blockedClients duplicate the same "empty -> clear / non-empty -> set"
logic already used for allowedModels; extract that pattern into a small helper
(e.g., applyArrayPatch(draftKey: string, sourceArray: any[])) and replace the
two blocks that reference dirtyFields.has("routing.allowedClients") /
dirtyFields.has("routing.blockedClients") and assign draft.allowed_clients /
draft.blocked_clients with calls to that helper using
state.routing.allowedClients and state.routing.blockedClients respectively so
the clear/set decision is centralized and reused.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
📒 Files selected for processing (8)
src/actions/providers.tssrc/app/[locale]/settings/providers/_components/batch-edit/build-patch-draft.tssrc/app/v1/_lib/proxy/forwarder.tssrc/lib/provider-patch-contract.tstests/unit/actions/providers-batch-field-mapping.test.tstests/unit/actions/providers-patch-contract.test.tstests/unit/proxy/proxy-forwarder-endpoint-audit.test.tstests/unit/settings/providers/build-patch-draft.test.ts
Move circuit breaker provider key Set from undoProviderPatch function scope to module level, avoiding repeated allocation on each call.
github-project-automation
bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
![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
This PR addresses three bug fixes with proper error handling and test coverage for the primary changes.
PR Size: M
- Lines changed: 250
- Files changed: 8
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
-
CB Cache Invalidation (Fix 2): The implementation correctly matches the existing
editProviderpattern with proper try/catch error handling and logging. While direct unit tests for this specific cache invalidation are not included, the defensive coding approach ensures failures are logged without breaking the operation. -
Endpoint Addition (Fix 1): The addition of
/v1/responses/compacttoSTANDARD_ENDPOINTSis straightforward and properly tested. -
Client Restrictions (Fix 3): The field handlers and validation are correctly implemented and tested across the pipeline (form state, validation, mapping).
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Clean (defensive with logging)
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Adequate (14 new tests)
- Code clarity - Good
Automated review by Claude AI
|
|
||
| await publishProviderCacheInvalidation(); | ||
|
|
||
| const hasCbFieldChange = changedFields.some( |
There was a problem hiding this comment.
[HIGH] [TEST-MISSING-CRITICAL] Circuit breaker cache invalidation lacks unit test coverage
Why this is a problem: applyProviderBatchPatch/undoProviderPatch now invalidate Redis + in-memory circuit breaker config caches when CB fields change (src/actions/providers.ts:1917-1935, src/actions/providers.ts:2045-2065). CLAUDE.md requires: "Test Coverage - All new features must have unit test coverage of at least 80%". Without targeted unit tests, regressions in the CB-field detection or invalidation loop can ship unnoticed.
Suggested fix:
// tests/unit/actions/providers-apply-engine.test.ts
it("should invalidate circuit breaker config caches when CB fields change", async () => {
findAllProvidersFreshMock.mockResolvedValue([makeProvider(1), makeProvider(2)]);
updateProvidersBatchMock.mockResolvedValue(2);
const { clearConfigCache } = await import("@/lib/circuit-breaker");
await setupPreviewAndApply([1, 2], { circuit_breaker_failure_threshold: { set: 10 } });
expect(redisDelMock).toHaveBeenCalledWith("circuit_breaker:config:1");
expect(redisDelMock).toHaveBeenCalledWith("circuit_breaker:config:2");
expect(clearConfigCache).toHaveBeenCalledWith(1);
expect(clearConfigCache).toHaveBeenCalledWith(2);
});// tests/unit/actions/providers-undo-engine.test.ts
it("should invalidate circuit breaker config caches on undo when CB fields were reverted", async () => {
const providers = [makeProvider(1), makeProvider(2)];
const { undoToken, operationId, undoProviderPatch } = await setupPreviewApplyAndGetUndo(
providers,
[1, 2],
{ circuit_breaker_open_duration: { set: 1234 } }
);
const { clearConfigCache } = await import("@/lib/circuit-breaker");
redisMocks.del.mockClear();
clearConfigCache.mockClear();
updateProvidersBatchMock.mockResolvedValue(2);
await undoProviderPatch({ undoToken, operationId });
expect(redisMocks.del).toHaveBeenCalledWith("circuit_breaker:config:1");
expect(redisMocks.del).toHaveBeenCalledWith("circuit_breaker:config:2");
expect(clearConfigCache).toHaveBeenCalledWith(1);
expect(clearConfigCache).toHaveBeenCalledWith(2);
});
There was a problem hiding this comment.
Code Review Summary
This PR fixes /v1/responses/compact endpoint classification, batch-edit client restrictions, and circuit breaker cache invalidation. The main remaining risk is that the new circuit breaker cache invalidation branches in applyProviderBatchPatch and undoProviderPatch are currently untested, which makes regressions more likely despite being production-impacting.
PR Size: M
- Lines changed: 250
- Files changed: 8
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 | 1 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
None.
High Priority Issues (Should Fix)
- [TEST-MISSING-CRITICAL] Missing unit tests for circuit breaker cache invalidation in
applyProviderBatchPatchandundoProviderPatch(src/actions/providers.ts:1917-1935,src/actions/providers.ts:2045-2065). Confidence: 88/100
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
There was a problem hiding this comment.
- PR detected: #833
fix: responses/compact endpoint, CB cache invalidation, client restrictions batch edit(base:dev) - PR size labeled:
size/M(250 lines, 8 files) - Inline review comment posted:
- [HIGH] [TEST-MISSING-CRITICAL] Missing unit tests for the new CB cache invalidation branches in
src/actions/providers.ts(apply/undo)
- [HIGH] [TEST-MISSING-CRITICAL] Missing unit tests for the new CB cache invalidation branches in
- Summary review submitted via
gh pr review --commentwith the same issue + coverage table
1 participant
Summary
Three bug fixes for provider management and proxy routing:
/v1/responses/compacttoSTANDARD_ENDPOINTSin forwarder, preventing misclassification as MCP request and bypassing endpoint pool selectioneditProviderbehavior)allowedClients/blockedClientshandlers inbuildPatchDraftFromFormStateandisValidSetValuevalidation, fixing silent drops in batch edit pipelineProblem
Endpoint Routing: Requests to
/v1/responses/compactwere being treated as MCP requests because the endpoint was not inSTANDARD_ENDPOINTS. This caused them to bypass proper endpoint pool selection, leading to routing issues.Circuit Breaker Cache Stale: When batch editing providers and modifying circuit breaker fields (failure threshold, open duration, half-open success threshold), the in-memory and Redis caches were not being invalidated. This meant new CB configs wouldn't take effect until cache expired (5 minutes).
Client Restrictions Batch Edit: PR fix(ui): improve mobile provider form UX and add client restriction config #822 added UI for configuring
allowed_clients/blocked_clients, but the batch edit pipeline was missing handlers for these fields, causing them to be silently dropped during batch operations.Related Issues:
/v1/responses/compactendpoint, this PR addresses endpoint routing while 中转codex cli的remote compact异常的BUG #797 tracks parameter injection and response format issuesFollow-up to:
Solution
Fix 1: Standard Endpoints
Added
/v1/responses/compactto theSTANDARD_ENDPOINTSarray inforwarder.ts, ensuring the endpoint goes through proper endpoint pool selection.Fix 2: CB Cache Invalidation
Added cache invalidation logic in
applyProviderBatchPatchandundoProviderPatchthat:circuit_breaker_failure_threshold,circuit_breaker_open_duration,circuit_breaker_half_open_success_threshold) changeddeleteProviderCircuitConfig()to clear Redis cacheclearConfigCache()to clear in-memory cacheeditProviderFix 3: Client Restrictions Batch Support
allowed_clientsandblocked_clientshandlers inbuildPatchDraftFromFormStatefor form-to-draft conversionisValidSetValuefunctionChanges
Core Changes
src/app/v1/_lib/proxy/forwarder.ts- Added/v1/responses/compactto STANDARD_ENDPOINTSsrc/actions/providers.ts- Added CB cache invalidation in apply/undo batch operationssrc/app/[locale]/settings/providers/_components/batch-edit/build-patch-draft.ts- Added client restrictions handlerssrc/lib/provider-patch-contract.ts- Added validation for client restriction fieldsTest Coverage
tests/unit/proxy/proxy-forwarder-endpoint-audit.test.ts- Tests for/v1/responses/compactroutingtests/unit/actions/providers-batch-field-mapping.test.ts- Tests for client restrictions field mappingtests/unit/actions/providers-patch-contract.test.ts- Tests for validation of client restrictionstests/unit/settings/providers/build-patch-draft.test.ts- Tests for form-to-draft conversionTest Plan
bun run typecheck-- no type errorsbun run lint-- no new lint violationsbun run test-- all existing + 14 new tests pass (2 pre-existing failures in unrelated files)/v1/responses/compact, verify it uses endpoint pool (check provider chain logs)Description enhanced by Claude AI
Greptile Summary
This PR fixes three distinct bugs in provider management and proxy routing:
Fix 1: Endpoint Routing - Added
/v1/responses/compacttoSTANDARD_ENDPOINTS, preventing it from being misclassified as an MCP request and ensuring proper endpoint pool selection.Fix 2: Circuit Breaker Cache Invalidation - Added cache invalidation (Redis + in-memory) in
applyProviderBatchPatchandundoProviderPatchwhen circuit breaker fields change, matching the behavior of single-provider edits.Fix 3: Client Restrictions Batch Support - Added missing handlers for
allowed_clients/blocked_clientsfields in the batch edit pipeline (buildPatchDraftFromFormStateandisValidSetValue), fixing silent drops during batch operations.All three fixes are well-implemented with comprehensive test coverage (14 new tests). The code follows existing patterns and maintains consistency with related functionality.
Confidence Score: 5/5
Important Files Changed
/v1/responses/compactto STANDARD_ENDPOINTS array to fix endpoint routingallowed_clientsandblocked_clientsfields in form-to-draft conversionallowed_clientsandblocked_clientsin isValidSetValue functionLast reviewed commit: 3aad53a