Skip to content

fix: dedupe insightsService's transport onto the shared aiProvider one #5626

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
atomantic merged 1 commit into from
Sep 2, 2026
Merged

fix: dedupe insightsService's transport onto the shared aiProvider one #5626

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
91 changes: 91 additions & 0 deletions server/services/aiProvider.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,3 +363,94 @@ describe('callProviderAISimple through the ChatGPT subscription', () => {
expect(result).toMatchObject({ text: 'fallback answer' });
});
});

// The regression these two guard: a non-JSON/blank 200 body used to return
// { text: '' } from the (now-deleted) insightsService copy of this transport,
// which refreshCrossDomainNarrative / generateThemeAnalysis then persisted over
// narrative.json / themes.json — overwriting the cached result with nothing.
// The shared transport must surface an error instead, so the `if (result.error)
// return` guard at both call sites bails before any write.
Comment thread
atomantic marked this conversation as resolved.
describe('callProviderAISimple — malformed / non-2xx responses', () => {
const provider = { id: 'provider-1', name: 'Example Provider', type: 'api', endpoint: 'https://api.example.com/v1' };

const respondWithText = (text, { ok = true, status = 200 } = {}) =>
vi.stubGlobal('fetch', vi.fn(async () => ({ ok, status, text: async () => text })));

afterEach(() => {
vi.unstubAllGlobals();
});

it('returns an error, not an empty success, on a non-JSON 200 body', async () => {
respondWithText('<html><body>502 Bad Gateway</body></html>');

const result = await callProviderAISimple(provider, 'model-1', 'prompt');

expect(result.text).toBeUndefined();
expect(result.error).toMatch(/malformed/i);
});

it('returns an error on a blank 200 body', async () => {
respondWithText('');

const result = await callProviderAISimple(provider, 'model-1', 'prompt');

expect(result.error).toMatch(/malformed/i);
});

it('returns an error with the status and body on a non-2xx response', async () => {
respondWithText('boom', { ok: false, status: 500 });

const result = await callProviderAISimple(provider, 'model-1', 'prompt');

expect(result.error).toMatch(/Provider returned 500: boom/);
});
});

describe('callProviderAISimple — endpoint guard (SSRF / key-exfiltration)', () => {
const provider = { id: 'provider-1', name: 'Example Provider', type: 'api', endpoint: 'https://api.example.com/v1' };

const respondWithHello = () => vi.stubGlobal('fetch', vi.fn(async () => ({
ok: true,
status: 200,
text: async () => JSON.stringify({ choices: [{ message: { content: 'hello' } }] }),
})));

afterEach(() => {
vi.unstubAllGlobals();
});

it('blocks a keyed provider pointed at a non-allowlisted endpoint and never calls fetch', async () => {
const fetchSpy = vi.fn();
vi.stubGlobal('fetch', fetchSpy);

const result = await callProviderAISimple(
{ ...provider, apiKey: 'secret-key', endpoint: 'https://not-an-allowlisted-host.example' },
'model-1', 'prompt',
);

expect(result.error).toContain('Provider endpoint blocked');
expect(fetchSpy).not.toHaveBeenCalled();
});

it('allows a keyed provider on a non-allowlisted host through when allowCustomEndpoint is true', async () => {
respondWithHello();

const result = await callProviderAISimple(
{ ...provider, apiKey: 'secret-key', endpoint: 'https://not-an-allowlisted-host.example', allowCustomEndpoint: true },
'model-1', 'prompt',
);

expect(result).toMatchObject({ text: 'hello' });
});

it('allows a keyless provider on a non-allowlisted host through (guard only applies when apiKey is set)', async () => {
respondWithHello();

const result = await callProviderAISimple(
{ ...provider, endpoint: 'https://not-an-allowlisted-host.example' },
'model-1', 'prompt',
);

expect(result).toMatchObject({ text: 'hello' });
});
});
70 changes: 1 addition & 69 deletions server/services/insightsService.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,7 @@ import { MARKER_CATEGORIES, CURATED_MARKERS } from '../lib/curatedGenomeMarkers.
import { getTasteProfile } from './taste-questionnaire.js';
import { getActiveProvider, getProviderById } from './providers.js';
import { getCorrelationData } from './appleHealthQuery.js';
import { stripCodeFences, parseLLMJSON } from './aiProvider.js';
import { fetchWithTimeout } from '../lib/fetchWithTimeout.js';
import { readResponseJson } from '../lib/readResponseJson.js';
import { ensureProviderReady as ensureOllamaProviderReady } from './ollamaManager.js';
import { evaluateSecretEndpoint } from '../lib/aiToolkit/internal/endpointGuard.js';

