Repository navigation
Support creating model providers with xAI OAuth and support responses API - #61
Conversation
NriotHrreion
changed the title
![proval-0e6d07[bot]](https://avatars.githubusercontent.com/in/4828992?s=60&v=4){kind=small}
| public async listModels(modelProviderId: number): Promise<ModelProviderModelListResponse> { | ||
| const modelProvider = await this.findById(modelProviderId); | ||
|
|
||
| if (modelProvider.authMethod === "xai_oauth") { |
There was a problem hiding this comment.
⚫️ [PROBLEM] xAI credential errors from listModels surface as HTTP 500
⚫️ [PROBLEM] This new branch lets getXaiModelList propagate credential errors — the 401/403 Error("xAI denied model discovery...") from apps/api/src/agent/llm/xai.ts:125 and XaiOAuthError from getAccessToken — but the catch in listModelProviderModels maps only the exact message "Model provider not found" to 404 and returns 500 for everything else (apps/api/src/api/model/model.controller.ts:111-114). An expired OAuth token therefore reports HTTP 500 on routine model discovery, creating false server-error alerts even though the error carries an explicit 4xx status that inputError() honors on the other endpoints. Map XaiOAuthError to error.status (and the permission error to 403) in that catch.
| availableModels = []; | ||
| modelListError = body.error ?? "Model discovery is unavailable. You can enter a model ID manually."; | ||
| } | ||
| } catch { |
There was a problem hiding this comment.
⚫️ [PROBLEM] Discovery failures still fall through to the generic empty-list message
⚫️ [PROBLEM] The new error surfacing only covers the non-ok HTTP branch above (lines 184-188). This catch still swallows network failures without setting modelListError, and an ok response whose body is {models: [], source: "unavailable"} — exactly what apps/api/src/api/model/model.service.ts:177 returns when xAI discovery fails non-authently — also leaves it empty. Both paths fall through to the generic "No models were returned by this provider." at line 723, misreporting a failed discovery as an empty provider list — the silent behavior this change was meant to fix. Set modelListError in the catch and when source === "unavailable".
|
Thanks for this PR — adding xAI OAuth, the OpenAI Responses sender, and centralizing sender construction is a lot of moving parts, and the care shows (especially around credential handling and the end-to-end OAuth test suite). a. Summary This PR introduces xAI OAuth device-flow authentication (service, controller, routes, migration 0046), a new b. Good points
c. Main Issues
d. Additional Notes
|
| const client = new OpenAI({ | ||
| apiKey: "oauth", | ||
| baseURL: xaiBaseUrl, | ||
| timeout: config.timeoutSecond * 1000, |
There was a problem hiding this comment.
⚫️ [PROBLEM] Model-list retries now span up to three timeout windows with no overall deadline
Removing maxRetries: 0 fixes the retry gap, but this client now inherits the SDK default of 2 retries while the only bound is the per-attempt timeout: config.timeoutSecond * 1000 at apps/api/src/agent/llm/xai.ts:112 — and timeoutSecond allows up to 7200 seconds (apps/api/src/api/model/model.service.ts:22). When api.x.ai is degraded (slow 5xx or per-attempt timeouts), client.models.list() at apps/api/src/agent/llm/xai.ts:117 can run up to roughly three full timeout windows plus backoff, so the /model-provider/:id/models request and the discovery UI hang far past the configured timeout — before this push it was a single attempt.
The sender path already solves this by passing AbortSignal.timeout(config.timeoutSecond * 1000) into each responses.create (apps/api/src/agent/llm/openai-responses.ts:16); mirror that so all retry attempts share one deadline:
const signal = AbortSignal.timeout(config.timeoutSecond * 1000);
const page = await client.models.list({}, { signal });
return page.data.map((model) => ({ id: model.id }));
|
Thanks for the follow-up — this push cleanly closes two of the findings from the last round: sender construction is now lazy inside a. Summary This push moves b. Good points
c. Main Issues
d. Additional Notes
|
|
Merge conflicts resolved. |
| const limiter = loadLimiter(modelProviderId, concurrency); | ||
| const fetch = createLLMFetch(); | ||
| switch (config.provider) { | ||
| if (sdkConfig.provider === "xai") return withConcurrencyLimit(createXaiSender(sdkConfig), limiter); |
There was a problem hiding this comment.
⚫️ [PROBLEM] Concurrency queue has no deadline, so timeoutSecond no longer bounds send()
⚫️ [PROBLEM] This PR routes xAI (and openai_responses below) through withConcurrencyLimit, but Limiter.acquire parks callers in a queue with no deadline or abort path (apps/api/src/agent/llm/concurrency.ts:41-49). The comment at factory.ts:31-32 says the SDK deadline owns the timeout — yet that clock only starts once a slot frees, so a request queued behind busy slots can wait far past timeoutSecond (allowed up to 7200s per attempt, multiplied by SDK retries). This is not just the agent loop: the HTTP verify endpoint POST /model-provider/:id/verify (apps/api/src/api/index.ts:55) calls verifySaved → sender.send directly (apps/api/src/api/model/model.service.ts:212) with the shared limiter for that provider id, so a dashboard verification can stall for as long as in-flight activity sends hold the slots. Cancellation has the same gap: the loop checks stopIfCanceled only before calling send (apps/api/src/agent/llm/loop.ts:167), so an activity canceled while still queued will still fire its LLM request — spending tokens — when a slot opens.
Consider bounding the queue wait (e.g. reject if not scheduled within timeoutSecond, or accept an AbortSignal so canceled sends can be dropped while queued) in Limiter.acquire/run.
|
Thanks for the follow-up — this push lands three substantial pieces on top of the earlier xAI/OAuth work: trusted target-branch AGENTS.md guidance, a settle-before-cleanup fix for parallel sub-agents, and a per-provider sender concurrency limit with full config plumbing (DB migration through client form). a. Summary Guidance files are discovered and read from the fetched target-head git objects rather than the PR checkout, then formatted under a strict character budget and threaded through all seven agent flows. The review pipeline now waits for every sub-agent to settle before rethrowing, so b. Good points
c. Main Issues
d. Additional Notes
|
|
Hi @NriotHrreion. It's a big change! It's quite late to review because I heard that there will be a massive changes from xAi but it turned out to be a new monthly plans so we can use existing xAi's OAuth, API without changed from this branch I'll test it in my local with my X subscription(I had none so I should purchase one) and merge it big thanks for you hard work |
|
I looked at your changes and run from my local environment and It seems to work well. Great job I'll merge this branch after adding some additional changes. mostly about structure for code/files and minor UX. 
and this Grok model feels smarter than I thought btw. I should use this model for this repo |
ModelForm to ModelAPIForm and ModelXaiForm
- Keep timeout and concurrency validation in the service zod schemas, including xAI OAuth attempt input - Flatten SenderConfig and SenderSDKConfig, and always build API key fetches with createLLMFetch - Fold XaiConnectionConfig into XaiSenderConfig - Drop the Sec-Fetch-Site check on xAI OAuth routes(already got X-Proval-OAuth) - Parse the raw xAI OAuth body in the service and keep the parsed value as attemptInput - Refresh xAI tokens from expires_in only, and shorten the early refresh window from 120 seconds to 60 seconds - Check xAI verification URLs by https and host only - Update the affected tests
| const original = unwrapConnectionCause(error); | ||
| if (original !== error) throw original; |
There was a problem hiding this comment.
⚫️ [PROBLEM] Network failures in getXaiModelList now surface as HTTP 500 instead of "unavailable"
⚫️ [PROBLEM] The reworked unwrapConnectionCause (apps/api/src/agent/llm/xai.ts:67) unwraps any OpenAI.APIConnectionError whose cause is an Error, and getXaiModelList rethrows that cause (lines 114-115). The previous unwrapXaiError only unwrapped the private XaiCredentialError wrapper, so ordinary connection failures fell through to return null and the caller rendered { models: [], source: "unavailable" }. Now the OpenAI SDK wraps a plain fetch failure (DNS error, connection refused — the raw TypeError) as APIConnectionError with the TypeError as cause, so original !== error holds and the raw error is thrown out of listModels.
Impact: for xai_oauth providers, GET /model-provider/:id/models now returns HTTP 500 with a raw "fetch failed"-style message whenever api.x.ai is unreachable (listModelProviderModels maps everything except "Model provider not found" to 500, apps/api/src/api/model/model.controller.ts:109-114), instead of the graceful empty-list response the sibling non-xAI branch still produces via its catch-all (apps/api/src/api/model/model.service.ts:191-201). This also extends the earlier thread on this endpoint (4226058907) from credential errors to all connection-level failures.
Fix: only rethrow the unwrapped cause when it is a credential/auth error (e.g. instanceof XaiOAuthError, or the "Refusing to send xAI credentials" guard error); otherwise return null so connection failures keep producing the "unavailable" response. Keeping unwrapConnectionCause as-is for createXaiSender.send is fine.
} catch (error) {
const original = unwrapConnectionCause(error);
if (original !== error && original instanceof XaiOAuthError) throw original;
if (error instanceof OpenAI.APIError && [401, 403].includes(error.status ?? 0)) {
throw new Error("xAI denied model discovery. Check the account permission or enter a model ID manually");
}
return null;
}
|
Thanks for the follow-up, @NriotHrreion — the form split (ModelAPIForm/ModelXaiForm), the factory config-type rework, and the Zod consolidation are all in good shape, and the deleted Overviewa. Summary This push completes the API-key/xAI form split: b. Good points
c. Main Issues
d. Additional Notes
Previously raised and still open, unchanged by this push: the concurrency queue has no deadline so |
|
Hi! I did some updates. started with minor changes but it looks big dashboard
API
Mostly about making structure/workflow more cleaner and more visible. Didn't want to deny your code changes but hope you understand. just wanted to maintain clean codebase to keep the project developed easily over time. Still it's really great PR. |
|
@seoes I think the original auth type selector modal is lighter than a page. Could you tell me your thoughts on this change? |
|
Oh yes. Modals are lighter and require less code. But when I tried to use dashboard, a modal window at that moment didn't feel quite natural (maybe it's just for me). and Although it creates an additional file but the It's not the right answer but we can discuss about it |
|
@proval-0e6d07 pros and cons for each modal / separate page on this workflow? (selecting model provider auth type) |
|
@seoes Put the select page in Or change the grid view to a list view? |
|
Good question to kick off a design discussion — here's how I see it for the current Separate page (current approach)Pros
Cons
Modal approachPros
Cons
My takeThe current separate-page approach is the right fit for the OAuth part specifically: device-code flows have multi-step, long-running state (code display → external authorize → backend polling → verify), and pages survive refresh/back-navigation far better. If the concern is friction, the cheap win is removing the "Continue" button and letting clicking a card navigate directly, which keeps the page-based routing benefits while matching a modal's speed. A modal would be better suited to the short, single-step API-key form if you wanted to optimize that path later. |
|
I saw the page again after you said, it really is 🤣 Good point. Maybe it should be go back with your modal design. consider it after Proval got several auth types |
|
Hi @NriotHrreion — another tidy follow-up. This push closes out the last open thread from the previous review and lands the modal rework you and @seoes settled on in the discussion. Overviewa. Summary The key behavioral fix is in On the client side, the heavyweight b. Good points
Main IssuesNone for this push — the changes both compile cleanly against the surrounding flow and the prior findings they address are resolved. Additional Notes
Overall this looks like a clean close of the round — nothing blocking, and the two noted items are judgment calls rather than defects. |
|
I merged this branch version |
2 participants
refs #9
close #40
Integrate OpenAI OAuthWhat changed
OpenAI Responses API
apps/api/src/agent/llm/openai-responses.tsand registered it as a new LLM sender.ModelFormxAI OAuth
Auth process:
Changes:
authMethod,oauthCredentialandoauthStatuscolumns tomodel_providertable.apps/api/src/api/model/xai-oauth.service.tsto process xAI OAuth requests.apps/api/src/api/model/model.service.ts, addedcreateModelProviderSender()to replacecreateSender().createModelProviderSender()consumes the provider info read from database, converting the provider info into a valid config and passing the config intocreateSender(). This can simplify the provider sender creation process, especially when the xAI provider is going to be added.verifyConfig()method.verifyConfig()is for verifying provider configurations not stored in the database, whileverifySaved()is for verifying providers read from the database.timeoutSecondandmaxOutputTokenconfig option toverifyConfig()method.UI