Repository navigation
feat(qoder): support PAT via qodercli and remove stale qoder.cn defaults - #913
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 transitions the Qoder provider from a direct API integration using HMAC signatures to a CLI-based transport via qodercli using Personal Access Tokens (PAT). It includes a major rewrite of the Qoder executor to handle child process spawning for both streaming and non-streaming requests, updates the UI to support PAT management, and makes browser-based OAuth experimental and environment-dependent. Feedback focuses on improving the robustness of process termination in the executor and ensuring cross-platform compatibility when resolving the user's home directory in the CLI service.
| const abortChild = () => { | ||
| try { | ||
| child.kill("SIGTERM"); | ||
| } catch {} | ||
| }; |
There was a problem hiding this comment.
The abortChild function should include a SIGKILL fallback after a short delay to ensure the CLI process is terminated if it doesn't respond to SIGTERM. This prevents orphaned processes from hanging around if the CLI becomes unresponsive. This pattern is correctly implemented in open-sse/services/qoderCli.ts but was missed here.
const abortChild = () => {
try {
child.kill("SIGTERM");
} catch {}
setTimeout(() => {
try {
child.kill("SIGKILL");
} catch {}
}, 250).unref?.();
};| import { spawn } from "child_process"; | ||
| import crypto from "crypto"; |
There was a problem hiding this comment.
Add the os module import to provide a more reliable, cross-platform way of retrieving the user's home directory.
| import { spawn } from "child_process"; | |
| import crypto from "crypto"; | |
| import { spawn } from "child_process"; | |
| import crypto from "crypto"; | |
| import os from "os"; |
| const home = String(process.env.HOME || "").trim(); | ||
| return home || process.cwd(); |
There was a problem hiding this comment.
process.env.HOME is not consistently available on all platforms, particularly Windows (where USERPROFILE is the standard). Using os.homedir() is the recommended cross-platform approach in Node.js.
| const home = String(process.env.HOME || "").trim(); | |
| return home || process.cwd(); | |
| const home = os.homedir(); | |
| return home || process.cwd(); |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| token: "https://qoder.cn/oauth/token", | ||
| auth: "https://qoder.cn/oauth", | ||
| token: process.env.QODER_OAUTH_TOKEN_URL || "", | ||
| auth: process.env.QODER_OAUTH_AUTHORIZE_URL || "", |
There was a problem hiding this comment.
WARNING: If env vars not set, OAuth endpoints default to empty string. Ensure documentation clarifies QODER_OAUTH_* vars required for browser OAuth.
| const child = spawn(cliCommand, args, { | ||
| env: { | ||
| ...process.env, | ||
| QODER_PERSONAL_ACCESS_TOKEN: pat, |
There was a problem hiding this comment.
WARNING: PAT passed via environment variable QODER_PERSONAL_ACCESS_TOKEN. Verify qodercli accepts this env var for auth.
| providerId={key} | ||
| provider={info} | ||
| stats={getProviderStats(key, "oauth")} | ||
| stats={getProviderStats(key, "free")} |
There was a problem hiding this comment.
SUGGESTION: Passing authType="free" for Qoder which has authType="apikey" in DB. The filter logic handles this (line 173), but consider naming consistency.
Code Review SummaryStatus: 3 Warnings Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (17 files)
The PR implements a significant migration for the Qoder provider from OAuth-based authentication to Personal Access Token (PAT) via qodercli transport. This is a well-structured change that:
No critical issues found. The warnings above are minor operational concerns. |
|
Thanks @carmin777 for this great contribution! 🎉 This PR has been integrated into the release/v3.4.5 branch and will be part of the next release. We appreciate your effort! |
…ted base-reds - Reconcile [3.8.33] CHANGELOG to 1:1 commit coverage (51 bullets) + env contract (QUOTA_PREFLIGHT_CUTOFF_ENABLED, KIRO_VERIFY_FULL_CRC) + README What's New range. - fix(translator): dedupe the duplicate input_audio handler in geminiHelper; mp3 normalizes to canonical audio/mpeg and the data: prefix is stripped (#912/#913). - fix(auth): wire admin-configured maxCooldownMs to all 4 markAccountUnavailable model-lockout sites (#4530 follow-up — combo.ts sites were already covered). - fix(api): add src/models/ to package.json files so the published --mcp closure ships it (#3578 gate). - test: align stale expectations to intentional code (10 essential MCP tools incl. web_fetch; busy_timeout 2s cap from v3.8.32). - chore(quality): rebaseline file-size for auth.ts 2279->2289 + db-core-init.test.ts.
…ercli feat(qoder): support PAT via qodercli and remove stale qoder.cn defaults
…ted base-reds - Reconcile [3.8.33] CHANGELOG to 1:1 commit coverage (51 bullets) + env contract (QUOTA_PREFLIGHT_CUTOFF_ENABLED, KIRO_VERIFY_FULL_CRC) + README What's New range. - fix(translator): dedupe the duplicate input_audio handler in geminiHelper; mp3 normalizes to canonical audio/mpeg and the data: prefix is stripped (diegosouzapw#912/diegosouzapw#913). - fix(auth): wire admin-configured maxCooldownMs to all 4 markAccountUnavailable model-lockout sites (diegosouzapw#4530 follow-up — combo.ts sites were already covered). - fix(api): add src/models/ to package.json files so the published --mcp closure ships it (diegosouzapw#3578 gate). - test: align stale expectations to intentional code (10 essential MCP tools incl. web_fetch; busy_timeout 2s cap from v3.8.32). - chore(quality): rebaseline file-size for auth.ts 2279->2289 + db-core-init.test.ts.
…ercli feat(qoder): support PAT via qodercli and remove stale qoder.cn defaults
2 participants
Summary
Details
Testing
Fixes #879