feat(settings): configure bounded streaming discovery - #1349
Conversation
|
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新增 Bounded Streaming Discovery 配置、版本化 Redis 会话绑定、分轮流式竞速、协议有效性解析、租约管理和响应最终化处理，并补充数据库迁移、管理表单、环境配置及测试覆盖。 ChangesBounded Streaming Discovery 配置与持久化
版本化 Redis 会话绑定
流式 Discovery 路由与最终化
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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
enhancement
![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 Bounded Streaming Discovery, an optional cold-start routing mode for streaming requests designed to reduce duplicate upstream spend. It includes database schema updates, UI configuration forms, a pure state machine for discovery coordination, protocol-specific stream chunk validation, and versioned session binding. The review feedback highlights several critical issues in the discovery path within ProxyForwarder, such as completed streams on lower-priority providers incorrectly throwing errors, potential request hangs when sticky providers fail immediately or when all initial candidates fail to launch, and unhandled promise rejections during provider launches. Additionally, a redundant dead-code block was identified in the DiscoveryCoordinator state machine.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/app/[locale]/settings/config/_components/system-settings-form.tsx (1)
713-721: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win修复数字输入框无法清空的 UX 问题。
在 React 中，当受控的
<input type="number">被完全删除清空时，event.target.value为""，Number("")会将其转换为0。这将导致输入框在退格清空时始终卡在0而无法留空，使得用户输入新数字时体验不佳（例如必须全选覆盖）。由于这里所有字段的
min属性均硬编码为1，0永远不会是用户的合法输入值。因此，可以通过在渲染时将0转换为空字符串，并补充required属性以保留浏览器的 HTML5 拦截校验，来轻量级地解决此问题。💡 改进建议
<Input id={`discovery-${key}`} type="number" min={min} - value={value} + required + value={value === 0 ? "" : value} onChange={(event) => setter(Number(event.target.value))} disabled={isPending || !discoveryEnabled} className={inputClassName} />🤖 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/app/`[locale]/settings/config/_components/system-settings-form.tsx around lines 713 - 721, Update the controlled number Input in the discovery settings form so its onChange preserves an empty string when the event value is empty instead of coercing it with Number(""). Render a stored value of 0 as an empty string, and add the required attribute while preserving the existing min validation and setter behavior for valid numeric input.messages/zh-TW/settings/config.json (1)
127-128: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value术语一致性（可选）
Line 127 使用“探索 SLA”、Line 128 使用“Sticky 黏性 SLA”，与本文件 131 描述文案及 zh-CN 版本统一使用的 “Discovery SLA / Sticky SLA” 术语不一致。建议统一标签用词以降低用户理解成本。
🤖 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 `@messages/zh-TW/settings/config.json` around lines 127 - 128, 统一 settings 配置中的 SLA 标签术语：将 discoverySlaMs 和 stickySlaMs 的中文标签调整为与本文件其他描述及 zh-CN 版本一致的 “Discovery SLA” 和 “Sticky SLA” 表述，保持毫秒单位信息不变。src/app/v1/_lib/proxy/discovery-coordinator.ts (1)
187-199: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
onRoundBoundary中currentFallback分支为死代码。到达第 188 行时，必然满足
pendingNormal.length > 0（否则第 180 行已返回），因此第 170 行的currentFallback && pendingNormal.length > 0为假只能是currentFallback为假值。故第 194 行if (currentFallback)恒不成立，currentFallback.pending = true永不执行，且两个分支返回值完全相同。请确认此处并非遗漏了“保留既有 fallback”的意图；若无此意图，可合并为单一返回以消除歧义。♻️ 建议简化
const fallback = pendingNormal[0]; fallback.kind = "fallback"; this.state = "FALLBACK_READY_HELD"; const losers = pendingNormal.slice(1).map((attempt) => attempt.id); for (const id of losers) this.attempts.get(id)!.pending = false; - - if (currentFallback) { - currentFallback.pending = true; - return { type: "cancel", attemptIds: losers }; - } return { type: "cancel", attemptIds: losers };🤖 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/app/v1/_lib/proxy/discovery-coordinator.ts` around lines 187 - 199, 简化 onRoundBoundary 中 currentFallback 的无效条件分支：在确认不存在保留既有 fallback 的需求后，移除恒不成立的 if (currentFallback) 及其 currentFallback.pending = true 赋值，直接返回取消 losers 的结果，并保留 fallback 状态更新和 pending 标记逻辑。src/app/v1/_lib/proxy/forwarder.ts (1)
3829-3858: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
discoveryEnabled被重复判断。第 3831 行已在
settings.discoveryEnabled !== true时提前返回，第 3848 行返回表达式中的settings.discoveryEnabled === true因此恒为真，属冗余条件，可移除以精简逻辑。🤖 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/app/v1/_lib/proxy/forwarder.ts` around lines 3829 - 3858, Remove the redundant settings.discoveryEnabled === true condition from the return expression in shouldUseStreamingDiscovery, since the earlier guard already guarantees discovery is enabled. Preserve all other endpoint, protocol, routing, and fallback checks unchanged.
🤖 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/app/api/admin/system-config/route.ts`:
- Around line 68-71: Update the validation response in the admin system-config
route to return the existing localized error key discoveryWindowInvalid as a
structured error identifier instead of the hardcoded Chinese message. Preserve
the HTTP 400 status and let the client resolve the message for the active
locale; apply the same approach to nearby user-facing hardcoded Chinese
validation errors only where they follow this pattern.
In `@src/app/v1/_lib/proxy/forwarder.ts`:
- Around line 5014-5035: Update the launch function’s failure path around
resolveStreamingHedgeEndpoint so that if endpoint resolution or subsequent
attempt setup throws, it removes the provider from launched and releases the
provider session reference recorded by session.recordProviderSessionRef.
Preserve normal successful launch behavior and ensure the rollback occurs only
for references actually registered by the rate-limit check.
- Around line 5115-5136: Update the read loop in resolveStreamingHedgeEndpoint
so EOF after attempt.ready is preserved as a committable candidate instead of
throwing EmptyResponseError or marking it retry_failed; only treat EOF as empty
when no valid response was produced. Also ensure the session reference created
by recordProviderSessionRef is released whenever endpoint resolution fails,
including exceptions from resolveStreamingHedgeEndpoint, so concurrent-session
accounting cannot leak.
In `@src/lib/api-client/v1/openapi-types.gen.ts`:
- Around line 11894-11895: Update the source OpenAPI schema description for
discoverySlaMs to use “首字节 Discovery SLA” or “First-byte Discovery SLA” instead
of “首字 Discovery SLA”, then regenerate openapi-types.gen.ts so all generated
occurrences are corrected.
---
Nitpick comments:
In `@messages/zh-TW/settings/config.json`:
- Around line 127-128: 统一 settings 配置中的 SLA 标签术语：将 discoverySlaMs 和 stickySlaMs
的中文标签调整为与本文件其他描述及 zh-CN 版本一致的 “Discovery SLA” 和 “Sticky SLA” 表述，保持毫秒单位信息不变。
In `@src/app/`[locale]/settings/config/_components/system-settings-form.tsx:
- Around line 713-721: Update the controlled number Input in the discovery
settings form so its onChange preserves an empty string when the event value is
empty instead of coercing it with Number(""). Render a stored value of 0 as an
empty string, and add the required attribute while preserving the existing min
validation and setter behavior for valid numeric input.
In `@src/app/v1/_lib/proxy/discovery-coordinator.ts`:
- Around line 187-199: 简化 onRoundBoundary 中 currentFallback 的无效条件分支：在确认不存在保留既有
fallback 的需求后，移除恒不成立的 if (currentFallback) 及其 currentFallback.pending = true
赋值，直接返回取消 losers 的结果，并保留 fallback 状态更新和 pending 标记逻辑。
In `@src/app/v1/_lib/proxy/forwarder.ts`:
- Around line 3829-3858: Remove the redundant settings.discoveryEnabled === true
condition from the return expression in shouldUseStreamingDiscovery, since the
earlier guard already guarantees discovery is enabled. Preserve all other
endpoint, protocol, routing, and fallback checks unchanged.
🪄 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: 10400501-13b3-4dc9-92c4-6b166638c138
📒 Files selected for processing (54)
docs/streaming-discovery.mddrizzle/0110_daffy_rawhide_kid.sqldrizzle/meta/0110_snapshot.jsondrizzle/meta/_journal.jsonmessages/en/settings/config.jsonmessages/ja/settings/config.jsonmessages/ru/settings/config.jsonmessages/zh-CN/settings/config.jsonmessages/zh-TW/settings/config.jsonpackage.jsonsrc/actions/system-config.tssrc/app/[locale]/settings/config/_components/system-settings-form.tsxsrc/app/[locale]/settings/config/page.tsxsrc/app/api/admin/system-config/route.tssrc/app/v1/_lib/proxy/discovery-coordinator.tssrc/app/v1/_lib/proxy/discovery-validity.tssrc/app/v1/_lib/proxy/forwarder.tssrc/app/v1/_lib/proxy/provider-selector.tssrc/app/v1/_lib/proxy/response-handler.tssrc/app/v1/_lib/proxy/session.tssrc/app/v1/_lib/proxy/stream-finalization.tssrc/drizzle/schema.tssrc/lib/api-client/v1/openapi-types.gen.tssrc/lib/api/v1/schemas/system-config.tssrc/lib/config/system-settings-cache.tssrc/lib/redis/client.tssrc/lib/redis/lua-scripts.tssrc/lib/redis/session-binding.tssrc/lib/session-manager.tssrc/lib/session-tracker.tssrc/lib/validation/schemas.tssrc/repository/_shared/transformers.tssrc/repository/system-config.tssrc/types/system-config.tstests/configs/integration.config.tstests/configs/session-binding.config.tstests/integration/proxy-hedge-lifecycle.test.tstests/integration/session-binding-versioning-redis.test.tstests/unit/lib/redis/client.test.tstests/unit/lib/redis/session-binding.test.tstests/unit/lib/session-manager-binding-smart.test.tstests/unit/lib/session-manager-terminate-provider-sessions.test.tstests/unit/lib/session-manager-terminate-session.test.tstests/unit/lib/session-manager-versioned-binding.test.tstests/unit/lib/session-tracker-cleanup.test.tstests/unit/proxy/discovery-coordinator.test.tstests/unit/proxy/discovery-validity.test.tstests/unit/proxy/provider-selector-cross-type-model.test.tstests/unit/proxy/provider-selector-model-mismatch-binding.test.tstests/unit/proxy/proxy-forwarder-hedge-first-byte.test.tstests/unit/proxy/response-handler-endpoint-circuit-isolation.test.tstests/unit/repository/system-config-degradation-ladder.test.tstests/unit/repository/system-config-update-missing-columns.test.tstests/unit/validation/system-settings-discovery.test.ts
![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: 4c9690d46b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
Well-structured configuration/rollout layer for bounded Streaming Discovery. The seven new system_settings columns are additive NOT NULL with safe defaults, the rolling-deployment degradation ladder is correctly extended for all seven columns, and the window invariant (racingTotalTimeoutMs >= stickySlaMs + maxDiscoveryRounds * discoverySlaMs) is consistently enforced across all three layers (schema superRefine, server-action merge, admin-API route). i18n keys are complete and identical across all five locales. Discovery stays inert behind discoveryEnabled === false plus a capability probe, so this PR does not alter routing on its own as claimed.
PR Size: XL
- Lines changed: 11,457 (10,932 + / 525 -) against
dev - Files changed: 54 (full stacked diff)
Stacked-PR note: This diff includes #1347 (versioned session binding) and #1348 (Discovery transport). The PR3-isolated scope (git diff pr1348...pr1349) is 31 files, +5,962 / -337. The stack split is the right call given the size; no further split is needed for the configuration layer itself. Most of the line volume is the auto-generated drizzle/meta/0110_snapshot.json (4,691 lines) and OpenAPI types, not hand-written logic.
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 | 1 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Medium Priority (Should Fix)
- [TEST-EDGE-CASE]
tests/unit/validation/system-settings-discovery.test.ts: the window-invariant boundary (total deadline exactly equal to the minimum) is not asserted, and the server-actioneffectiveDiscoveryWindowmerge — one of the three enforcement layers — has no unit test. See inline comment.
Observations (no action required)
- Defaults for the seven settings are duplicated as literals in
DEFAULT_SETTINGS(cache),createFallbackSettings()(repository), andtoSystemSettings()(transformer). The PR description states these "share one source of truth," but the repository and transformer still hardcode the literals. This matches the existing convention for every other setting in those files, so it is not flagged as a defect — just noting the description is slightly stronger than the implementation. - The
shouldUseStreamingDiscoverycapability check usestypeof SessionManager.ensureVersionedBindingCapability === "function"guards. Within a single build these always resolve the same way, so the secondary branch is effectively dead — but it is intentional defensive code for the rolling-upgrade scenario described in the PR body.
Review Coverage
- Logic and correctness
- Security (OWASP Top 10) — admin routes gate on
role === "admin"before any settings read/write - Error handling
- Type safety —
discovery*correctly promoted optional -> required across types, repository, cache, transformer - Documentation accuracy
- Test coverage — gap noted above
- Code clarity
Automated review by Claude AI
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 754c1a9578
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
# Conflicts: # src/app/v1/_lib/proxy/discovery-validity.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0880619929
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51cbc6961d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
# Conflicts: # tests/unit/proxy/discovery-validity.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: acda2e3b39
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d98f64163d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
…0723' into agent/discovery-1349-on-integration # Conflicts: # drizzle/meta/_journal.json # src/app/v1/_lib/proxy/forwarder.ts # src/app/v1/_lib/proxy/response-handler.ts # tests/unit/proxy/discovery-validity.test.ts # tests/unit/proxy/response-handler-endpoint-circuit-isolation.test.ts
28ee73f
into
ding113:integration/discovery-stack-20260723
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
2 participants
Summary
This is PR 3/3 for #1340 and is stacked on #1348. It exposes the bounded streaming Discovery scheduler through persistence, Admin API, OpenAPI, documentation, and the system settings UI.
discoveryEnabledswitch.2, rounds2, Discovery SLA10s, Sticky SLA20s, total racing deadline60s, cooldown5m.2and validate that the total deadline covers the configured Sticky plus Discovery windows.docs/streaming-discovery.mdrollout guidance.Safety
Validation
bun run test:integration(real Discovery loopback 5/5; Redis/PostgreSQL suites skip without service URLs).bun run typecheckbun run lintbun run format:checkbun run validate:migrationsbun run openapi:checkbun run openapi:lintbun run i18n:audit-messages-no-emoji:failbun run buildCloses #1340.
Migration ordering
Migration
0109_nice_shriekis owned by #1336, which must merge before this PR. After #1336 lands, rebase this branch ontodevand regenerate the0110snapshot and journal metadata against0109before merge. Deploying0110first would cause the older0109migration to be skipped by Drizzle timestamp ordering.Greptile Summary
This PR completes the bounded streaming Discovery feature by wiring seven new system-settings fields through database migration, repository degradation ladder, Admin API, v1 OpenAPI, server action, and the system settings UI. Discovery stays disabled after upgrade until an administrator explicitly enables it.
0110adds sevenNOT NULL DEFAULTcolumns; the degradation ladder in the repository handles pre-migration nodes gracefully. Both the server action and the Admin API route perform a merged-value window check (racingTotalTimeoutMs ≥ stickySlaMs + maxDiscoveryRounds × discoverySlaMs) against current DB values so partial updates cannot silently create an invalid configuration.DiscoverySetupReservation(candidate and rectifier-retry variants) so async provider-selection and endpoint-setup steps occupy a coordinator slot under the Discovery SLA without blocking other candidates; thestickyTimeoutCooldownBoundedcleanup replaces the previous unboundedawaitinsettleFailure.DISCOVERY_WINDOW_INVALIDandDISCOVERY_SETTINGS_INVALIDare mapped to localised toasts in all five locale files.Confidence Score: 5/5
Safe to merge after confirming migration ordering (0109 must land first on the target branch).
All previous review findings have been addressed. The merged-value window check is applied consistently across the server action and Admin API route, the shared DEFAULT_SETTINGS constant is now the single source of truth for the server action, and error codes are aligned. The forwarder's new setup-reservation machinery is well-encapsulated and the degradation ladder handles pre-migration nodes correctly. No correctness issues were found in this pass.
src/app/v1/_lib/proxy/forwarder.ts is the most complex file in the diff; the stickyTimeoutWaveReservation slot-management and the bounded cleanup path are worth a second read if there are any concerns about sticky-timeout interaction during terminal failure.
Important Files Changed
Sequence Diagram
sequenceDiagram participant Admin as Admin / UI participant Action as saveSystemSettings participant Schema as UpdateSystemSettingsSchema participant DB as Repository (system-config) participant Cache as SystemSettingsCache participant FWD as ProxyForwarder participant Coord as DiscoveryCoordinator Admin->>Action: POST settings (discoveryEnabled, window params) Action->>DB: getSystemSettings() (fetch current for merge) Action->>Schema: .parse(formData) — field-level + superRefine (all-4-present) Action->>Action: "merged-value window check (racingTotal >= sticky + rounds x discovery)" alt window invalid Action-->>Admin: ok:false, errorCode: DISCOVERY_WINDOW_INVALID else valid Action->>DB: updateSystemSettings(payload) Action->>Cache: invalidateSystemSettingsCache() Action-->>Admin: ok:true, data: updated end Note over FWD,Coord: Per-request Discovery flow (when discoveryEnabled=true) FWD->>FWD: prepareStreamingDiscovery — check enabled, rollout%, Redis capability, lease FWD->>Coord: new DiscoveryCoordinator(concurrency, maxRounds) FWD->>Coord: "add placeholder (setupOnly=true) per candidate slot" FWD->>FWD: async provider select + endpoint setup (under discoverySlaMs) FWD->>Coord: "bindSetupProvider(id, providerId) — rejected if roundOpen=false" FWD->>Coord: notifyReady(id) — setupOnly blocks winner selection Coord-->>FWD: commit_normal / promote_fallback / launch / terminal_failure FWD->>FWD: cancelSetupReservations on settleFailure / round boundaryReviews (15): Last reviewed commit: "fix(discovery): align fallback error han..." | Re-trigger Greptile