Skip to content

fix(memory): forward BYOK chat model through /api/memory/extract #5177

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alchemistklk merged 7 commits into from
Jul 15, 2026
Merged

fix(memory): forward BYOK chat model through /api/memory/extract #5177

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d180a1b
fix: forward BYOK chat model to memory extraction (#5162)
xxiaoxiong Jul 5, 2026
b3aebe8
fix(contracts): add model field to ByokChatProviderConfig for BYOK me…
xxiaoxiong Jul 5, 2026
a62c6eb
fix(byok): copy config.model into byokOpenCodeProvider snapshot
xxiaoxiong Jul 6, 2026
988b652
test: add model field to BYOK payload expectations in web workspace t…
xxiaoxiong Jul 9, 2026
0951009
fix(daemon): forward BYOK chatProvider to post-turn memory extraction
xxiaoxiong Jul 14, 2026
8e0d6a1
fix(daemon): normalize byokProvider shape for post-turn memory extrac…
xxiaoxiong Jul 14, 2026
ca7b152
fix(daemon): support keyless BYOK endpoints in post-turn memory extra…
xxiaoxiong Jul 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions apps/daemon/src/routes/memory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,6 +588,10 @@ export function registerMemoryRoutes(app: Express, ctx: RegisterMemoryRoutesDeps
};
}
}
const chatModel =
typeof body.chatModel === 'string' && body.chatModel.trim()
? body.chatModel.trim()
: '';
let attemptedLLM = false;
if (userMessage.trim().length > 0 && hasAssistant) {
attemptedLLM = true;
Expand All @@ -600,6 +604,7 @@ export function registerMemoryRoutes(app: Express, ctx: RegisterMemoryRoutesDeps
projectRoot: PROJECT_ROOT,
chatAgentId: null,
chatProvider,
...(chatModel ? { chatModel } : {}),
},
),
)
Expand Down
26 changes: 26 additions & 0 deletions apps/daemon/src/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6157,10 +6157,36 @@ export async function startServer({
// a Claude Code (anthropic) chat from triggering OpenAI/gpt-4o-
// mini extraction in the background just because the user has
// an OpenAI key parked in media-config.
//
// Also normalize the BYOK provider shape: web side sends
// `{ protocol, ... }` via the chat body as `byokProvider`,
// but memory-llm.pickProvider expects `{ provider, ... }`
// with `provider` being a PROVIDER_DEFAULTS key. We apply the
// same mapping the web pre-turn path does (ProjectView.tsx
// constructs `{ provider: byokOpenCodeProvider.protocol, ... }`).
const memoryChatProvider: {
provider?: string;
apiKey?: string;
baseUrl?: string;
apiVersion?: string;
model?: string;
} | null = byokProvider
? {
provider: (byokProvider as { protocol?: string }).protocol ?? undefined,

@mrcfps mrcfps Jul 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please preserve the keyless-provider policy in this normalization. byokProvider may legitimately carry requiresApiKey: false—the existing daemon chat-route coverage uses exactly that shape for a local OpenAI-compatible endpoint—but this new object copies only protocol, key, URL, API version, and model. Downstream, pickProvider() enters the chat-BYOK branch only when apiKey is non-empty, so a successful keyless vLLM or local Ollama chat skips its supplied endpoint/model during post-turn extraction and can fall through to unrelated environment or media-config credentials (including the gpt-4o-mini default this PR is avoiding). Please thread requiresApiKey through, allow the BYOK branch when it is explicitly false, and omit the Authorization header when no key is required. A regression test should exercise a post-turn provider snapshot with an empty key, requiresApiKey: false, a local base URL, and a selected model, then assert that extraction calls that URL/model rather than a fallback.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

apiKey: (byokProvider as { apiKey?: string }).apiKey,
baseUrl: (byokProvider as { baseUrl?: string }).baseUrl,
apiVersion: (byokProvider as { apiVersion?: string }).apiVersion,
model: (byokProvider as { model?: string }).model,
}
: null;
const memoryOptions = {
projectRoot: PROJECT_ROOT,
chatAgentId: typeof agentId === 'string' ? agentId : null,
chatModel: typeof safeModel === 'string' ? safeModel : null,
// Forward the per-call BYOK provider snapshot so pickProvider()
// can run "Same as chat" extraction against the user's actual
// provider/endpoint/model instead of falling back to defaults.
chatProvider: memoryChatProvider,
// Scope the extractor's duplicate-turn de-dup to this conversation, so a
// re-fired turn collapses but an identical (message, reply) in another
// conversation is still examined.
Expand Down
2 changes: 2 additions & 0 deletions apps/web/src/components/ProjectView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1239,6 +1239,7 @@ function byokOpenCodeProviderFromConfig(
protocol: config.apiProtocol,
apiKey: config.apiKey.trim(),
baseUrl: config.baseUrl,
model: config.model,

@mrcfps mrcfps Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This field fixes the runtime wiring, but it also changes the BYOK provider payload shape that the web run-isolation tests lock down. The live Web workspace tests job on this head is failing in tests/components/ProjectView.run-isolation.test.tsx at the BYOK request assertions around lines 1531 and 1564: the actual calls now include byokProvider.model (llama3.2 / model), while the expected payloads still omit it. Because Validate workspace fails only through that web test failure, the PR cannot merge until the test contract matches this intentional shape change. Please update those expectations to include the selected model, or narrow the assertions if the exact nested provider object is not meant to be the contract.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

...(selectedProvider?.requiresApiKey === false ? { requiresApiKey: false } : {}),
apiVersion:
config.apiProtocol === 'azure'
Expand Down Expand Up @@ -5894,6 +5895,7 @@ export function ProjectView({
apiKey: byokOpenCodeProvider.apiKey,
baseUrl: byokOpenCodeProvider.baseUrl,
apiVersion: byokOpenCodeProvider.apiVersion,
model: byokOpenCodeProvider.model,

@mrcfps mrcfps Jul 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new field currently breaks the web typecheck because byokOpenCodeProvider is typed as ByokChatProviderConfig, and that shared contract only exposes protocol, apiKey, baseUrl, apiVersion, and requiresApiKey; it does not include model. CI is failing on this exact line with TS2339: Property 'model' does not exist on type 'ByokChatProviderConfig', so the PR cannot merge as-is even though the runtime intent is right. Please thread the selected chat model from a typed source, for example by adding an optional model field to ByokChatProviderConfig in packages/contracts/src/api/chat.ts and populating it from config.model in byokOpenCodeProviderFromConfig, or by building this memory-only snapshot directly from config.model where the request is made.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

@mrcfps mrcfps Jul 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still does not actually send the selected BYOK chat model. The added field reads byokOpenCodeProvider.model, but byokOpenCodeProvider is created by byokOpenCodeProviderFromConfig() and that factory still returns only protocol, apiKey, baseUrl, requiresApiKey, and apiVersion; it never copies config.model. As a result this property serializes as undefined, memory.ts receives no chatProvider.model, and pickProvider() continues to choose envOverrideModel || explicitModel || defaults.model with explicitModel === '', falling back to gpt-4o-mini for OpenAI-compatible BYOK endpoints like MiniMax. That is the bug this PR is meant to fix. Please populate the provider snapshot from the active chat model, for example by adding model: config.model in byokOpenCodeProviderFromConfig() or by setting model: config.model directly in this memory-only byokChatProvider object.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

@mrcfps mrcfps Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now copies the selected model into the BYOK snapshot, but this snapshot still appears to be attached only to the pre-turn /api/memory/extract call. The request body just below sends userMessage, projectId, conversationId, and chatProvider, but no assistantMessage; in apps/daemon/src/routes/memory.ts, extractWithLLM() only runs when hasAssistant is true. The separate daemon close-handler path in apps/daemon/src/server.ts does have the assistant output, but it only passes chatAgentId: byok-opencode and chatModel; it does not pass this chatProvider, and chatProtocolFromAgentId() does not map byok-opencode to the user's actual BYOK provider/base URL.

That means the model value added here does not seem to reach the post-turn pickProvider() call that was falling back to gpt-4o-mini, so MiniMax/OpenAI-compatible BYOK memory extraction can still take the legacy env/media-config path instead of the user's BYOK endpoint. Please either send a post-turn /api/memory/extract request with both assistantMessage and the same byokChatProvider, or thread the BYOK provider snapshot into the daemon close-handler memoryOptions so pickProvider() receives the provider, base URL, API key, and selected model on the actual LLM extraction pass.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

@mrcfps mrcfps Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now includes the selected BYOK model in the /api/memory/extract snapshot, but this request still appears to run only the pre-turn heuristic path: the JSON body below sends userMessage and chatProvider, but no assistantMessage, while apps/daemon/src/routes/memory.ts only calls extractWithLLM() when hasAssistant is true. The actual post-turn extraction for byok-opencode is queued from the daemon close handler in apps/daemon/src/server.ts, and that path forwards chatAgentId/chatModel but not the byokProvider snapshot; chatProtocolFromAgentId() also does not map byok-opencode to the user's BYOK endpoint. So the model added here may still not reach the main LLM extraction pass that was falling back to gpt-4o-mini for MiniMax/OpenAI-compatible BYOK users.

Please either send a post-turn /api/memory/extract call with both assistantMessage and this same byokChatProvider, or thread the run-scoped byokProvider through the daemon close-handler memoryOptions as chatProvider so pickProvider() receives the provider, base URL, API key, and selected model on the extraction pass that has the assistant reply.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

}
: undefined;
if (userText.length > 0) {
Expand Down
2 changes: 2 additions & 0 deletions apps/web/tests/components/ProjectView.run-isolation.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1719,6 +1719,7 @@ describe('ProjectView conversation run isolation', () => {
baseUrl: 'http://localhost:11434',
requiresApiKey: false,
apiVersion: '',
model: 'llama3.2',
},
model: 'llama3.2',
}));
Expand Down Expand Up @@ -1752,6 +1753,7 @@ describe('ProjectView conversation run isolation', () => {
baseUrl: 'http://127.0.0.1:8000/v1',
requiresApiKey: false,
apiVersion: '',
model: 'model',
},
model: 'model',
}));
Expand Down
7 changes: 7 additions & 0 deletions packages/contracts/src/api/chat.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,13 @@ export interface ByokChatProviderConfig {
apiVersion?: string;
/** Explicit run-scoped provider policy for presets that do not require bearer credentials. */
requiresApiKey?: boolean;
/**
* Run-scoped chat model id selected in the chat UI. Forwarded to the daemon
* so BYOK-backed utilities (e.g. memory extraction) can honor the user's
* model picker instead of falling back to a hardcoded default. Optional
* because some presets (e.g. Ollama) infer the model from baseUrl/protocol.
*/
model?: string;
}

export interface ByokMediaDefaults {
Expand Down
Loading