const DEFAULT_AI_TIMEOUT_MS = 300000;
import { stripCodeFences, parseLLMJSON, callProviderAISimple } from './aiProvider.js';

const MARKER_BY_RSID = new Map(CURATED_MARKERS.map(m => [m.rsid, m]));

Expand Down Expand Up @@ -163,68 +157,6 @@ function getLatestBloodValues(tests) {
return latest;
}

/**
* Replicate callProviderAISimple pattern from taste-questionnaire.js.
* API-type providers only. Returns { text } on success, { error } on failure.
* Exported for unit testing of the non-JSON-body guard — the public entries that
* reach it (refreshCrossDomainNarrative / generateThemeAnalysis) need the full
* genome/taste/health context mocked, so the guard is exercised directly.
*/
export async function callProviderAISimple(provider, model, prompt, { temperature = 0.3, max_tokens = 1000 } = {}) {
const timeout = provider.timeout || DEFAULT_AI_TIMEOUT_MS;

if (provider.type === 'api') {
const ready = await ensureOllamaProviderReady(provider).catch((err) => ({ success: false, error: err.message }));
if (!ready.success) {
return { error: `Ollama is not running and PortOS could not start it: ${ready.error || 'unknown error'}` };
}

// Never send the API key to an arbitrary/metadata host (SSRF / key
// exfiltration). Keyless local-LLM calls skip this guard entirely.
if (provider.apiKey) {
const guard = evaluateSecretEndpoint(provider.endpoint, {
allowCustomEndpoint: provider.allowCustomEndpoint === true,
});
if (!guard.allowed) {
return { error: `Provider endpoint blocked: ${guard.reason}` };
}
}

const headers = { 'Content-Type': 'application/json' };
if (provider.apiKey) headers['Authorization'] = `Bearer ${provider.apiKey}`;

const response = await fetchWithTimeout(`${provider.endpoint}/chat/completions`, {
method: 'POST',
headers,
body: JSON.stringify({
model,
messages: [{ role: 'user', content: prompt }],
temperature,
max_tokens
})
}, timeout);

if (!response.ok) {
const errorText = await response.text().catch(() => 'Unknown error');
return { error: `Provider returned ${response.status}: ${errorText}` };
}

// Sentinel fallback: a non-JSON/blank 200 body must surface as an error, not
// an empty `{ text: '' }` success — both callers persist the result
// (refreshCrossDomainNarrative → narrative.json, generateThemeAnalysis →
// themes.json), so a masqueraded-empty success would overwrite the cached
// narrative/themes with nothing. A valid body (even one with empty content)
// still flows through unchanged.
const data = await readResponseJson(response, { fallback: null, emptyValue: null });
if (!data) {
return { error: `Provider returned a non-JSON response (${response.status})` };
}
return { text: data.choices?.[0]?.message?.content || '' };
}

return { error: 'Insights analysis requires an API-based provider' };
}

// =============================================================================
// EXPORTED FUNCTIONS
// =============================================================================
Expand Down
106 changes: 13 additions & 93 deletions server/services/insightsService.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,101 +1,21 @@
import { describe, it, expect, vi, beforeEach } from 'vitest';

// Mock the provider-readiness check and the HTTP client so the call runs offline.
// Mock the provider-readiness check so any code path that touches it runs offline.
vi.mock('./ollamaManager.js', () => ({
ensureProviderReady: vi.fn().mockResolvedValue({ success: true }),
}));
vi.mock('../lib/fetchWithTimeout.js', () => ({
fetchWithTimeout: vi.fn(),
}));

import { fetchWithTimeout } from '../lib/fetchWithTimeout.js';
import { mockJsonResponse, mockTextResponse } from '../lib/testHelper.js';
import {
callProviderAISimple,
getThemeAnalysis,
getCrossDomainNarrative,
} from './insightsService.js';

const PROVIDER = { type: 'api', endpoint: 'http://localhost:1234/v1' };

