feat(core,runtime,desktop): expose thinking levels for OpenAI Responses relays - #3300
feat(core,runtime,desktop): expose thinking levels for OpenAI Responses relays#3300hbw00111 wants to merge 0 commit into
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed at fe6377fe. Small, coherent, and correct as far as it goes: extending per-model relay declarations from openai-compatible to openai-responses-compatible behind one predicate is the right shape, and — the thing most likely to be got wrong here — all three gates are converted consistently. I checked each: the read seam (relayModelProfile), the store boundary (rejectForeignProfiles), and the UI (showsCapabilities). A half-converted set would have produced the worst outcome, an editor that offers declarations the store then rejects.
I also checked the downgrade direction, because widening what may be persisted usually owes a migration. It does not here: rejectForeignProfiles is reached only from the create and update normalization paths, never from decode, so an older build reading a document with profiles on a Responses relay does not throw — it silently ignores them at the read seam and preserves them in the document. Declarations disappear on rollback and return on re-upgrade. That is acceptable degradation rather than a brick, and worth one line in the PR body.
One cross-PR interaction to settle before this and #3296 both land. #3296 adds thinkingOptionsForConnection, which resolves thinking from the inventory and the metadata chain and never consults relayModelProfile — its first statement is if (inventory?.capabilities?.reasoning === false) return undefined. After this PR, a Responses relay is exactly the case where the user's declaration is supposed to be authoritative because the relay's own /models report cannot be trusted about the backing model. So if a call site reaches the thinking switcher through #3296's function rather than the relay path, a relay reporting reasoning: false silently overrides the user's declaration and the switcher vanishes — the precise scenario the declaration exists to override. The two PRs each look right alone; together they leave the precedence between a user declaration and an inventory-derived veto undefined. Decide it explicitly and encode it in one function.
Both PRs also touch model-thinking.ts and connection-catalog-codec.ts, as does #3129 — different hunks in the codec, so no textual conflict, but three concurrent PRs on the same two files is worth coordinating.
Reviewed with Claude Opus as an analysis assistant. Every claim here was verified by reading source at this head and at #3296's head — including whether rejectForeignProfiles is reachable from decode and whether any relay-profile gate was left on the old literal. Nothing was executed.
| } | ||
|
|
||
| /** Custom OpenAI relay providers whose model capabilities are user-declared. */ | ||
| export function isRelayProviderType( |
There was a problem hiding this comment.
[P3] Put this predicate with the provider taxonomy, and prefer deriving it. isRelayProviderType answers a question about ProviderType, not about thinking — yet it lives in model-thinking.ts, so connection-catalog-codec.ts now imports a provider-classification predicate from the thinking module while already importing ProviderType and PROVIDER_DEFAULTS from llm-connections.js on line 1. The hand-maintained two-member union is the other half: PROVIDER_REGISTRY is the authority on what each provider is, and every entry already carries structured facts such as modelDiscovery and runtimeAdapter, so a third relay provider should not require anyone to remember to edit this union — the definition of a relay here is precisely "a provider whose backing models are unknown to metadata", which is a registry fact. Confirmed by reading code at this head; no runtime impact today, since the union is currently complete. Move it next to the registry and derive it from a registry field if one fits. Regression test: not applicable; if you derive it, assert every provider the registry marks as a relay is accepted by rejectForeignProfiles.
Astro-Han
left a comment
There was a problem hiding this comment.
Approving at fe6377fe. Only a P3 remains from my review — where isRelayProviderType lives — which is a seam preference, not a defect.
The substance is right: all three gates (read seam, store boundary, UI) are converted consistently, and the downgrade direction owes no migration because rejectForeignProfiles is unreachable from decode.
The cross-PR precedence question with #3296 stands, but it is not this PR's to fix alone — whichever of the two lands second should resolve it.
fe6377f to
e5cd748
Compare
e5cd748 to
fe6377f
Compare
fe6377f to
e5cd748
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
The relay-taxonomy half of this change is right, and it answers my earlier P3 exactly as asked: isRelayProviderType now lives with the provider taxonomy in llm-connections.ts and derives from a relayModelProfiles registry flag instead of restating a literal at three call sites. That is the correct seam — a new relay provider becomes one registry line.
The serviceTier / Fast mode half is new since my last pass, and I cannot reach the same conclusion for it. Two blockers below: the desktop import of isRelayProviderType names a module that does not export it, and the declared tier is dropped by every pinned SDK on its way to the wire for exactly the relay models this feature exists to serve. Neither is visible yet because this PR's workflow runs are still action_required and have never executed.
Architecture: this branch now carries two intents — generalizing relay profiles to the second relay provider type, and adding a brand-new per-model capability spanning UI copy, draft editing, the catalog codec and the runtime wire. AGENTS.md scopes a PR to what one revert must undo, and reverting Fast mode should not have to revert the taxonomy fix. The taxonomy half is one import line from mergeable; splitting it out would land it now and leave Fast mode to the wire evidence it still needs.
AI disclosure: this review was produced with Claude Code (Opus 5). I read model-thinking.ts, llm-connections.ts, model-factory.ts, the catalog codec and the desktop panel at e5cd748, resolved @maka/core's export map, and read the pinned @ai-sdk/openai@4.0.42, @ai-sdk/openai-compatible@3.0.30 and @ai-sdk/open-responses@2.0.28 option schemas in this repository's node_modules. I did not run the build or the tests. Per AGENTS.md this is not independent human review.
| import { | ||
| DECLARABLE_RELAY_THINKING_LEVELS, | ||
| THINKING_LEVELS, | ||
| isRelayProviderType, |
There was a problem hiding this comment.
[P0] Import isRelayProviderType from @maka/core/llm-connections, not @maka/core/model-thinking. At this head model-thinking.ts imports the symbol on line 20 and uses it on line 240 but never re-exports it, and packages/core/package.json maps ./model-thinking straight to ./dist/model-thinking.js with no barrel in between — so this specifier resolves to a module that has no such export. Confirmed by reading those files and the export map at e5cd748; not executed, because this PR's workflow runs are still action_required and typecheck has never run against it. The effect is TS2305 at build time, and if a build ever got past it, isRelayProviderType is not a function when the connection detail panel mounts — taking down the whole capabilities panel, not just Fast mode. Note that the new isRelayProviderType only accepts the two custom OpenAI relay providers test imports from ../llm-connections.js and so stays green while this is broken; the check that would actually catch it is the desktop typecheck lane, so the regression requirement here is simply that it runs and passes on this branch.
| case 'openai-compatible': | ||
| return { | ||
| [openAiCompatibleProviderOptionsKey(adapter, connection)]: { reasoningEffort }, | ||
| [openAiCompatibleProviderOptionsKey(adapter, connection)]: { |
There was a problem hiding this comment.
[P1] Prove serviceTier reaches a request body before shipping the toggle, because on the pinned SDKs it does not for the relay models this feature targets. @ai-sdk/openai-compatible@3.0.30 parses this namespace through z.object({ user, reasoningEffort, textVerbosity, strictJsonSchema }) and zod v4 strips unknown keys, so an openai-compatible connection loses serviceTier before the body is assembled; @ai-sdk/open-responses@2.0.28's options carry only reasoningEffort, so the open-responses contract branch above drops it too; and on the one path that does forward it, @ai-sdk/openai@4.0.42 derives supportsPriorityProcessing from the model-id string alone (gpt-4*, gpt >= 5 excluding nano/chat, o-series >= 3) and runs delete baseArgs.service_tier for anything else — which is every relay model whose id is not literally an OpenAI one, i.e. the premise relayModelProfiles exists for. Confirmed by reading those three packages at their pinned versions under this repository's node_modules; not executed. Meanwhile the desktop panel renders the Fast selector for both relay provider types unconditionally, so a user sets it, watches it persist, and gets nothing on the wire. The regression test this needs is the body capture the sibling declared relay levels reach the actual chat-completions request body test already demonstrates: assert service_tier in the captured body for each provider type the selector is offered on, and gate the selector on the ones that survive.
| ): SharedV4ProviderOptions { | ||
| const { adapter, wire, reasoningReplay } = resolveModelRuntime(connection, modelId); | ||
| const reasoningEffort = level ? (level === 'off' ? 'none' : level) : undefined; | ||
| const serviceTier = connection.relayModelProfiles?.[modelId]?.serviceTier; |
There was a problem hiding this comment.
[P2] Read the declaration through relayModelProfile(connection, modelId) instead of indexing connection.relayModelProfiles raw. Every other consumer here reaches declarations through that accessor — thinkingVariantsForConnection, called a few lines down, does — and the accessor exists to apply the provider gate; connection-catalog-codec.ts gives the reason in its own words, that a table on another provider would "silently shadow metadata for the ungated read seams". This line is that seam. Store-boundary validation makes it unreachable today, so this is a contract/consistency finding rather than a reproduced defect, but it is load-bearing for the change on line 563: now that the early return is conditioned on serviceTier, a table on a non-relay connection reaches the switch with level === undefined, and case 'google' then returns { google: { thinkingConfig: { includeThoughts: true, thinkingLevel: undefined } } } — thinking silently forced on for a model that declared nothing. Gating the read closes both at once. Regression test: assert buildProviderOptions returns {} for an anthropic and a google connection carrying a serviceTier profile.
| baseUrl: 'https://relay.example/v1', | ||
| relayModelProfiles: { 'fast-model': { serviceTier: 'fast' } }, | ||
| }; | ||
| assert.deepEqual(buildProviderOptions(chat, 'fast-model'), { |
There was a problem hiding this comment.
[P2] Assert the request body here rather than the intermediate providerOptions object. This file already draws that line for itself: the sibling declared relay levels reach the actual chat-completions request body test opens by noting that matching intermediate objects does not prove the wire carries the effort, and captures the real body instead. These assertions match objects that the pinned SDK then strips or deletes, so the suite reports green on a feature that sends nothing — which is how the P1 above reached review unnoticed. Extend the existing capture helper to the fast-tier case for each provider type the UI offers the selector on, so the test fails the day the declaration stops reaching service_tier.
|
|
||
| export function isRelayProviderType( | ||
| providerType: ProviderType, | ||
| ): providerType is 'openai-compatible' | 'openai-responses-compatible' { |
There was a problem hiding this comment.
[P3] Derive the narrowed union from the registry flag instead of restating it by hand. The predicate decides at runtime by reading PROVIDER_REGISTRY[providerType].relayModelProfiles, but narrows to a literal 'openai-compatible' | 'openai-responses-compatible'; the two agree at this head and nothing keeps them agreeing. Setting the flag on a third provider — precisely the one-line extension this refactor is meant to enable — would make the predicate return true while narrowing callers to a union that excludes it, and the unsoundness would surface as a puzzling type error or a wrong narrowing far from the registry. Inference, not a defect here. A type computed from the registry entries whose relayModelProfiles is true removes the drift; the new isRelayProviderType only accepts the two custom OpenAI relay providers test guards the runtime half but cannot guard the narrowing.
e5cd748 to
b1655d5
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
The P0 is fixed at b1655d5 — the import now names @maka/core/llm-connections. My review at e5cd748 is no longer at head; its P1/P2/P3 threads sit in files untouched since, so they carry over unchanged.
A note on attribution: that P0 came from the taxonomy refactor, not Fast mode — it was the tail of my own earlier P3. At fe6377fe2 the predicate was exported from model-thinking.ts:224 and the desktop still used a literal; at e5cd748 it moved to llm-connections.ts (right seam, exactly as asked) and the desktop gained a new call site, whose import named the old location.
On splitting: the two halves aren't at the same stage. The taxonomy half is done, and I approved its substance at the previous head. Fast mode isn't a typo away — on the pinned SDKs serviceTier survives to a request body only on the @ai-sdk/openai Responses path, and only when supportsPriorityProcessing accepts the model id from the string alone. So it can work for an openai-responses-compatible relay fronting a genuinely OpenAI-named model, and never for openai-compatible. That needs a deliberate decision — it sits awkwardly against the premise relayModelProfiles was built on. No reason for a ready change to wait on it.
The UI, copy, draft-editing and codec work all read correctly to me; the only blocker is that nothing proves the declaration reaches a wire.
(Same tooling basis as my review on the previous head.)
b1655d5 to
f0a1c41
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
The split landed, and it settles most of what I raised. Verified at f0a1c41:
- P0 import — fixed;
isRelayProviderTypenow comes from@maka/core/llm-connections. - P1
serviceTiernever reaching a wire — moot, Fast mode is out of this branch. Note that thread re-anchored ontomodel-factory.ts:560, a line it no longer has anything to do with; safe to resolve. - P2 ungated
relayModelProfilesread inbuildFamilyWire— moot, that read left withserviceTier. - P2 wire-capture test gap — moot as filed, since it was about the fast tier. Not re-raising it.
- P3 at
llm-connections.ts:54(the narrowed literal union versus the registry flag) is untouched and still applies.
What is left is small but blocking, and neither item is a design problem — both are residue from lifting Fast mode out by hand. Worth saying plainly: this branch currently has no check runs at all, so nothing has told you about either one.
Architecturally I have nothing further. Splitting was the right call and the remaining change is what I approved the substance of at fe6377f: one registry flag decides which providers carry relay profiles, and the predicate reads it instead of restating a literal.
Still COMMENT rather than approve only because of the P1 below; fix the two blank lines and I expect this to be approvable.
AI disclosure: this review was produced with Claude Code (Opus 5). I diffed b1655d5..f0a1c41, read the changed files at this head, and reproduced the formatting failure by running this repository's pinned @biomejs/biome@2.5.6 binary on an equivalent input in a scratch directory outside the repository. I did not run the project's own build or tests. Per AGENTS.md this is not independent human review.
| }); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
[P1] Delete this blank line — format:check fails on it, which fails CI. Biome collapses any run of more than one consecutive empty line, and lines 409–410 are now the only such run in this file; the identical thing happened at packages/runtime/src/__tests__/model-factory-thinking.test.ts:569–570. Both are residue from lifting setDraftServiceTier and the fast-tier test out of the branch. Reproduced by execution: I ran this repository's pinned @biomejs/biome@2.5.6 over an equivalent input and it deletes the second blank line, so biome format . — which is what npm run format:check invokes, and what the test job's "Check formatting" step runs — exits non-zero. This branch has no check runs on it at all right now, so nothing has surfaced it yet. No new regression test is owed; letting the existing format lane run green on this head is the check.
| [openAiCompatibleProviderOptionsKey(adapter, connection)]: { reasoningEffort }, | ||
| [openAiCompatibleProviderOptionsKey(adapter, connection)]: { | ||
| ...(reasoningEffort ? { reasoningEffort } : {}), | ||
| ...(reasoningEffort ? { reasoningEffort } : {}), |
There was a problem hiding this comment.
[P2] Collapse these two identical spreads back to { reasoningEffort }. Lines 561 and 562 spread the same conditional object twice, so the value is unchanged — this is not a defect, it is what was left behind when the serviceTier spread was removed from the pair. It is worth fixing anyway because this is the one file where a reviewer most needs to trust that the split was complete, and a line that is visibly the residue of a hand-edit works against that. The guard above already returns early unless reasoningEffort is truthy, so the conditional is redundant here too; the pre-PR form { reasoningEffort } is both correct and shorter. Confirmed by reading the code at f0a1c41; no behavioural change either way, so nothing is owed in tests.
f0a1c41 to
d0e110c
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Both items from f0a1c41 are still open at d0e110c4, so this is a short re-post rather than a new review.
The two blank lines I pointed at are gone, but an identical one appeared in a third file — so format:check still fails, just somewhere else. That is the second time this branch has moved the same defect rather than removed it, which suggests it is worth running npm run format over the branch once instead of fixing the reported line each round. There are still no check runs on this head, so nothing but this review is telling you.
The duplicated spread in model-factory.ts is unchanged.
Nothing else has changed and I have no new findings. Substance-wise this is the same change I approved at fe6377f, and once npm run format is run and those two lines collapse, I expect to approve it.
AI disclosure: produced with Claude Code (Opus 5). I diffed f0a1c41..d0e110c4, scanned all four affected files at this head for consecutive blank-line runs, and reproduced the formatting failure by running this repository's pinned @biomejs/biome@2.5.6 over the file and diffing its output. Per AGENTS.md this is not independent human review.
| assert.deepEqual([...thinkingVariantsForConnection(declaredOff, 'm')], ['low']); | ||
| }); | ||
|
|
||
|
|
There was a problem hiding this comment.
[P1] Run npm run format over the branch — this is the same format:check failure as last round, relocated. Lines 28-29 are now the only consecutive-blank-line run in any file this PR touches; the two I flagged at f0a1c41 (use-connection-detail.ts and model-factory-thinking.test.ts) are both clean now, and this one is new. Reproduced by execution: I ran this repository's pinned @biomejs/biome@2.5.6 over this file at d0e110c4 and its output deletes line 29, so biome format . — which is what npm run format:check and the test job's "Check formatting" step both run — exits non-zero. Fixing the specific line I report each round is what keeps reintroducing it; one npm run format pass over the whole branch settles it, and the format lane running green on this head is the check.
| [openAiCompatibleProviderOptionsKey(adapter, connection)]: { reasoningEffort }, | ||
| [openAiCompatibleProviderOptionsKey(adapter, connection)]: { | ||
| ...(reasoningEffort ? { reasoningEffort } : {}), | ||
| ...(reasoningEffort ? { reasoningEffort } : {}), |
There was a problem hiding this comment.
[P2] Still unchanged from f0a1c41 — lines 561 and 562 spread the same conditional object twice. Behaviour is identical either way, so this is not a defect; it is the residue left when the serviceTier spread was lifted out of the pair for #3309. The guard above already returns early unless reasoningEffort is truthy, so the pre-PR form { reasoningEffort } is both correct and shorter. Worth taking because this is the one file where a reviewer most needs to trust the split was complete.
d0e110c to
edd5321
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewed at edd5321e3 (force-pushed over d0e110c4e, so no incremental diff was available — I re-verified the whole change).
The substance of the change is now right. Extending relay declarations to openai-responses-compatible is expressed once, as a registry fact (ProviderDefaults.relayModelProfiles), and every gate reads it through isRelayProviderType. I checked the migration is complete rather than partial: on current main there are exactly three non-test sites that gate on providerType === 'openai-compatible' — model-thinking.ts:234, connection-catalog-codec.ts:215, provider-connection-detail.tsx:166 — and this PR converts all three. Every other reader of relayModelProfiles (the storage document pruner, runtime-policy-coordinator, execution-model-authority, session-catalog-coordinator, the desktop IPC layer) is already provider-agnostic and picks the second provider up for free. No seam is left behind.
Coverage now reaches both ends: runtime-policy-codec.test.ts proves the codec accepts a Responses-relay table, and model-factory-thinking.test.ts proves a declared level reaches providerOptions on the Responses wire — including that an undeclared xhigh is dropped. That answers the earlier "asserting the intermediate object" objection for the new path.
Still not mergeable, for one mechanical reason. format:check fails, as it has on the last three heads. The added blank line at packages/core/src/__tests__/model-thinking.test.ts:29 gives a diff under the repo-pinned Biome, and format:check gates the required test check. I verified this by piping the file at this exact head through ./node_modules/.bin/biome format --stdin-file-path=... and diffing — one line, 29d28. npm run format on the branch fixes it. The existing thread is still open on it.
The duplicated spread at model-factory.ts:561-562 is also unchanged. It is worth noting it is not merely redundant: :556 already did if (!reasoningEffort) return {};, so both conditionals are statically true and the whole object is { reasoningEffort }. Its thread is open. The isRelayProviderType narrowing thread stands as filed (P3): the predicate tests a registry flag but narrows to a hand-written two-member union, so a third provider flagged in the registry would return true at runtime while being excluded from the type.
No new findings at this head. Carrying P1 → COMMENT, not approval; I will approve once format:check is green.
AI disclosure: reviewed with Claude Code. I read every changed file at edd5321e3 myself, ran the Biome check against the head content, and enumerated the remaining openai-compatible gates across the repo to confirm the migration is complete. Nothing here is unverified model output.
edd5321 to
19b7c54
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Approving at 19b7c54d5. The P1 is fixed: I re-ran the repo-pinned Biome against every changed file at this head, reading the content from the head rather than the working tree, and all eight non-excluded TS files now produce byte-identical output. format:check should pass, which was the only thing blocking the required test check.
The change itself I verified last round and re-confirmed here: extending relay declarations to openai-responses-compatible is expressed once as a registry fact and read everywhere through isRelayProviderType, and the migration is complete rather than partial — the three non-test sites that gated on providerType === 'openai-compatible' are exactly the three this PR converts, and every other reader of relayModelProfiles was already provider-agnostic. Both ends are covered: the codec accepts a Responses-relay table, and a declared level reaches providerOptions on the Responses wire with an undeclared xhigh dropped.
Two things stay open. Neither blocks, and I am documenting them here rather than holding the PR.
The duplicated spread at model-factory.ts:560-563 is unchanged. It is worth saying plainly what it is: :556 already returned on a falsy reasoningEffort, so both conditionals are statically true and the object is exactly { reasoningEffort } — the two spreads are a no-op. More to the point, that hunk is the entire model-factory.ts change in this PR, and it contributes nothing to extending relay profiles to Responses relays. The cleanest resolution is to drop the hunk rather than collapse it: it is unrelated to this PR's intent and a revert would carry it along.
The isRelayProviderType narrowing is also unchanged (P3, thread already open). The predicate tests a registry flag but narrows to a hand-written two-member union, so a third provider flagged relayModelProfiles: true in the registry would return true at runtime while being excluded from the type — the callers would then be quietly mistyped rather than failing loudly. Deriving the union from the flag removes the possibility.
AI disclosure: reviewed with Claude Code. I ran the read-only Biome check against this head's file contents myself and re-confirmed the two open findings at their current lines. The approval is mine and rests on those checks.
19b7c54 to
57e08d8
Compare
Summary
OpenAI Responses-compatible custom relays now reuse the existing per-model relay capability profile. The profile supports thinking levels, vision, and context window.
The change accepts profiles for
openai-responses-compatibleconnections, exposes the existing per-model capability editor for both custom OpenAI relay types, sends declared thinking options through the Responses provider options, and derives relay classification from the provider registry.Refs #2219
Verification
npm --workspace @maka/core run buildnpm --workspace @maka/runtime run buildgit diff --checkpassedpackages/uitype drift (inlineCompletion,settledText,conversationKey,unlockAutoFollow,trailingAction)Review focus
This PR intentionally covers the reviewed relay taxonomy and thinking declarations. Fast service-tier behavior is split into #3303 with a real Responses request-body test.
AI use
Tool(s) and scope: Codex performed codebase research, implementation, test authoring, and verification.
Checklist
Does this PR entail a change in behavior?