Repository navigation
Feat/features batch v3.8.0 - #2431
Conversation
Registers t3-web / t3chat-alias executor, 24-model registry, WEB_COOKIE_PROVIDERS entry, i18n hints, docs row, and 19-case test suite (all passing). Endpoint URL and SSE chunk schema require a post-devtools-capture follow-up — marked with TODO(post-devtools-capture) comments throughout.
…UI panel (#2306) - Refactor dockerDetect.ts to accept optional deps for testability - Add Docker guard (HTTP 422 + zedDockerEnvironment flag) to import route - Add POST /api/providers/zed/manual-import endpoint with Zod validation - Add collapsible Manual Token Import panel to Zed provider page (auto-expands on Docker error) - Add 4 unit tests for isRunningInDocker via dependency injection - Add docs/providers/ZED-DOCKER.md Docker setup guide
…-cost notes for #1812
![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 several new features and improvements, including Zed IDE Docker support, a new 'llama.cpp' local provider, and a robust API key rotation and status command set for the CLI. It also adds a 't3.chat' web provider and implements per-connection OIDC client isolation for Kiro accounts to prevent session conflicts. I have reviewed the code and identified several high-severity issues where functions like runProviderTest, statusColor, and getModelContextLimitForModelString are referenced but not imported, which will cause runtime crashes. Additionally, there is a suggestion to refactor redundant database write logic in the new provider rotation command.
| try { | ||
| const fresh = findProviderConnection(db3, connection.id); | ||
| if (fresh) { | ||
| const result = await runProviderTest(db3, fresh); |
There was a problem hiding this comment.
The function runProviderTest is used here but it is not imported or defined within this module. This will cause a runtime error when the command attempts to perform a post-rotation connectivity test.
| const expiryColor = statusColor(expiryStatus); | ||
| const testColor = statusColor(testStatus); |
There was a problem hiding this comment.
The function statusColor is used but not defined or imported in this file. This will cause the providers status command to crash when attempting to format the output table.
| const estimatedInputTokens = estimateTokens(body?.messages ?? []); | ||
| if (estimatedInputTokens > 0) { | ||
| const filteredByContext = eligibleTargets.filter((target) => { | ||
| const limit = getModelContextLimitForModelString(target.modelStr); |
There was a problem hiding this comment.
The function getModelContextLimitForModelString is used but not imported. It should likely be imported from ./modelCapabilities.ts where other model metadata helpers reside.
| const serverUp = await isServerUp(); | ||
| if (serverUp) { | ||
| try { | ||
| const res = await apiFetch(`/api/providers/${encodeURIComponent(connection.id)}`, { | ||
| method: "PATCH", | ||
| body: { apiKey: newKey, testStatus: "unknown", lastError: null, rateLimitedUntil: null, backoffLevel: 0 }, | ||
| retry: false, | ||
| acceptNotOk: true, | ||
| }); | ||
| if (!res.ok) { | ||
| console.error(t("common.error", { message: `HTTP ${res.status}` })); | ||
| return 1; | ||
| } | ||
| } catch { | ||
| // Fall through to direct DB write | ||
| const { db: db2 } = await openOmniRouteDb(); | ||
| try { | ||
| updateProviderApiKey(db2, connection.id, encryptCredential(newKey)); | ||
| } finally { | ||
| db2.close(); | ||
| } | ||
| } | ||
| } else { | ||
| const { db: db2 } = await openOmniRouteDb(); | ||
| try { | ||
| updateProviderApiKey(db2, connection.id, encryptCredential(newKey)); | ||
| } finally { | ||
| db2.close(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for updating the provider API key in the database is duplicated across the try-catch and else blocks. Additionally, the database is opened and closed multiple times (openOmniRouteDb). Consider refactoring the database write into a single block and keeping the connection open if multiple operations are needed to improve efficiency.
Code Review Summary (Incremental Review)Status: 10 Issues Found (2 CRITICAL) | Recommendation: PR already merged — address urgently via follow-up fix PR Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Other Observations (not in diff)Issues found in unchanged code or outside diff hunks:
Files Reviewed (68 files)
Reviewed by qwen3.6-plus · 1,429,127 tokens |
![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: a9cef6dbad
ℹ️ 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".
| const missing = !rawCreds.cookies | ||
| ? "cookies" | ||
| : !rawCreds.convexSessionId | ||
| ? "convexSessionId" |
There was a problem hiding this comment.
When execute() is called without a credentials object (e.g., misconfigured provider connection), this branch dereferences rawCreds.cookies/rawCreds.convexSessionId before any null-safe check, which throws a TypeError instead of returning the intended 400 error payload. That turns a recoverable validation error into an unhandled executor failure path for t3-web requests.
Useful? React with 👍 / 👎.
| // #1731: Per-request in-memory set of providers whose quota is fully exhausted. | ||
| // When a target returns a quota-exhausted 429, remaining same-provider targets are skipped. | ||
| const exhaustedProviders = new Set<string>(); | ||
| let globalAttempts = 0; |
There was a problem hiding this comment.
exhaustedProviders is scoped outside the for (setTry...) loop, so once a provider is marked exhausted in the first pass, later set retries skip that provider entirely instead of retrying it. With maxSetRetries > 0, this can make set retries no-ops and surface misleading terminal behavior (e.g., generic inactive-accounts response) even though the feature is supposed to retry the full target set.
Useful? React with 👍 / 👎.
…ation/i18n/test regressions Resets 68 files to release/v3.8.0 state to prevent: - Auth: allExpired detection, apiKeyHealth sync, terminal connection handling - Security: federation leaderboard auth, pbkdf2 hashing, antiCheat zScore - Executor: fixToolPairs after fixToolAdjacency (Claude tool adjacency) - Provider: apiKeyHealth cleanup on key rotation - UI: NotificationToast onClick stopPropagation - i18n: ~50 deleted keys in en.json + all 41 locales - Tests: 500+ lines of deleted tests restored New features (t3.chat, combo exhaustion, Kiro multi-account, Zed Docker, context window filter, CLI rotate, E2E failover) remain intact.
…nflicts with release version
…batch-v3.8.0 Integrated into release/v3.8.0 — features batch with regression-safe resets New features: - feat(combo): provider-level exhaustion tracking (diegosouzapw#1731) - feat(combo): context window filtering (diegosouzapw#1808) - fix(combo): cost blending in auto-combo scoring (diegosouzapw#1812) - feat(providers): t3.chat web provider (diegosouzapw#1909) - feat(cli): providers rotate command (diegosouzapw#1881) - feat(kiro): multi-account OAuth isolation (diegosouzapw#2328) - feat(errors): sanitized upstream error details (diegosouzapw#1718) - feat(installer): Termux detection (diegosouzapw#1764) - feat(zed): Docker integration + manual import (diegosouzapw#2306) - test(e2e): system failover test suite 68 regressive files were reset to release/v3.8.0 state to prevent regressions.
Sync release/v3.8.0 into main — includes features batch (diegosouzapw#2431), AgentRouter docs (diegosouzapw#2429), Gemini 3.5 Flash (diegosouzapw#2423)
…batch-v3.8.0 Integrated into release/v3.8.0 — features batch with regression-safe resets New features: - feat(combo): provider-level exhaustion tracking (diegosouzapw#1731) - feat(combo): context window filtering (diegosouzapw#1808) - fix(combo): cost blending in auto-combo scoring (diegosouzapw#1812) - feat(providers): t3.chat web provider (diegosouzapw#1909) - feat(cli): providers rotate command (diegosouzapw#1881) - feat(kiro): multi-account OAuth isolation (diegosouzapw#2328) - feat(errors): sanitized upstream error details (diegosouzapw#1718) - feat(installer): Termux detection (diegosouzapw#1764) - feat(zed): Docker integration + manual import (diegosouzapw#2306) - test(e2e): system failover test suite 68 regressive files were reset to release/v3.8.0 state to prevent regressions.
Sync release/v3.8.0 into main — includes features batch (diegosouzapw#2431), AgentRouter docs (diegosouzapw#2429), Gemini 3.5 Flash (diegosouzapw#2423)
…batch-v3.8.0 Integrated into release/v3.8.0 — features batch with regression-safe resets New features: - feat(combo): provider-level exhaustion tracking (diegosouzapw#1731) - feat(combo): context window filtering (diegosouzapw#1808) - fix(combo): cost blending in auto-combo scoring (diegosouzapw#1812) - feat(providers): t3.chat web provider (diegosouzapw#1909) - feat(cli): providers rotate command (diegosouzapw#1881) - feat(kiro): multi-account OAuth isolation (diegosouzapw#2328) - feat(errors): sanitized upstream error details (diegosouzapw#1718) - feat(installer): Termux detection (diegosouzapw#1764) - feat(zed): Docker integration + manual import (diegosouzapw#2306) - test(e2e): system failover test suite 68 regressive files were reset to release/v3.8.0 state to prevent regressions.
Sync release/v3.8.0 into main — includes features batch (diegosouzapw#2431), AgentRouter docs (diegosouzapw#2429), Gemini 3.5 Flash (diegosouzapw#2423)
1 participant
Summary
Related Issues
Validation
npm run lintnpm run test:unitnpm run test:coverage>= 60%for statements, lines, functions, and branchesTests Added Or Updated
Coverage Notes
src/,open-sse/,electron/, orbin/, explain which tests cover the change.Reviewer Notes