Skip to content

Add Qoder provider support and docs#2366

Open
PKFireBarry wants to merge 1 commit intorouter-for-me:mainfrom
PKFireBarry:qoder-provider
Open

Add Qoder provider support and docs#2366
PKFireBarry wants to merge 1 commit intorouter-for-me:mainfrom
PKFireBarry:qoder-provider

Conversation

@PKFireBarry
Copy link
Copy Markdown

  • Add Qoder provider support across auth, registry, watcher, and executor wiring
  • Expose Qoder OAuth device-flow login via CLI flag and management endpoint
  • Add Qoder model catalog entries and config examples
  • Update docs (EN/CN/JA) and note management UI asset refresh

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request adds full support for the Qoder AI provider, implementing OAuth2 device flow, COSY encryption, and a specialized request executor. The changes span the CLI, management API, and model registry. Review feedback identifies critical security concerns regarding weak hashing and IV selection, as well as logic issues in token persistence and metadata unmarshalling. Recommendations also include consolidating duplicate authentication logic and improving user-facing labels in the management dashboard.

// UpdateCredentials updates the API credentials
func (api *QoderAPI) UpdateCredentials(token, userID, name, email string) {
api.token = token
api.userID = userID
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Using the AES key as the Initialization Vector (IV) for CBC mode is generally not recommended. IVs should be unpredictable and unique for each encryption to prevent certain cryptographic attacks. While this might be done to match an existing JavaScript implementation, it's a security concern that should be addressed if possible, perhaps by generating a random IV and prepending it to the ciphertext.

if data == "[DONE]" {
return
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

MD5 is considered cryptographically weak for new applications, especially for signatures where collision resistance is important. It's recommended to use a stronger hashing algorithm like SHA-256 for security purposes.

}

auth.UpdateTokenStorage(storage, tokenData)
return storage.SaveTokenToFile("")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Passing an empty string to storage.SaveTokenToFile means the file will be saved to the current working directory. This is likely not the intended behavior for persistent auth files and can lead to tokens being saved in an unexpected or insecure location. The authFilePath should be explicitly provided, typically derived from the configured auth directory and a unique filename.

Comment on lines +173 to +179
var storage qoderauth.QoderTokenStorage
if raw, errMarshal := json.Marshal(metadata); errMarshal == nil {
if errUnmarshal := json.Unmarshal(raw, &storage); errUnmarshal == nil {
if strings.TrimSpace(storage.Type) == "" {
storage.Type = "qoder"
}
a.Storage = &storage
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The code attempts to unmarshal the entire metadata map directly into a qoderauth.QoderTokenStorage struct. The metadata map contains fields (like email, project_id, priority, note) that are not part of QoderTokenStorage. This will result in an incomplete or incorrect QoderTokenStorage object, as only fields matching its struct tags will be populated. It's better to explicitly extract and assign relevant fields from metadata to construct the QoderTokenStorage object, or ensure metadata strictly adheres to the QoderTokenStorage structure if that's the intention.

Comment on lines +259 to +267
var storage qoderauth.QoderTokenStorage
if raw, errMarshal := json.Marshal(metadata); errMarshal == nil {
if errUnmarshal := json.Unmarshal(raw, &storage); errUnmarshal == nil {
if strings.TrimSpace(storage.Type) == "" {
storage.Type = "qoder"
}
auth.Storage = &storage
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Similar to internal/watcher/synthesizer/file.go, this code attempts to unmarshal the entire metadata map directly into a qoderauth.QoderTokenStorage struct. This will likely result in an incomplete or incorrect QoderTokenStorage object, as only fields matching its struct tags will be populated. It's better to explicitly extract and assign relevant fields from metadata to construct the QoderTokenStorage object, or ensure metadata strictly adheres to the QoderTokenStorage structure if that's the intention.

ID: fileName,
Provider: "qoder",
FileName: fileName,
Label: "Qoder User",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The label "Qoder User" is generic. If storage.Name or storage.Email is available, it would be more descriptive and user-friendly to use that for the label.

Comment on lines +107 to +113
// generateCodeVerifier generates a cryptographically random string for PKCE
func generateCodeVerifier() (string, error) {
bytes := make([]byte, 32)
if _, err := rand.Read(bytes); err != nil {
return "", err
}
return base64.RawURLEncoding.EncodeToString(bytes), nil
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The generateCodeVerifier function is a duplicate of generateDeviceCodeVerifier found in internal/auth/qoder/cosy.go. Consider consolidating these into a single shared utility function to reduce code duplication and improve maintainability.

Comment on lines +116 to +119
// generateCodeChallenge creates a SHA-256 hash of the code verifier
func generateCodeChallenge(codeVerifier string) string {
hash := sha256.Sum256([]byte(codeVerifier))
return base64.RawURLEncoding.EncodeToString(hash[:])
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The generateCodeChallenge function is a duplicate of generateDeviceCodeChallenge found in internal/auth/qoder/cosy.go. Consider consolidating these into a single shared utility function to reduce code duplication and improve maintainability.

}

scanner := bufio.NewScanner(httpResp.Body)
scanner.Buffer(nil, 52_428_800) // 50MB max line
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Allocating a 50MB buffer for the bufio.Scanner is a very large memory footprint. While it might be necessary for extremely large SSE events, it's worth evaluating if this is always needed or if a more dynamic approach could be used to conserve memory, especially if typical SSE events are much smaller.

Comment on lines +83 to +86
// Create token storage
tokenStorage := authSvc.CreateTokenStorage(tokenData, deviceFlow.MachineID)
if tokenData.UserID != "" {
_, _ = authSvc.SaveUserInfo(ctx, tokenData.AccessToken, tokenData.UserID, name, email)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The authSvc.SaveUserInfo call updates name and email within the QoderAuth service instance, but these updated values are not explicitly propagated back to the tokenStorage object before it's used to set tokenStorage.Email and tokenStorage.Name later. This could lead to tokenStorage having stale name and email if FetchUserInfo was successful in updating them.

Copy link
Copy Markdown

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

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: 16ae758534

ℹ️ 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".

}

auth.UpdateTokenStorage(storage, tokenData)
return storage.SaveTokenToFile("")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Persist Qoder refresh tokens using a real auth file path

doRefreshToken always calls SaveTokenToFile(""), so any successful refresh still returns a file-write error and never updates the on-disk credential record. Because ExecuteStream only logs this error, runtime requests may continue with in-memory tokens while the saved refresh token stays stale, which can break authentication after a restart when refresh tokens rotate.

Useful? React with 👍 / 👎.

continue
}
out <- cliproxyexecutor.StreamChunk{Payload: chunkBytes}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Surface scanner errors before ending Qoder SSE streams

The stream loop exits when scanner.Scan() returns false, but there is no scanner.Err() check after the loop. In cases like mid-stream connection resets or scanner token-size failures, the code closes the stream as if it completed normally, which can silently truncate model output and return partial results without an error chunk.

Useful? React with 👍 / 👎.

}

// Read request body for COSY signing
bodyBytes, err := io.ReadAll(req.Body)
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 Handle nil request bodies before COSY signing

HttpRequest unconditionally reads req.Body, but requests created without a body (for example GET/HEAD) can have Body == nil. Calling io.ReadAll on a nil body panics, so generic HTTP execution against qoder can crash unless every caller provides an explicit empty reader.

Useful? React with 👍 / 👎.

Comment on lines +33 to +35
httpClient: &http.Client{
Timeout: 180 * time.Second,
},
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 Route Qoder HTTP calls through proxy-aware client

The executor constructs a fixed http.Client once and reuses it for all qoder calls, bypassing the repository’s proxy-aware client path used by other providers. This ignores configured cfg.ProxyURL/per-auth proxy settings, so qoder requests can fail in environments that require outbound traffic through the configured proxy.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator

@luispater luispater left a comment

Choose a reason for hiding this comment

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

Summary:
This PR adds broad Qoder support across auth, management routes, model registry, and executor wiring. The integration shape is good, but there are two correctness/reliability issues that should be fixed before merge.

Blocking issues:

  1. internal/runtime/executor/qoder_executor.go: http.Client{Timeout: 180 * time.Second} in the Qoder executor introduces hard cutoff risk for long streaming sessions. This can break valid responses and conflicts with existing timeout policy for established upstream streams.
  2. internal/auth/qoder/api.go: refresh persistence uses storage.SaveTokenToFile(""), which does not provide a writable auth file path. This will fail file creation and causes token refresh persistence to fail.

Suggested tests:

  • Auth refresh regression test: expired/nearly-expired Qoder token should refresh and persist successfully.
  • Streaming executor test: long-running SSE stream should not be aborted by client timeout.
  • Tool-calls path test: verify tool call chunks are accumulated and returned in non-stream response mode.

Decision:
Request changes due to the two blocking reliability bugs above.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants