feat: add GitHub Copilot App support via OpenAI-compatible chat completions - #279
Conversation
Expose POST /v1/chat/completions so GitHub Copilot App and other OpenAI-compatible clients can use OpenCodex as a Model provider via GET /v1/models + chat completions, reusing the existing Responses path.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12fb3107cd
ℹ️ 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".
| // Prefer item_id matching when present; otherwise use the last open tool index. | ||
| let toolIndex = nextToolIndex > 0 ? nextToolIndex - 1 : 0; | ||
| const itemId = typeof data.item_id === "string" ? data.item_id : undefined; | ||
| if (itemId) { | ||
| // Some bridges put call_id on the item, not item_id; keep last index. | ||
| } |
There was a problem hiding this comment.
When a Responses stream opens more than one function_call before argument deltas arrive, such as parallel tool calls from the native Responses passthrough path, this branch ignores data.item_id and always emits every argument chunk for nextToolIndex - 1. That attaches earlier tools' arguments to the last opened tool in the Chat Completions stream, so the client can execute the wrong tool call with empty or corrupted arguments. Keep a map from Responses item.id/item_id to the Chat tool index and use it for these deltas.
Useful? React with 👍 / 👎.
| const upstream = await handleResponses(internalReq, config, logCtx, { | ||
| abortSignal: req.signal, | ||
| }); |
There was a problem hiding this comment.
When the selected model routes to the native openai-responses passthrough path, handleResponses returns a native passthrough SSE response that responseWithDeferredRequestLog deliberately does not wrap; the existing /v1/responses path compensates by passing native terminal/cancel callbacks into handleResponses. This new endpoint only passes abortSignal, so Copilot/App chat requests against bare native gpt-* models stream successfully but never finalize in the request log/usage view. Pass the same native passthrough terminal and cancel logging hooks here before wrapping the response.
Useful? React with 👍 / 👎.
| if (route.provider.adapter === "openai-responses") { | ||
| _nativeRoute = true; | ||
| // ChatGPT backend rejects store:true and unsupported sampling knobs. | ||
| internalBody.store = false; | ||
| delete internalBody.max_output_tokens; | ||
| delete internalBody.temperature; | ||
| delete internalBody.top_p; | ||
| delete internalBody.stop; | ||
| delete internalBody.user; |
There was a problem hiding this comment.
This branch runs for every openai-responses provider, including the openai-apikey OpenAI API route, not just the ChatGPT forward backend mentioned in the comment. In that scenario a Chat Completions caller's max_tokens, temperature, top_p, stop, and user fields are removed before the platform /v1/responses request, so clients cannot cap output length or apply sampling/stop controls and may get unexpectedly long or differently sampled completions. Gate this scrubbing to the ChatGPT forward provider and leave supported API-key parameters intact.
Useful? React with 👍 / 👎.
|
|
||
| const headers = new Headers({ "content-type": "application/json" }); | ||
| for (const name of FORWARD_HEADERS) { | ||
| if (name === "authorization") continue; |
There was a problem hiding this comment.
When the OpenAI provider is configured in Direct mode, handleResponses requires the caller's ChatGPT Authorization bearer and forwards that identity upstream. This new Chat Completions replay path explicitly drops the caller authorization header before constructing the internal /v1/responses request, and may substitute the main login token instead, so direct-mode clients either get a 401 when no main token exists or silently run as the wrong account. Preserve the caller bearer for direct-mode native routes while still keeping proxy admission secrets out of upstream requests.
Useful? React with 👍 / 👎.
| const text = responseFormatToText(raw.response_format); | ||
| if (text) body.text = text; |
There was a problem hiding this comment.
When a Chat Completions client requests JSON mode or a JSON schema against a non-native routed chat provider, this stores the request as Responses text.format, but the parser only reduces that to a boolean _structuredOutput and the openai-chat adapter never serializes it back to response_format. The model call therefore runs unconstrained even though the client asked for JSON/schema output, which can break clients that parse the reply as structured data. Carry the format/schema through to the routed chat adapter or reject unsupported formats instead of silently dropping them.
Useful? React with 👍 / 👎.
|
Thanks — this is substantial work and the endpoint itself is wanted (it's effectively the general
Tests to add alongside: parallel tool deltas, direct-mode identity, structured output (or its 400), and native terminal/cancel logging. Also please document the supported-field subset (penalties, |
Ingwannu
left a comment
There was a problem hiding this comment.
This integration is valuable and the auth/destination/secret posture is generally consistent with the existing data-plane, but two correctness blockers must be fixed before merge.
-
response.function_call_arguments.deltacarriesitem_id, but the new bridge ignores it and appends every delta to the last opened tool index. Interleaved parallel calls therefore corrupt arguments: in my two-call repro, call 1 ended empty and call 2 received concatenated malformed JSON.response.output_item.donecan also drop final arguments after an added frame. Track tool calls by item id/call id and emit each delta/final snapshot to the matching index. -
response.failedand truncated internal streams are converted into a normal chat-completion assistant delta containing[error] ...followed by[DONE]; non-streaming collection then returns HTTP 200 with model-authored-looking error text. Preserve typed failure/truncation state and return an OpenAI-compatible error/non-success response instead of success content.
Please add adversarial coverage for two interleaved function calls, done-frame final arguments, streaming failure, non-streaming failure, and truncated internal streams. The current full suite/CI passes because those cases are not covered. No malware, backdoor, credential persistence, unsafe process/filesystem behavior, or dependency concern was found.
Route parallel tool argument deltas by Responses item_id, preserve direct-mode caller Authorization with Responses-style admission, reject unsupported routed response_format, and finalize native passthrough request logs. Document the verified Chat Completions field subset. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…ion-review [WRONG BRANCH] fix(chat-completions): address Copilot App review blockers
|
Thanks for you guys review. I will update it when im home. |
|
Re-reviewed head
Full CI is green on the new head. Two remaining blockers before merge:
Nice-to-have (non-blocking): a client-cancel logging test, and a remote-mode test covering |
Owner review blockers on lidge-jun#279: - Failures no longer masquerade as HTTP 200/[DONE] success completions. Stream emits an OpenAI-style error frame and closes without [DONE]; non-stream returns an error status + error body. - output_item.done final arguments are last-write-wins when a tool call was already registered from output_item.added. - Regression tests for failure, truncation, and done-frame snapshots.
|
@lidge-jun Updated on head 1. Failures no longer masquerade as HTTP 200 / clean
|
…tGPT clients GitHub Copilot App / replace-style clients overwrite the whole function object on each tool_calls delta. Emitting arguments-only deltas after output_item.added dropped function.name, so the next turn 400'd with: tool_calls entries require function.name - Always re-emit function.name on args deltas and done-frame snapshots - Recover missing names from earlier same-call_id function_calls on inbound - Regression tests for named deltas + call_id name recovery
|
Follow-up fix on head for ChatGPT / Copilot App tool-call history: Bug: Cause: after Fix:
|
Narrow toolIndex after Map.get so done-frame name maps accept number, and avoid ReadableStreamReadResult assignability mismatch in collectChatCompletion.
|
CI typecheck fix pushed on head:
Local: |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Ingwannu
left a comment
There was a problem hiding this comment.
Re-reviewed the final head after all requested fixes. Parallel tool deltas now route by item id, done-frame arguments are reconciled, direct-mode identity/admission semantics match Responses, routed structured output fails explicitly, native request logs finalize, and failed/truncated internal streams no longer become successful assistant text. I also removed the trailing diff-check whitespace with the contributor branch’s maintainer-edit permission. The five-PR current-dev integration stack passed 3,667 tests / 0 failures, and this final head passed Linux, macOS, Windows, npm-global, typecheck, GUI, and CodeRabbit checks. The lone label failure is a fork-token 403 while applying a label, not a code failure. Approved.
…(local stack, review doc 070)
…chat-completions SSE via shared decoder (Sol audit blockers 1+3)
…omptly releases idle upstream (Sol re-verify blocker)
…audited fixups), 3651 tests green
…idge-jun#307/lidge-jun#309/lidge-jun#279/lidge-jun#303/lidge-jun#318/lidge-jun#319 + v2.7.34 joined with local lidge-jun#304 merge and lidge-jun#279 fixups (models-auth revert, abortable SSE decoder)
Extend the API Access page and /api/keys metadata for issue #357 follow-up work. PR #279 already provides /v1/chat/completions on dev; this slice focuses on dashboard guidance, endpoint metadata, model catalog visibility, and copy/test UX. - add buildApiAccessEndpoints() and richer /api/keys response - expand API Access UI with base URL, protocol endpoints, auth notes, and examples - show externally callable models with copy/test actions - add regression test and i18n coverage
Summary
POST /v1/chat/completionsso GitHub Copilot App can use OpenCodex as a custom Model provider.docs/github-copilot-app.md) and cover discovery/chat endpoint behavior with tests.This is a client integration for GitHub Copilot App (BYOK / Model providers). It is separate from the existing experimental upstream
github-copilotprovider.Replaces #278 (opened against
mainby mistake).How to use
ocx start)http://127.0.0.1:10100/v1provider/model)Test plan
bun test tests/chat-completions-endpoint.test.tsbun test tests/server-auth.test.ts -t "root fallback"GET /v1/modelsreturns OpenAI list shapePOST /v1/chat/completionsstream + non-stream succeed for configured providers/v1and selecting an authenticated model