Repository navigation
chore(release): v3.3.11 — analytics, backup control, CLI fixes, workflow unification - #860
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![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 updates the project to version 3.3.11, introducing several bug fixes and features. Key changes include updating the Qwen provider's base URL and adding specific retry logic for its quota limits, expanding the CLI binary size detection limit to 350MB, and refining the Codex executor to prevent schema rejections. Additionally, the release workflow was automated, and OAuth popup blocking is now handled with a manual fallback. Feedback suggests improving test isolation by using temporary directories instead of the home directory, removing placeholder comments in documentation, and refactoring provider-specific logic out of the core chat handler to improve maintainability.
| const testRoot = path.join(os.homedir(), ".omniroute-test-tmp"); | ||
| if (!fs.existsSync(testRoot)) { | ||
| fs.mkdirSync(testRoot, { recursive: true }); | ||
| } | ||
| return fs.mkdtempSync(path.join(testRoot, "cli-test-")); |
There was a problem hiding this comment.
Creating test directories in the user's home directory (os.homedir()) is intrusive and can leave clutter on the developer's machine. It may also fail in CI environments with restricted home directory access. It is better to use os.tmpdir() for temporary test files.
const testRoot = path.join(os.tmpdir(), ".omniroute-test-tmp");
if (!fs.existsSync(testRoot)) {
fs.mkdirSync(testRoot, { recursive: true });
}
return fs.mkdtempSync(path.join(testRoot, "cli-test-"));| ### 11. Pull main and create tag | ||
| ### 11. Create Git Tag and GitHub Release (MANDATORY) | ||
|
|
||
| // turbo |
There was a problem hiding this comment.
The string // turbo appears to be a leftover placeholder or an incorrectly formatted comment. In Markdown, comments should use the <!-- comment --> syntax, or if this is a note for the user, it should be formatted as plain text or a blockquote.
| @@ -959,7 +959,8 @@ export async function handleChatCore({ | |||
| if ( | |||
| targetFormat === FORMATS.OPENAI && | |||
| !bodyToSend.prompt_cache_key && | |||
| Array.isArray(bodyToSend.messages) | |||
| Array.isArray(bodyToSend.messages) && | |||
| !["nvidia", "codex", "xai"].includes(provider) | |||
There was a problem hiding this comment.
Hardcoding provider-specific exclusions for prompt caching directly in the core handler reduces maintainability. As the number of providers grows, this list will become difficult to manage. Consider moving this configuration into the providerRegistry or adding a capability flag to the provider definition.
| ); | ||
| const rawResult = await withRateLimit(provider, connectionId, modelToCall, async () => { | ||
| let attempts = 0; | ||
| const maxAttempts = provider === "qwen" ? 3 : 1; |
There was a problem hiding this comment.
Provider-specific retry logic (like maxAttempts for "qwen") should ideally be encapsulated within the provider's executor or defined in the registry. Placing it here in handleChatCore couples the core logic to specific provider quirks.
Code Review SummaryStatus: 1 Observation Found | Recommendation: Merge Overview
Observation Details (click to expand)SUGGESTION
Files Reviewed (17 files)
Previous Issues StatusAll 7 previously commented issues remain unchanged in this review:
Code Quality AssessmentAll changes in this PR appear sound:
The single observation (JSDoc inconsistency at line 565) is a minor documentation issue that doesn't affect functionality. The code implementation correctly reflects the 350MB limit. |
There was a problem hiding this comment.
Pull request overview
Release prep for v3.3.11, primarily addressing test regressions and CLI runtime detection behavior, along with additional provider/runtime fixes and version bumps across packages/docs.
Changes:
- Fix CLI runtime detection to respect
CLI_{TOOL}_BINoverrides and raise the known-path binary size ceiling. - Update Qwen registry URL expectation and add/adjust runtime/provider behaviors (Qwen retry handling, Codex request sanitization, OAuth popup-block fallback).
- Bump versions and update release artifacts (OpenAPI version, changelog entry, release workflow steps).
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/t28-model-catalog-updates.test.mjs | Updates Qwen registry base URL expectation in unit test. |
| tests/unit/cli-runtime-detection.test.mjs | Adjusts temp dir strategy and padding in CLI detection tests. |
| src/shared/services/cliRuntime.ts | Raises known-path size limit; ensures env override bypasses known-path discovery rules. |
| src/shared/components/OAuthModal.tsx | Handles popup-blocked case by switching to manual input flow for Codex localhost flow. |
| open-sse/handlers/chatCore.ts | Adds provider gating for prompt_cache_key injection; adds Qwen 429 quota backoff retries. |
| open-sse/executors/codex.ts | Strips messages/prompt fields to satisfy strict Codex Responses schema. |
| open-sse/config/providerRegistry.ts | Adds gpt-5.4-mini; updates Qwen base URL. |
| package.json | Bumps app version to 3.3.11. |
| package-lock.json | Updates lockfile versions to 3.3.11 (including @omniroute/open-sse). |
| open-sse/package.json | Bumps @omniroute/open-sse version to 3.3.11. |
| electron/package.json | Bumps desktop app version to 3.3.11. |
| docs/openapi.yaml | Bumps OpenAPI info version to 3.3.11. |
| CHANGELOG.md | Adds 3.3.11 release notes. |
| .agents/workflows/generate-release.md | Consolidates tag + release creation steps in the release workflow doc. |
Comments suppressed due to low confidence (1)
.agents/workflows/generate-release.md:193
- Phase 2 step numbering now jumps from "11" to "14" (since old steps 12/13 were removed). Please renumber the headings (or reintroduce missing numbers) to keep the workflow instructions unambiguous.
### 11. Create Git Tag and GitHub Release (MANDATORY)
// turbo
```bash
git checkout main
git pull origin main
VERSION=$(node -p "require('./package.json').version")
git tag -a "v$VERSION" -m "Release v$VERSION"
git push origin --tags
gh release create "v$VERSION" --title "v$VERSION" --notes "OmniRoute v$VERSION Release" --target main
14. 🐳 Trigger Docker Hub build (MANDATORY — keep npm and Docker in sync)
</details>
---
💡 <a href="/diegosouzapw/OmniRoute/new/main?filename=.github/instructions/*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.
| const testRoot = path.join(os.homedir(), ".omniroute-test-tmp"); | ||
| if (!fs.existsSync(testRoot)) { | ||
| fs.mkdirSync(testRoot, { recursive: true }); | ||
| } | ||
| return fs.mkdtempSync(path.join(testRoot, "cli-test-")); | ||
| } | ||
|
|
There was a problem hiding this comment.
createTempDir now creates a persistent ~/.omniroute-test-tmp directory but the tests only rmSync the mkdtemp subdirectory, leaving the root directory behind after test runs. Consider reverting to os.tmpdir() (typical for unit tests) or explicitly removing the root directory when it becomes empty to avoid polluting developers’ home dirs and potential permission issues in restricted CI environments.
| const testRoot = path.join(os.homedir(), ".omniroute-test-tmp"); | |
| if (!fs.existsSync(testRoot)) { | |
| fs.mkdirSync(testRoot, { recursive: true }); | |
| } | |
| return fs.mkdtempSync(path.join(testRoot, "cli-test-")); | |
| } | |
| // Use the OS temp directory for test isolation and to avoid polluting $HOME. | |
| return fs.mkdtempSync(path.join(os.tmpdir(), "cli-test-")); | |
| } | |
| } |
| // CLI binaries should be > 30 bytes and < 350MB | ||
| // npm .cmd wrappers on Windows are ~300-500 bytes, JS wrappers on Linux can be ~44 bytes | ||
| // Minimum catches empty/suspicious files while allowing legitimate thin wrappers | ||
| if (stat.size < 30 || stat.size > 100 * 1024 * 1024) { | ||
| // Many modern CLIs (like Claude Code and OpenCode) build as single ~150-250MB binaries | ||
| if (stat.size < 30 || stat.size > 350 * 1024 * 1024) { | ||
| return { installed: false, commandPath: null, reason: "suspicious_size" }; |
There was a problem hiding this comment.
The JSDoc above checkKnownPath still mentions a 100MB upper bound, but this change raises the limit to 350MB. Please update the JSDoc accordingly so documentation matches the implemented security checks.
| { | ||
| "name": "omniroute", | ||
| "version": "3.3.10", | ||
| "version": "3.3.11", | ||
| "description": "Smart AI Router with auto fallback — route to FREE & cheap models, zero downtime. Works with Cursor, Cline, Claude Desktop, Codex, and any OpenAI-compatible tool.", | ||
| "type": "module", | ||
| "bin": { |
There was a problem hiding this comment.
PR description/title say this is only "test regression fixes" (CLI override + DashScope/Qwen endpoint test correction), but this diff also includes additional functional changes (e.g., Qwen 429 retry logic in chatCore, Codex request body schema stripping, OAuth popup-block fallback behavior, Codex model registry update). Please update the PR description to reflect the full scope (or split unrelated changes) so reviewers/release notes match what’s actually shipping.
37095c2 to
ee4df28
Compare
diegosouzapw
changed the title
….3.11 chore(release): v3.3.11 — analytics, backup control, CLI fixes, workflow unification
CodeQL js/incomplete-url-substring-sanitization, alerts #860 and #861: volcengineConsoleAutoLogin accepted any cookie whose `domain` merely *contained* "volcengine.com". That check is an authorization decision, not a string test. The console auto-login harvests `digest`, `AccountID`, `csrfToken` and `userInfo` out of the Playwright context and persists them as the operator's Volcengine credentials, so a cookie set by `volcengine.com.attacker.tld` — or `notvolcengine.com` — was captured and stored as a provider connection. Add `matchesCookieDomain()` (open-sse/utils/cookieDomain.ts): exact host or dot-boundary suffix, leading dots and case normalized on both sides, failing closed on an empty expected domain. Same shape as the existing `isAdobeCookieDomain` in adobeFireflyBrowserLogin.ts, which already got this right. While sweeping the class, inAppLoginService's cookie capture had the identical weakness — `c.domain.includes(domain.replace(/^\./, ""))` — with the identical consequence: a look-alike host's cookie stored as the operator's credential. CodeQL did not flag it because the expected domain comes from TOKEN_EXTRACTION_CONFIGS rather than a literal. Both callsites now share the helper. tests/unit/volcengine-cookie-domain-suffix.test.ts — 5 tests, red before the fix, covering the real domains, seven look-alikes, empty/missing input, and the config-supplied path. Co-authored-by: Xiangzhe <bakryun0718@proton.me>
#11452) Clears CodeQL js/incomplete-url-substring-sanitization alerts #860-#865: - volcengineConsoleAutoLogin: cookie domain filter now uses an exact/ dot-suffix helper (isVolcengineCookieDomain) instead of substring includes(), rejecting look-alike hosts like volcengine.com.evil.test - security-s1-s2-s4 tests: agent-card/agent.json URL assertions compare parsed origin equality instead of startsWith prefix Co-authored-by: Markus Hartung <mail@hartmark.se>
….3.11 chore(release): v3.3.11 — analytics, backup control, CLI fixes, workflow unification
cheaper-inference#11452) Clears CodeQL js/incomplete-url-substring-sanitization alerts cheaper-inference#860-cheaper-inference#865: - volcengineConsoleAutoLogin: cookie domain filter now uses an exact/ dot-suffix helper (isVolcengineCookieDomain) instead of substring includes(), rejecting look-alike hosts like volcengine.com.evil.test - security-s1-s2-s4 tests: agent-card/agent.json URL assertions compare parsed origin equality instead of startsWith prefix Co-authored-by: Markus Hartung <mail@hartmark.se>
…-inference#11429) CodeQL js/incomplete-url-substring-sanitization, alerts cheaper-inference#860 and cheaper-inference#861: volcengineConsoleAutoLogin accepted any cookie whose `domain` merely *contained* "volcengine.com". That check is an authorization decision, not a string test. The console auto-login harvests `digest`, `AccountID`, `csrfToken` and `userInfo` out of the Playwright context and persists them as the operator's Volcengine credentials, so a cookie set by `volcengine.com.attacker.tld` — or `notvolcengine.com` — was captured and stored as a provider connection. Add `matchesCookieDomain()` (open-sse/utils/cookieDomain.ts): exact host or dot-boundary suffix, leading dots and case normalized on both sides, failing closed on an empty expected domain. Same shape as the existing `isAdobeCookieDomain` in adobeFireflyBrowserLogin.ts, which already got this right. While sweeping the class, inAppLoginService's cookie capture had the identical weakness — `c.domain.includes(domain.replace(/^\./, ""))` — with the identical consequence: a look-alike host's cookie stored as the operator's credential. CodeQL did not flag it because the expected domain comes from TOKEN_EXTRACTION_CONFIGS rather than a literal. Both callsites now share the helper. tests/unit/volcengine-cookie-domain-suffix.test.ts — 5 tests, red before the fix, covering the real domains, seven look-alikes, empty/missing input, and the config-supplied path. Co-authored-by: Xiangzhe <bakryun0718@proton.me>
🚀 Release v3.3.11
🚀 Features
OMNIROUTE_DISABLE_AUTO_BACKUPenv flag (Add env flag to disable automatic SQLite backups #846)gpt-5.4-minito Codex provider ([Feature] Add model gpt-5.4-mini #756)🐛 Bug Fixes
chat.qwen.aiWeb Inference API ([BUG] Incorrect API key provided (qwen) #844, [BUG] Reopened(751) – Qwen Code continues to default to the Alibaba API key and fails to use OAuth despite a successful Qwen OAuth connection #807, [BUG] Unexpected Qwen quota (429) errors when using OmniRoute as proxy despite low request rate #832)application/jsonboundary handling ([BUG] Claude Token Refresh Failed #836)CLI_{PROVIDER}_BIN)prompt_cache_keyfor non-Anthropic ([BUG] [400]: Validation: Unsupported parameter(s):prompt_cache_key[nvidia] #848)🔧 Workflow Unification
/review-prs,/resolve-issues,/implement-featuresworkflowsrelease/vX.Y.Zbranchfix/,feat/, topic branches — single branch per release cycle🧹 Branch Cleanup
coder/provider-limits-last-refreshed,fix/issue-815,release/v3.3.9,feat/subscription-utilizationTests