Conversation
There was a problem hiding this comment.
Code Review
This pull request modularizes the combo routing logic under open-sse/services/combo/ by splitting it into specialized modules for auto-routing, round-robin, quota management, context filtering, DAG resolution, and shadow routing. The review feedback highlights several critical issues: a potential memory leak in chat.ts due to an unremoved abort event listener, a resource leak in shadow.ts from draining a cloned response instead of the original, and a compatibility risk in sorting.ts from using Array.prototype.at(). Additionally, the reviewer noted a violation of the repository style guide requiring tests for any production code changes.
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.
| if (target?.modelAbortSignal) { | ||
| if (target.modelAbortSignal.aborted) { | ||
| timeoutController.abort(new Error("hedge-cancelled")); | ||
| } else { | ||
| target.modelAbortSignal.addEventListener("abort", () => { | ||
| timeoutController.abort(new Error("hedge-cancelled")); | ||
| }); | ||
| } | ||
| } | ||
| try { | ||
| return await Promise.race([ | ||
| handleSingleModel(b, modelStr, targetWithSignal).catch((err) => { | ||
| if (timedOut) { | ||
| // Inner call rejected because we aborted it. The synthetic 524 from | ||
| // timeoutPromise already wins the race; return an empty response so | ||
| // the loser branch resolves cleanly without leaking err.message. | ||
| return new Response(null, { status: 599 }); | ||
| } | ||
| return errorResponse(502, err?.message ?? "Upstream model error"); | ||
| }), | ||
| timeoutPromise, | ||
| ]); | ||
| } finally { | ||
| clearTimeout(timeoutId); | ||
| } |
There was a problem hiding this comment.
The event listener added to target.modelAbortSignal is never removed. If target.modelAbortSignal is long-lived or if many speculative/hedged tasks are created during a request, this can lead to memory leaks and accumulation of event listeners.
Please ensure that the event listener is cleaned up in the finally block.
let abortListener: (() => void) | undefined;
if (target?.modelAbortSignal) {
if (target.modelAbortSignal.aborted) {
timeoutController.abort(new Error("hedge-cancelled"));
} else {
abortListener = () => {
timeoutController.abort(new Error("hedge-cancelled"));
};
target.modelAbortSignal.addEventListener("abort", abortListener);
}
}
try {
return await Promise.race([
handleSingleModel(b, modelStr, targetWithSignal).catch((err) => {
if (timedOut) {
// Inner call rejected because we aborted it. The synthetic 524 from
// timeoutPromise already wins the race; return an empty response so
// the loser branch resolves cleanly without leaking err.message.
return new Response(null, { status: 599 });
}
return errorResponse(502, err?.message ?? "Upstream model error");
}),
timeoutPromise,
]);
} finally {
clearTimeout(timeoutId);
if (target?.modelAbortSignal && abortListener) {
target.modelAbortSignal.removeEventListener("abort", abortListener);
}
}| }), | ||
| shadowConfig.timeoutMs | ||
| ); | ||
| await drainShadowResponse(response.clone()); |
There was a problem hiding this comment.
Using response.clone() and then draining the clone leaves the original response body stream unconsumed. In many fetch environments (like Node.js/undici), unconsumed response bodies can cause socket and resource leaks.
Since the original response body is not needed elsewhere (only response.ok and response.status are read), you should drain the original response directly and avoid cloning it entirely.
| await drainShadowResponse(response.clone()); | |
| await drainShadowResponse(response); |
| if (random <= 0) return target; | ||
| } | ||
|
|
||
| return targets.at(-1); |
There was a problem hiding this comment.
Using Array.prototype.at() requires ES2022 support. If the project's target environment or transpiler configuration is older and does not include polyfills, this can cause runtime crashes.
Using standard array indexing targets[targets.length - 1] is safer and fully backward-compatible.
| return targets.at(-1); | |
| return targets[targets.length - 1]; |
| @@ -0,0 +1,13 @@ | |||
| // Re-export everything to maintain backward compatibility | |||
There was a problem hiding this comment.
According to Rule 9 of the Repository Style Guide:
Always include tests when changing production code (
src/,open-sse/,electron/,bin/).
Since this PR introduces a major modularization of the combo routing logic under open-sse/services/combo/, please ensure that corresponding unit or integration tests are added or updated under the tests/ directory to maintain coverage and prevent regressions.
References
- Always include tests when changing production code (src/, open-sse/, electron/, bin/). (link)
|
Thanks @oyi77 — blockers for the combo modularization:
|
ba5e277 to
bae7f6a
Compare
…he-poisoning + SC2086 (diegosouzapw#3965)
Integrated into release/v3.8.27
Integrated into release/v3.8.27
Integrated into release/v3.8.27
…iegosouzapw#3954) (diegosouzapw#3995) rate_limited_until is a TEXT column, but setConnectionRateLimitUntil (Antigravity full-quota path) persists a raw epoch number that SQLite coerces to a numeric string ("1781696905131.0"). The selection predicate isAccountUnavailable then did new Date("1781696905131.0") -> NaN, so the cooling connection was never skipped and the router kept dispatching to rate-limited accounts. Normalize numeric-epoch strings (and number/Date/ISO) via a shared cooldownUntilMs() helper in isAccountUnavailable / getEarliestRateLimitedUntil / filterAvailableAccounts / parseFutureDateMs. ISO behavior preserved.
…w#3976) (diegosouzapw#3996) llm7 and byteplus carry a real modelsUrl but were not classified by any live-fetch branch of the model-import route, so their hardcoded 4-entry registry catalog was served (source local_catalog) instead of the upstream catalog. Add both to NAMED_OPENAI_STYLE_PROVIDERS so the route probes <baseUrl>/models and serves the live list, falling back to the local catalog only on fetch failure.
…mount ref (diegosouzapw#3972) (diegosouzapw#3997) The auto-refresh interval gated each tick on visibleRef, seeded once at mount and updated only by a visibilitychange event. A tab mounted while document.visibilityState is 'hidden' (background load, bfcache, embedded/proxied webviews) with no later visibilitychange left the ref false forever, so the interval ticked but never fetched — only the manual button worked. Read the live document.visibilityState in the tick instead.
…iegosouzapw#3959) (diegosouzapw#3998) strict-random shuffled only the deck-selected slot 0 and left the fallback remainder in fixed priority order, so after a failing deck pick the chain always fell through to the same top-priority model — a persistently-failing model was retried on essentially every request and fallback load never spread across peers. Shuffle the remainder too (like the random strategy).
Integrated into release/v3.8.27
…aude OAuth path (diegosouzapw#3974) (diegosouzapw#3999) The client-negotiated anthropic-beta: tool-search-tool-2025-10-19 was dropped on both Claude code paths (default executor rebuilt from static ANTHROPIC_BETA_CLAUDE_OAUTH; selectBetaFlags only read the client beta to gate thinking/effort), so claude.ai rejected deferred-tool requests with 400 'Tool reference not found'. Add an allowlist-merge (mergeClientAnthropicBeta) that unions the client's allowlisted betas into the outbound set on both paths, preserving diegosouzapw#3415 (no forced thinking/effort).
Integrated into release/v3.8.27
Integrated into release/v3.8.27
…iegosouzapw#3943) Integrated into release/v3.8.27
Integrated into release/v3.8.27
Integrated into release/v3.8.27
… qwen body-check) The qwen-web validation body-check merged in diegosouzapw#3958 pushed validation.ts past its frozen size on the integrated release tip. Bump the baseline with justification; no logic is separately extractable from the existing qwen-web validation branch.
Integrated into release/v3.8.27 — low-risk group (playwright 1.60→1.61 minor + transitive patches; fumadocs-core 16.9→16.10 minor).
… modularization The provider-registry modularization (diegosouzapw#3993) was cut from a base predating the byteplus (diegosouzapw#3877) and mimocode (diegosouzapw#3837) registry entries, so merging it silently dropped both providers (getRegistryEntry returned undefined → validation reported 'not supported'). Re-add them as registry modules in the new structure; registered count 159→161, provider-consistency 161/232/0. Also align the pre-existing qwen-web validator test to diegosouzapw#3958: since the validator now requires a real `user` object in the 200 body, the mock must carry one.
Modularize validation schemas (diegosouzapw#3594). Integrated into release/v3.8.27 after rebase (reconciled the merged hiddenSidebarGroupLabels diegosouzapw#3971 + intelligenceSyncRequestSchema into the new modules) + behavior verification (typecheck, 195 schema/settings/validation tests, build 556/556). Co-authored-by: diegosouzapw <diegosouza.pw@gmail.com>
…nai/codex pricing (diegosouzapw#4005) Integrated into release/v3.8.27 — openai model-discovery honors custom base URL (SSRF-guarded) + pricing rows for new openai/codex models. Tested + baselines bumped.
Integrated into release/v3.8.27 — repair LiveWS sidecar (startup, same-origin /live-ws, main→sidecar compression.completed bridge, early-msg queue). Fixed the cookie-parse regex (\s) + added a focused unit test; baseline bumped for the non-blocking chatCore bridge.
… gitignore it A worktree node_modules symlink (-> the main checkout's node_modules) was staged by a `git add -A` during the diegosouzapw#3988 merge and committed into 05213ac. The symlink points at the repo's own node_modules path, so checking it out turns the main checkout's node_modules into a self-referential symlink (breaking tsx/all node ops). Untrack it and add a root-anchored /node_modules ignore so the symlink form can't be re-committed (the existing 'node_modules/' only matches directories).
…uzapw#4014) Integrated into release/v3.8.27. Adjustments before merge: - Synced with the current release tip (was 11 commits behind). - Added the 3 LLMLingua-2 ONNX optional-runtime deps to dependency-allowlist.json (@atjsh/llmlingua-2, @tensorflow/tfjs, js-tiktoken) — the only gate that was red. - socks was allowlisted directly on release (separate fix d7db5c7; it was declared by diegosouzapw#4004 but never allowlisted, leaving check:deps red release-wide). Verified locally: check:deps OK, file-size OK, public-creds OK, provider-consistency 161/232/0, typecheck:core clean, 24/24 LLMLingua tests pass. The only remaining Fast-QG red is the pre-existing diegosouzapw#3972 orphan test (request-logger-autorefresh-visibility-3972.test.tsx), which is release-wide and unrelated to this PR.
… runner collects it tests/unit/request-logger-autorefresh-visibility-3972.test.tsx (added by diegosouzapw#3972 via diegosouzapw#3997) sat at the top level of tests/unit/ as a .tsx vitest test, which NO runner collects: the node runner only globs *.test.ts, and test:vitest:ui only runs tests/unit/ui. So the diegosouzapw#3972 regression guard never executed in CI and check:test-discovery was red release-wide. Move it under tests/unit/ui/ (the collected vitest:ui path) and fix the relative import depth. Verified: the test now runs and passes (2/2), and check:test-discovery is green.
… Lite schema fix (diegosouzapw#3952) (diegosouzapw#4018) Captures the net-new value from diegosouzapw#3960 (per-engine breakdown analytics) and diegosouzapw#3952 (Lite engine schema fix) onto release/v3.8.27. Fast QG green; 622/622 compression+analytics tests pass.
…mimocode) (diegosouzapw#4015) Real bugfix: guard model-less registry entries (mimocode) in getUnsupportedParams so handleChatCore no longer throws 'entry.models is not iterable' / reports 'All models failed' for unrelated requests. Includes a regression test. Fast QG green.
…AST-smoke, mutation infra) (diegosouzapw#4016) * docs(ops): add quality-gate assessment + replication playbook (Fase 9 foundation) * feat(ci): flip oasdiff breaking-change gate to blocking (ratchet) * docs(ops): deliver main branch-protection ruleset for owner to apply * fix(ci): run typecheck:core in PR->release fast-gates (close fast-gates hole, part 1) * perf(mutation): enable Stryker incremental mode + cache (scales the 60/80 rollout) * feat(ci): commit CodeQL advanced config (security-extended), replacing default-setup * feat(ci): version semgrep SAST workflow (owasp/secrets), advisory * feat(quality): TIA test-impact map builder (import-graph; map built at runtime, gitignored) * feat(quality): TIA impacted-test selector with run-all fail-safe * fix(ci): run TIA-impacted unit tests in PR->release fast-gates (build map at runtime, fail-safe full) * feat(ci): DAST-smoke per-PR (schemathesis subset + promptfoo injection-guard, blocking) * fix(ci): unbreak Fase 9 PR CI (MDX frontmatter, CodeQL conflict, dast-smoke advisory) - Add MDX frontmatter to docs/ops/{BRANCH_PROTECTION_MAIN,QUALITY_GATE_PLAYBOOK}.md. fumadocs rejects frontmatter-less docs -> 'npm run build' failed -> broke dast-smoke's build step (the release fast-gates never runs build, so this only surfaced on the PR). - codeql.yml: workflow_dispatch-only until the owner switches repo CodeQL Default->Advanced (advanced configs cannot be processed while default setup is enabled; documented inline). - dast-smoke.yml: job-level continue-on-error (advisory) so this brand-new gate matures before it blocks (repo convention: advisory -> blocking). * ci(quality): make TIA unit-test step advisory until release test-debt is cleared release/v3.8.27 carries ~17 pre-existing failing unit tests (budget diegosouzapw#3537, apiKey this PR — the new 'run tests on PR->release' gate surfaced them. Per the repo's advisory->blocking convention, this step enters advisory (it still runs + reports) so pre-existing debt doesn't block the gate program. typecheck:core stays blocking. Flip to blocking (remove continue-on-error) once the release suite is green.
…_uses) (diegosouzapw#4021) Opt-in Claude-only delegated compression: injects context_management.clear_tool_uses_20250919 at the Claude pre-serialization chokepoint (composes with clear_thinking, thinking first), threaded via ExecuteInput from handleChatCore. Pure edit-builder + 11 tests (7 unit + 4 e2e fetch-capture). Beta context-management-2025-06-27 already advertised; allowlist done. Telemetry/400-fallback/claude-web coverage deferred.
…iegosouzapw#4024) randomUUID non-HTTPS fallback + static CompareTab import; raw HTTP TRACE->405 method guard wired into dev + standalone servers. Integrated into release/v3.8.27.
…pw#4020) Presentation/relabel refactor of the Settings dashboard (API Manager -> API Keys), card relocations, Toggle adoption, present-but-disabled engine steps. Auth-file changes are string/comment-only (no behavior change). Integrated into release/v3.8.27.
…rizations (diegosouzapw#4030) Restores schema fields (combo reasoningTokenBuffer, budget-0 diegosouzapw#3537, openrouter preset, proxy family diegosouzapw#3777, resilience degradation/providerCooldown), qwen-web v2 endpoint+catalog, mimocode models key — all dropped by diegosouzapw#3988/diegosouzapw#3993 — and aligns 3 tests to diegosouzapw#3941/diegosouzapw#3993. Verified: 8 failing regression tests on release tip -> 131/131 green on this branch. Integrated into release/v3.8.27.
) - Splits the 4,600-line combo.ts monolithic file into 12 domain-specific files - Extracts Auto-Combo, Round-Robin, Quota tracking, Shadow routing, and Context Affinity logic into dedicated modules - Uses folder-as-module pattern with index.ts facade for zero backward-compatibility breakages - Auto-prunes and manages all imports for strict typechecking
…ervices Decompose the monolithic handleComboChat function (lines 221-1574) into 6 focused sub-modules under services/combo/chat/: - types.ts: ExecutionContext, MutableRef<T>, TargetAttemptResult (zero-any) - routing.ts: Init, config resolution, strategy routing, pre-screening - auto.ts: Auto-strategy filtering, scoring, selection - executor.ts: Per-target execution, success/error processing - handler.ts: Set-retry loop, concurrency dispatch, response assembly - timeout.ts: Per-model timeout wrapper (existing) The original chat.ts is now a ~128-line thin orchestrator that sequences routing → execution. All files under 800 lines. Line counts: chat.ts=128, auto.ts=559, executor.ts=584, handler.ts=197, routing.ts=168, timeout.ts=77, types.ts=79 (total=1792) No circular dependencies. Re-exports preserve backward compatibility.
- Extracted auto-strategy logic to chat/auto.ts - Extracted execution loop to chat/handler.ts - Extracted target executor to chat/executor.ts - Extracted routing/init to chat/routing.ts - Shared types in chat/types.ts - chat.ts now barrel re-export + handleComboChat orchestrator All files <800 lines. Fixed import paths for nested module.
|
Thanks for all the modularization work here, @oyi77 🙏. We've decided to hold the per-module "non-stacked" refactors and run the decomposition as one coordinated pass after the in-flight quality-gate work lands, instead of merging them piecemeal. Reason: on the two we did merge (#3993, #3988) we caught logic being dropped during the move — and the gates (provider-consistency / typecheck) don't detect internal-logic loss — so each of these needs a full lossless audit, which isn't tractable across many overlapping PRs against a moving release branch right now. The coordinated modularization is tracked in #3501 / #3594; we'd genuinely value your input on that plan once it's up. Closing for now — purely sequencing, not a reflection on the effort. |
…iegosouzapw#3992) Split open-sse/services/combo.ts into 22 focused modules: - auto.ts, constants.ts, context.ts, dag.ts, roundRobin.ts, shadow.ts - sorting.ts, state.ts, types.ts, utils.ts - chat/ (auto, executor, handler, routing, timeout, types) - quota/ (config, orchestration, resetAware) - index.ts (barrel) Sub-split oversized modules: chat.ts (1623→6 files), quota.ts (850→3 files). Original file replaced with thin re-export.
Replaces #3798 as an independent, non-stacked PR branched from release/v3.8.27.
Extracts combo.ts into modular structure.
Supersedes: #3798