feat(qwen): Alibaba Qwen built-in over Model Studio's OpenAI-compatible mode #27

2026-08-12T20:08:15Z

steve commented

2026-08-12 20:08:15 +00:00

Adds the qwen built-in provider and the qwen:// DSN scheme, keyed by
QWEN_API_KEY, defaulting to Model Studio's international host
(https://dashscope-intl.aliyuncs.com/compatible-mode/v1). Like kimi
(ADR-0026), it is provider/openai pointed somewhere else — no new client.

OpenAI-compat vs Anthropic-compat

Model Studio serves the same models over two protocols, so the real
decision here was which wire format the built-in speaks. ADR-0027 records why
it is the OpenAI one — every difference cuts the same way, and each failure
mode is silent rather than loud:

	OpenAI-compat (chosen)	Anthropic-compat shim
`ReasoningEffort`	top-level `reasoning_effort`, already sent by the openai client	`provider/anthropic` ignores it by design (first-party Claude has no such knob)
`Request.Schema`	`response_format: json_schema`, native on Max/Plus	`provider/anthropic` uses the first-party GA `output_config.format`, which the shim does not implement — unknown field ignored ⇒ unconstrained prose reported as success
Cache accounting	`usage.prompt_tokens_details.cached_tokens` → `Usage.CacheReadTokens`	reads `cache_read_input_tokens`, which the shim has no reason to emit
Thinking blocks	n/a	the shim's headline feature, and `provider/anthropic` discards them in both decoders anyway
Missing-key hint	`WithAPIKeyName("QWEN_API_KEY")`	anthropic client has no such option — a keyless qwen would say `ANTHROPIC_API_KEY`

The shim remains reachable ad hoc without library changes:
LLM_QWEN_ANTHROPIC=anthropic://[email protected]/apps/anthropic.

Shared factory

The kimi and qwen DSN factories were byte-identical, so they now share one
openaiCompatScheme helper — "credential comes from the DSN token, and the
missing-key hint names LLM_<NAME>" holds by construction rather than by copy,
so the next OpenAI-compat built-in inherits both rules.

Tests

Hermetic (capturing RoundTripper, no network, no credentials) and
break-checked — all six fail on a deliberate mutation:

built-in resolves in Parse, hits the right base URL, sends Bearer $QWEN_API_KEY
missing key ⇒ synthetic 401 naming QWEN_API_KEY, never OPENAI_API_KEY, no network hit
reverse leak: a registry that can see QWEN_API_KEY must not hand it to the openai built-in (asserted with a real OPENAI_API_KEY set, so the request actually goes out — otherwise the assertion is vacuous)
qwen:// DSN round-trips against the China host
keyless qwen:// names LLM_QCN, not QWEN_API_KEY
reasoning_effort asserted on the wire body — the ADR's load-bearing claim, and invisible without decoding the request

captureRT now records the request body (and closes it, per the RoundTripper
contract).

Gates

go build · go vet · gofmt -l (empty) · go test -race -count=1 ./... ·
go mod tidy clean.

Docs updated in the same commit: README built-in table, Qwen paragraph, DSN
scheme list, support matrix (footnote ⁴), .env.example, ADR-0027 + index,
progress.md.

Two Alibaba-side quirks documented rather than papered over: thinking is on by
default for some models (e.g. qwen3.7-plus), and Qwen3 open-source models
require streaming while thinking, so a buffered Generate wants a Max/Plus
model.

🤖 Generated with Claude Code

Adds the `qwen` built-in provider and the `qwen://` DSN scheme, keyed by `QWEN_API_KEY`, defaulting to Model Studio's international host (`https://dashscope-intl.aliyuncs.com/compatible-mode/v1`). Like kimi (ADR-0026), it is `provider/openai` pointed somewhere else — no new client. ## OpenAI-compat vs Anthropic-compat Model Studio serves the same models over **two** protocols, so the real decision here was which wire format the built-in speaks. ADR-0027 records why it is the OpenAI one — every difference cuts the same way, and each failure mode is silent rather than loud: | | OpenAI-compat (chosen) | Anthropic-compat shim | |---|---|---| | `ReasoningEffort` | top-level `reasoning_effort`, already sent by the openai client | `provider/anthropic` ignores it **by design** (first-party Claude has no such knob) | | `Request.Schema` | `response_format: json_schema`, native on Max/Plus | `provider/anthropic` uses the first-party GA `output_config.format`, which the shim does not implement — unknown field ignored ⇒ unconstrained prose reported as success | | Cache accounting | `usage.prompt_tokens_details.cached_tokens` → `Usage.CacheReadTokens` | reads `cache_read_input_tokens`, which the shim has no reason to emit | | Thinking blocks | n/a | the shim's headline feature, and `provider/anthropic` discards them in both decoders anyway | | Missing-key hint | `WithAPIKeyName("QWEN_API_KEY")` | anthropic client has no such option — a keyless qwen would say `ANTHROPIC_API_KEY` | The shim remains reachable ad hoc without library changes: `LLM_QWEN_ANTHROPIC=anthropic://[email protected]/apps/anthropic`. ## Shared factory The kimi and qwen DSN factories were byte-identical, so they now share one `openaiCompatScheme` helper — "credential comes from the DSN token, and the missing-key hint names `LLM_<NAME>`" holds by construction rather than by copy, so the next OpenAI-compat built-in inherits both rules. ## Tests Hermetic (capturing RoundTripper, no network, no credentials) and **break-checked — all six fail on a deliberate mutation**: - built-in resolves in Parse, hits the right base URL, sends `Bearer $QWEN_API_KEY` - missing key ⇒ synthetic 401 naming `QWEN_API_KEY`, never `OPENAI_API_KEY`, no network hit - **reverse leak**: a registry that can see `QWEN_API_KEY` must not hand it to the openai built-in (asserted with a real `OPENAI_API_KEY` set, so the request actually goes out — otherwise the assertion is vacuous) - `qwen://` DSN round-trips against the China host - keyless `qwen://` names `LLM_QCN`, not `QWEN_API_KEY` - `reasoning_effort` asserted **on the wire body** — the ADR's load-bearing claim, and invisible without decoding the request `captureRT` now records the request body (and closes it, per the RoundTripper contract). ## Gates `go build` · `go vet` · `gofmt -l` (empty) · `go test -race -count=1 ./...` · `go mod tidy` clean. Docs updated in the same commit: README built-in table, Qwen paragraph, DSN scheme list, support matrix (footnote ⁴), `.env.example`, ADR-0027 + index, progress.md. Two Alibaba-side quirks documented rather than papered over: thinking is on by default for some models (e.g. `qwen3.7-plus`), and Qwen3 *open-source* models require streaming while thinking, so a buffered `Generate` wants a Max/Plus model. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

steve added 1 commit 2026-08-12 20:08:16 +00:00

feat(qwen): Alibaba Qwen built-in over Model Studio's OpenAI-compatible mode

Gadfly review (reusable) / review (pull_request) Successful in 5m14s

Details

Adversarial Review (Gadfly) / review (pull_request) Successful in 5m14s

Details

CI / Tidy (pull_request) Successful in 9m24s

Details

CI / Build & Test (pull_request) Successful in 9m53s

Details

02cd561eaf

Adds the `qwen` built-in provider and the `qwen://` DSN scheme, keyed by
QWEN_API_KEY and defaulting to Model Studio's international host. Like kimi
(ADR-0026) it is `provider/openai` pointed elsewhere — no new client.

Model Studio serves the same models over two protocols, so the real decision
was which wire format to speak. ADR-0027 records why it is the OpenAI one:
down the anthropic client `ReasoningEffort` is ignored by design, structured
output rides the first-party `output_config.format` mechanism the shim does
not implement, and cached-token accounting reads Anthropic-only usage fields.
Each of those fails silently rather than loudly, which is what makes the
choice worth writing down. The shim stays reachable ad hoc via an
`anthropic://` DSN.

The kimi and qwen DSN factories were byte-identical, so they now share one
`openaiCompatScheme` helper: the "credential comes from the DSN token, and
the missing-key hint names LLM_<NAME>" rules hold by construction instead of
by copy.

Tests are hermetic and break-checked (all six fail on a deliberate mutation),
including the reverse credential leak — a visible QWEN_API_KEY must not
authenticate the openai built-in — and reasoning_effort asserted on the wire
body, which is the ADR's load-bearing claim.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

gitea-actions bot commented

2026-08-12 20:10:01 +00:00

🪰 Gadfly — live review status

4/4 reviewers finished · updated 2026-08-12 20:46:54Z

`claude-code/opus` · claude-code — ✅ done

✅ security — No material issues found
✅ correctness — No material issues found
✅ maintainability — Minor issues
✅ performance — No material issues found
✅ error-handling — No material issues found

`claude-code/sonnet` · claude-code — ✅ done

✅ security — No material issues found
✅ correctness — No material issues found
✅ maintainability — No material issues found
✅ performance — No material issues found
✅ error-handling — No material issues found

`glm-5.2:cloud` · ollama-cloud — ✅ done

✅ security — No material issues found
✅ correctness — No material issues found
✅ maintainability — Minor issues
✅ performance — No material issues found
✅ error-handling — No material issues found

`kimi-k2.6:cloud` · ollama-cloud — ✅ done

✅ security — No material issues found
✅ correctness — No material issues found
✅ maintainability — No material issues found
✅ performance — No material issues found
✅ error-handling — No material issues found

_{Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.}

## 🪰 Gadfly — live review status 4/4 reviewers finished · updated 2026-08-12 20:46:54Z #### `claude-code/opus` · claude-code — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — No material issues found #### `claude-code/sonnet` · claude-code — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — No material issues found - ✅ **performance** — No material issues found - ✅ **error-handling** — No material issues found #### `glm-5.2:cloud` · ollama-cloud — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — No material issues found #### `kimi-k2.6:cloud` · ollama-cloud — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — No material issues found - ✅ **performance** — No material issues found - ✅ **error-handling** — No material issues found Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.

gitea-actions bot commented

2026-08-12 20:13:32 +00:00

🪰 Gadfly review — consensus across 4 models

Verdict: Minor issues · 3 findings (0 with multi-model agreement)

3 single-model findings (lower confidence)

	Finding	Where	Model	Lens
🟡	ProviderQwen const keeps stale alignment padding while sibling ProviderKimi had its padding stripped in this same PR	`builtin.go:26`	glm-5.2:cloud	maintainability
⚪	DSN.Scheme doc comment lists kimi but not the sibling qwen scheme added in this PR	`env.go:30`	claude-code/opus	maintainability
⚪	envKeyForProvider doc-comment example inverts the function's direction (LLM_M1 → "m1" is name → var backwards)	`env.go:44`	glm-5.2:cloud	maintainability

Per-model detail

claude-code/opus (claude-code) — Minor issues

Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

I've verified the credential-handling paths against the checked-out code. This is my security-lens review.

VERDICT: No material issues found

This PR adds the qwen built-in by refactoring the existing kimi credential-wiring into two shared helpers (registerOpenAICompatBuiltin, openaiCompatScheme) plus a centralized envKeyForProvider. Through the security lens, the credential-isolation properties are preserved and actually hardened (moved from copy-paste-with-a-comment to by-construction). Specifics I checked:

No cross-provider key fallthrough. registerOpenAICompatBuiltin passes WithAPIKey(r.envLookup(keyEnv)) unconditionally (builtin.go:88). Verified in provider/openai/openai.go:117 that New defaults apiKey to os.Getenv("OPENAI_API_KEY"); passing the option unconditionally (even when empty) overrides that default, so an unset QWEN_API_KEY cannot silently authenticate a qwen request with an OpenAI key. The empty key then surfaces as a synthetic 401 at request time (provider/openai/model.go:76), not a misdirected authenticated call.
No secret leakage in error output. The missing-key hint is "...set " + m.p.apiKeyName + " or use WithAPIKey" (provider/openai/model.go:85) — it names the variable (QWEN_API_KEY / LLM_QCN), never the key value. WithAPIKeyName carries only the name (openai.go:72).
Reverse-leak direction covered. QWEN_API_KEY is read only for the qwen provider; the openai built-in is constructed from its own default and never receives the qwen credential. This is directly asserted by TestQwenBuiltinKeyDoesNotLeakToOpenAI (with a real OPENAI_API_KEY set so the request actually egresses), which I read and consider a genuine security regression test.
No SSRF / URL-injection surface introduced. The qwen built-in base URL is a compile-time constant (builtin.go:49, https://). DSN hosts flow through DSN.BaseURL() which force-prefixes https:// (env.go:41), preventing scheme downgrade; and DSN values are operator-supplied LLM_* config, not attacker-controlled request input, so they are out of the injection/taint model.
envKeyForProvider (env.go:52) only uppercases and replaces -→_; no format-string, path, or command construction. Its two consumers (lazy resolution registry.go:266, and the DSN missing-key hint) now share one definition, removing a silent-drift risk rather than adding one.

No authn/authz gaps, injection, unsafe deserialization, or insecure defaults in this change's lane. The deleted kimi test file is fully subsumed by the new table-driven suite (same assertions), so no security coverage was dropped.

🎯 Correctness — No material issues found

I've verified the change end-to-end. My findings through the Correctness lens:

VERDICT: No material issues found

This is a faithful extraction-plus-addition, and I confirmed each correctness-sensitive claim against the checked-out code:

envKeyForProvider is behavior-preserving. env.go:52-54 produces "LLM_" + ToUpper(ReplaceAll(name, "-", "_")) — byte-identical to the inline expression it replaced at registry.go:266 (old providerFor) and the old kimi scheme's WithAPIKeyName. The DSN-name path (qcn → LLM_QCN) matches the lazy-resolution key used in providerFor, so missing-key hints stay consistent. Verified by reading both call sites.
The kimi refactor is equivalent. registerOpenAICompatBuiltin (builtin.go:84-92) and openaiCompatScheme (builtin.go:60-69) reproduce the old kimi registration exactly: same openaiOpts wrap, WithAPIKey(r.envLookup(keyEnv)) eager / WithAPIKey(dsn.Token) for the scheme, and the same WithAPIKeyName. No semantic drift from the consolidation.
The load-bearing credential rule holds. WithAPIKey (provider/openai/openai.go:49-51) sets p.apiKey = key unconditionally, so passing WithAPIKey("") overrides the OPENAI_API_KEY default (openai.go:117) with empty rather than falling through to it. That is what makes an unset QWEN_API_KEY fail closed with a synthetic 401 instead of authenticating as OpenAI. Verified the option is a plain assignment, not a conditional.
Base URLs re-derived and correct. International (Singapore) OpenAI-compat host https://dashscope-intl.aliyuncs.com/compatible-mode/v1 and the China host https://dashscope.aliyuncs.com/compatible-mode/v1 reached via DSN.BaseURL() (https:// + host, env.go:41) both match Alibaba Model Studio's documented endpoints. No transposed intl/China hosts, no wrong path segment.
No compile/resolution problems. strings was removed from builtin.go imports because its last use moved to env.go (which already imports strings); the removal is correct. r.providers/r.schemes/r.envLookup all exist and are written the same way as the surrounding built-ins.

The remaining substance of the PR (the OpenAI-vs-Anthropic-compat decision, reasoning_effort on the wire) is documentation and test-side, and the tests assert exactly the claims the ADR makes. Nothing in the correctness lane is wrong.

🧹 Code cleanliness & maintainability — Minor issues

I've verified the finding against the actual code. The env.go doc comment (lines 28-30) does enumerate "foreman", "ollama", "ollama-cloud", "openai", "kimi", "anthropic", "google"/"gemini" — it names the sibling built-in kimi but not the qwen scheme this PR adds. The finding is accurate and remains a trivial, illustrative nit. All other draft observations were confirmations of correct code, not findings. The single trivial finding survives.

VERDICT: Minor issues

This is a clean, well-motivated refactor for my lens. The three extractions are the right calls and I verified each against the checked-out tree:

envKeyForProvider (env.go:52) genuinely dedupes two previously-copied "LLM_"+strings.ToUpper(strings.ReplaceAll(...)) sites; openaiCompatScheme (env.go:66) now calls it, and strings is still imported/used in env.go. ✅
The strings import removed from builtin.go is correct — the file no longer contains any strings. reference, so it still compiles. ✅
registerOpenAICompatBuiltin / openaiCompatScheme collapse the byte-identical kimi/qwen scheme closures into one helper; SchemeFactory is the real type used for r.schemes, and the two credential rules (unconditional WithAPIKey, WithAPIKeyName naming keyEnv) are preserved by construction. Passing wrap as a parameter is necessary (the package-level helper can't close over the openaiOpts local). ✅
Both kimi and qwen are now installed via the single registerOpenAICompatBuiltin call (builtin.go:147, builtin.go:152) with the correct base URLs and key env vars. ✅

One trivial nit:

env.go:30 — the DSN.Scheme doc comment enumerates example schemes ("foreman", "ollama", "ollama-cloud", "openai", "kimi", "anthropic", ...) and now names kimi but not the sibling qwen added here. The list is already non-exhaustive (it also omits llama-swap/llama-swaps), so this is illustrative rather than authoritative — but given it explicitly calls out kimi, adding qwen beside it keeps the sibling built-ins in step with the project's "docs in sync in the same commit" convention. Purely optional; no behavioral impact.

No duplicated logic, dead code, confusing names, or pattern deviations otherwise — the new built-in follows the kimi precedent exactly and actually reduces total surface area.

⚡ Performance — No material issues found

I've verified the changed code. This PR is entirely provider-registration and DSN-scheme plumbing plus docs/tests.

VERDICT: No material issues found

Through the ⚡ Performance lens, this change is clean:

registerOpenAICompatBuiltin / openaiCompatScheme (builtin.go:60-92) run only during registerBuiltins, i.e. once per registry construction at startup, not on any request path. Refactoring two byte-identical inline blocks into a shared factory changes structure, not the amount of work — the same openai.New(...) calls happen the same number of times. No new allocations on a hot path, no added indirection in Generate.
envKeyForProvider (env.go:52-54) is two string ops (ToUpper+ReplaceAll) invoked at lazy DSN resolution and DSN-scheme construction — both cold, low-frequency. Extracting it from the two former inline copies (registry.go:266, and the old kimi scheme) is net-neutral: same computation, called at the same rare sites. No per-request cost.
registry.go:266 swaps an inline string build for the identical function call — no behavioral or cost change.
captureRT.RoundTrip (builtin_openaicompat_test.go) now does io.ReadAll/Close on the request body, but that's test-only code and correctly honors the RoundTripper body-ownership contract; irrelevant to production performance.
Everything else in the diff is Markdown docs (README, ADR-0027, progress.md, .env.example).

There are no loops, no per-item queries, no unbounded growth, no blocking calls added to any hot path, and no quadratic behavior introduced. Nothing to flag in my lane.

🧯 Error handling & edge cases — No material issues found

I've verified the production changes against the checked-out code. This is a behavior-preserving refactor (extracting envKeyForProvider, openaiCompatScheme, registerOpenAICompatBuiltin) plus a new built-in that reuses the existing openai client. Through the error-handling & edge-cases lens:

VERDICT: No material issues found

openaiCompatScheme / registerOpenAICompatBuiltin return path (builtin.go:60-92): the factory does return openai.New(...), nil. I checked provider/openai/openai.go:114 — func New(opts ...Option) *Provider returns no error, so nothing is being swallowed here. The dropped-error smell is a false alarm.
r.envLookup(keyEnv) at construction (builtin.go:88): could panic if envLookup were nil, but New sets envLookup: os.Getenv (registry.go:173) and only overrides it with a non-nil func, all before registerBuiltins runs (registry.go:187-190). Pre-existing invariant, unchanged by this PR. Safe.
Keyless DSN unhappy path (builtin.go:65-66): empty dsn.Token → WithAPIKey("") + WithAPIKeyName(envKeyForProvider(name)). The old kimi factory used the identical "LLM_"+ToUpper(ReplaceAll(name,"-","_")) expression now centralized in envKeyForProvider (env.go:52), so the missing-token 401-hint behavior is unchanged; the new table test asserts it names LLM_QCN. Verified the two are byte-identical.
registerOpenAICompatBuiltin writes r.providers/r.schemes without the mutex (builtin.go:85-91): matches every other line in registerBuiltins, which runs single-threaded during New before the registry is published. No new race.
envKeyForProvider on empty name: New never calls the scheme factory with an empty name (LoadEnv skips LLM_ with nothing after it, env.go:90), and ParseDSN rejects empty host. No new panic surface.
Test captureRT.RoundTrip ignores io.ReadAll error (builtin_openaicompat_test.go): c.reqBody, _ = io.ReadAll(...). Benign in a hermetic test against an in-memory body; not production code.

No swallowed errors, missing cleanup, or unhandled edge cases introduced by this change. The refactor preserves the existing unhappy-path semantics exactly.

claude-code/sonnet (claude-code) — No material issues found

Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

VERDICT: No material issues found

This PR only adds a new OpenAI-compat built-in (qwen) by reusing the existing provider/openai client and refactors the identical kimi/qwen DSN-factory logic into shared helpers (registerOpenAICompatBuiltin, openaiCompatScheme, envKeyForProvider). I verified the security-relevant claims directly:

Credential isolation is sound. registerOpenAICompatBuiltin (builtin.go:84-92) passes openai.WithAPIKey(r.envLookup(keyEnv)) unconditionally, and provider/openai.New (openai.go:112-128) only falls back to os.Getenv("OPENAI_API_KEY") when WithAPIKey is never called — so an unset QWEN_API_KEY cannot silently pick up OPENAI_API_KEY. Confirmed by reading both files directly, not just trusting the PR's stated tests.
No secret leakage in error paths. The missing-key error (model.go:80-86) interpolates only m.p.apiKeyName (the variable name, e.g. "QWEN_API_KEY"), never the key value. envKeyForProvider (env.go:52-54) similarly only derives an env-var name string.
envKeyForProvider dedup is behavior-preserving. Its new single definition (env.go:52-54) is byte-identical to the two call sites it replaces (registry.go:266, and the old inline kimi scheme), so no drift was introduced.
DSN host handling is unchanged. DSN.BaseURL() (env.go:41) and ParseDSN (env.go:59-77) are untouched by this diff; the qwen:// scheme reuses the same host-from-DSN pattern kimi already had (operator-controlled env DSNs, not attacker-controlled input — no new SSRF surface).
No secrets appear in .env.example, ADR, README, or test fixtures — all placeholders (sk-..., qwen-secret, etc.).

The one thing this PR deliberately doesn't do — route through Model Studio's Anthropic-compat shim — is a correctness/behavior choice (documented in ADR-0027), not a security regression; if anything it avoids a surface with weaker validation guarantees for Request.Schema.

🎯 Correctness — No material issues found

Review — 🎯 Correctness

VERDICT: No material issues found

I traced the actual wiring, not just the diff hunks:

envKeyForProvider (env.go:52) is used identically by registry.go:266 (lazy DSN resolution) and by openaiCompatScheme (builtin.go:66) — verified both call sites now share the one function, closing the "two copies that could drift" risk the PR calls out. No behavior change vs. the pre-refactor kimi scheme.
registerOpenAICompatBuiltin (builtin.go:84-92) writes r.providers[name] and r.schemes[name] with WithAPIKey passed unconditionally in both the eager built-in and the DSN scheme, so an unset QWEN_API_KEY/DSN token can't fall through to openai.New's OPENAI_API_KEY default (confirmed openai.New defaults apiKey: os.Getenv("OPENAI_API_KEY") only when WithAPIKey isn't supplied — provider/openai/openai.go:117).
Verified provider/openai/wire.go:25,204 already serializes Request.ReasoningEffort as top-level reasoning_effort, and wire.go:131-148 already maps prompt_tokens_details.cached_tokens → Usage.CacheReadTokens — so the ADR's central claim (qwen inherits both for free by reusing the openai client) holds against the actual client code, not just the PR narrative.
TestQwenBuiltinKeyDoesNotLeakToOpenAI correctly uses t.Setenv("OPENAI_API_KEY", ...) rather than the registry's injected envLookup, because the openai built-in provider (unlike kimi/qwen) is constructed with no WithAPIKey option and so reads the real process env at construction — checked against registerBuiltins (builtin.go:131) and openai.New (openai.go:114-118). The test is exercising the right seam.
The table-driven test (builtin_openaicompat_test.go) computes dsnName from tc.dsnVar and constructs DSN scheme values as tc.name + "://tok@" + tc.dsnHost, which correctly registers provider qcn (from LLM_QCN) using scheme qwen — matches r.schemes[ProviderQwen] = openaiCompatScheme(wrap) in builtin.go:152.
No duplicate/shadowed symbols, no leftover strings import misuse (correctly dropped from builtin.go now that envKeyForProvider lives in env.go), no signature mismatches between SchemeFactory and the new openaiCompatScheme return type.

I could not execute go build/go test in this sandbox (tool approval declined), so compilation was verified by manual symbol/signature tracing rather than the compiler — I'm confident in the read but flagging the method.

Nothing in this diff introduces a logic bug, incorrect magic number, or wrong control flow within the correctness lens.

🧹 Code cleanliness & maintainability — No material issues found

Nothing unusual there. Everything checks out — no cleanliness issues.

Review

VERDICT: No material issues found

Reviewed through the code cleanliness / maintainability lens:

builtin.go: the new openaiCompatScheme + registerOpenAICompatBuiltin helpers cleanly remove the byte-identical kimi/qwen duplication that would otherwise have been copy-pasted a third time. Verified openaiOpts is still defined/used at builtin.go:125 and both registerOpenAICompatBuiltin calls (builtin.go:147, 152) pass it through correctly. The strings import drop is justified — the only user ("LLM_"+strings.ToUpper(...)) moved into the new shared envKeyForProvider in env.go:52, and registry.go still uses strings for its own strings.Cut call (registry.go:195), so no orphaned/broken imports.
env.go:52 (envKeyForProvider) is a good dedup of the providerFor env-key derivation and the DSN scheme's key-hint logic — previously these were two independent literals that had to be kept in sync by convention only.
Test consolidation (builtin_kimi_test.go deleted, replaced by a table-driven builtin_openaicompat_test.go + a slim builtin_qwen_test.go for qwen-specific cases) removes near-duplicate test functions and keeps the shared contract in one place; targetsOf/newTestRegistry helpers it relies on already exist in parse_test.go:14,24, so nothing is left dangling.
The ProviderQwen/ProviderAnthropic/... const alignment looks odd at a glance (ProviderQwen = "qwen" next to a single-spaced ProviderKimi = "kimi") but this is correct gofmt behavior — comment lines break contiguous alignment groups, so ProviderKimi (bracketed by comments) formats alone while the uninterrupted run from ProviderQwen through ProviderLlamaSwap aligns together. Not a real issue.

Diff is well-factored with no dead code, no confusing names, and no copy-paste left behind after the refactor.

⚡ Performance — No material issues found

VERDICT: No material issues found

This PR is registration-time/administrative code (provider setup at Registry construction), documentation, and tests — there's no hot-path code introduced.

Checked specifically for performance impact:

registerOpenAICompatBuiltin / openaiCompatScheme (builtin.go:60-92): called exactly twice (kimi, qwen) inside registerBuiltins, which itself runs once per Registry construction — not per-request. Verified by reading registerBuiltins in full; no loop or per-call invocation path.
envKeyForProvider (env.go): cheap strings.ToUpper/ReplaceAll. Verified its only production call sites are openaiCompatScheme (once per DSN-scheme invocation, i.e. once per new provider name resolved) and registry.go:266 inside providerFor, which caches the resolved provider in r.providers before returning (registry.go:276-283) — so this runs at most once per distinct lazily-resolved provider name, not per request.
captureRT.RoundTrip's new io.ReadAll(r.Body) (builtin_openaicompat_test.go) is test-only scaffolding, not production code.

No N+1 patterns, no unbounded growth, no new allocations in a loop, no blocking calls added to a request hot path. Nothing in this lens warrants a finding.

🧯 Error handling & edge cases — No material issues found

Review — 🧯 Error handling & edge cases lens

VERDICT: No material issues found

This is a mechanical refactor (extracting registerOpenAICompatBuiltin/openaiCompatScheme/envKeyForProvider) plus a straightforward new built-in that follows the existing kimi pattern byte-for-byte. I verified the actual checked-out code:

builtin.go:60-92 — openaiCompatScheme and registerOpenAICompatBuiltin preserve the exact credential-handling invariants from the original kimi code (WithAPIKey always passed, even empty, so no fallthrough to OPENAI_API_KEY; WithAPIKeyName always set). Verified against provider/openai/openai.go:112-128 — openai.New never fails and only defaults apiKey from OPENAI_API_KEY when WithAPIKey isn't supplied at all, which never happens here.
registry.go:266 and env.go:52-54 — envKeyForProvider is a pure, side-effect-free extraction of previously-inlined logic; behavior on empty/hyphenated names is unchanged.
registerBuiltins still writes to r.providers/r.schemes maps without holding r.mu (builtin.go:85-91), but this is pre-existing behavior (same as the code it replaced) — registration happens before the registry is exposed, so no new concurrency edge case was introduced.
The scheme factories (old and new) always return a nil error from openai.New, matching the pre-existing kimi scheme — no regression in how a hypothetical future construction error would propagate.

One minor, non-blocking observation in new test scaffolding:

builtin_openaicompat_test.go:32-33 — captureRT.RoundTrip swallows the error from io.ReadAll(r.Body) (c.reqBody, _ = io.ReadAll(r.Body)) and the error from r.Body.Close(). In practice the bodies here are always in-memory JSON buffers from the openai client, so this can't realistically fail, and a failure would still surface indirectly (nil reqBody → json.Unmarshal fails in TestQwenReasoningEffortReachesWire with a clear t.Fatalf). Not worth blocking on — flagging only because it's a literally swallowed error in code newly added by this PR.

No missing cleanup/defer, no nil/empty-collection panics, and no off-by-one issues were found in the production code touched by this diff.

glm-5.2:cloud (ollama-cloud) — Minor issues

Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

Verdict: No material issues found

Through the security lens, I traced the credential flow and validated the load-bearing claims against the actual code:

Credential isolation / fail-closed on missing key (builtin.go:84-92, provider/openai/openai.go:114-128): registerOpenAICompatBuiltin passes WithAPIKey(r.envLookup(keyEnv)) unconditionally, even when the lookup is empty. WithAPIKey("") sets p.apiKey = "", which overrides New's os.Getenv("OPENAI_API_KEY") default. So an unset QWEN_API_KEY cannot silently authenticate the qwen built-in as OpenAI. Verified by reading openai.go:49-51,114-127.
Reverse-leak direction (builtin_qwen_test.go): the openai built-in is constructed with no WithAPIKey, so it reads os.Getenv("OPENAI_API_KEY"); qwen's key is injected only into the qwen provider via WithAPIKey. The two never share an env-var source, so QWEN_API_KEY cannot reach the openai client. The test sets a real OPENAI_API_KEY to make the assertion non-vacuous — correct design.
DSN credential path (builtin.go:60-69, env.go:41): openaiCompatScheme uses dsn.Token as the bearer credential and envKeyForProvider(name) for the missing-key hint; a keyless DSN yields WithAPIKey("") → fails closed, not falls through. Base URL is forced to https://+host. No injection surface (operator-controlled env, same as kimi/openai).
envKeyForProvider consolidation (env.go:52-54, registry.go:266): the lazy-resolution key and the missing-key hint now share one definition, so a keyless DSN names the exact variable the operator must set — no drift that could misdirect credential setup.

No authn/authz gaps, credential leakage, injection, or insecure defaults found in this change.

🎯 Correctness — No material issues found

Verdict: No material issues found

I verified the correctness-critical pieces of this change against the checked-out code:

envKeyForProvider consolidation (env.go:52, registry.go:266): the new helper is byte-identical to the two prior call sites — LLM_ + uppercase with -→_. The old inline form in providerFor and the old kimi DSN factory both produced the same string; the refactor preserves behavior. Verified by reading both call sites.
registerOpenAICompatBuiltin / openaiCompatScheme parity (builtin.go:60-92): the eager path (WithAPIKey(r.envLookup(keyEnv)), WithAPIKeyName(keyEnv)) and the DSN path (WithAPIKey(dsn.Token), WithAPIKeyName(envKeyForProvider(name))) reproduce exactly what the deleted kimi code did — confirmed against provider/openai/openai.go:49-73 (WithAPIKey/WithAPIKeyName semantics) and provider/openai/model.go:76-86 (the synthetic-401 names apiKeyName). The "pass empty key unconditionally so it can't fall through to OPENAI_API_KEY default" rule holds, since openai.New only falls back to os.Getenv("OPENAI_API_KEY") when no WithAPIKey option is supplied (openai.go:114-128).
reasoning_effort on the wire (provider/openai/wire.go:204,25): buildRequest copies req.ReasoningEffort into chatRequest.ReasoningEffort with json tag reasoning_effort,omitempty; the test asserting sent["reasoning_effort"]=="high" is a real wire assertion, not a tautology.
Reverse-leak test (builtin_qwen_test.go): the openai built-in is constructed with no WithAPIKey, so its key comes from os.Getenv("OPENAI_API_KEY") (openai.go:117), set via t.Setenv. The registry envLookup returns qwen-secret only for QWEN_API_KEY and "" otherwise, so the qwen credential cannot reach the openai built-in. The assertion is non-vacuous (a real request goes out). Verified the construction path.
DSN round-trip URLs: DSN.BaseURL() (env.go:41) is "https://"+Host, and do appends /chat/completions (model.go:92). For qwen://[email protected]/compatible-mode/v1 → https://dashscope.aliyuncs.com/compatible-mode/v1/chat/completions, matching wantDSNURL. Confirmed.

No logic bugs, semantic errors, or incorrect constants in my lens.

🧹 Code cleanliness & maintainability — Minor issues

Both findings verified against the actual source:

builtin.go:20 is ProviderKimi = "kimi" (padding stripped), while builtin.go:26 is ProviderQwen = "qwen" (padding retained) — inconsistent spacing within the same const block. Confirmed.
env.go:44 reads // name (LLM_M1 → "m1", so "my-prov" → LLM_MY_PROV). while the function (line 52) returns the LLM_ variable for a given name, so the first arrow LLM_M1 → "m1" inverts the actual direction. Confirmed.

Verdict: Minor issues

builtin.go:26 — stale alignment padding in the const block. The PR stripped the alignment whitespace from ProviderKimi (ProviderKimi = "kimi" at line 20) but left it on the newly-added ProviderQwen (ProviderQwen = "qwen" at line 26). The two sibling constants in the same block now use inconsistent spacing (gofmt accepts both because the intervening doc comments split alignment runs, so this is cosmetic only). Fix: write ProviderQwen = "qwen" to match the ProviderKimi form this PR just established.
env.go:44 — doc-comment example reads backwards. envKeyForProvider returns the LLM_ variable for a given name (line 52), but the comment's first half LLM_M1 → "m1" shows the inverse direction, undercutting the "single definition on purpose" point the comment is making. Suggested: // name → variable, e.g. "m1" → "LLM_M1", "my-prov" → "LLM_MY_PROV".

⚡ Performance — No material issues found

No material issues found

Reviewed through the performance lens: the change is additive wiring that reuses the existing provider/openai client. openai.New is called once per built-in at registration (registerOpenAICompatBuiltin) and once per DSN provider on first lazy resolution in providerFor (registry.go:266-283), with the result cached in r.providers afterward — no per-request allocation or repeated construction on hot paths. openaiCompatScheme is a tiny closure with no per-call work beyond constructing options. The test captureRT.RoundTrip now drains/closes the request body; that's test-only and the body is a single small JSON request, not a concern. No N+1, unbounded growth, blocking calls, or quadratic behavior introduced.
Verified providerFor caching (registry.go:255-283) and openai.New cost (provider/openai/openai.go:114-128) to confirm no repeated provider construction on the request path.

🧯 Error handling & edge cases — No material issues found

Verified the full error-handling chain. The diff is clean through my lens.

Verdict: No material issues found

Missing-key path (eager built-in): registerOpenAICompatBuiltin passes WithAPIKey(r.envLookup(keyEnv)) unconditionally. Verified at provider/openai/model.go:76-87: an empty apiKey short-circuits to a synthetic *llm.APIError (401, missing_api_key) naming apiKeyName, and never reaches client.Do. So an unset QWEN_API_KEY fails closed and names QWEN_API_KEY rather than falling through to openai.New's OPENAI_API_KEY default (openai.go:117). No network hit, error not swallowed.
Keyless DSN path: openaiCompatScheme passes WithAPIKey(dsn.Token). An empty dsn.Token (e.g. qwen://host) triggers the same synthetic-401 path, now naming envKeyForProvider(name) = LLM_<NAME>. Verified the env-key derivation at env.go:52-54 matches the lazy-resolution form in registry.go:266. No silent auth.
Factory error handling: openaiCompatScheme returns (provider, nil) — consistent with every other scheme factory in the file (openai, anthropic, ollama all return nil error). No ignored error introduced; malformed DSNs are caught earlier by ParseDSN (env.go:59-77: missing ://, missing host both return explicit ErrInvalidDSN).
No panics on bad input: envKeyForProvider handles any string (including empty / hyphenated) via strings.ReplaceAll; no nil deref or index risk. openai.New never fails by contract (openai.go:112-113).

No unhandled edge cases in the unhappy paths this change introduces.

kimi-k2.6:cloud (ollama-cloud) — No material issues found

Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

No material issues found

The security-relevant changes are well-engineered:

Credential isolation is enforced by construction: registerOpenAICompatBuiltin passes WithAPIKey unconditionally (even when empty), which overrides openai.New's default OPENAI_API_KEY fallback. This prevents a missing QWEN_API_KEY from silently authenticating as OpenAI.
Missing-key hints name the correct variable: WithAPIKeyName(keyEnv) ensures synthetic 401s tell the operator to set QWEN_API_KEY, not OPENAI_API_KEY.
DSN factory isolation is correct: openaiCompatScheme uses WithAPIKey(dsn.Token) unconditionally and names the defining LLM_<NAME> env var via envKeyForProvider, so a keyless qwen:// DSN hints to add a token to that DSN — not to set QWEN_API_KEY.
Reverse leak is tested: TestQwenBuiltinKeyDoesNotLeakToOpenAI verifies that a registry with QWEN_API_KEY visible does not hand it to the openai built-in.
No secret exposure in errors: The synthetic 401 message only prints the name of the env var, never its value.
No new injection or SSRF surface: DSN hosts are operator-configured, and BaseURL() is hardcoded to https:// prefix; no untrusted input reaches URL construction.

🎯 Correctness — No material issues found

No material issues found

The change is clean and correct under the correctness lens.

registerOpenAICompatBuiltin unconditionally passes WithAPIKey (even when empty) and WithAPIKeyName(keyEnv), preventing the exact fallthrough-to-OPENAI_API_KEY bug it claims to prevent. Verified by reading builtin.go:84-92 and provider/openai/openai.go:114-118.
openaiCompatScheme uses envKeyForProvider(name) for the DSN missing-key hint, which is byte-identical to the lazy-resolution key form in providerFor. Verified by comparing builtin.go:66, env.go:52, and registry.go:266.
The shared envKeyForProvider deduplicates a transformation that previously existed in two places with only a comment asserting they matched. This is a genuine correctness improvement (elimination of a silent drift hazard).
The reverse-leak test (TestQwenBuiltinKeyDoesNotLeakToOpenAI) is correctly structured: t.Setenv("OPENAI_API_KEY", ...) seeds the openai client's direct os.Getenv call at construction time, while singleKeyEnv restricts the registry's envLookup to QWEN_API_KEY. This proves the openai built-in receives its own credential, not qwen's. Verified by tracing newTestRegistry (parse_test.go:14-21) and openai.New (provider/openai/openai.go:114-118).
The reasoning_effort wire assertion in TestQwenReasoningEffortReachesWire is correct: it unmarshals the captured request body and checks the top-level field, which is exactly what Model Studio's OpenAI-compatible surface requires. Verified by reading builtin_qwen_test.go:58-86.
captureRT now drains and closes r.Body per the RoundTripper contract. Verified by reading builtin_openaicompat_test.go:36-51.

Nothing in this diff introduces a logic bug, incorrect constant, conversion error, or semantic domain mistake.

🧹 Code cleanliness & maintainability — No material issues found

No material issues found

Minor issues

builtin.go:20-26 — In the provider-name const block, ProviderKimi = "kimi" lost the alignment spaces that ProviderQwen and ProviderAnthropic still have, so the three = signs are no longer in the same column. The original line had the alignment (visible in the diff); the PR removed it only for ProviderKimi. Since the rest of the block is hand-aligned, keep them consistent — either align all three or align none.

⚡ Performance — No material issues found

No material issues found

The changes in this PR are purely structural: adding a new qwen built-in provider, refactoring shared kimi/qwen registration logic into registerOpenAICompatBuiltin and openaiCompatScheme, centralizing env-key naming via envKeyForProvider, and adding hermetic tests. None of these touch any hot path:

No request-path allocations: Generate, Parse, and provider resolution follow the exact same code paths as before; the qwen provider is the existing openai client with a different base URL and API key.
No N+1 or unbounded behavior: registerOpenAICompatBuiltin and openaiCompatScheme run once at registry construction/DSN load time, not per-request.
No new blocking calls: The qwen provider reuses the same async HTTP client as all other OpenAI-compatible providers.
Test-only captureRT body capture: The io.ReadAll in captureRT.RoundTrip is test infrastructure; it does not affect production latency or memory.

🧯 Error handling & edge cases — No material issues found

Verdict: No material issues found

Reviewed the diff through the 🧯 Error handling & edge cases lens. Checked builtin.go, env.go, registry.go, builtin_openaicompat_test.go, and builtin_qwen_test.go.

Production code: All error paths are explicit. registerOpenAICompatBuiltin passes WithAPIKey unconditionally (including empty) to prevent silent fallback to OPENAI_API_KEY—this is a deliberate, well-documented guard. openaiCompatScheme and envKeyForProvider handle empty inputs safely (no panics). No deferred cleanup is needed in the added helpers.
captureRT.RoundTrip (test only): The io.ReadAll(r.Body) error is discarded with _. I verified this is test infrastructure and the body is always an in-memory buffer; even in the failure case, downstream json.Unmarshal or the reqBody == nil guard would cause the test to fail. Not material.
Edge cases (nil, empty, missing): Empty dsn.Token is an intentional signal for the missing-key synthetic 401. Nil request bodies in captureRT are guarded. Empty provider names would produce "LLM_", which is harmless. No integer arithmetic or boundary conditions were introduced.

Nothing in this diff mishandles errors or creates unhandled edge cases in production code.

_{Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.}

## 🪰 Gadfly review — consensus across 4 models **Verdict: Minor issues** · 3 findings (0 with multi-model agreement) <details><summary>3 single-model findings (lower confidence)</summary> | | Finding | Where | Model | Lens | |--|--|--|--|--| | 🟡 | ProviderQwen const keeps stale alignment padding while sibling ProviderKimi had its padding stripped in this same PR | `builtin.go:26` | glm-5.2:cloud | maintainability | | ⚪ | DSN.Scheme doc comment lists kimi but not the sibling qwen scheme added in this PR | `env.go:30` | claude-code/opus | maintainability | | ⚪ | envKeyForProvider doc-comment example inverts the function's direction (LLM_M1 → "m1" is name → var backwards) | `env.go:44` | glm-5.2:cloud | maintainability | </details> <details><summary>Per-model detail</summary> <details><summary>claude-code/opus (claude-code) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary>🔒 Security — No material issues found</summary> I've verified the credential-handling paths against the checked-out code. This is my security-lens review. ## VERDICT: No material issues found This PR adds the `qwen` built-in by refactoring the existing kimi credential-wiring into two shared helpers (`registerOpenAICompatBuiltin`, `openaiCompatScheme`) plus a centralized `envKeyForProvider`. Through the security lens, the credential-isolation properties are preserved and actually *hardened* (moved from copy-paste-with-a-comment to by-construction). Specifics I checked: - **No cross-provider key fallthrough.** `registerOpenAICompatBuiltin` passes `WithAPIKey(r.envLookup(keyEnv))` unconditionally (`builtin.go:88`). Verified in `provider/openai/openai.go:117` that `New` defaults `apiKey` to `os.Getenv("OPENAI_API_KEY")`; passing the option unconditionally (even when empty) overrides that default, so an unset `QWEN_API_KEY` cannot silently authenticate a qwen request with an OpenAI key. The empty key then surfaces as a synthetic 401 at request time (`provider/openai/model.go:76`), not a misdirected authenticated call. - **No secret leakage in error output.** The missing-key hint is `"...set " + m.p.apiKeyName + " or use WithAPIKey"` (`provider/openai/model.go:85`) — it names the *variable* (`QWEN_API_KEY` / `LLM_QCN`), never the key value. `WithAPIKeyName` carries only the name (`openai.go:72`). - **Reverse-leak direction covered.** `QWEN_API_KEY` is read only for the qwen provider; the openai built-in is constructed from its own default and never receives the qwen credential. This is directly asserted by `TestQwenBuiltinKeyDoesNotLeakToOpenAI` (with a real `OPENAI_API_KEY` set so the request actually egresses), which I read and consider a genuine security regression test. - **No SSRF / URL-injection surface introduced.** The qwen built-in base URL is a compile-time constant (`builtin.go:49`, `https://`). DSN hosts flow through `DSN.BaseURL()` which force-prefixes `https://` (`env.go:41`), preventing scheme downgrade; and DSN values are operator-supplied `LLM_*` config, not attacker-controlled request input, so they are out of the injection/taint model. - **`envKeyForProvider` (`env.go:52`)** only uppercases and replaces `-`→`_`; no format-string, path, or command construction. Its two consumers (lazy resolution `registry.go:266`, and the DSN missing-key hint) now share one definition, removing a silent-drift risk rather than adding one. No authn/authz gaps, injection, unsafe deserialization, or insecure defaults in this change's lane. The deleted kimi test file is fully subsumed by the new table-driven suite (same assertions), so no security coverage was dropped. </details> <details><summary>🎯 Correctness — No material issues found</summary> I've verified the change end-to-end. My findings through the **Correctness** lens: ## VERDICT: No material issues found This is a faithful extraction-plus-addition, and I confirmed each correctness-sensitive claim against the checked-out code: - **`envKeyForProvider` is behavior-preserving.** `env.go:52-54` produces `"LLM_" + ToUpper(ReplaceAll(name, "-", "_"))` — byte-identical to the inline expression it replaced at `registry.go:266` (old `providerFor`) and the old kimi scheme's `WithAPIKeyName`. The DSN-name path (`qcn` → `LLM_QCN`) matches the lazy-resolution key used in `providerFor`, so missing-key hints stay consistent. Verified by reading both call sites. - **The kimi refactor is equivalent.** `registerOpenAICompatBuiltin` (`builtin.go:84-92`) and `openaiCompatScheme` (`builtin.go:60-69`) reproduce the old kimi registration exactly: same `openaiOpts` wrap, `WithAPIKey(r.envLookup(keyEnv))` eager / `WithAPIKey(dsn.Token)` for the scheme, and the same `WithAPIKeyName`. No semantic drift from the consolidation. - **The load-bearing credential rule holds.** `WithAPIKey` (`provider/openai/openai.go:49-51`) sets `p.apiKey = key` unconditionally, so passing `WithAPIKey("")` overrides the `OPENAI_API_KEY` default (`openai.go:117`) with empty rather than falling through to it. That is what makes an unset `QWEN_API_KEY` fail closed with a synthetic 401 instead of authenticating as OpenAI. Verified the option is a plain assignment, not a conditional. - **Base URLs re-derived and correct.** International (Singapore) OpenAI-compat host `https://dashscope-intl.aliyuncs.com/compatible-mode/v1` and the China host `https://dashscope.aliyuncs.com/compatible-mode/v1` reached via `DSN.BaseURL()` (`https://` + host, `env.go:41`) both match Alibaba Model Studio's documented endpoints. No transposed intl/China hosts, no wrong path segment. - **No compile/resolution problems.** `strings` was removed from `builtin.go` imports because its last use moved to `env.go` (which already imports `strings`); the removal is correct. `r.providers`/`r.schemes`/`r.envLookup` all exist and are written the same way as the surrounding built-ins. The remaining substance of the PR (the OpenAI-vs-Anthropic-compat decision, `reasoning_effort` on the wire) is documentation and test-side, and the tests assert exactly the claims the ADR makes. Nothing in the correctness lane is wrong. </details> <details><summary>🧹 Code cleanliness & maintainability — Minor issues</summary> I've verified the finding against the actual code. The env.go doc comment (lines 28-30) does enumerate `"foreman", "ollama", "ollama-cloud", "openai", "kimi", "anthropic", "google"/"gemini"` — it names the sibling built-in `kimi` but not the `qwen` scheme this PR adds. The finding is accurate and remains a trivial, illustrative nit. All other draft observations were confirmations of correct code, not findings. The single trivial finding survives. ## VERDICT: Minor issues This is a clean, well-motivated refactor for my lens. The three extractions are the right calls and I verified each against the checked-out tree: - `envKeyForProvider` (env.go:52) genuinely dedupes two previously-copied `"LLM_"+strings.ToUpper(strings.ReplaceAll(...))` sites; `openaiCompatScheme` (env.go:66) now calls it, and `strings` is still imported/used in env.go. ✅ - The `strings` import removed from builtin.go is correct — the file no longer contains any `strings.` reference, so it still compiles. ✅ - `registerOpenAICompatBuiltin` / `openaiCompatScheme` collapse the byte-identical kimi/qwen scheme closures into one helper; `SchemeFactory` is the real type used for `r.schemes`, and the two credential rules (unconditional `WithAPIKey`, `WithAPIKeyName` naming `keyEnv`) are preserved by construction. Passing `wrap` as a parameter is necessary (the package-level helper can't close over the `openaiOpts` local). ✅ - Both kimi and qwen are now installed via the single `registerOpenAICompatBuiltin` call (builtin.go:147, builtin.go:152) with the correct base URLs and key env vars. ✅ One trivial nit: - **env.go:30** — the `DSN.Scheme` doc comment enumerates example schemes (`"foreman", "ollama", "ollama-cloud", "openai", "kimi", "anthropic", ...`) and now names `kimi` but not the sibling `qwen` added here. The list is already non-exhaustive (it also omits `llama-swap`/`llama-swaps`), so this is illustrative rather than authoritative — but given it explicitly calls out kimi, adding `qwen` beside it keeps the sibling built-ins in step with the project's "docs in sync in the same commit" convention. Purely optional; no behavioral impact. No duplicated logic, dead code, confusing names, or pattern deviations otherwise — the new built-in follows the kimi precedent exactly and actually reduces total surface area. </details> <details><summary>⚡ Performance — No material issues found</summary> I've verified the changed code. This PR is entirely provider-registration and DSN-scheme plumbing plus docs/tests. ## VERDICT: No material issues found Through the **⚡ Performance** lens, this change is clean: - **`registerOpenAICompatBuiltin` / `openaiCompatScheme` (builtin.go:60-92)** run only during `registerBuiltins`, i.e. once per registry construction at startup, not on any request path. Refactoring two byte-identical inline blocks into a shared factory changes structure, not the amount of work — the same `openai.New(...)` calls happen the same number of times. No new allocations on a hot path, no added indirection in `Generate`. - **`envKeyForProvider` (env.go:52-54)** is two string ops (`ToUpper`+`ReplaceAll`) invoked at lazy DSN resolution and DSN-scheme construction — both cold, low-frequency. Extracting it from the two former inline copies (registry.go:266, and the old kimi scheme) is net-neutral: same computation, called at the same rare sites. No per-request cost. - **`registry.go:266`** swaps an inline string build for the identical function call — no behavioral or cost change. - **`captureRT.RoundTrip` (builtin_openaicompat_test.go)** now does `io.ReadAll`/`Close` on the request body, but that's test-only code and correctly honors the RoundTripper body-ownership contract; irrelevant to production performance. - Everything else in the diff is Markdown docs (README, ADR-0027, progress.md, `.env.example`). There are no loops, no per-item queries, no unbounded growth, no blocking calls added to any hot path, and no quadratic behavior introduced. Nothing to flag in my lane. </details> <details><summary>🧯 Error handling & edge cases — No material issues found</summary> I've verified the production changes against the checked-out code. This is a behavior-preserving refactor (extracting `envKeyForProvider`, `openaiCompatScheme`, `registerOpenAICompatBuiltin`) plus a new built-in that reuses the existing openai client. Through the error-handling & edge-cases lens: ## VERDICT: No material issues found - **`openaiCompatScheme` / `registerOpenAICompatBuiltin` return path** (`builtin.go:60-92`): the factory does `return openai.New(...), nil`. I checked `provider/openai/openai.go:114` — `func New(opts ...Option) *Provider` returns no error, so nothing is being swallowed here. The dropped-error smell is a false alarm. - **`r.envLookup(keyEnv)` at construction** (`builtin.go:88`): could panic if `envLookup` were nil, but `New` sets `envLookup: os.Getenv` (registry.go:173) and only overrides it with a non-nil func, all before `registerBuiltins` runs (registry.go:187-190). Pre-existing invariant, unchanged by this PR. Safe. - **Keyless DSN unhappy path** (`builtin.go:65-66`): empty `dsn.Token` → `WithAPIKey("")` + `WithAPIKeyName(envKeyForProvider(name))`. The old kimi factory used the identical `"LLM_"+ToUpper(ReplaceAll(name,"-","_"))` expression now centralized in `envKeyForProvider` (env.go:52), so the missing-token 401-hint behavior is unchanged; the new table test asserts it names `LLM_QCN`. Verified the two are byte-identical. - **`registerOpenAICompatBuiltin` writes `r.providers`/`r.schemes` without the mutex** (`builtin.go:85-91`): matches every other line in `registerBuiltins`, which runs single-threaded during `New` before the registry is published. No new race. - **`envKeyForProvider` on empty name**: `New` never calls the scheme factory with an empty `name` (LoadEnv skips `LLM_` with nothing after it, env.go:90), and `ParseDSN` rejects empty host. No new panic surface. - **Test `captureRT.RoundTrip` ignores `io.ReadAll` error** (`builtin_openaicompat_test.go`): `c.reqBody, _ = io.ReadAll(...)`. Benign in a hermetic test against an in-memory body; not production code. No swallowed errors, missing cleanup, or unhandled edge cases introduced by this change. The refactor preserves the existing unhappy-path semantics exactly. </details> </details> <details><summary>claude-code/sonnet (claude-code) — No material issues found</summary> **Verdict: No material issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary>🔒 Security — No material issues found</summary> **VERDICT: No material issues found** This PR only adds a new OpenAI-compat built-in (`qwen`) by reusing the existing `provider/openai` client and refactors the identical kimi/qwen DSN-factory logic into shared helpers (`registerOpenAICompatBuiltin`, `openaiCompatScheme`, `envKeyForProvider`). I verified the security-relevant claims directly: - **Credential isolation is sound.** `registerOpenAICompatBuiltin` (builtin.go:84-92) passes `openai.WithAPIKey(r.envLookup(keyEnv))` *unconditionally*, and `provider/openai.New` (openai.go:112-128) only falls back to `os.Getenv("OPENAI_API_KEY")` when `WithAPIKey` is never called — so an unset `QWEN_API_KEY` cannot silently pick up `OPENAI_API_KEY`. Confirmed by reading both files directly, not just trusting the PR's stated tests. - **No secret leakage in error paths.** The missing-key error (`model.go:80-86`) interpolates only `m.p.apiKeyName` (the *variable name*, e.g. `"QWEN_API_KEY"`), never the key value. `envKeyForProvider` (env.go:52-54) similarly only derives an env-var *name* string. - **`envKeyForProvider` dedup is behavior-preserving.** Its new single definition (`env.go:52-54`) is byte-identical to the two call sites it replaces (`registry.go:266`, and the old inline kimi scheme), so no drift was introduced. - **DSN host handling is unchanged.** `DSN.BaseURL()` (env.go:41) and `ParseDSN` (env.go:59-77) are untouched by this diff; the `qwen://` scheme reuses the same host-from-DSN pattern kimi already had (operator-controlled env DSNs, not attacker-controlled input — no new SSRF surface). - No secrets appear in `.env.example`, ADR, README, or test fixtures — all placeholders (`sk-...`, `qwen-secret`, etc.). The one thing this PR *deliberately doesn't do* — route through Model Studio's Anthropic-compat shim — is a correctness/behavior choice (documented in ADR-0027), not a security regression; if anything it avoids a surface with weaker validation guarantees for `Request.Schema`. </details> <details><summary>🎯 Correctness — No material issues found</summary> ## Review — 🎯 Correctness **VERDICT: No material issues found** I traced the actual wiring, not just the diff hunks: - `envKeyForProvider` (env.go:52) is used identically by `registry.go:266` (lazy DSN resolution) and by `openaiCompatScheme` (builtin.go:66) — verified both call sites now share the one function, closing the "two copies that could drift" risk the PR calls out. No behavior change vs. the pre-refactor kimi scheme. - `registerOpenAICompatBuiltin` (builtin.go:84-92) writes `r.providers[name]` and `r.schemes[name]` with `WithAPIKey` passed unconditionally in both the eager built-in and the DSN scheme, so an unset `QWEN_API_KEY`/DSN token can't fall through to `openai.New`'s `OPENAI_API_KEY` default (confirmed `openai.New` defaults `apiKey: os.Getenv("OPENAI_API_KEY")` only when `WithAPIKey` isn't supplied — provider/openai/openai.go:117). - Verified `provider/openai/wire.go:25,204` already serializes `Request.ReasoningEffort` as top-level `reasoning_effort`, and `wire.go:131-148` already maps `prompt_tokens_details.cached_tokens` → `Usage.CacheReadTokens` — so the ADR's central claim (qwen inherits both for free by reusing the openai client) holds against the actual client code, not just the PR narrative. - `TestQwenBuiltinKeyDoesNotLeakToOpenAI` correctly uses `t.Setenv("OPENAI_API_KEY", ...)` rather than the registry's injected `envLookup`, because the `openai` built-in provider (unlike kimi/qwen) is constructed with no `WithAPIKey` option and so reads the real process env at construction — checked against `registerBuiltins` (builtin.go:131) and `openai.New` (openai.go:114-118). The test is exercising the right seam. - The table-driven test (`builtin_openaicompat_test.go`) computes `dsnName` from `tc.dsnVar` and constructs DSN scheme values as `tc.name + "://tok@" + tc.dsnHost`, which correctly registers provider `qcn` (from `LLM_QCN`) using scheme `qwen` — matches `r.schemes[ProviderQwen] = openaiCompatScheme(wrap)` in builtin.go:152. - No duplicate/shadowed symbols, no leftover `strings` import misuse (correctly dropped from builtin.go now that `envKeyForProvider` lives in env.go), no signature mismatches between `SchemeFactory` and the new `openaiCompatScheme` return type. I could not execute `go build`/`go test` in this sandbox (tool approval declined), so compilation was verified by manual symbol/signature tracing rather than the compiler — I'm confident in the read but flagging the method. Nothing in this diff introduces a logic bug, incorrect magic number, or wrong control flow within the correctness lens. </details> <details><summary>🧹 Code cleanliness & maintainability — No material issues found</summary> Nothing unusual there. Everything checks out — no cleanliness issues. ## Review **VERDICT: No material issues found** Reviewed through the code cleanliness / maintainability lens: - `builtin.go`: the new `openaiCompatScheme` + `registerOpenAICompatBuiltin` helpers cleanly remove the byte-identical kimi/qwen duplication that would otherwise have been copy-pasted a third time. Verified `openaiOpts` is still defined/used at `builtin.go:125` and both `registerOpenAICompatBuiltin` calls (`builtin.go:147`, `152`) pass it through correctly. The `strings` import drop is justified — the only user (`"LLM_"+strings.ToUpper(...)`) moved into the new shared `envKeyForProvider` in `env.go:52`, and `registry.go` still uses `strings` for its own `strings.Cut` call (`registry.go:195`), so no orphaned/broken imports. - `env.go:52` (`envKeyForProvider`) is a good dedup of the `providerFor` env-key derivation and the DSN scheme's key-hint logic — previously these were two independent literals that had to be kept in sync by convention only. - Test consolidation (`builtin_kimi_test.go` deleted, replaced by a table-driven `builtin_openaicompat_test.go` + a slim `builtin_qwen_test.go` for qwen-specific cases) removes near-duplicate test functions and keeps the shared contract in one place; `targetsOf`/`newTestRegistry` helpers it relies on already exist in `parse_test.go:14,24`, so nothing is left dangling. - The `ProviderQwen`/`ProviderAnthropic`/... const alignment looks odd at a glance (`ProviderQwen = "qwen"` next to a single-spaced `ProviderKimi = "kimi"`) but this is correct gofmt behavior — comment lines break contiguous alignment groups, so `ProviderKimi` (bracketed by comments) formats alone while the uninterrupted run from `ProviderQwen` through `ProviderLlamaSwap` aligns together. Not a real issue. Diff is well-factored with no dead code, no confusing names, and no copy-paste left behind after the refactor. </details> <details><summary>⚡ Performance — No material issues found</summary> **VERDICT: No material issues found** This PR is registration-time/administrative code (provider setup at `Registry` construction), documentation, and tests — there's no hot-path code introduced. Checked specifically for performance impact: - `registerOpenAICompatBuiltin` / `openaiCompatScheme` (`builtin.go:60-92`): called exactly twice (kimi, qwen) inside `registerBuiltins`, which itself runs once per `Registry` construction — not per-request. Verified by reading `registerBuiltins` in full; no loop or per-call invocation path. - `envKeyForProvider` (`env.go`): cheap `strings.ToUpper`/`ReplaceAll`. Verified its only production call sites are `openaiCompatScheme` (once per DSN-scheme *invocation*, i.e. once per new provider name resolved) and `registry.go:266` inside `providerFor`, which caches the resolved provider in `r.providers` before returning (`registry.go:276-283`) — so this runs at most once per distinct lazily-resolved provider name, not per request. - `captureRT.RoundTrip`'s new `io.ReadAll(r.Body)` (`builtin_openaicompat_test.go`) is test-only scaffolding, not production code. No N+1 patterns, no unbounded growth, no new allocations in a loop, no blocking calls added to a request hot path. Nothing in this lens warrants a finding. </details> <details><summary>🧯 Error handling & edge cases — No material issues found</summary> ## Review — 🧯 Error handling & edge cases lens **VERDICT: No material issues found** This is a mechanical refactor (extracting `registerOpenAICompatBuiltin`/`openaiCompatScheme`/`envKeyForProvider`) plus a straightforward new built-in that follows the existing kimi pattern byte-for-byte. I verified the actual checked-out code: - `builtin.go:60-92` — `openaiCompatScheme` and `registerOpenAICompatBuiltin` preserve the exact credential-handling invariants from the original kimi code (`WithAPIKey` always passed, even empty, so no fallthrough to `OPENAI_API_KEY`; `WithAPIKeyName` always set). Verified against `provider/openai/openai.go:112-128` — `openai.New` never fails and only defaults `apiKey` from `OPENAI_API_KEY` when `WithAPIKey` isn't supplied at all, which never happens here. - `registry.go:266` and `env.go:52-54` — `envKeyForProvider` is a pure, side-effect-free extraction of previously-inlined logic; behavior on empty/hyphenated names is unchanged. - `registerBuiltins` still writes to `r.providers`/`r.schemes` maps without holding `r.mu` (`builtin.go:85-91`), but this is pre-existing behavior (same as the code it replaced) — registration happens before the registry is exposed, so no new concurrency edge case was introduced. - The scheme factories (old and new) always return a `nil` error from `openai.New`, matching the pre-existing kimi scheme — no regression in how a hypothetical future construction error would propagate. One minor, non-blocking observation in new test scaffolding: - `builtin_openaicompat_test.go:32-33` — `captureRT.RoundTrip` swallows the error from `io.ReadAll(r.Body)` (`c.reqBody, _ = io.ReadAll(r.Body)`) and the error from `r.Body.Close()`. In practice the bodies here are always in-memory JSON buffers from the openai client, so this can't realistically fail, and a failure would still surface indirectly (nil `reqBody` → `json.Unmarshal` fails in `TestQwenReasoningEffortReachesWire` with a clear `t.Fatalf`). Not worth blocking on — flagging only because it's a literally swallowed error in code newly added by this PR. No missing cleanup/defer, no nil/empty-collection panics, and no off-by-one issues were found in the production code touched by this diff. </details> </details> <details><summary>glm-5.2:cloud (ollama-cloud) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary>🔒 Security — No material issues found</summary> Verdict: No material issues found Through the security lens, I traced the credential flow and validated the load-bearing claims against the actual code: - **Credential isolation / fail-closed on missing key** (builtin.go:84-92, provider/openai/openai.go:114-128): `registerOpenAICompatBuiltin` passes `WithAPIKey(r.envLookup(keyEnv))` unconditionally, even when the lookup is empty. `WithAPIKey("")` sets `p.apiKey = ""`, which overrides `New`'s `os.Getenv("OPENAI_API_KEY")` default. So an unset `QWEN_API_KEY` cannot silently authenticate the qwen built-in as OpenAI. Verified by reading `openai.go:49-51,114-127`. - **Reverse-leak direction** (builtin_qwen_test.go): the openai built-in is constructed with no `WithAPIKey`, so it reads `os.Getenv("OPENAI_API_KEY")`; qwen's key is injected only into the qwen provider via `WithAPIKey`. The two never share an env-var source, so `QWEN_API_KEY` cannot reach the openai client. The test sets a real `OPENAI_API_KEY` to make the assertion non-vacuous — correct design. - **DSN credential path** (builtin.go:60-69, env.go:41): `openaiCompatScheme` uses `dsn.Token` as the bearer credential and `envKeyForProvider(name)` for the missing-key hint; a keyless DSN yields `WithAPIKey("")` → fails closed, not falls through. Base URL is forced to `https://`+host. No injection surface (operator-controlled env, same as kimi/openai). - **`envKeyForProvider` consolidation** (env.go:52-54, registry.go:266): the lazy-resolution key and the missing-key hint now share one definition, so a keyless DSN names the exact variable the operator must set — no drift that could misdirect credential setup. No authn/authz gaps, credential leakage, injection, or insecure defaults found in this change. </details> <details><summary>🎯 Correctness — No material issues found</summary> Verdict: No material issues found I verified the correctness-critical pieces of this change against the checked-out code: - **`envKeyForProvider` consolidation** (env.go:52, registry.go:266): the new helper is byte-identical to the two prior call sites — `LLM_` + uppercase with `-`→`_`. The old inline form in `providerFor` and the old kimi DSN factory both produced the same string; the refactor preserves behavior. Verified by reading both call sites. - **`registerOpenAICompatBuiltin` / `openaiCompatScheme` parity** (builtin.go:60-92): the eager path (`WithAPIKey(r.envLookup(keyEnv))`, `WithAPIKeyName(keyEnv)`) and the DSN path (`WithAPIKey(dsn.Token)`, `WithAPIKeyName(envKeyForProvider(name))`) reproduce exactly what the deleted kimi code did — confirmed against `provider/openai/openai.go:49-73` (`WithAPIKey`/`WithAPIKeyName` semantics) and `provider/openai/model.go:76-86` (the synthetic-401 names `apiKeyName`). The "pass empty key unconditionally so it can't fall through to OPENAI_API_KEY default" rule holds, since `openai.New` only falls back to `os.Getenv("OPENAI_API_KEY")` when no `WithAPIKey` option is supplied (openai.go:114-128). - **`reasoning_effort` on the wire** (provider/openai/wire.go:204,25): `buildRequest` copies `req.ReasoningEffort` into `chatRequest.ReasoningEffort` with json tag `reasoning_effort,omitempty`; the test asserting `sent["reasoning_effort"]=="high"` is a real wire assertion, not a tautology. - **Reverse-leak test** (builtin_qwen_test.go): the openai built-in is constructed with no `WithAPIKey`, so its key comes from `os.Getenv("OPENAI_API_KEY")` (openai.go:117), set via `t.Setenv`. The registry `envLookup` returns `qwen-secret` only for `QWEN_API_KEY` and `""` otherwise, so the qwen credential cannot reach the openai built-in. The assertion is non-vacuous (a real request goes out). Verified the construction path. - **DSN round-trip URLs**: `DSN.BaseURL()` (env.go:41) is `"https://"+Host`, and `do` appends `/chat/completions` (model.go:92). For `qwen://[email protected]/compatible-mode/v1` → `https://dashscope.aliyuncs.com/compatible-mode/v1/chat/completions`, matching `wantDSNURL`. Confirmed. No logic bugs, semantic errors, or incorrect constants in my lens. </details> <details><summary>🧹 Code cleanliness & maintainability — Minor issues</summary> Both findings verified against the actual source: - `builtin.go:20` is `ProviderKimi = "kimi"` (padding stripped), while `builtin.go:26` is `ProviderQwen = "qwen"` (padding retained) — inconsistent spacing within the same const block. Confirmed. - `env.go:44` reads `// name (LLM_M1 → "m1", so "my-prov" → LLM_MY_PROV).` while the function (line 52) returns the LLM_ variable for a given name, so the first arrow `LLM_M1 → "m1"` inverts the actual direction. Confirmed. **Verdict: Minor issues** - **`builtin.go:26` — stale alignment padding in the const block.** The PR stripped the alignment whitespace from `ProviderKimi` (`ProviderKimi = "kimi"` at line 20) but left it on the newly-added `ProviderQwen` (`ProviderQwen = "qwen"` at line 26). The two sibling constants in the same block now use inconsistent spacing (gofmt accepts both because the intervening doc comments split alignment runs, so this is cosmetic only). Fix: write `ProviderQwen = "qwen"` to match the `ProviderKimi` form this PR just established. - **`env.go:44` — doc-comment example reads backwards.** `envKeyForProvider` returns the LLM_ variable for a given name (line 52), but the comment's first half `LLM_M1 → "m1"` shows the inverse direction, undercutting the "single definition on purpose" point the comment is making. Suggested: `// name → variable, e.g. "m1" → "LLM_M1", "my-prov" → "LLM_MY_PROV"`. </details> <details><summary>⚡ Performance — No material issues found</summary> No material issues found - Reviewed through the performance lens: the change is additive wiring that reuses the existing `provider/openai` client. `openai.New` is called once per built-in at registration (`registerOpenAICompatBuiltin`) and once per DSN provider on first lazy resolution in `providerFor` (registry.go:266-283), with the result cached in `r.providers` afterward — no per-request allocation or repeated construction on hot paths. `openaiCompatScheme` is a tiny closure with no per-call work beyond constructing options. The test `captureRT.RoundTrip` now drains/closes the request body; that's test-only and the body is a single small JSON request, not a concern. No N+1, unbounded growth, blocking calls, or quadratic behavior introduced. - Verified `providerFor` caching (registry.go:255-283) and `openai.New` cost (provider/openai/openai.go:114-128) to confirm no repeated provider construction on the request path. </details> <details><summary>🧯 Error handling & edge cases — No material issues found</summary> Verified the full error-handling chain. The diff is clean through my lens. **Verdict: No material issues found** - **Missing-key path (eager built-in):** `registerOpenAICompatBuiltin` passes `WithAPIKey(r.envLookup(keyEnv))` unconditionally. Verified at `provider/openai/model.go:76-87`: an empty `apiKey` short-circuits to a synthetic `*llm.APIError` (401, `missing_api_key`) naming `apiKeyName`, and never reaches `client.Do`. So an unset `QWEN_API_KEY` fails closed and names `QWEN_API_KEY` rather than falling through to `openai.New`'s `OPENAI_API_KEY` default (`openai.go:117`). No network hit, error not swallowed. - **Keyless DSN path:** `openaiCompatScheme` passes `WithAPIKey(dsn.Token)`. An empty `dsn.Token` (e.g. `qwen://host`) triggers the same synthetic-401 path, now naming `envKeyForProvider(name)` = `LLM_<NAME>`. Verified the env-key derivation at `env.go:52-54` matches the lazy-resolution form in `registry.go:266`. No silent auth. - **Factory error handling:** `openaiCompatScheme` returns `(provider, nil)` — consistent with every other scheme factory in the file (openai, anthropic, ollama all return `nil` error). No ignored error introduced; malformed DSNs are caught earlier by `ParseDSN` (`env.go:59-77`: missing `://`, missing host both return explicit `ErrInvalidDSN`). - **No panics on bad input:** `envKeyForProvider` handles any string (including empty / hyphenated) via `strings.ReplaceAll`; no nil deref or index risk. `openai.New` never fails by contract (`openai.go:112-113`). No unhandled edge cases in the unhappy paths this change introduces. </details> </details> <details><summary>kimi-k2.6:cloud (ollama-cloud) — No material issues found</summary> **Verdict: No material issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary>🔒 Security — No material issues found</summary> No material issues found The security-relevant changes are well-engineered: - **Credential isolation is enforced by construction**: `registerOpenAICompatBuiltin` passes `WithAPIKey` unconditionally (even when empty), which overrides `openai.New`'s default `OPENAI_API_KEY` fallback. This prevents a missing `QWEN_API_KEY` from silently authenticating as OpenAI. - **Missing-key hints name the correct variable**: `WithAPIKeyName(keyEnv)` ensures synthetic 401s tell the operator to set `QWEN_API_KEY`, not `OPENAI_API_KEY`. - **DSN factory isolation is correct**: `openaiCompatScheme` uses `WithAPIKey(dsn.Token)` unconditionally and names the defining `LLM_<NAME>` env var via `envKeyForProvider`, so a keyless `qwen://` DSN hints to add a token to that DSN — not to set `QWEN_API_KEY`. - **Reverse leak is tested**: `TestQwenBuiltinKeyDoesNotLeakToOpenAI` verifies that a registry with `QWEN_API_KEY` visible does not hand it to the `openai` built-in. - **No secret exposure in errors**: The synthetic 401 message only prints the *name* of the env var, never its value. - **No new injection or SSRF surface**: DSN hosts are operator-configured, and `BaseURL()` is hardcoded to `https://` prefix; no untrusted input reaches URL construction. </details> <details><summary>🎯 Correctness — No material issues found</summary> **No material issues found** The change is clean and correct under the correctness lens. - `registerOpenAICompatBuiltin` unconditionally passes `WithAPIKey` (even when empty) and `WithAPIKeyName(keyEnv)`, preventing the exact fallthrough-to-`OPENAI_API_KEY` bug it claims to prevent. Verified by reading `builtin.go:84-92` and `provider/openai/openai.go:114-118`. - `openaiCompatScheme` uses `envKeyForProvider(name)` for the DSN missing-key hint, which is byte-identical to the lazy-resolution key form in `providerFor`. Verified by comparing `builtin.go:66`, `env.go:52`, and `registry.go:266`. - The shared `envKeyForProvider` deduplicates a transformation that previously existed in two places with only a comment asserting they matched. This is a genuine correctness improvement (elimination of a silent drift hazard). - The reverse-leak test (`TestQwenBuiltinKeyDoesNotLeakToOpenAI`) is correctly structured: `t.Setenv("OPENAI_API_KEY", ...)` seeds the openai client's direct `os.Getenv` call at construction time, while `singleKeyEnv` restricts the registry's `envLookup` to `QWEN_API_KEY`. This proves the openai built-in receives its own credential, not qwen's. Verified by tracing `newTestRegistry` (`parse_test.go:14-21`) and `openai.New` (`provider/openai/openai.go:114-118`). - The `reasoning_effort` wire assertion in `TestQwenReasoningEffortReachesWire` is correct: it unmarshals the captured request body and checks the top-level field, which is exactly what Model Studio's OpenAI-compatible surface requires. Verified by reading `builtin_qwen_test.go:58-86`. - `captureRT` now drains and closes `r.Body` per the `RoundTripper` contract. Verified by reading `builtin_openaicompat_test.go:36-51`. Nothing in this diff introduces a logic bug, incorrect constant, conversion error, or semantic domain mistake. </details> <details><summary>🧹 Code cleanliness & maintainability — No material issues found</summary> No material issues found Minor issues - `builtin.go:20-26` — In the provider-name const block, `ProviderKimi = "kimi"` lost the alignment spaces that `ProviderQwen` and `ProviderAnthropic` still have, so the three `=` signs are no longer in the same column. The original line *had* the alignment (visible in the diff); the PR removed it only for `ProviderKimi`. Since the rest of the block is hand-aligned, keep them consistent — either align all three or align none. </details> <details><summary>⚡ Performance — No material issues found</summary> No material issues found The changes in this PR are purely structural: adding a new `qwen` built-in provider, refactoring shared `kimi`/`qwen` registration logic into `registerOpenAICompatBuiltin` and `openaiCompatScheme`, centralizing env-key naming via `envKeyForProvider`, and adding hermetic tests. None of these touch any hot path: - **No request-path allocations**: `Generate`, `Parse`, and provider resolution follow the exact same code paths as before; the `qwen` provider is the existing `openai` client with a different base URL and API key. - **No N+1 or unbounded behavior**: `registerOpenAICompatBuiltin` and `openaiCompatScheme` run once at registry construction/DSN load time, not per-request. - **No new blocking calls**: The qwen provider reuses the same async HTTP client as all other OpenAI-compatible providers. - **Test-only `captureRT` body capture**: The `io.ReadAll` in `captureRT.RoundTrip` is test infrastructure; it does not affect production latency or memory. </details> <details><summary>🧯 Error handling & edge cases — No material issues found</summary> **Verdict: No material issues found** Reviewed the diff through the 🧯 Error handling & edge cases lens. Checked `builtin.go`, `env.go`, `registry.go`, `builtin_openaicompat_test.go`, and `builtin_qwen_test.go`. - **Production code:** All error paths are explicit. `registerOpenAICompatBuiltin` passes `WithAPIKey` unconditionally (including empty) to prevent silent fallback to `OPENAI_API_KEY`—this is a deliberate, well-documented guard. `openaiCompatScheme` and `envKeyForProvider` handle empty inputs safely (no panics). No deferred cleanup is needed in the added helpers. - **`captureRT.RoundTrip` (test only):** The `io.ReadAll(r.Body)` error is discarded with `_`. I verified this is test infrastructure and the body is always an in-memory buffer; even in the failure case, downstream `json.Unmarshal` or the `reqBody == nil` guard would cause the test to fail. Not material. - **Edge cases (nil, empty, missing):** Empty `dsn.Token` is an intentional signal for the missing-key synthetic 401. Nil request bodies in `captureRT` are guarded. Empty provider names would produce `"LLM_"`, which is harmless. No integer arithmetic or boundary conditions were introduced. Nothing in this diff mishandles errors or creates unhandled edge cases in production code. </details> </details> </details> Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.

steve added 1 commit 2026-08-12 20:17:20 +00:00

refactor(test): gadfly round 1 — share the OpenAI-compat test fixtures

CI / Build & Test (pull_request) Successful in 9m50s

Details

CI / Tidy (pull_request) Successful in 9m25s

Details

31d6b59356

Both findings were the same one, and both were fair: the PR that retires two
byte-identical DSN factories into openaiCompatScheme then copy-pasted the test
fixtures. qwenResponse was byte-identical to kimiResponse, and the single-key
env-lookup closure appeared three times in the new file (plus a fourth in the
kimi file, which neither reviewer was looking at).

Fixed for the class rather than for qwen: captureRT, the canned Chat
Completions body (now chatCompletionOK), and a new singleKeyEnv helper move to
builtin_openaicompat_test.go, owned by no single provider. The kimi tests adopt
them too, so the next OpenAI-compat built-in has nothing left to copy — the
same argument the production helper makes.

Also aligned the test model ids to the current Model Studio names
(qwen3.8-max / qwen3.7-plus), which the docs already cited. One reviewer called
those ids fictional and named the 2025 ones instead; they shipped 2026-08-03
and 2026-05-21 respectively, so that finding is stale model knowledge, not a
defect — but having tests and prose name the same models removes the smell that
prompted it. A dotted id also now proves it passes through verbatim.

Break-checked again after the refactor: all six mutations still fail their test.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

steve commented

2026-08-12 20:17:30 +00:00

Round 1: both findings were real, and both were the same finding — the PR argued for retiring byte-identical production code into one helper, then copy-pasted the test fixtures. Fixed for the class rather than for qwen (31d6b59):

captureRT, the canned Chat Completions body (now chatCompletionOK), and a new singleKeyEnv helper moved to builtin_openaicompat_test.go, owned by no single provider. The kimi tests adopt them too — sonnet counted 3 copies of the env-lookup closure in the new file and was right, but there was a 4th in builtin_kimi_test.go, outside the diff.

One finding I'm not taking, with reasons:

glm-5.2 (correctness lens, flagged trivial/doc-only) called qwen3.8-max and qwen3.7-plus "not real Alibaba Model Studio model ids" and proposed the 2025 names instead. They are real: Qwen3.8-Max shipped 2026-08-03 and Qwen3.7-Plus 2026-05-21, both GA on Model Studio. This is stale training knowledge asserted against current docs — the same failure mode ADR-0059 covers in mort. I did align the test ids to those names anyway, since the docs already used them and having the two disagree is what drew the eye; a dotted id now also proves ids pass through verbatim.

Break-checks re-run after the refactor — all six mutations still fail their named test. Gates green.

@gadfly review

Round 1: both findings were real, and both were the same finding — the PR argued for retiring byte-identical production code into one helper, then copy-pasted the test fixtures. Fixed for the class rather than for qwen (31d6b59): - `captureRT`, the canned Chat Completions body (now `chatCompletionOK`), and a new `singleKeyEnv` helper moved to `builtin_openaicompat_test.go`, owned by no single provider. The **kimi** tests adopt them too — sonnet counted 3 copies of the env-lookup closure in the new file and was right, but there was a 4th in `builtin_kimi_test.go`, outside the diff. One finding I'm not taking, with reasons: - glm-5.2 (correctness lens, flagged trivial/doc-only) called `qwen3.8-max` and `qwen3.7-plus` "not real Alibaba Model Studio model ids" and proposed the 2025 names instead. They are real: Qwen3.8-Max shipped 2026-08-03 and Qwen3.7-Plus 2026-05-21, both GA on Model Studio. This is stale training knowledge asserted against current docs — the same failure mode ADR-0059 covers in mort. I did align the *test* ids to those names anyway, since the docs already used them and having the two disagree is what drew the eye; a dotted id now also proves ids pass through verbatim. Break-checks re-run after the refactor — all six mutations still fail their named test. Gates green. @gadfly review

👀 1

steve added 1 commit 2026-08-12 20:25:40 +00:00

refactor: gadfly round 2 — both halves of an OpenAI-compat built-in register together

CI / Build & Test (pull_request) Successful in 9m50s

Details

CI / Tidy (pull_request) Successful in 9m25s

Details

f1f2b653c3

Same class of finding as round 1, one level in: I factored the DSN-scheme half
of the kimi/qwen duplication into openaiCompatScheme and left the eager
provider half copy-pasted, so a third built-in still had six lines to clone —
including both credential rules, which is exactly the pair you do not want
re-typed.

registerOpenAICompatBuiltin now installs both halves from one call. The rules
that matter hold by construction for every future caller: WithAPIKey passed
unconditionally (an unset key must not fall through to OPENAI_API_KEY), and
WithAPIKeyName naming that same variable in the 401 hint. Registering kimi and
qwen is now one line each.

Also fixed a cross-reference the ADR got wrong: Qwen's image-input caveat is
README matrix footnote ⁴, not ³ — ³ is kimi's. I wrote "³, shared with kimi"
in the ADR and then gave Qwen its own footnote in the README.

The break-check harness needed fixing before any of this could be trusted:
three of its mutations targeted lines this refactor moved, so they matched
nothing, the code was never broken, and the suite reported "test still passed"
— identical output to a test that genuinely misses the bug. Mutations are now
verified to have landed (sha before/after) and the suite fails loudly if one
doesn't. Two new cases cover the helper: dropping the unconditional WithAPIKey,
and dropping the scheme-half registration. 8/8 apply and are caught.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

steve commented

2026-08-12 20:26:06 +00:00

Round 2: both findings real, both fixed in f1f2b65.

sonnet, builtin.go:144 — the sharpest finding of the review, and the same class as round 1 one level in: I factored the DSN-scheme half of the kimi/qwen duplication and left the eager provider half copy-pasted, so a third built-in still had six lines to clone — including both credential rules, which are exactly what you don't want re-typed. registerOpenAICompatBuiltin now installs both halves; kimi and qwen are one line each, and "WithAPIKey unconditionally / WithAPIKeyName names that variable" holds by construction for whoever comes next.
opus, ADR line 90 — real drift I introduced: the ADR said footnote ³ "shared with kimi", then the README gave Qwen its own ⁴. Corrected.

Worth recording: fixing the first finding broke my break-check harness, and it failed silently. Three of its mutations targeted lines this refactor moved, so they matched nothing — the code was never broken, the tests passed for the right reason, and the suite printed "test still passed", which is byte-identical to what a genuinely blind test looks like. I nearly read that as three tests going bad. Mutations are now verified to have landed (sha before/after) and the suite aborts loudly if one doesn't. Two new cases cover the new helper (dropping the unconditional WithAPIKey; dropping the scheme-half registration): 8/8 apply and are caught.

Gates green.

@gadfly review

Round 2: both findings real, both fixed in f1f2b65. - **sonnet, `builtin.go:144`** — the sharpest finding of the review, and the same class as round 1 one level in: I factored the *DSN-scheme* half of the kimi/qwen duplication and left the *eager provider* half copy-pasted, so a third built-in still had six lines to clone — including both credential rules, which are exactly what you don't want re-typed. `registerOpenAICompatBuiltin` now installs both halves; kimi and qwen are one line each, and "WithAPIKey unconditionally / WithAPIKeyName names that variable" holds by construction for whoever comes next. - **opus, ADR line 90** — real drift I introduced: the ADR said footnote ³ "shared with kimi", then the README gave Qwen its own ⁴. Corrected. Worth recording: fixing the first finding **broke my break-check harness**, and it failed silently. Three of its mutations targeted lines this refactor moved, so they matched nothing — the code was never broken, the tests passed for the right reason, and the suite printed "test still passed", which is byte-identical to what a genuinely blind test looks like. I nearly read that as three tests going bad. Mutations are now verified to have landed (sha before/after) and the suite aborts loudly if one doesn't. Two new cases cover the new helper (dropping the unconditional `WithAPIKey`; dropping the scheme-half registration): 8/8 apply and are caught. Gates green. @gadfly review

👀 1

steve added 1 commit 2026-08-12 20:41:00 +00:00

refactor: gadfly round 3 — one table owns the OpenAI-compat contract

CI / Build & Test (pull_request) Successful in 10m23s

Details

CI / Tidy (pull_request) Successful in 9m21s

Details

8670ed22be

Three findings, and the first two are the same recurring shape.

envKeyForProvider (env.go) is now the single definition of the LLM_<NAME>
form. It lived in two places — lazy resolution in registry.go and the
missing-key hint in openaiCompatScheme — with a comment on the second asserting
it matched the first. A comment is not enforcement: if either had drifted, a
keyless DSN target would have named a variable that does nothing, and nothing
would have failed.

The kimi and qwen test files had become near-identical, which is round 1's
finding at the level above it: I deduped the fixtures, then left two parallel
suites asserting the same four things. They are now ONE table in
builtin_openaicompat_test.go — endpoint + credential, missing key fails closed
naming its own variable and never reaching the network, the name:// DSN
reaching another host, and a keyless DSN naming LLM_<NAME> instead of the
built-in's key. Adding an OpenAI-compat built-in is a table row that
immediately owes all four; builtin_kimi_test.go is deleted because the table
covers it. Only genuinely qwen-specific tests remain in the qwen file: the
reverse credential leak and the reasoning_effort wire claim ADR-0027 rests on.

Also trimmed ProviderQwen's doc comment, which restated the ADR-0027 rationale
already given at the registration site.

The break-check suite caught its own rot again — two mutations went stale when
these tests were renamed, and the landed-check reported them loudly instead of
passing them off as green. Now 9 cases, including one that drifts
envKeyForProvider to prove the shared helper is load-bearing. 9/9 apply and are
caught.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

steve commented

2026-08-12 20:44:00 +00:00

Round 3: three of four real, fixed in 8670ed2.

kimi-k2.6, builtin.go:70 — the sharpest of the round. The LLM_<NAME> expression was byte-identical in registry.go's lazy resolution and in openaiCompatScheme's missing-key hint, and my comment on the second asserted it "matches the lazy-resolution key form in providerFor". That is a comment claiming a coupling nothing enforced: had either drifted, a keyless DSN target would name a variable that does nothing and no test would notice. Now one envKeyForProvider in env.go, with a break-check case that drifts it to prove the hint really depends on it.
sonnet, builtin_qwen_test.go:17 — round 1's finding one level up. I deduped the fixtures and left two parallel suites asserting the same four things about different providers. The shared contract is now one table: endpoint + credential, missing key fails closed naming its own variable with no network, name:// DSN reaching another host, keyless DSN naming LLM_<NAME>. builtin_kimi_test.go is deleted because the table covers it; a new OpenAI-compat built-in is one row that immediately owes all four. Only genuinely qwen-specific tests remain in the qwen file (reverse credential leak, the reasoning_effort wire claim).
sonnet, builtin.go:22 — right, I wrote the ADR-0027 rationale on the const and at the registration site. Trimmed to a pointer.

Not taking one:

kimi-k2.6, captureRT ignores the io.ReadAll error. It reads an in-memory body the openai client just marshalled; that read cannot fail, and if it did, the only consumer (the wire assertion) fails loudly on an empty body rather than passing. Three other reviewers looked at the same line and agreed. Graded a defensible false positive, not a careless one.

The break-check suite caught its own rot a second time — two mutations went stale when the tests were renamed, and the landed-check reported that loudly instead of scoring them green. 9 cases now, 9/9 apply and are caught.

Gates green, go mod tidy clean.

@gadfly review

Round 3: three of four real, fixed in 8670ed2. - **kimi-k2.6, `builtin.go:70`** — the sharpest of the round. The `LLM_<NAME>` expression was byte-identical in `registry.go`'s lazy resolution and in `openaiCompatScheme`'s missing-key hint, and my comment on the second *asserted* it "matches the lazy-resolution key form in providerFor". That is a comment claiming a coupling nothing enforced: had either drifted, a keyless DSN target would name a variable that does nothing and no test would notice. Now one `envKeyForProvider` in `env.go`, with a break-check case that drifts it to prove the hint really depends on it. - **sonnet, `builtin_qwen_test.go:17`** — round 1's finding one level up. I deduped the fixtures and left two parallel suites asserting the same four things about different providers. The shared contract is now **one table**: endpoint + credential, missing key fails closed naming its own variable with no network, `name://` DSN reaching another host, keyless DSN naming `LLM_<NAME>`. `builtin_kimi_test.go` is deleted because the table covers it; a new OpenAI-compat built-in is one row that immediately owes all four. Only genuinely qwen-specific tests remain in the qwen file (reverse credential leak, the `reasoning_effort` wire claim). - **sonnet, `builtin.go:22`** — right, I wrote the ADR-0027 rationale on the const *and* at the registration site. Trimmed to a pointer. Not taking one: - **kimi-k2.6, `captureRT` ignores the `io.ReadAll` error.** It reads an in-memory body the openai client just marshalled; that read cannot fail, and if it did, the only consumer (the wire assertion) fails loudly on an empty body rather than passing. Three other reviewers looked at the same line and agreed. Graded a defensible false positive, not a careless one. The break-check suite caught its own rot a second time — two mutations went stale when the tests were renamed, and the landed-check reported that loudly instead of scoring them green. 9 cases now, 9/9 apply and are caught. Gates green, `go mod tidy` clean. @gadfly review

👀 1

gitea-actions bot reviewed 2026-08-12 20:46:54 +00:00

gitea-actions bot left a comment

🪰 Gadfly consensus review — 2 inline findings on changed lines. See the consensus comment for the full ranked summary.

_{Advisory only — does not block merge.}

🪰 **Gadfly consensus review** — 2 inline findings on changed lines. See the consensus comment for the full ranked summary. Advisory only — does not block merge.

builtin.go

						
				@@ -22,0 +23,4 @@

					// (like kimi and llama-swap); keyed by QWEN_API_KEY, default base URL

					// qwenBaseURL. ADR-0027 records why the OpenAI surface and not the

					// Anthropic-compatible one Model Studio also exposes.

					ProviderQwen        = "qwen"

gitea-actions bot commented

2026-08-12 20:46:54 +00:00

🟡 ProviderQwen const keeps stale alignment padding while sibling ProviderKimi had its padding stripped in this same PR

maintainability · flagged by 1 model

builtin.go:20 is ProviderKimi = "kimi" (padding stripped), while builtin.go:26 is ProviderQwen = "qwen" (padding retained) — inconsistent spacing within the same const block. Confirmed. - env.go:44 reads // name (LLM_M1 → "m1", so "my-prov" → LLM_MY_PROV). while the function (line 52) returns the LLM_ variable for a given name, so the first arrow LLM_M1 → "m1" inverts the actual direction. Confirmed.

_{🪰 Gadfly · advisory}

🟡 **ProviderQwen const keeps stale alignment padding while sibling ProviderKimi had its padding stripped in this same PR** _maintainability · flagged by 1 model_ - `builtin.go:20` is `ProviderKimi = "kimi"` (padding stripped), while `builtin.go:26` is `ProviderQwen = "qwen"` (padding retained) — inconsistent spacing within the same const block. Confirmed. - `env.go:44` reads `// name (LLM_M1 → "m1", so "my-prov" → LLM_MY_PROV).` while the function (line 52) returns the LLM_ variable for a given name, so the first arrow `LLM_M1 → "m1"` inverts the actual direction. Confirmed. 🪰 Gadfly · advisory

env.go

						
				@@ -41,2 +41,4 @@

				func (d DSN) BaseURL() string { return "https://" + d.Host }

				// envKeyForProvider returns the LLM_* variable that defines the provider named

				// name (LLM_M1 → "m1", so "my-prov" → LLM_MY_PROV).

gitea-actions bot commented

2026-08-12 20:46:54 +00:00

⚪ envKeyForProvider doc-comment example inverts the function's direction (LLM_M1 → "m1" is name → var backwards)

maintainability · flagged by 1 model

builtin.go:20 is ProviderKimi = "kimi" (padding stripped), while builtin.go:26 is ProviderQwen = "qwen" (padding retained) — inconsistent spacing within the same const block. Confirmed. - env.go:44 reads // name (LLM_M1 → "m1", so "my-prov" → LLM_MY_PROV). while the function (line 52) returns the LLM_ variable for a given name, so the first arrow LLM_M1 → "m1" inverts the actual direction. Confirmed.

_{🪰 Gadfly · advisory}

⚪ **envKeyForProvider doc-comment example inverts the function's direction (LLM_M1 → "m1" is name → var backwards)** _maintainability · flagged by 1 model_ - `builtin.go:20` is `ProviderKimi = "kimi"` (padding stripped), while `builtin.go:26` is `ProviderQwen = "qwen"` (padding retained) — inconsistent spacing within the same const block. Confirmed. - `env.go:44` reads `// name (LLM_M1 → "m1", so "my-prov" → LLM_MY_PROV).` while the function (line 52) returns the LLM_ variable for a given name, so the first arrow `LLM_M1 → "m1"` inverts the actual direction. Confirmed. 🪰 Gadfly · advisory

steve added 1 commit 2026-08-12 20:53:07 +00:00

docs: gadfly round 4 — two doc-comment fixes

CI / Build & Test (pull_request) Successful in 10m17s

Details

CI / Tidy (pull_request) Successful in 9m35s

Details

0760cf96d4

DSN.Scheme's list named kimi but not qwen — the same sibling drift this PR
keeps finding, in a doc comment this time (I updated the README's scheme list
and not this one). Added qwen, and llama-swap/llama-swaps while there, since
they were missing too.

envKeyForProvider's example ran backwards: "LLM_M1 → m1" describes registry
naming, not what the function does. Now reads name → variable in one
direction.

Not taking the third: the const block's alignment is gofmt's own output
(gofmt -l is empty), and the uneven padding is forced by the doc comments that
split the block into alignment groups. glm-5.2 reached that same conclusion in
round 2 before flagging it here.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

steve commented

2026-08-12 20:53:36 +00:00

Round 4: two of three real, fixed in 0760cf9. Both doc-only — the code findings have stopped.

opus, env.go:30 — DSN.Scheme's list named kimi but not qwen: the same sibling drift this PR keeps finding, in a doc comment. I'd updated the README's scheme list and not this one. Added qwen, plus llama-swap/llama-swaps which were also missing.
glm-5.2, env.go:44 — right, and a genuinely confusing comment: "LLM_M1 → m1" describes registry naming while the function does the reverse, and I put both directions in one parenthetical. Now reads name → variable only.

Declining the third with evidence:

glm-5.2, const alignment — gofmt -l is empty, so that padding is gofmt's output; the uneven groups are forced by the doc comments splitting the block. Changing it means fighting gofmt, and CI's gofmt gate would reject it. Worth noting glm-5.2 reached exactly this conclusion in round 2 ("This is gofmt-mandated... Not a real issue") and then filed it as a finding here — the verification was done and not applied.

Four rounds: 13 findings, 11 real and fixed, 2 declined with reasons. The recurring shape was one thing at descending levels — deduped production code but copied the test fixtures, deduped the scheme half but not the provider half, deduped fixtures but left two parallel suites, then a doc comment listing one sibling and not the other. Each fix moved a level up; the contract is now a table, so the next OpenAI-compat built-in is one row that owes all four checks.

Gates green, go mod tidy clean, 9/9 break-checks land and are caught. Merging once this round's CI is green.

Round 4: two of three real, fixed in 0760cf9. Both doc-only — the code findings have stopped. - **opus, `env.go:30`** — `DSN.Scheme`'s list named kimi but not qwen: the same sibling drift this PR keeps finding, in a doc comment. I'd updated the README's scheme list and not this one. Added qwen, plus `llama-swap`/`llama-swaps` which were also missing. - **glm-5.2, `env.go:44`** — right, and a genuinely confusing comment: "LLM_M1 → m1" describes registry naming while the function does the reverse, and I put both directions in one parenthetical. Now reads name → variable only. Declining the third with evidence: - **glm-5.2, const alignment** — `gofmt -l` is empty, so that padding *is* gofmt's output; the uneven groups are forced by the doc comments splitting the block. Changing it means fighting gofmt, and CI's gofmt gate would reject it. Worth noting glm-5.2 reached exactly this conclusion in round 2 ("This is gofmt-mandated... Not a real issue") and then filed it as a finding here — the verification was done and not applied. Four rounds: 13 findings, 11 real and fixed, 2 declined with reasons. The recurring shape was one thing at descending levels — deduped production code but copied the test fixtures, deduped the scheme half but not the provider half, deduped fixtures but left two parallel suites, then a doc comment listing one sibling and not the other. Each fix moved a level up; the contract is now a table, so the next OpenAI-compat built-in is one row that owes all four checks. Gates green, `go mod tidy` clean, 9/9 break-checks land and are caught. Merging once this round's CI is green.

steve added 1 commit 2026-08-12 20:57:34 +00:00

docs(progress): describe the shape this PR actually landed in

CI / Build & Test (pull_request) Successful in 10m28s

Details

CI / Tidy (pull_request) Successful in 9m22s

Details

f8ced9c629

The progress entry was written before four review rounds reshaped the change:
it credited openaiCompatScheme alone, listed the tests as six per-provider
cases, and mentioned a captureRT detail that has since moved. Rewritten to
match what merges — registerOpenAICompatBuiltin owning both halves,
envKeyForProvider as the single LLM_<NAME> definition, and the shared table
that every OpenAI-compat built-in is now checked against.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

steve merged commit f837115a55 into main

2026-08-12 21:03:34 +00:00

steve deleted branch feat/qwen-builtin

2026-08-12 21:03:34 +00:00

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: steve/majordomo#27

feat(qwen): Alibaba Qwen built-in over Model Studio's OpenAI-compatible mode #27

OpenAI-compat vs Anthropic-compat

Shared factory

Tests

Gates

🪰 Gadfly — live review status

claude-code/opus · claude-code — ✅ done

claude-code/sonnet · claude-code — ✅ done

glm-5.2:cloud · ollama-cloud — ✅ done

kimi-k2.6:cloud · ollama-cloud — ✅ done

🪰 Gadfly review — consensus across 4 models

VERDICT: No material issues found

VERDICT: No material issues found

VERDICT: Minor issues

VERDICT: No material issues found

VERDICT: No material issues found

Review — 🎯 Correctness

Review

Review — 🧯 Error handling & edge cases lens

`claude-code/opus` · claude-code — ✅ done

`claude-code/sonnet` · claude-code — ✅ done

`glm-5.2:cloud` · ollama-cloud — ✅ done

`kimi-k2.6:cloud` · ollama-cloud — ✅ done