fix(custom): wire = responses|anthropic for openai-compatible + opencode-zen muse-spark (rescue of #5716) - #5719
fix(custom): wire = responses|anthropic for openai-compatible + opencode-zen muse-spark (rescue of #5716)#5719Hmbown wants to merge 9 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Claude finished @Hmbown's task in 2m 29s —— View job Claude's review of #5719
SummaryThe core fix is sound: Two things worth a look, neither blocking: 1. Duplicated wire-alias logic (reuse/simplification) 2. No test coverage for the new routing behavior (tests) Minor, non-blocking: Good calls: scoping the muse-spark fallback to |
|
Claude finished @Hmbown's task in 3m 5s —— View job Claude's review of #5719 (re-review after synchronize)
SummaryThe new commits since the last review ( Correctness: Route/provider safety (v0.8.65): the Still open from the earlier review pass (unaddressed by the synchronize commits), posted as inline comments:
Good calls, unchanged from before: scoping the muse-spark fallback to Note on verification: this sandboxed session's |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| ) | ||
| } | ||
|
|
||
| fn wire_config_prefers_responses(wire: Option<&str>) -> bool { |
There was a problem hiding this comment.
Reuse/simplification (still open from the previous review pass): wire_config_prefers_responses is defined identically here and in crates/tui/src/client.rs:1788 (both files are in the same codewhale-tui crate, so there's no crate-boundary reason for the split). This mirrors the pre-existing duplication of wire_config_prefers_anthropic between the same two files. provider_wire_dialect() (added in this PR) now gives a single reader for the raw wire string, but the alias-matching tables themselves can still drift independently between the two copies — e.g. adding "response-api" in one and not the other would silently produce different behavior for the client vs. the capability report. Consider making config.rs's copies the single definition (pub(crate)) and having client.rs call them.
| // even when no exact bundled offering exists — fail open to | ||
| // responses rather than failing closed to "unproven". | ||
| if provider_kind == ProviderKind::OpencodeZen | ||
| && raw.to_ascii_lowercase().contains("muse-spark") |
There was a problem hiding this comment.
Tests: this substring fallback (raw.to_ascii_lowercase().contains("muse-spark"), scoped correctly to ProviderKind::OpencodeZen) and wire_config_prefers_responses's alias table are new protocol-routing logic with no accompanying unit test. resolver.rs already has a #[cfg(test)] module (line 637) — a cheap addition there asserting an unlisted muse-spark-* variant on OpencodeZen resolves to endpoint_key == "responses" (and that the same raw string on a different ProviderKind does not fall into this branch) would guard the fallback against silent regressions, e.g. if the match condition is ever loosened to another provider.
| * api_key_env = "OPENCODE_ZEN_API_KEY" | ||
| * # proxy speaks chat to CodeWhale, responses to upstream | ||
| * | ||
| * Prefer the native fix (no proxy needed): |
There was a problem hiding this comment.
Minor, non-blocking: this script's own header says "Prefer the native fix (no proxy needed)" — and the native fix (the wire/muse-spark routing in this same PR) now ships. From the moment this merges, the proxy is a dead workaround for a bug that no longer exists, which cuts against the repo guidance against parallel/bridging paths for something now handled natively. Worth confirming with a maintainer whether this file should be dropped rather than added.
…nd="openai-compatible" Custom provider was fixed to ChatCompletions, ignoring providers.<name>.wire. Now honors per-config wire in both client::provider_wire_format_for_config and config::provider_capability, keeping Custom::wire_policy default as Chat for compat. Aliases: responses/openai-responses/responses-api -> Responses; anthropic/messages/claude -> AnthropicMessages; default -> Chat. Fixes custom muse-spark-1.2 on opencode.ai/zen/v1 needing Responses.
Muse Spark 1.2 contributor-free on https://opencode.ai/zen/v1 only supports POST /v1/responses (Responses API) and rejects Chat Completions. Previously the bundled offering roster and ModelAware resolver treated unknown muse-spark variants as chat or failed closed to unproven, so CodeWhale sent chat payloads that 404. - Add muse-spark-1.2, -contributor, -contributor-free to OPENCODE_ZEN_RESPONSES_MODELS (bundled_offerings) - Add resolver fallback: any muse-spark* under OpencodeZen resolves to endpoint_key responses even without exact catalog match - Update config.example.toml docs (GPT/Muse Spark -> Responses) and add muse-spark-1.2-contributor-free example - Add scripts/opencode-chat2responses-proxy.mjs as zero-Rust chat->responses shim for chat-only clients Custom gateways can already use wire="responses" (ff50458); this fix makes the first-class opencode-zen provider work without hand-written wire config.
PR #5716 diverted OpenaiCodex credential resolution to the generic key resolver whenever provider_uses_custom_endpoint() is true, which dropped an explicit OPENAI_CODEX_ACCESS_TOKEN for custom-base-url setups. The shared-seam wiremock test proves the regression: the mock only answers Bearer test-token, so the request came back 404 on all three CI OSes (client::responses::tests::responses_stream_open_preserves_wire_headers_ through_shared_seam). The manual if-condition formatting also failed the Lint job's cargo fmt --check. Restore the pre-PR precedence by trying codex_credentials() first: env credentials still win on custom endpoints (codex_credentials checks env before the official-endpoint consent grant), the official endpoint keeps propagating OAuth errors, and only a custom endpoint with no env token falls back to deepseek_api_key() — preserving the contributor's goal of letting a custom endpoint authenticate with its own configured key. Signed-off-by: CodeWhale Bot <bot@codewhale.net>
The wire= feature read providers.<id>.wire in two places (client wire
resolution and the capability reporter), and provider_capability_with_
wire was exported but never called with a real value — a parallel entry
point that reported Chat for custom providers the client actually speaks
Responses/Messages to.
- Add Config::provider_wire_dialect() as the single trimmed, non-empty
wire reader; use it in provider_wire_format_for_config and the doctor
capability report (provider_capability_with_wire).
- Drop the over-broad '|| normalized.contains("responses")' from
wire_config_prefers_responses in both modules: every listed alias
except the singular 'response'/'response-api' spellings already
contains the substring, so the fallback only admitted unintended
values like 'not-responses'.
- Remove the vestigial 'let _ = provider_kind;' marker in the resolver
arm that now genuinely uses provider_kind.
Signed-off-by: CodeWhale Bot <bot@codewhale.net>
The PR added a Build Windows x64 workflow triggering on every push to main. That build is already covered: release-artifacts.yml builds both x86_64-pc-windows-msvc and aarch64-pc-windows-msvc release binaries, nightly.yml rebuilds them nightly, and ci.yml runs the full test matrix on windows-latest. A fourth always-on Windows build only spends CI minutes on every main push and grants the job an actions:write permission it does not need. Contributor CI-workflow additions are outside this feature's scope; restoring main's tree (no such file). Signed-off-by: CodeWhale Bot <bot@codewhale.net>
|
Rebuilt the branch to strip the assistant co-author trailers from @whp233's four commits — exactly what the credit check instructs ('remove bot/tool co-author trailer; contributor trailers are for humans'). Their author name/email and commit content are byte-identical; only the bot trailer lines are gone. Tree verified content-identical to the previously tested head ( |
b736fe2 to
45eaadc
Compare
|
Claude finished @Hmbown's task in 1m 11s —— View job Claude's review of #5719 (re-review after synchronize)
SummaryThe only new commit since the last review ( 1. Duplicated wire-alias logic (still open) 2. No test coverage for the new routing behavior (still open) 3. Dead-on-arrival proxy script (minor, unaddressed) Correctness / route safety — unchanged from prior passes, still hold up: the CI: all checks are currently in progress; none have failed as of this review. None of this is blocking — it's the same feedback as the two prior review passes, restated because the synchronize didn't change the reviewable diff. |
…, greening shared macOS/Windows CI Hosted `Test (macos-latest)` and `Test (windows-latest)` have been red on main since S1 landed, and every open PR riding main inherited the same failures (Hmbown#5712 Hmbown#5719 Hmbown#5720 Hmbown#5721 Hmbown#5703 Hmbown#5722 — verified from each exact head's own job logs). macOS (6 failures in sandbox::read_guard::tests): the hosted runner's $TMPDIR is /var/folders/... — a symlink into /private/var/... — so `canonicalize` and `current_dir` hand back the resolved spelling while the rule was only lexically normalized against the literal one. The canonicalized candidate could therefore never match a rule, and none of the symlink / denied-tree tests fired. Prior local verification passed only because it ran with TMPDIR on a plain volume. That is a product hole, not a test artifact: on macOS /etc, /var and /tmp are symlinks into /private, so `read_file /private/etc/sudoers` walked around the built-in /etc/sudoers rule (the Seatbelt setter already canonicalized its own copy of the list; the in-process matcher did not). A subtree rule now remembers its resolved spelling (`DenyRule::subtree`) and `check` matches a candidate against either spelling. Exemptions still compare the configured spelling only — unchanged, out of scope here. Windows (1 failure): root_parent_traversal_does_not_escape_above_root asserted a Unix `/etc` while `normalize_lexically("/../../etc")` correctly resolves a rooted-but-driveless path against the cwd's drive (`D:\etc` on the runner). The test now spells the traversal from the current drive root and keeps the Unix literal under `cfg!(unix)`. Defeat tests (fail on the unpatched matcher on every host, independent of TMPDIR): rule_spelled_through_a_symlinked_root_matches_the_resolved_spelling (unix) and macos_private_spelling_of_a_machine_wide_rule_is_refused (macOS). Local, hosted-equivalent (RUSTFLAGS=-Dwarnings, RUST_MIN_STACK=16MiB, cargo nextest run -p codewhale-tui --lib --all-features --locked --profile ci, TMPDIR = macOS default /var/folders/...): baseline at 20e9574: 22 run / 16 passed / 6 failed (the hosted six); after: 24 run / 24 passed. Defeat probe with rule resolution disabled (tests kept): plain TMPDIR 6 run / 2 failed = exactly the two new tests; symlinked TMPDIR 6 run / 4 failed. fmt clean; changelog synced. Hosted CI is the final gate. Not touched (branch-specific, not shared-main): Hmbown#5712's Windows runtime_api::tests::events_endpoint_respects_since_seq_cursor timeout (8.91 s vs the 8 s ci_scaled budget; Hmbown#5712 does not touch runtime_api) and Hmbown#5722's Ubuntu fleet::executor failure. No-Issue: shared-main CI repair; no tracking issue exists for the hosted-runner symlink shape. Signed-off-by: CodeWhale Bot <bot@codewhale.net> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @Hmbown's task in 1m 5s —— View job Claude's review of #5719 (re-review after synchronize)
SummaryThe only new commits since the last review pass ( 1. Duplicated wire-alias logic (still open) 2. No test coverage for the new routing behavior (still open) 3. Dead-on-arrival proxy script (minor, unaddressed) Correctness / route safety — unchanged, still hold up: the CI: no new CI-relevant changes landed since the last pass; this branch's own diff is unchanged. Same feedback as the two prior review passes — restated because the synchronize commits (an unrelated upstream CI fix + a routine main merge) didn't touch this PR's reviewable diff. None of this is blocking. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if api_provider == ApiProvider::Custom { | ||
| if wire_config_prefers_anthropic(wire) { | ||
| return WireFormat::AnthropicMessages; | ||
| } | ||
| if wire_config_prefers_responses(wire) { | ||
| return WireFormat::Responses; | ||
| } |
There was a problem hiding this comment.
🔴 Custom wire selection lost during dispatch
With custom wire set, provider_wire_format_for_config honors it only during direct construction. Turn dispatch rebuilds from a Chat candidate and sends the wrong protocol.
Prompt for agents
Custom wire overrides are applied only in crates/tui/src/client.rs provider_wire_format_for_config. Runtime turns resolve ProviderKind::Custom through crates/config/src/route/resolver.rs, whose descriptor remains Fixed(ChatCompletions), and DeepSeekClient::from_candidate then binds candidate.protocol(), discarding wire = responses or anthropic. Represent the selected custom dialect in the executable route candidate, or apply one consistent override when candidates are created and consumed. Ensure normal turn dispatch, route preflight, model rebinding, and doctor capability reporting all use the same effective protocol. Add focused coverage that sends a normal engine turn for named custom providers using Responses and Anthropic wires.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const outputText = data.output?.flatMap((item) => item.content ?? []).filter((c) => c.type === "output_text").map((c) => c.text).join("") ?? data.output_text ?? ""; | ||
| const chatRes = { | ||
| id: data.id ?? "chatcmpl-proxy", | ||
| object: "chat.completion", | ||
| created: Math.floor(Date.now() / 1000), | ||
| model, | ||
| choices: [{ index: 0, message: { role: "assistant", content: outputText }, finish_reason: "stop" }], | ||
| usage: data.usage ? { prompt_tokens: data.usage.input_tokens, completion_tokens: data.usage.output_tokens, total_tokens: (data.usage.input_tokens ?? 0) + (data.usage.output_tokens ?? 0) } : undefined, |
There was a problem hiding this comment.
🟡 Non-streaming tool calls disappear
When upstream returns a function call without streaming, chatRes extracts only text and reports stop. Chat clients never execute the requested tool.
Prompt for agents
In scripts/opencode-chat2responses-proxy.mjs, the non-streaming conversion only collects output_text and always emits finish_reason stop. Translate Responses output items of type function_call into Chat Completions message.tool_calls entries, preserve each call id, name, and arguments, and emit finish_reason tool_calls when any are present. Cover text-only, one tool call, and multiple tool calls.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if provider_kind == ProviderKind::OpencodeZen | ||
| && raw.to_ascii_lowercase().contains("muse-spark") |
There was a problem hiding this comment.
🔍 Muse fallback weakens closed routing
The contains("muse-spark") fallback accepts unrelated selectors containing that text. Confirm the closed Zen roster permits this broad forward-compatible boundary.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const translated = translateResponsesSseToChat(buf, model); | ||
| if (translated) res.write(translated); | ||
| } | ||
| res.write(`data: [DONE]\n\n`); |
There was a problem hiding this comment.
🔍 Proxy emits duplicate completion markers
translateResponsesSseToChat forwards upstream [DONE], then EOF writes another marker. Most clients ignore it, but strict consumers can observe duplicate termination.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let body = ""; | ||
| req.on("data", (chunk) => (body += chunk)); |
There was a problem hiding this comment.
🟨 Unbounded proxy bodies exhaust memory
The proxy accumulates body without a size limit. A large local request can exhaust memory and terminate the process.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (apiKey) headers.authorization = `Bearer ${apiKey}`; | ||
| const upstreamRes = await fetch(upstreamUrl, { method: "POST", headers, body: JSON.stringify(responsesBody) }); |
There was a problem hiding this comment.
🟨 Plaintext upstream exposes bearer tokens
When UPSTREAM_BASE uses HTTP, the proxy forwards authorization unchanged. A configuration mistake exposes the bearer token over plaintext transport.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Rescue + fix-forward of #5716 (whp233's fork branch can't be force-pushed; this branch carries their commits
9cf3243285..7b683d598intact in its ancestry, so #5716 auto-links as MERGED when this lands). Full credit to @whp233 for the wire-dialect design — thewire = "responses" | "anthropic" | "chat"selector forkind = "openai-compatible"custom providers and the muse-spark Responses routing originate entirely in their commits.What this adds on top (root-caused, not papered over):
The two red checks, root-caused:
Lintwas pure rustfmt drift in whp233's hand-reformatted credential block (client.rs:1195) — reformatted.Test(all three OSes, one test):DeepSeekClient::newdroppedOPENAI_CODEX_ACCESS_TOKENfor codex configs with a custombase_url, so the wire-preservation test 404'd against its wiremock. Fix14ec3a051: for OpenaiCodex,codex_credentials()is tried first (env token still wins on custom endpoints; the official endpoint still propagates OAuth errors exactly as before); only a custom endpoint with no env token falls back to the config key — preserving the contributor's goal without the regression.Cleanups riding the fix: one
Config::provider_wire_dialect()reader (the dialect is read once, trimmed, non-empty —provider_capability_reportnow actually consumes the previously-deadprovider_capability_with_wire); the over-broadcontains("responses")alias match narrowed to the two live aliases; the vestigiallet _ = provider_kind;removed; the contributor-added always-on Windows build workflow reverted (release-artifacts.yml+nightly.ymlalready build both Windows targets and ci.yml tests windows — a fourth build only burns minutes and carried an unneededactions: write).Route Contract conformance: the wire key selects the payload dialect on the EXISTING custom-provider config surface — no second provider catalog, no new provider kind; resolver additions mirror the existing capability-gate branches.
Verification on the final tree: full TUI lib 11,347 passed / 13 skipped; config 621 passed / 1 skipped; CLI 304 passed; the previously failing
responses_stream_open_preserves_wire_headers_through_shared_seampasses; fmt clean; CI-exact clippy exit 0.Closes #5713