mirror of
https://github.com/garrytan/gbrain.git
synced 2026-07-27 22:15:33 +00:00
fix(facts): probe the effective facts model + merge DB-plane provider_base_urls (takeover of #2233)
Two salvaged fixes from #2233: - extractFactsFromTurn probed isAvailable('chat') with no model, silently no-oping extraction whenever the facts model routes through a different provider than the global chat model. It now probes the effective (per-call/config) facts model. - 'gbrain config set provider_base_urls.<id> <url>' writes DB plane (the prefix is advertised in KNOWN_CONFIG_KEY_PREFIXES) but loadConfigWithEngine never merged it back, so the gateway never saw the configured proxy. Merged with file-plane winning, fail-open for engine shims without listConfigKeys. Dropped from the original PR: the openrouter_api_key plumbing (already on master) and the hardcoded personal-name normalization + regex turn pre-filter + kind normalizer (privacy-rule violation; fork-specific heuristics that belong in per-install config, not the shared extractor). Co-authored-by: sene1337 <sene1337@users.noreply.github.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
sene1337
Claude Fable 5
parent
2307a827d8
commit
4e5c09eef6
+30
-1
@@ -620,7 +620,10 @@ export function loadConfig(): GBrainConfig | null {
|
||||
* size the schema and must be stable across engine connect.
|
||||
*/
|
||||
export async function loadConfigWithEngine(
|
||||
engine: { getConfig(key: string): Promise<string | null | undefined> },
|
||||
engine: {
|
||||
getConfig(key: string): Promise<string | null | undefined>;
|
||||
listConfigKeys?(prefix: string): Promise<string[]>;
|
||||
},
|
||||
base?: GBrainConfig | null,
|
||||
): Promise<GBrainConfig | null> {
|
||||
// Codex /ship finding #3: when there's no file config AND no env DB URL,
|
||||
@@ -669,10 +672,36 @@ export async function loadConfigWithEngine(
|
||||
const dbEmbeddingColumns = await dbStr('embedding_columns');
|
||||
const dbSearchEmbeddingColumn = await dbStr('search_embedding_column');
|
||||
|
||||
// `gbrain config set provider_base_urls.<id> <url>` writes DB plane (the
|
||||
// prefix is advertised in KNOWN_CONFIG_KEY_PREFIXES) but pre-fix nothing
|
||||
// ever merged it back, so the gateway never saw the configured proxy.
|
||||
const dbProviderBaseUrls: Record<string, string> = {};
|
||||
if (typeof engine.listConfigKeys === 'function') {
|
||||
try {
|
||||
const keys = await engine.listConfigKeys('provider_base_urls.');
|
||||
for (const key of keys) {
|
||||
const provider = key.slice('provider_base_urls.'.length).trim();
|
||||
if (!provider) continue;
|
||||
const value = await dbStr(key);
|
||||
if (value !== undefined && value.trim()) dbProviderBaseUrls[provider] = value.trim();
|
||||
}
|
||||
} catch {
|
||||
// Minimal engine shims (tests) may not support prefix listing; keep
|
||||
// file/env behavior.
|
||||
}
|
||||
}
|
||||
|
||||
// DB applies only when env did NOT win. Env presence is detected by the
|
||||
// sync loadConfig() already setting the field. For each flag, prefer the
|
||||
// existing fileConfig value when defined; otherwise fall through to DB.
|
||||
const merged: GBrainConfig = { ...fileConfig };
|
||||
if (Object.keys(dbProviderBaseUrls).length > 0) {
|
||||
// File-plane entries win over DB values per the documented precedence.
|
||||
merged.provider_base_urls = {
|
||||
...dbProviderBaseUrls,
|
||||
...(merged.provider_base_urls ?? {}),
|
||||
};
|
||||
}
|
||||
if (merged.embedding_multimodal === undefined && dbMultimodal !== undefined) {
|
||||
merged.embedding_multimodal = dbMultimodal;
|
||||
}
|
||||
|
||||
@@ -173,16 +173,19 @@ export async function extractFactsFromTurn(input: ExtractInput): Promise<Extract
|
||||
cleaned = cleaned.trim();
|
||||
if (!cleaned) return [];
|
||||
|
||||
if (!isAvailable('chat')) {
|
||||
// No chat gateway → no extraction. Caller still inserts facts via direct
|
||||
// `gbrain take add` paths.
|
||||
return [];
|
||||
}
|
||||
|
||||
const cap = Math.max(1, Math.min(input.maxFactsPerTurn ?? 10, 25));
|
||||
const defaultModel = await getFactsExtractionModel(input.engine);
|
||||
const maxTokens = await getFactsExtractionMaxTokens(input.engine);
|
||||
const model = input.model ?? defaultModel;
|
||||
|
||||
if (!isAvailable('chat', model)) {
|
||||
// No chat gateway for the EFFECTIVE facts-extraction model → no
|
||||
// extraction. Probing the global chat model here silently no-ops
|
||||
// extraction whenever facts route through a different provider than
|
||||
// chat (e.g. chat on Anthropic, facts on a private openai-compatible
|
||||
// endpoint). Caller still inserts facts via direct `gbrain take add`.
|
||||
return [];
|
||||
}
|
||||
const userContent = `<turn>\n${cleaned}\n</turn>\n\nExtract up to ${cap} facts.${
|
||||
input.entityHints && input.entityHints.length
|
||||
? ` Known entity slugs the user already mentioned: ${input.entityHints.slice(0, 5).join(', ')}.`
|
||||
|
||||
@@ -24,6 +24,7 @@ import {
|
||||
isAvailable,
|
||||
resetGateway,
|
||||
__setChatTransportForTests,
|
||||
__setGenerateTextTransportForTests,
|
||||
getChatModel,
|
||||
} from '../src/core/ai/gateway.ts';
|
||||
import { extractFactsFromTurn } from '../src/core/facts/extract.ts';
|
||||
@@ -116,6 +117,48 @@ describe('facts extract — silent-no-op regression (v0.31.6 bug class)', () =>
|
||||
expect(facts).toEqual([]);
|
||||
});
|
||||
|
||||
test('facts model override availability is independent of the global chat model', () => {
|
||||
// A brain can leave the global chat model on Anthropic while routing only
|
||||
// facts extraction through an OpenAI-compatible/private endpoint. The
|
||||
// extractor must probe the EFFECTIVE facts model, not the global chat model.
|
||||
configureGateway({
|
||||
chat_model: 'anthropic:claude-sonnet-4-6',
|
||||
base_urls: { openrouter: 'http://127.0.0.1:8806/v1' },
|
||||
env: { OPENROUTER_API_KEY: 'unused' },
|
||||
});
|
||||
expect(isAvailable('chat')).toBe(false); // no ANTHROPIC_API_KEY
|
||||
expect(isAvailable('chat', 'openrouter:private/gemma4-31b')).toBe(true);
|
||||
});
|
||||
|
||||
test('extractFactsFromTurn extracts via the per-call facts model even when the global chat model is unavailable', async () => {
|
||||
// Pre-fix, extract probed isAvailable('chat') with NO model — so a brain
|
||||
// whose facts model differed from the (unavailable) global chat model
|
||||
// silently extracted zero facts (takeover of #2233).
|
||||
configureGateway({
|
||||
chat_model: 'anthropic:claude-sonnet-4-6',
|
||||
base_urls: { openrouter: 'http://127.0.0.1:8806/v1' },
|
||||
env: { OPENROUTER_API_KEY: 'unused' },
|
||||
});
|
||||
expect(isAvailable('chat')).toBe(false);
|
||||
__setGenerateTextTransportForTests(async () => ({
|
||||
content: [{ type: 'text', text: JSON.stringify({ facts: [
|
||||
{ fact: 'The user prefers short answers', kind: 'preference', confidence: 0.9, notability: 'medium' },
|
||||
] }) }],
|
||||
finishReason: 'stop',
|
||||
usage: { inputTokens: 1, outputTokens: 1 },
|
||||
}) as any);
|
||||
try {
|
||||
const facts = await extractFactsFromTurn({
|
||||
turnText: 'I prefer short answers, always.',
|
||||
source: 'test:facts-model-override',
|
||||
model: 'openrouter:private/gemma4-31b',
|
||||
});
|
||||
expect(facts.length).toBeGreaterThan(0);
|
||||
} finally {
|
||||
__setGenerateTextTransportForTests(null);
|
||||
}
|
||||
});
|
||||
|
||||
test('extractFactsFromTurn USES the chat transport when available — does NOT silently return []', async () => {
|
||||
// The smoking-gun test: when chat IS available, extract MUST actually call
|
||||
// the chat transport. If it silently returns [] without calling chat, the
|
||||
|
||||
@@ -9,6 +9,7 @@ import { loadConfigWithEngine, type GBrainConfig } from '../src/core/config.ts';
|
||||
|
||||
interface FakeEngine {
|
||||
getConfig(key: string): Promise<string | null | undefined>;
|
||||
listConfigKeys?(prefix: string): Promise<string[]>;
|
||||
}
|
||||
|
||||
function makeEngine(map: Record<string, string | null | undefined>): FakeEngine {
|
||||
@@ -16,6 +17,9 @@ function makeEngine(map: Record<string, string | null | undefined>): FakeEngine
|
||||
async getConfig(key: string) {
|
||||
return map[key];
|
||||
},
|
||||
async listConfigKeys(prefix: string) {
|
||||
return Object.keys(map).filter(k => k.startsWith(prefix));
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
@@ -47,6 +51,40 @@ describe('loadConfigWithEngine (Phase 4 / F3)', () => {
|
||||
expect(merged?.embedding_columns?.embedding_voyage?.dimensions).toBe(1024);
|
||||
});
|
||||
|
||||
test('DB-plane provider_base_urls.<provider> merge reaches runtime config', async () => {
|
||||
// `gbrain config set provider_base_urls.openrouter <url>` writes DB plane
|
||||
// (KNOWN_CONFIG_KEY_PREFIXES advertises it) — pre-fix nothing merged it
|
||||
// back, so the gateway never saw the configured proxy.
|
||||
const base: GBrainConfig = { engine: 'pglite' };
|
||||
const engine = makeEngine({
|
||||
'provider_base_urls.openrouter': 'http://127.0.0.1:8806/v1',
|
||||
'provider_base_urls.llama-server': 'http://127.0.0.1:8081/v1',
|
||||
});
|
||||
const merged = await loadConfigWithEngine(engine, base);
|
||||
expect(merged?.provider_base_urls?.openrouter).toBe('http://127.0.0.1:8806/v1');
|
||||
expect(merged?.provider_base_urls?.['llama-server']).toBe('http://127.0.0.1:8081/v1');
|
||||
});
|
||||
|
||||
test('file-plane provider_base_urls win over DB-plane provider overrides', async () => {
|
||||
const base: GBrainConfig = {
|
||||
engine: 'pglite',
|
||||
provider_base_urls: { openrouter: 'http://file-plane/v1' },
|
||||
};
|
||||
const engine = makeEngine({
|
||||
'provider_base_urls.openrouter': 'http://db-plane/v1',
|
||||
});
|
||||
const merged = await loadConfigWithEngine(engine, base);
|
||||
expect(merged?.provider_base_urls?.openrouter).toBe('http://file-plane/v1');
|
||||
});
|
||||
|
||||
test('engine without listConfigKeys keeps working (older shims)', async () => {
|
||||
const base: GBrainConfig = { engine: 'pglite' };
|
||||
const engine = { async getConfig() { return undefined; } };
|
||||
const merged = await loadConfigWithEngine(engine, base);
|
||||
expect(merged?.engine).toBe('pglite');
|
||||
expect(merged?.provider_base_urls).toBeUndefined();
|
||||
});
|
||||
|
||||
test('DB flag fills in when file/env did not set it', async () => {
|
||||
const base: GBrainConfig = { engine: 'pglite' };
|
||||
const engine = makeEngine({
|
||||
|
||||
Reference in New Issue
Block a user