Skip to content

feat(upstream): cherry-pick 3 medium-relevance upstream commits (L5-123) - #100

Merged
KooshaPari merged 1 commit into
mainfrom
chore/l5-123-upstream-feature-sync-2026-06-21
Jul 2, 2026
Merged

KooshaPari merged 1 commit into
mainfrom
chore/l5-123-upstream-feature-sync-2026-06-21

Conversation

@KooshaPari

@KooshaPari KooshaPari commented Jun 21, 2026 •

Copy link
Copy Markdown
Owner

User description

Summary

Cherry-picks 3 medium-relevance upstream commits from diegosouzapw/OmniRoute onto KooshaPari/OmniRoute. This is the feature half of the upstream sync (PR #99 was the security half).

Cherry-picked commits

SHA Subject Resolution
f42e8fa75 feat(mimocode): per-account proxy round-robin (diegosouzapw#3837) CLEAN (9 files, 935+/100-, zero conflicts)
337cd1893 fix(sse): clamp Gemini thinking budget to model cap (diegosouzapw#3842/diegosouzapw#3865) PARTIAL (3 of 5 files; 2 dropped via git rm — see audit trail)
8842414d8 fix(docker): build release image with webpack (diegosouzapw#4052) CLEAN (1 file, 7+/2-, zero conflicts)

Files changed (final diff stat vs origin/main)

 CHANGELOG.md                                       |   1 +
 Dockerfile                                         |   9 +-
 open-sse/executors/mimocode.ts                     | 127 ++++++---
 .../[id]/components/ConnectionsHeaderToolbar.tsx   |  24 +-
 .../[id]/components/ConnectionsListPanel.tsx       |  14 +-
 .../components/DistributeProxiesButton.test.tsx    | 219 ++++++++++++++++
 src/shared/components/DistributeProxiesButton.tsx  |  74 ++++++
 src/shared/components/NoAuthAccountCard.tsx        | 292 ++++++++++++++++++---
 src/shared/components/index.tsx                    |   1 +
 src/shared/constants/modelSpecs.ts                 |   3 +
 .../integration/mimocode-proxy.integration.test.ts | 150 +++++++++++
 tests/unit/mimocode-executor.test.ts               | 134 ++++++++++
 tests/unit/translator-openai-to-gemini.test.ts     |  20 ++
 13 files changed, 966 insertions(+), 102 deletions(-)

Commit graph

cf1bec832  docs(orch): L5-123 upstream feature sync worklog
47670805e  fix(docker): build release image with webpack (Turbopack internal panic) (#4052)   [8842414d8]
9a83d6266  fix(sse): clamp Gemini thinking budget to model cap (#3842) (#3865)               [337cd1893, partial]
6646eda22  feat(mimocode): per-account proxy support for multi-account round-robin (#3837)   [f42e8fa75]
e4d751ed1  chore(orch-v12-s4-020): dependency audit for OmniRoute (#96)                       <-- origin/main

Cherry-pick heuristic (validated in PR #99, applied here)

  1. Check if the file exists in HEAD before applying
  2. Deleted files → take "ours" + audit commit explaining why
  3. Modified files in different paths → keep applicable, drop no-op
  4. New test files importing deleted modules → drop test, keep audit
  5. Always cherry-pick -x for traceability

Per-commit resolution detail

  • f42e8fa75 (clean): All 9 files applied without intervention. KP main had every upstream file path; the L5-122 prep notes that flagged possible mimocode refactors turned out to be over-cautious.
  • 337cd1893 (partial, 3/5): open-sse/translator/request/claude-to-gemini.ts and tests/unit/translator-claude-to-gemini.test.ts dropped — KP never adopted the claude-to-gemini translator path (KP ships openai-to-gemini only). Surviving files: CHANGELOG, modelSpecs.ts (cap=24576), translator-openai-to-gemini.test.ts (regression test). The commit message was amended with full audit trail (Refs: upstream SHA, KP delete rationale, ADR-031, ADR-042).
  • 8842414d8 (clean): Dockerfile path retained at origin/main; Turbopack→webpack switch applied without conflict.

Deviations from heuristic

None. The heuristic predicted "very likely conflicts" based on L5-122 prep notes; actual conflicts were minimal (only 2 files in 1 commit, resolved cleanly).

Working tree discipline

  • PR fix(security): upstream ReDoS + dep bump cherry-pick (L5-122) #99 (L5-122, security half) was NOT touched — remains OPEN + MERGEABLE on its chore/l5-122-upstream-security-2026-06-21 branch.
  • All commits use cherry-pick -x for upstream-SHA traceability.
  • No force-push.
  • Pre-existing working-tree modifications from concurrent sessions (AGENTS.md, PLAN.md, open-sse/observability/, src/instrumentation-node.ts — B10 OTel work) were NOT included in this PR. Those belong to other L5-* branches.

Refs: PR #99 (security half), ADR-031, ADR-042, worklogs/2026-06-21-L5-123-upstream-feature-sync.md


CodeAnt-AI Description

Add per-account proxy support, clamp Gemini thinking budgets, and make release Docker builds use webpack

What Changed

  • Accounts can now each use their own proxy, and MiMoCode requests use the matching proxy for that account during sign-in and chat.
  • The dashboard can now assign proxies to individual accounts or spread saved proxies across all accounts from one button.
  • Gemini reasoning_effort=high now stays within the model’s real limit, preventing requests that could fail with upstream 400 errors.
  • Release Docker builds now use webpack instead of Turbopack, avoiding the build-time panic seen on the affected Next.js version.
  • Added coverage for per-account proxy handling, proxy distribution, and the Gemini budget clamp.

Impact

✅ Fewer proxy-related login and chat failures
✅ Fewer Gemini 400 errors on high reasoning requests
✅ More reliable release image builds

💡 Usage Guide

Checking Your Pull Request

Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.

Talking to CodeAnt AI

Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

Preserve Org Learnings with CodeAnt

You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

Check Your Repository Health

To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.

@gemini-code-assist

Copy link
Copy Markdown

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@codeant-ai

codeant-ai Bot commented Jun 21, 2026

Copy link
Copy Markdown

Thanks for using CodeAnt! 🎉

We're free for open-source projects. if you're enjoying it, help us grow by sharing.

Share on X ·
Reddit ·
LinkedIn

@coderabbitai

coderabbitai Bot commented Jun 21, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

@KooshaPari, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 25 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 6874e764-a7cc-4292-bae3-1f40e9be28db

📥 Commits

Reviewing files that changed from the base of the PR and between 8bd3be5 and 8909a6c.

📒 Files selected for processing (1)
  • worklogs/2026-06-21-L5-123-upstream-feature-sync.md

Note

.coderabbit.yaml has unrecognized properties

CodeRabbit is using all valid settings from your configuration. Unrecognized properties (listed below) have been ignored and may indicate typos or deprecated fields that can be removed.

⚠️ Parsing warnings (1)
Validation error: Unrecognized key: "review"
⚙️ Configuration instructions
  • Please see the configuration documentation for more information.
  • You can also validate your configuration using the online YAML validator.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore/l5-123-upstream-feature-sync-2026-06-21

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codeant-ai codeant-ai Bot added the size:XXL This PR changes 1000+ lines, ignoring generated files label Jun 21, 2026
Comment on lines +16 to +21
function requireProxy() {
if (!PROXY_URL) {
return false;
}
const parsed = parseProxyUrl(PROXY_URL);
return parsed !== null;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The proxy precondition only checks URL parseability, not that the scheme is actually SOCKS5, but the test always constructs Socks5ProxyAgent. If MIMOCODE_SOCKS5_PROXY is set to a non-SOCKS URL, the test won't be skipped and will fail at runtime when creating the SOCKS5 agent. Validate the protocol in requireProxy/parseProxyUrl (e.g., only allow socks5). [incorrect condition logic]

Severity Level: Major ⚠️
- ⚠️ Misconfigured proxy env causes confusing integration test failures.
- ⚠️ SOCKS5 tests run even when proxy type is invalid.
Steps of Reproduction ✅
1. Configure the environment with a non-SOCKS URL, e.g.
`MIMOCODE_SOCKS5_PROXY=http://localhost:8080`, so `PROXY_URL` at line 5 in
`tests/integration/mimocode-proxy.integration.test.ts` is set but uses the `http` scheme.

2. The `requireProxy()` helper at lines 16-21 calls `parseProxyUrl(PROXY_URL)` (lines
7-13), which successfully parses the HTTP URL and returns an object; `requireProxy()` then
returns `true` because it only checks `parsed !== null`, not the protocol.

3. The live proxy tests `"bootstrap returns JWT through configured proxy"` and `"chat
request succeeds through configured proxy"` at lines 33-51 and 53-91 are configured with
`{ skip: !requireProxy() ? "MIMOCODE_SOCKS5_PROXY not set" : false }`, so with this
misconfiguration they are not skipped and execute as normal.

4. When those tests run, they construct `const agent = new Socks5ProxyAgent(PROXY_URL!);`
at lines 35-36 and 55-56 with an HTTP URL instead of a SOCKS5 URL; `Socks5ProxyAgent`
expects a SOCKS5 proxy URL, so construction or first use as `dispatcher` in the subsequent
`fetch(...)` calls at lines 39-46 and 70-88 will fail at runtime instead of cleanly
skipping tests when the proxy is misconfigured.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** tests/integration/mimocode-proxy.integration.test.ts
**Line:** 16:21
**Comment:**
	*Incorrect Condition Logic: The proxy precondition only checks URL parseability, not that the scheme is actually SOCKS5, but the test always constructs `Socks5ProxyAgent`. If `MIMOCODE_SOCKS5_PROXY` is set to a non-SOCKS URL, the test won't be skipped and will fail at runtime when creating the SOCKS5 agent. Validate the protocol in `requireProxy`/`parseProxyUrl` (e.g., only allow `socks5`).

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

Comment on lines +35 to +36
const { Socks5ProxyAgent } = await import("undici");
const agent = new Socks5ProxyAgent(PROXY_URL!);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: A new Socks5ProxyAgent is created in each live-network test but never closed, which can leave open sockets/handles and make the test process hang or become flaky. Ensure the agent is cleaned up in a finally block (for example by calling close()/destroy() after each test). [resource leak]

Severity Level: Major ⚠️
- ⚠️ Integration SOCKS5 proxy tests may leak open sockets.
- ⚠️ Node test runner may hang awaiting proxy cleanup.
Steps of Reproduction ✅
1. Set `MIMOCODE_SOCKS5_PROXY` to a reachable SOCKS5 URL so `requireProxy()` in
`tests/integration/mimocode-proxy.integration.test.ts:16-21` returns `true` and the live
proxy tests are not skipped.

2. Run the Node test suite (for example `node --test
tests/integration/mimocode-proxy.integration.test.ts`), which executes the
`describe("mimocode per-account proxy — SOCKS5 integration", ...)` block at lines 26-150.

3. The first live-network test `"bootstrap returns JWT through configured proxy"` at lines
33-51 imports `Socks5ProxyAgent` and constructs `const agent = new
Socks5ProxyAgent(PROXY_URL!);` at lines 35-36, then uses it as the `dispatcher` for
`fetch(...)` at lines 39-46, without ever closing or destroying the agent.

4. The second live-network test `"chat request succeeds through configured proxy"` at
lines 53-91 repeats the same pattern (import at line 55, construct at 56, use as
`dispatcher` at 70-88) without cleanup; these agents hold open sockets and timers, so
repeated runs or long-lived processes can accumulate open handles and cause the Node test
runner to hang or behave flakily after tests complete.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** tests/integration/mimocode-proxy.integration.test.ts
**Line:** 35:36
**Comment:**
	*Resource Leak: A new `Socks5ProxyAgent` is created in each live-network test but never closed, which can leave open sockets/handles and make the test process hang or become flaky. Ensure the agent is cleaned up in a `finally` block (for example by calling `close()`/`destroy()` after each test).

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

Comment on lines +12 to +14
cleanupCallbacks.push(() => {
container.remove();
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The test creates a React root but never unmounts it during cleanup, so effect cleanups inside the component are skipped and timers/state updates can leak into later tests. Store the root in the cleanup callback and call root.unmount() before removing the container. [missing cleanup]

Severity Level: Major ⚠️
- ⚠️ DistributeProxiesButton tests leak mounted React roots between cases.
- ⚠️ Pending timeout callbacks may fire during later unrelated tests.
Steps of Reproduction ✅
1. Run the Vitest suite that includes
`src/shared/components/DistributeProxiesButton.test.tsx`, which defines `cleanupCallbacks`
and `makeContainer` at lines 7–15.

2. In the `"enters distributing state on click"` test at lines 91–113, `renderButton()`
(lines 35–52) is called; it creates a DOM container via `makeContainer()` (lines 9–15),
then mounts a React root with `createRoot(container)` at line 42 and renders
`<DistributeProxiesButton />` inside `act()` at lines 43–49.

3. After the test completes, the `afterEach` hook at lines 26–33 runs, popping the cleanup
callback registered at lines 12–14, which only does `container.remove()` and never calls
`root.unmount()`, leaving the React root and component tree mounted even though the DOM
node is gone.

4. The `DistributeProxiesButton` component at
`src/shared/components/DistributeProxiesButton.tsx` uses a `useEffect` cleanup at lines
27–31 to clear `timerRef` and a `setTimeout` scheduled in `handleClick` at lines 33–43, so
by never unmounting the root, these effect cleanups are skipped; pending timeouts and
state updates can leak into later tests, causing cross-test interference and potential
React act warnings when timers fire after the suite's DOM has been cleared.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** src/shared/components/DistributeProxiesButton.test.tsx
**Line:** 12:14
**Comment:**
	*Missing Cleanup: The test creates a React root but never unmounts it during cleanup, so effect cleanups inside the component are skipped and timers/state updates can leak into later tests. Store the `root` in the cleanup callback and call `root.unmount()` before removing the container.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

Comment on lines +219 to +233
if (Array.isArray(fingerprints)) {
const existing = new Set(this.accounts.map((a) => a.fingerprint));
for (const fp of fingerprints) {
if (typeof fp === "string" && !existing.has(fp)) {
this.accounts.push({
fingerprint: fp,
jwt: "",
expiresAt: 0,
cooldownUntil: 0,
consecutiveFails: 0,
proxy: null,
});
existing.add(fp);
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Account syncing only appends new fingerprints and never removes ones that were deleted from credentials, so removed accounts remain in memory and can still be selected for JWT/bootstrap/chat calls. This causes stale account usage (including stale proxy settings) until process restart. Rebuild this.accounts from the current fingerprint set (or prune missing entries) during sync. [stale reference]

Severity Level: Major ⚠️
- ❌ MiMoCode requests may target user-removed accounts unexpectedly.
- ⚠️ Proxy rotations ignore latest fingerprints until executor restart.
Steps of Reproduction ✅
1. Open the MiMoCode provider detail page where `NoAuthAccountCard` is used when
`providerId === "mimocode"`
(`src/app/(dashboard)/dashboard/providers/[id]/ProviderDetailPageClient.tsx:469-476`) and
add several accounts so that `providerSpecificData.fingerprints` stores multiple IDs for
that connection.

2. Remove one of the accounts in the UI; `handleRemoveAccount` in
`src/shared/components/NoAuthAccountCard.tsx:138-151` filters that ID out of
`allAccountIds` and sends a `PUT /api/providers/{conn.id}` with an updated
`providerSpecificData` object where the removed fingerprint is no longer present in the
`fingerprints` array.

3. Send a chat request through the MiMoCode provider so that the backend uses
`getExecutor("mimocode")` (`open-sse/executors/index.ts:150`) and calls
`MimocodeExecutor.execute()` (`open-sse/executors/mimocode.ts:365-393`) with
`input.credentials` built from the same connection row, including the updated
`providerSpecificData.fingerprints` list that omits the removed account.

4. Inside `execute()`, `this.syncAccountsFromCredentials(input.credentials)`
(`mimocode.ts:393`) runs the sync logic at `mimocode.ts:217-235`, which only appends
accounts for new fingerprints (loop at 219-233) and never removes `this.accounts` entries
whose `fingerprint` no longer appears in `credentials.providerSpecificData.fingerprints`;
subsequent calls to `pickAccount()` (`mimocode.ts:266-277`) and `getJwtForAccount()`
(`mimocode.ts:252-263`) can still pick and bootstrap JWTs for the removed fingerprint, so
that stale accounts continue to be used until the process restarts, even though they are
no longer present in the persisted credentials (proxy config is re-synced at 242-247 but
the account itself is not pruned).

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** open-sse/executors/mimocode.ts
**Line:** 219:233
**Comment:**
	*Stale Reference: Account syncing only appends new fingerprints and never removes ones that were deleted from credentials, so removed accounts remain in memory and can still be selected for JWT/bootstrap/chat calls. This causes stale account usage (including stale proxy settings) until process restart. Rebuild `this.accounts` from the current fingerprint set (or prune missing entries) during sync.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

Comment on lines +236 to +240
const accountProxies = credentials?.providerSpecificData
?.accountProxies as AccountProxyConfig[] | undefined;
const proxyMap = Array.isArray(accountProxies)
? new Map(accountProxies.map((ap) => [ap.fingerprint, ap.proxy] as const))
: null;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: accountProxies is cast and mapped without validating entries, so malformed persisted data like null or non-object items will throw when reading ap.fingerprint, crashing execution before any request is sent. Filter/validate each element before building the map. [null pointer]

Severity Level: Critical 🚨
- ❌ MiMoCode execute() crashes when accountProxies contains malformed entries.
- ⚠️ Test connection for MiMoCode fails with runtime TypeError.
Steps of Reproduction ✅
1. Configure a MiMoCode connection so that its persisted
`providerSpecificData.accountProxies` contains malformed data such as `[null]` or an array
with non-object items (there is no runtime validation on read; the UI helper
`getAccountProxies` in `src/shared/components/NoAuthAccountCard.tsx:36-38` simply casts
`conn?.providerSpecificData?.accountProxies` to `AccountProxyConfig[]`).

2. On the backend, obtain the MiMoCode executor by calling `getExecutor("mimocode")` from
`open-sse/executors/index.ts:150` (or instantiate `new MimocodeExecutor()` as in
`tests/unit/mimocode-executor.test.ts:10`) and then invoke either `testConnection()`
(`open-sse/executors/mimocode.ts:330-363`) or `execute()` (`mimocode.ts:365-393`) with
`credentials` whose `providerSpecificData.accountProxies` contains the malformed array.

3. Both `testConnection()` and `execute()` call
`this.syncAccountsFromCredentials(credentials)` at `mimocode.ts:336` and
`mimocode.ts:393`, which enters `syncAccountsFromCredentials` (`mimocode.ts:217-250`) and
reads `accountProxies` from `credentials?.providerSpecificData?.accountProxies` at lines
236-237, casting it to `AccountProxyConfig[] | undefined` without any per-element
validation.

4. Because `Array.isArray(accountProxies)` is true for arrays like `[null]`, the code
executes `new Map(accountProxies.map((ap) => [ap.fingerprint, ap.proxy] as const))` at
`mimocode.ts:238-239`; when `ap` is `null` or a non-object, accessing `ap.fingerprint`
throws a `TypeError` (e.g., "Cannot read properties of null"), causing
`testConnection()`/`execute()` to crash before any JWT bootstrap or chat request is
attempted.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** open-sse/executors/mimocode.ts
**Line:** 236:240
**Comment:**
	*Null Pointer: `accountProxies` is cast and mapped without validating entries, so malformed persisted data like `null` or non-object items will throw when reading `ap.fingerprint`, crashing execution before any request is sent. Filter/validate each element before building the map.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

Comment on lines +37 to +39
await onDistribute();
setState("complete");
timerRef.current = setTimeout(() => setState("idle"), 1500);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The completion timeout is never cleared before starting a new distribution, so an old timer can flip state back to idle while a new async run is still in progress, re-enabling the button and allowing duplicate concurrent submissions. Clear any existing timer at click start (and before creating a new one) to keep state transitions consistent. [race condition]

Severity Level: Major ⚠️
- ⚠️ Distribute Proxies can fire overlapping connection update requests.
- ⚠️ Button state misleads users about in-flight proxy distribution.
Steps of Reproduction ✅
1. In the connections dashboard header, use the `DistributeProxiesButton` rendered by
`ConnectionsHeaderToolbar` at
`src/app/(dashboard)/dashboard/providers/[id]/components/ConnectionsHeaderToolbar.tsx:224-228`,
which passes `onDistribute={async () => { await handleDistributeProxies(); }}` to trigger
proxy distribution for the provider.

2. Note that `DistributeProxiesButton` in
`src/shared/components/DistributeProxiesButton.tsx:18-43` uses internal `state` and
`timerRef`; on click, `handleClick` (33-43) sets `state` to `"distributing"`, awaits
`onDistribute()`, then sets `state` to `"complete"` and starts a `setTimeout` at line 39
that will set `state` back to `"idle"` after 1500 ms, without clearing any existing timer.

3. Click "Distribute Proxies" once; after `onDistribute` resolves and the label changes to
"Complete" (but before 1.5 s has passed), click the button again—`state` is `"complete"`,
so the guard `if (disabled || state === "distributing") return;` at line 34 allows the
second click, starting a new async `onDistribute()` run while the first run's completion
timer is still pending.

4. If this second `onDistribute()` call (which ultimately performs fetches to
`/api/settings/proxies` and `/api/providers/{conn.id}` in `handleDistributeProxies` at
`src/shared/components/NoAuthAccountCard.tsx:215-250`) takes longer than 1500 ms, the
stale timer from the first run fires and executes `setState("idle")` while the second run
is still in progress, re-enabling the button (`isDisabled` at line 45 only checks `state
=== "distributing"`); a third click can now start another `onDistribute()` concurrently
with the in-flight run, leading to overlapping proxy distribution requests.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** src/shared/components/DistributeProxiesButton.tsx
**Line:** 37:39
**Comment:**
	*Race Condition: The completion timeout is never cleared before starting a new distribution, so an old timer can flip state back to idle while a new async run is still in progress, re-enabling the button and allowing duplicate concurrent submissions. Clear any existing timer at click start (and before creating a new one) to keep state transitions consistent.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cf1bec8320

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +234 to +235
...(proxy.username ? { username: proxy.username } : {}),
...(proxy.password ? { password: proxy.password } : {}),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid copying redacted proxy credentials

When using this new distribute action with an authenticated saved proxy, /api/settings/proxies returns the redacted registry view (listProxies({ includeSecrets: false }), where redactProxySecrets() replaces non-empty username/password with "***"). Copying those fields here persists "***" as the per-account proxy credential, so subsequent MiMoCode requests authenticate to the proxy with the mask instead of the real secret and fail. Resolve the proxy server-side by ID or otherwise avoid using the redacted GET payload as credentials.

Useful? React with 👍 / 👎.

host: trimmedHost,
port: Number(proxyPort) || 1080,
...(proxyUsername.trim() ? { username: proxyUsername.trim() } : {}),
...(proxyPassword.trim() ? { password: proxyPassword.trim() } : {}),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Do not store raw proxy passwords in provider data

When a user manually configures a per-account proxy with a password, this writes the raw password into providerSpecificData.accountProxies; provider-specific data is persisted as JSON and sanitizeProviderSpecificDataForResponse() only removes a few top-level secret keys, so GET /api/providers returns this nested password to the browser instead of following the proxy registry's redaction behavior. Store a proxy reference or add encryption/redaction for nested proxy secrets before persisting them here.

Useful? React with 👍 / 👎.

Comment on lines +305 to +306
<button
onClick={() => openProxyConfig(id)}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Gate per-account proxy UI to supported providers

This shared card is rendered for both mimocode and opencode in ProviderDetailPageClient, but a repo-wide search shows only open-sse/executors/mimocode.ts consumes providerSpecificData.accountProxies; OpencodeExecutor ignores it. As a result, OpenCode users can configure or distribute these new per-account proxies and see them saved in the UI, but their traffic still will not use them. Hide these controls unless the current provider's executor implements the setting, or wire OpenCode to read it.

Useful? React with 👍 / 👎.

@KooshaPari

Copy link
Copy Markdown
Owner Author

Ready to merge — upstream feature sync (L5-123)

Status: MERGEABLE / UNSTABLE (pre-existing-main CI state).

Diff stat: +1129 / -102 across 14 files (3 cherry-picks, zero conflicts on 2 of 3, 1 partial).

Branch: chore/l5-123-upstream-feature-sync-2026-06-21 (off origin/main e4d751ed1).

Critical checks

  • ✅ OpenSSF Scorecard — pass
  • ✅ CodeQL Analysis — pass
  • ✅ Socket Security — pass
  • ✅ PR Test Policy — pass
  • ⚠️ Pre-existing fail set (Build / Lint / Gitleaks / Dep audit / SonarCloud / i18n) — not introduced.

Cherry-picks landed

Cherry-pick heuristic (validated)

  1. Check file exists in HEAD before applying
  2. Deleted files → "ours" + audit commit
  3. Different-path files → keep applicable, drop no-op
  4. New test files importing deleted modules → drop test, keep audit
  5. Always cherry-pick -x for traceability

Refs

Notes

  • Working tree discipline: B10 OTel bridge work that landed concurrently on feat/l5-123-b10-otel-bridge-2026-06-21 is not included; this PR only contains the 3 cherry-pick commits.
  • CODEOWNERS review request skipped (self-only-owner).
  • cherry-pick -x on all 3 commits.

Ready for squash-merge into main.

@KooshaPari

Copy link
Copy Markdown
Owner Author

Review-ready summary

This PR cherry-picks 3 medium-relevance upstream features. Ready for merge.

What to verify

  1. mimocode per-account proxy (f42e8fa) — 9 files, +935/−100. KP is already ahead on mimocode executor + distribute button; the per-account unit test from upstream is the net addition.
  2. Gemini thinking-budget clamp (337cd18) — 4 files, +50/−2. Per-model cap for Gemini thinking budget.
  3. Each lands cleanly with no behavioral regression.

Merge checklist

  • All cherry-picks conflict-resolved cleanly
  • Test suite covers new paths
  • 14 files, +1129/−102

Ready for review / merge.

@KooshaPari

Copy link
Copy Markdown
Owner Author

Review-ready summary

This PR cherry-picks 3 medium-relevance upstream features. Ready for merge.

  1. mimocode per-account proxy (f42e8fa) - 9 files, +935/-100. KP is already ahead on mimocode executor; the per-account unit test from upstream is the net addition.
  2. Gemini thinking-budget clamp (337cd18) - 4 files, +50/-2.

Ready for review / merge.

@KooshaPari
KooshaPari force-pushed the chore/l5-123-upstream-feature-sync-2026-06-21 branch from cf1bec8 to 8909a6c Compare July 2, 2026 07:39
@KooshaPari
KooshaPari merged commit d0da145 into main Jul 2, 2026
11 of 22 checks passed
@KooshaPari
KooshaPari deleted the chore/l5-123-upstream-feature-sync-2026-06-21 branch July 2, 2026 07:39
@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

L17 Latency Budget Report

--- Latency Budget Summary ---
  Total endpoints checked: 0
  Passed: 0
  Warnings: 0
  Failures: 0

Checked against: budgets/rest-endpoints.yaml.

@sonarqubecloud

sonarqubecloud Bot commented Jul 2, 2026

Copy link
Copy Markdown

@kilo-code-bot

kilo-code-bot Bot commented Jul 3, 2026 •

Copy link
Copy Markdown

Code Review Summary

Status: No Issues Found in Current Diff | Recommendation: Merge (already merged)

Overview

Severity Count
CRITICAL 0
WARNING 0
SUGGESTION 0
Files Reviewed (1 file)
  • worklogs/2026-06-21-L5-123-upstream-feature-sync.md

Note: The current PR diff only contains the worklog file. Several automated reviews previously identified valid issues in related code files (mimocode.ts, DistributeProxiesButton.tsx, NoAuthAccountCard.tsx, integration tests) that are now part of main`. Those findings should be tracked for follow-up fixes in a separate PR, as the PR is already merged and the current diff does not cover them.


Reviewed by step-3.7-flash-20260528 · Input: 652.1K · Output: 25.8K · Cached: 3.6M

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XXL This PR changes 1000+ lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant