Add first-class OpenAI-compatible LLM provider - #1505
Conversation
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
Chris0Jeky
added
backend
llm
revival
Security/configuration adversarial review — exact head
|
Correctness / protocol adversarial review — exact head
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf4549a7ba
ℹ️ 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".
Summary
Testing
Notable Risks / Follow-ups
|
Summary
Files Touched
Testing
Risks and Follow-ups
|
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
Combined review-fix evidence — exact head
|
…patible Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ee17a4849
ℹ️ 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".
| if (parsed.Error is not null) | ||
| { | ||
| yield return TerminalError(parsed.Error); | ||
| yield break; |
There was a problem hiding this comment.
When a compatible endpoint accepts the request but the first SSE event is malformed, the response exceeds a limit before any delta, or the body read fails early, this emits only an error terminal. ChatService.StreamResponseAsync treats an error-only stream as never started and releases its reservation, even though the upstream request was dispatched and may have been billed, allowing repeated failed calls to bypass Taskdeck's quota. Propagate explicit pre/post-dispatch billing metadata and commit the estimate for post-dispatch failures.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid finding, accepted as changes requested and tracked as item 1 in the #1306 parked-head acceptance remainder: #1306 (comment) . PR #1505 is parked under the two-round ceiling, so this thread intentionally remains unresolved until a future scoped implementation supplies a tested fix.
| if (((ICollection<KeyValuePair<string, ProviderFailureState>>)_providerFailures).Remove( | ||
| new KeyValuePair<string, ProviderFailureState>(circuitName, existing))) | ||
| { | ||
| RecordState(circuitName, CircuitState.Closed); |
There was a problem hiding this comment.
The companion body/protocol breaker writes Closed into the same public snapshot used by Polly without checking Polly's actual state. If one concurrent request opens Polly on a 5xx while an already-in-flight request later succeeds, this success removes only the companion state and overwrites the snapshot with Closed, although Polly continues rejecting calls until its break expires; the health endpoint therefore reports a false closed state. Track the two gates separately or publish Closed only when both are closed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid finding, accepted as changes requested and tracked as item 4 in the #1306 parked-head acceptance remainder: #1306 (comment) . PR #1505 is parked under the two-round ceiling, so this thread intentionally remains unresolved until a future scoped implementation supplies a tested fix.
| bytes = checked(bytes + Encoding.UTF8.GetByteCount([next.Value])); | ||
| if (bytes > maxBytes) | ||
| throw new LlmProviderResponseLimitException("single SSE line byte budget exceeded"); |
There was a problem hiding this comment.
For non-BMP text such as emoji, counting each UTF-16 code unit independently encodes the two surrogate halves as replacement sequences, reporting 6 bytes instead of the actual 4-byte UTF-8 scalar. An emoji-heavy SSE line or response can therefore be rejected as over budget even while its wire bytes remain within the configured limits; count complete surrogate pairs/Unicode scalars or enforce the limit on raw bytes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid finding, accepted as changes requested and tracked as item 6 in the #1306 parked-head acceptance remainder: #1306 (comment) . PR #1505 is parked under the two-round ceiling, so this thread intentionally remains unresolved until a future scoped implementation supplies a tested fix.
| - `docs/GOLDEN_PRINCIPLES.md` | ||
|
|
||
| ## Current Verified Totals (2026-05-16) | ||
| ## Last Cross-Stack Verified Baseline (2026-05-16) |
There was a problem hiding this comment.
Renaming this heading leaves docs/STATUS.md:1151 directing readers to the former “Current Verified Totals” section and docs/LIVING_DOCUMENTS_GUIDE.md:100 still naming that section as the testing guide's high-churn contract. Preserve the established heading or update both canonical references so the documented navigation remains valid.
AGENTS.md reference: AGENTS.md:L116-L118
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid finding, accepted as changes requested and tracked as item 13 in the #1306 parked-head acceptance remainder: #1306 (comment) . PR #1505 is parked under the two-round ceiling, so this thread intentionally remains unresolved until a future scoped implementation supplies a tested fix.
| Last Updated: 2026-07-27 | ||
|
|
||
| REVIVAL-10 OpenAI-compatible provider slice (2026-07-27, `#1306`): | ||
| - **The backend has a first-class, opt-in `OpenAICompatible` provider for OpenAI Chat Completions-compatible endpoints, with the review-found transport and persistence gaps closed locally.** Production/non-development selection requires an API key, model, positive budgets, and a public HTTPS base URL; gated `http://localhost` remains development-only. The compatible client alone disables ambient proxies, refuses every redirect without following it, retains URL plus connection-time DNS/IP SSRF checks, and ensures egress-envelope violations and logs record only sanitized origins. Response bodies, SSE lines/events, and full response time are bounded; LF, CRLF, and CR framing are accepted. When authoritative usage is absent after upstream dispatch, quota settlement conservatively commits the original reservation estimate. Transport/protocol/timeout/limit failures count against the shared buffered/streaming circuit, ordinary degraded finish reasons do not, and cancellation or early disposal releases a half-open probe. Empty degraded terminals persist as sanitized reloadable assistant history, while normal SSE wire events omit false/null degradation fields. The concrete transport is internal and production construction is confined to reviewed API registration. |
There was a problem hiding this comment.
This new current-state entry says OpenAICompatible is supported, while the same source-of-truth document's active constraint at docs/STATUS.md:230 still says the LLM flow supports only OpenAI and Gemini and attributes extraction/context behavior only to those providers. Update the older inventory so operators do not receive contradictory supported-provider guidance from STATUS.
AGENTS.md reference: AGENTS.md:L8-L8
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid finding, accepted as changes requested and tracked as item 14 in the #1306 parked-head acceptance remainder: #1306 (comment) . PR #1505 is parked under the two-round ceiling, so this thread intentionally remains unresolved until a future scoped implementation supplies a tested fix.
| "Live provider response body failed.", | ||
| LlmProviderFailureKind.ResponseBody); | ||
| } | ||
| catch (Exception ex) |
There was a problem hiding this comment.
This broad catch also captures EgressViolationException raised when the compatible pipeline blocks a redirect or unregistered host and converts it into an ordinary degraded completion. Although the handler still blocks and logs the request, callers lose the typed security-boundary signal used for dedicated persistence and telemetry handling elsewhere, contrary to the exception's explicit never-swallow contract at EgressEnvelopeHandler.cs:395. Rethrow this exception before the generic fallback catch and cover both buffered and streaming paths.
AGENTS.md reference: AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid finding, accepted as changes requested and tracked as item 8 in the #1306 parked-head acceptance remainder: #1306 (comment) . PR #1505 is parked under the two-round ceiling, so this thread intentionally remains unresolved until a future scoped implementation supplies a tested fix.
| the same URL and DNS-level SSRF checks as OpenAI. Plain HTTP is accepted only | ||
| for loopback development endpoints when both the development environment and | ||
| `AllowLiveProvidersInDevelopment` gate permit it. Keep keys in a secret store; |
There was a problem hiding this comment.
This says plain HTTP works for loopback development endpoints, but ValidateLlmProviderUrl permits non-HTTPS only when the host is literally localhost, and compatible-provider validation rejects numeric IP hosts. Consequently http://127.0.0.1/...—the loopback form explicitly advertised earlier in this guide for LM Studio and LocalAI—selects Mock even with both development gates enabled. Narrow the claim to the localhost hostname or make validation accept loopback IPs consistently.
AGENTS.md reference: AGENTS.md:L116-L118
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid finding, accepted as changes requested and tracked as item 12 in the #1306 parked-head acceptance remainder: #1306 (comment) . PR #1505 is parked under the two-round ceiling, so this thread intentionally remains unresolved until a future scoped implementation supplies a tested fix.
| using var reader = new StreamReader( | ||
| stream, | ||
| new UTF8Encoding(encoderShouldEmitUTF8Identifier: false, throwOnInvalidBytes: true), | ||
| detectEncodingFromByteOrderMarks: true, | ||
| bufferSize: 4096, | ||
| leaveOpen: false); |
There was a problem hiding this comment.
Enabling BOM-based encoding detection lets a UTF-16 or UTF-32 BOM replace the supplied strict UTF-8 decoder. The parser then enforces line and aggregate limits against decoded text re-encoded as UTF-8 rather than bytes actually received, so an ASCII JSON stream encoded as UTF-16 can consume roughly twice MaxResponseBytes on the wire while remaining below the counter. Disable encoding auto-detection, optionally handle only a UTF-8 BOM, or enforce limits on the raw response stream.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid finding, accepted as changes requested and tracked as item 6 in the #1306 parked-head acceptance remainder: #1306 (comment) . PR #1505 is parked under the two-round ceiling, so this thread intentionally remains unresolved until a future scoped implementation supplies a tested fix.
| if (!choice.TryGetProperty("message", out var message) || | ||
| message.ValueKind != JsonValueKind.Object || | ||
| !message.TryGetProperty("content", out var contentElement) || | ||
| contentElement.ValueKind != JsonValueKind.String) | ||
| return false; |
There was a problem hiding this comment.
Buffered parsing requires message.content to be a string before it examines finish_reason. A content-filter or refusal completion with content: null and finish_reason: "content_filter" is therefore recorded as a protocol failure, making the explicit content-filter degradation handling unreachable and allowing repeated user safety refusals to open the provider circuit. Parse the finish reason/refusal shape first and surface a sanitized degraded completion without counting it as provider failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid finding, accepted as changes requested and tracked as item 9 in the #1306 parked-head acceptance remainder: #1306 (comment) . PR #1505 is parked under the two-round ceiling, so this thread intentionally remains unresolved until a future scoped implementation supplies a tested fix.
| private static bool IsStreamingRejection(HttpStatusCode statusCode) => | ||
| statusCode is HttpStatusCode.BadRequest or HttpStatusCode.NotFound or HttpStatusCode.MethodNotAllowed or | ||
| HttpStatusCode.NotImplemented or HttpStatusCode.UnprocessableEntity; |
There was a problem hiding this comment.
The provider classifies HTTP 501 as a streaming-rejection signal and intends to retry without SSE, but the production named client wraps requests in HandleTransientHttpError, which treats every 5xx as a circuit failure. With the valid CircuitBreaker:FailureThreshold=1, the first 501 opens Polly before the fallback executes, so the second request through the same client is rejected instead of producing the documented buffered completion. Exclude this recognized compatibility response from the compatible breaker or route the fallback through a path unaffected by that newly opened circuit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid finding, accepted as changes requested and tracked as item 10 in the #1306 parked-head acceptance remainder: #1306 (comment) . PR #1505 is parked under the two-round ceiling, so this thread intentionally remains unresolved until a future scoped implementation supplies a tested fix.
|
CI recovery checkpoint for exact head
The PR remains blocked: red CI has not yet cleared, fresh final reviews have additional unresolved correctness/security findings, and the maintainer live-provider/UI-stream canary is still outstanding. No merge conclusion is being drawn from GitHub's lenient protection. |
Fresh adversarial security / egress / deployment reviewReviewed exact head Disposition: changes required; do not merge this head. HIGH
MEDIUM
All 11 current unresolved inline threads were inspected; this comment does not repeat the remaining correctness/docs findings, which still require one-pass triage under the repository review policy. EvidenceFocused read-only test runs: The worktree remained clean at the exact head. Hosted checks are not green: 36 current check runs = 23 success, 12 skipped, 1 failure. Windows Verified controls and residualsStatic/current-head inspection did verify that the compatible client itself disables proxies and redirects, uses the egress envelope plus DNS-level connection callback, validates its URL/API key/headers, applies response deadlines and bounded parsing, and uses redacted exception summaries. Hosted Gitleaks and CodeQL are green. NOT verified: a maintainer-key live canary against OpenRouter/Groq/DeepSeek; visible incremental streaming through the real chat endpoint/UI; a system-proxy canary; an end-to-end request through the untouched production named-handler chain (the redirect test replaces |
Fresh adversarial correctness / state-machine review — exact head
|
Parked after bounded final review — exact head
|
1 participant
Status
CHANGES REQUESTED / PARKED at exact head
6ee17a4849427d679722a9d1e034d4622536f4fe. Do not merge or start a third fix/review round on this head.Final review found 15 valid unresolved items: 14 are recorded in #1306's parked-head acceptance remainder, and the cross-provider ambient-proxy/SSRF boundary is tracked in #1513. The Windows concurrent-card CI recurrence is tracked in #1512. All 11 open inline threads have been triaged and intentionally remain unresolved until a future scoped implementation supplies fixes.
Summary
OpenAICompatibleprovider without changing OpenAI defaultsProduct alignment
This is the bounded REVIVAL-10 provider slice: it lowers transcript-triage integration friction while preserving Taskdeck's review-first trust boundary. It does not add direct mutation authority. The final review showed that quota, circuit, raw-byte, fallback, persistence, docs, and shared proxy enforcement still need another scoped implementation before this can ship.
Verification — parked head
NOT verified
Refs #1306, #1512, and #1513.