describe('insightsService.callProviderAISimple — non-JSON-body guard', () => {
beforeEach(() => {
vi.clearAllMocks();
});

// The regression this guards: a non-JSON 200 body used to return { text: '' },
// which refreshCrossDomainNarrative / generateThemeAnalysis then persisted over
// narrative.json / themes.json — overwriting the cached result with nothing.
// It must surface as { error } so the `if (result.error) return` guard bails
// before any write.
it('returns { error } (not empty success) on a non-JSON 200 body', async () => {
fetchWithTimeout.mockResolvedValue(mockTextResponse('<html><body>502 Bad Gateway</body></html>'));
const result = await callProviderAISimple(PROVIDER, 'm', 'prompt');
expect(result.text).toBeUndefined();
expect(result.error).toMatch(/non-JSON response/);
});

it('returns { error } on a blank 200 body', async () => {
fetchWithTimeout.mockResolvedValue(mockTextResponse(''));
const result = await callProviderAISimple(PROVIDER, 'm', 'prompt');
expect(result.error).toMatch(/non-JSON response/);
});

// A valid body with empty content is a legitimate (if unusual) result and must
// still flow through as { text: '' } — the guard must not conflate valid-empty
// with a parse failure.
it('returns { text: "" } for a valid body with empty content', async () => {
fetchWithTimeout.mockResolvedValue(mockJsonResponse({ choices: [{ message: { content: '' } }] }));
const result = await callProviderAISimple(PROVIDER, 'm', 'prompt');
expect(result).toEqual({ text: '' });
});

it('returns the content for a valid populated body', async () => {
fetchWithTimeout.mockResolvedValue(mockJsonResponse({ choices: [{ message: { content: 'hello' } }] }));
const result = await callProviderAISimple(PROVIDER, 'm', 'prompt');
expect(result).toEqual({ text: 'hello' });
});

it('returns { error } with the status code on a non-2xx response', async () => {
fetchWithTimeout.mockResolvedValue(mockTextResponse('boom', { ok: false, status: 500 }));
const result = await callProviderAISimple(PROVIDER, 'm', 'prompt');
expect(result.error).toMatch(/Provider returned 500: boom/);
});
// generateThemeAnalysis/refreshCrossDomainNarrative now call the shared
// aiProvider.callProviderAISimple transport (see aiProvider.test.js for its own
// contract coverage) — stub only that export so the disk-only read paths below
// can assert it's never reached, while stripCodeFences/parseLLMJSON (also
// imported from this module) stay real.
vi.mock('./aiProvider.js', async (importOriginal) => {
const actual = await importOriginal();
return { ...actual, callProviderAISimple: vi.fn() };
});

describe('insightsService.callProviderAISimple — endpoint guard (SSRF / key-exfiltration)', () => {
beforeEach(() => {
vi.clearAllMocks();
});

it('blocks a keyed provider pointed at a non-allowlisted endpoint and never calls fetch', async () => {
const result = await callProviderAISimple(
{ ...PROVIDER, apiKey: 'secret-key', endpoint: 'https://not-an-allowlisted-host.example' },
'm', 'prompt',
);
expect(result.error).toContain('Provider endpoint blocked');
expect(fetchWithTimeout).not.toHaveBeenCalled();
});

it('allows a keyed provider on a non-allowlisted host through when allowCustomEndpoint is true', async () => {
fetchWithTimeout.mockResolvedValue(mockJsonResponse({ choices: [{ message: { content: 'hello' } }] }));
const result = await callProviderAISimple(
{ ...PROVIDER, apiKey: 'secret-key', endpoint: 'https://not-an-allowlisted-host.example', allowCustomEndpoint: true },
'm', 'prompt',
);
expect(result).toEqual({ text: 'hello' });
expect(fetchWithTimeout).toHaveBeenCalled();
});

it('allows a keyless provider on a non-allowlisted host through (guard only applies when apiKey is set)', async () => {
fetchWithTimeout.mockResolvedValue(mockJsonResponse({ choices: [{ message: { content: 'hello' } }] }));
const result = await callProviderAISimple(
{ ...PROVIDER, endpoint: 'https://not-an-allowlisted-host.example' },
'm', 'prompt',
);
expect(result).toEqual({ text: 'hello' });
});
});
import { callProviderAISimple } from './aiProvider.js';
import { getThemeAnalysis, getCrossDomainNarrative } from './insightsService.js';

// Enforces the no-cold-bootstrap trigger contract documented at the generation
// entry points: the cached-read paths the Insights page mounts with must be
Expand All @@ -109,13 +29,13 @@ describe('insightsService read paths — disk-only, no provider call', () => {

it('getThemeAnalysis performs no provider call (returns not_generated when uncached)', async () => {
const result = await getThemeAnalysis();
expect(fetchWithTimeout).not.toHaveBeenCalled();
expect(callProviderAISimple).not.toHaveBeenCalled();
Comment thread
atomantic marked this conversation as resolved.
expect(result.available === false || result.available === true).toBe(true);
});

it('getCrossDomainNarrative performs no provider call (returns not_generated when uncached)', async () => {
const result = await getCrossDomainNarrative();
expect(fetchWithTimeout).not.toHaveBeenCalled();
expect(callProviderAISimple).not.toHaveBeenCalled();
expect(result.available === false || result.available === true).toBe(true);
});
});