Repository navigation
fix: bug-fix batch round 3 (#2469, #2470, #2468, #2467, #2480, #2483) - #2495
Conversation
![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 fixes to the settings, API, and provider integration layers, including restoring the Global System Prompt on startup and after imports, and switching its injection logic to append content for better instruction priority. Additionally, the model listing API now filters results based on active provider connections, and Antigravity and Kiro integrations were updated to handle project IDs and token refreshes more reliably. Review feedback suggests using a more robust falsy check for connection activity and maintaining consistency in import paths by removing explicit file extensions.
| try { | ||
| const connections = await getProviderConnections(); | ||
| for (const conn of connections) { | ||
| if (conn.isActive === false) continue; |
There was a problem hiding this comment.
The strict check conn.isActive === false is potentially problematic. If isActive is stored as an integer (e.g., 0 for inactive) in the database, or if it is null or undefined for unvalidated connections, this check will fail to skip them. A more robust and idiomatic approach would be to use a falsy check to ensure only explicitly active/validated connections are included in the list.
| if (conn.isActive === false) continue; | |
| if (!conn.isActive) continue; |
| import { backupDbFile } from "@/lib/db/backup"; | ||
| import { isAuthRequired, isAuthenticated } from "@/shared/utils/apiAuth"; | ||
| import { getSettings } from "@/lib/db/settings"; | ||
| import { setSystemPromptConfig } from "@omniroute/open-sse/services/systemPrompt.ts"; |
There was a problem hiding this comment.
Inconsistent use of the .ts extension in the import path. Other imports in this file (e.g., line 6) and throughout the project omit the extension. For consistency and to follow standard TypeScript practices in this codebase, the extension should be removed.
| import { setSystemPromptConfig } from "@omniroute/open-sse/services/systemPrompt.ts"; | |
| import { setSystemPromptConfig } from "@omniroute/open-sse/services/systemPrompt"; |
| import { isAuthRequired, isAuthenticated } from "@/shared/utils/apiAuth"; | ||
| import { runJsonMigration, type LegacyJsonData } from "@/lib/db/jsonMigration"; | ||
| import { getSettings } from "@/lib/db/settings"; | ||
| import { setSystemPromptConfig } from "@omniroute/open-sse/services/systemPrompt.ts"; |
There was a problem hiding this comment.
Inconsistent use of the .ts extension in the import path. Please remove the extension to maintain consistency with the rest of the project's import style.
| import { setSystemPromptConfig } from "@omniroute/open-sse/services/systemPrompt.ts"; | |
| import { setSystemPromptConfig } from "@omniroute/open-sse/services/systemPrompt"; |
| import { startReasoningCacheCleanupJob } from "./lib/jobs/reasoningCacheCleanupJob"; | ||
| import { getSettings } from "./lib/db/settings"; | ||
| import { applyRuntimeSettings } from "./lib/config/runtimeSettings"; | ||
| import { setSystemPromptConfig } from "@omniroute/open-sse/services/systemPrompt.ts"; |
There was a problem hiding this comment.
Inconsistent use of the .ts extension in the import path. Following the established style in this file (e.g., lines 9-10), module extensions should be omitted.
| import { setSystemPromptConfig } from "@omniroute/open-sse/services/systemPrompt.ts"; | |
| import { setSystemPromptConfig } from "@omniroute/open-sse/services/systemPrompt"; |
![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: f4534c12e6
ℹ️ 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 importedSettings = getSettings(); | ||
| if (importedSettings.systemPrompt) { | ||
| setSystemPromptConfig(importedSettings.systemPrompt); |
There was a problem hiding this comment.
getSettings() is async, but this code reads importedSettings.systemPrompt from the unresolved Promise, so the condition is always false and the in-memory global system prompt is never refreshed after JSON import. This leaves runtime prompt state stale until a restart, which means the import path still does not apply the imported prompt settings.
Useful? React with 👍 / 👎.
| const importedSettings = getSettings(); | ||
| if (importedSettings.systemPrompt) { | ||
| setSystemPromptConfig(importedSettings.systemPrompt); |
There was a problem hiding this comment.
This route has the same async misuse: getSettings() returns a Promise, but importedSettings.systemPrompt is accessed synchronously. As a result, the newly added rehydrate logic is effectively a no-op and the imported database’s global system prompt is not restored in-memory after backup import.
Useful? React with 👍 / 👎.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| provider, | ||
| authType: "apikey", | ||
| apiKey: `test-key-${provider}`, | ||
| testStatus: "active", |
There was a problem hiding this comment.
SUGGESTION: testStatus: "active" is misleading — this field is stored separately and does NOT control whether the connection is active. The connection is active because createProviderConnection defaults isActive: true when data.isActive is undefined (providers.ts:251), regardless of testStatus.
If a future change modifies that default, this test would silently pass for the wrong reason (empty model list from no active connections) and the second assertion ("configured anthropic must be present") would fail confusingly.
Consider using isActive: true explicitly to make the test's intent clear, or remove testStatus since it plays no role here.
Code Review SummaryStatus: 1 SUGGESTION Found | Recommendation: Merge (minor test clarity improvement) Overview
Files Reviewed (9 source files)
Inline Comments
Notes
Reviewed by qwen3.6-plus · 1,098,303 tokens |
|
Bug: missing - const importedSettings = getSettings();
+ const importedSettings = await getSettings();Without |
|
Kilo Code Review could not run — your account is out of credits. Add credits or switch to a free model to enable reviews on this change. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c293bc773d
ℹ️ 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".
| if (!response.ok || data.error === "authorization_pending" || data.error === "slow_down") { | ||
| return NextResponse.json({ | ||
| pending: true, | ||
| error: data.error || "authorization_pending", | ||
| }); |
There was a problem hiding this comment.
The poll handler currently returns { pending: true } for every non-2xx response, not just authorization_pending/slow_down. That makes terminal failures (for example expired_token, access_denied, or invalid device code) look retriable forever, so the client keeps polling and the user never gets a hard failure state. Restrict pending: true to actual retryable error codes and surface other upstream errors as non-pending failures.
Useful? React with 👍 / 👎.
| const providerSpecificData: Record<string, any> = { | ||
| authMethod: "imported", | ||
| provider: provider.charAt(0).toUpperCase() + provider.slice(1), | ||
| }; |
There was a problem hiding this comment.
This code assumes provider is a string and immediately calls charAt/slice, but the new route no longer uses schema validation. A truthy non-string payload (e.g. { provider: {} }) passes the current !provider check and throws here, which is caught by the outer catch and returned as a 500 instead of a 400 validation error. Add request validation (or at least a typeof provider === "string" guard) before these string operations.
Useful? React with 👍 / 👎.
…x-batch-v3.8.2-r3 # Conflicts: # CHANGELOG.md # tests/unit/translator-openai-to-gemini.test.ts
…to worktree-fix-batch-v3.8.2-r3
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29c12f029f
ℹ️ 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".
| if (pollData.success) { | ||
| if (pollRef.current) clearInterval(pollRef.current); | ||
| pollRef.current = null; | ||
| setStep("success"); | ||
| onSuccess?.(); | ||
| } |
There was a problem hiding this comment.
The polling loop only stops when pollData.success is true, and it silently ignores every other result. In scenarios where /api/oauth/kiro/social-exchange returns a terminal failure (for example access_denied, expired_token, 401, or any non-pending error), the UI remains stuck on "Waiting for authorization..." and keeps polling indefinitely instead of surfacing an actionable error. Clear the interval and move to the error state when the response indicates a non-retriable failure.
Useful? React with 👍 / 👎.
…atch-v3.8.2-r3 fix: bug-fix batch round 3 (diegosouzapw#2469, diegosouzapw#2470, diegosouzapw#2468, diegosouzapw#2467, diegosouzapw#2480, diegosouzapw#2483)
2 participants
Summary
Round-3 bug-fix batch from
/resolve-issues-cc— 6 confirmed fixes with tests.bin/omniroute.mjsexecutable (mode 755) so the global CLI runs without a manualchmod +x. (thanks @disonjer)authMethod === "imported") via the social-auth endpoint instead of AWS SSO OIDC (the registered OIDC client can't refresh a social-issued token). (thanks @disonjer)projectIdfromproviderSpecificDataas a fallback and preserve it across token refresh — fixes the spurious422 Missing Google projectIdon the Gemini/v1betastreaming path.GET /v1beta/modelsnow lists only models whose provider has an active/validated connection, matching/v1/models, instead of the entire catalog.Tests
system-prompt.test.ts(prepend→append),token-refresh-service.test.ts(+1 imported-token case),translator-openai-to-gemini.test.ts(+2 projectId fallback),v1beta-models-route.test.ts(+1 active-filter, existing test now seeds a connection).Notes
[Unreleased];check-docs-syncpasses.release/v3.8.2(includes fix(security): replace Math.random with crypto.randomUUID and fix URL hostname check #2489).