mirror of
https://github.com/FJurmanovic/sugarr.git
synced 2026-11-10 23:31:08 +00:00
Fix final review findings: key leak, model fallback, error resilience, docs
- ApiClient: strip query string from ApiError messages so API keys (e.g. TMDb's api_key query param) never leak into 500 response bodies or logs. - LlmTool: accept an optional defaultModel, wired from AppConfig's LLM_MODEL, so Langfuse prompts that omit a model config fall back to the configured LLM rather than silently requesting gpt-4o-mini. - RecommendationPipeline: restore the Go original's graceful degradation — library/Radarr fetch failures, per-suggestion TMDb resolution failures, and per-signal TMDb similarity/recommendation failures are now logged and skipped instead of aborting the whole run (applies to both runInternal and searchAsync). - RecommendationsController: validate the ?count= query param (must be a positive integer) instead of silently producing NaN. - Commit the Langfuse prompt definitions (langfuse/*.prompt.json) and add a README covering setup, prompt seeding, cache invalidation, and the HTTP API surface — the branch was not otherwise runnable by anyone but the author.
This commit is contained in:
@@ -28,6 +28,20 @@ describe('RecommendationsController', () => {
|
||||
expect(runAsync).toHaveBeenCalledWith({ count: 3, useLlm: true });
|
||||
});
|
||||
|
||||
it('falls back to the configured count for a malformed or non-positive count query param', async () => {
|
||||
const runAsync = vi.fn().mockResolvedValue([]);
|
||||
const app = makeApp({ RECOMMEND_COUNT: 10, LLM_ENABLED: true }, { runAsync } as unknown as RecommendationPipeline);
|
||||
|
||||
await app.inject({ method: 'GET', url: '/recommendations?count=abc' });
|
||||
expect(runAsync).toHaveBeenCalledWith({ count: 10, useLlm: true });
|
||||
|
||||
await app.inject({ method: 'GET', url: '/recommendations?count=-5' });
|
||||
expect(runAsync).toHaveBeenCalledWith({ count: 10, useLlm: true });
|
||||
|
||||
await app.inject({ method: 'GET', url: '/recommendations?count=0' });
|
||||
expect(runAsync).toHaveBeenCalledWith({ count: 10, useLlm: true });
|
||||
});
|
||||
|
||||
it('POST /recommendations/trigger auto-requests via Radarr when enabled', async () => {
|
||||
const recs = [{ title: 'Dune', year: 2021, tmdbId: 1, reason: 'r' }];
|
||||
const runAsync = vi.fn().mockResolvedValue(recs);
|
||||
|
||||
@@ -171,6 +171,119 @@ describe('RecommendationPipeline.runAsync', () => {
|
||||
expect(tool.runAsync).not.toHaveBeenCalled();
|
||||
expect(result).toEqual([{ title: 'Similar A', year: 2010, tmdbId: 20, reason: 'Recommended by TMDb' }]);
|
||||
});
|
||||
|
||||
it('continues with an incomplete exclusion set when fetching the library fails', async () => {
|
||||
const jellyfin = {
|
||||
getFavoriteMovies: vi.fn().mockResolvedValue([movie('Dune', 2021, '1')]),
|
||||
getRecentlyWatched: vi.fn(),
|
||||
getAllMovies: vi.fn().mockRejectedValue(new Error('jellyfin down')),
|
||||
} as unknown as JellyfinClient;
|
||||
const tmdb = {
|
||||
searchMovie: vi.fn().mockResolvedValue({ id: 3, title: 'Interstellar', year: 2014 } as TMDbMovie),
|
||||
} as unknown as TmdbClient;
|
||||
const tool = {
|
||||
runAsync: vi.fn().mockResolvedValue([{ title: 'Interstellar', year: 2014, reason: 'r' }]),
|
||||
} as unknown as MovieRecommenderTool;
|
||||
const searchTool = { runAsync: vi.fn() } as unknown as MovieSearchTool;
|
||||
const logger = { warn: vi.fn() } as unknown as Logger;
|
||||
|
||||
const pipeline = new RecommendationPipeline(makeTracer(), logger, {
|
||||
jellyfin,
|
||||
tmdb,
|
||||
tool,
|
||||
searchTool,
|
||||
minRatedMovies: 1,
|
||||
maxRetry: 0,
|
||||
});
|
||||
|
||||
const result = await pipeline.runAsync({ count: 1, useLlm: true });
|
||||
|
||||
expect(result).toEqual([{ title: 'Interstellar', year: 2014, tmdbId: 3, reason: 'r' }]);
|
||||
expect(logger.warn).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ err: expect.any(Error) }),
|
||||
'Could not fetch library; exclusion may be incomplete'
|
||||
);
|
||||
});
|
||||
|
||||
it('skips a suggestion whose TMDb resolution fails instead of aborting the run', async () => {
|
||||
const jellyfin = {
|
||||
getFavoriteMovies: vi.fn().mockResolvedValue([movie('Dune', 2021, '1')]),
|
||||
getRecentlyWatched: vi.fn(),
|
||||
getAllMovies: vi.fn().mockResolvedValue([]),
|
||||
} as unknown as JellyfinClient;
|
||||
const tmdb = {
|
||||
searchMovie: vi
|
||||
.fn()
|
||||
.mockRejectedValueOnce(new Error('tmdb down'))
|
||||
.mockResolvedValueOnce({ id: 3, title: 'Interstellar', year: 2014 } as TMDbMovie),
|
||||
} as unknown as TmdbClient;
|
||||
const tool = {
|
||||
runAsync: vi.fn().mockResolvedValue([
|
||||
{ title: 'Unresolvable', year: 1999, reason: 'r1' },
|
||||
{ title: 'Interstellar', year: 2014, reason: 'r2' },
|
||||
]),
|
||||
} as unknown as MovieRecommenderTool;
|
||||
const searchTool = { runAsync: vi.fn() } as unknown as MovieSearchTool;
|
||||
const logger = { warn: vi.fn() } as unknown as Logger;
|
||||
|
||||
const pipeline = new RecommendationPipeline(makeTracer(), logger, {
|
||||
jellyfin,
|
||||
tmdb,
|
||||
tool,
|
||||
searchTool,
|
||||
minRatedMovies: 1,
|
||||
maxRetry: 0,
|
||||
});
|
||||
|
||||
const result = await pipeline.runAsync({ count: 2, useLlm: true });
|
||||
|
||||
expect(result).toEqual([{ title: 'Interstellar', year: 2014, tmdbId: 3, reason: 'r2' }]);
|
||||
expect(logger.warn).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ err: expect.any(Error), title: 'Unresolvable' }),
|
||||
'TMDb search failed, skipping suggestion'
|
||||
);
|
||||
});
|
||||
|
||||
it('skips a signal movie whose TMDb similar/recommendations lookup fails in the fallback path', async () => {
|
||||
const jellyfin = {
|
||||
getFavoriteMovies: vi
|
||||
.fn()
|
||||
.mockResolvedValue([movie('Broken', 2000, '1'), movie('Dune', 2021, '2')]),
|
||||
getRecentlyWatched: vi.fn(),
|
||||
getAllMovies: vi.fn().mockResolvedValue([]),
|
||||
} as unknown as JellyfinClient;
|
||||
const tmdb = {
|
||||
getSimilar: vi.fn().mockImplementation((id: number) => {
|
||||
if (id === 1) return Promise.reject(new Error('tmdb down'));
|
||||
return Promise.resolve([{ id: 20, title: 'Similar A', year: 2010 } as TMDbMovie]);
|
||||
}),
|
||||
getRecommendations: vi.fn().mockImplementation((id: number) => {
|
||||
if (id === 1) return Promise.reject(new Error('tmdb down'));
|
||||
return Promise.resolve([]);
|
||||
}),
|
||||
searchMovie: vi.fn().mockResolvedValue({ id: 20, title: 'Similar A', year: 2010 } as TMDbMovie),
|
||||
} as unknown as TmdbClient;
|
||||
const tool = { runAsync: vi.fn() } as unknown as MovieRecommenderTool;
|
||||
const searchTool = { runAsync: vi.fn() } as unknown as MovieSearchTool;
|
||||
const logger = { warn: vi.fn() } as unknown as Logger;
|
||||
|
||||
const pipeline = new RecommendationPipeline(makeTracer(), logger, {
|
||||
jellyfin,
|
||||
tmdb,
|
||||
tool,
|
||||
searchTool,
|
||||
minRatedMovies: 2,
|
||||
maxRetry: 0,
|
||||
});
|
||||
|
||||
const result = await pipeline.runAsync({ count: 1, useLlm: false });
|
||||
|
||||
expect(result).toEqual([{ title: 'Similar A', year: 2010, tmdbId: 20, reason: 'Recommended by TMDb' }]);
|
||||
expect(logger.warn).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ err: expect.any(Error), tmdbId: 1 }),
|
||||
'TMDb similar/recommendations failed, skipping signal movie'
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('RecommendationPipeline.autoRequest', () => {
|
||||
@@ -283,4 +396,34 @@ describe('RecommendationPipeline.searchAsync', () => {
|
||||
expect(result).toEqual([{ title: 'New Find', year: 2020, tmdbId: 6, reason: 'matches query' }]);
|
||||
expect(searchTool.runAsync).toHaveBeenCalledWith({ query: 'sci-fi like Dune', count: 5 }, expect.anything());
|
||||
});
|
||||
|
||||
it('continues with an empty exclusion set when fetching the library fails', async () => {
|
||||
const jellyfin = {
|
||||
getAllMovies: vi.fn().mockRejectedValue(new Error('jellyfin down')),
|
||||
} as unknown as JellyfinClient;
|
||||
const tmdb = {
|
||||
searchMovie: vi.fn().mockResolvedValue({ id: 6, title: 'New Find', year: 2020 } as TMDbMovie),
|
||||
} as unknown as TmdbClient;
|
||||
const searchTool = {
|
||||
runAsync: vi.fn().mockResolvedValue([{ title: 'New Find', year: 2020, reason: 'matches query' }]),
|
||||
} as unknown as MovieSearchTool;
|
||||
const logger = { warn: vi.fn() } as unknown as Logger;
|
||||
|
||||
const pipeline = new RecommendationPipeline(makeTracer(), logger, {
|
||||
jellyfin,
|
||||
tmdb,
|
||||
tool: {} as MovieRecommenderTool,
|
||||
searchTool,
|
||||
minRatedMovies: 1,
|
||||
maxRetry: 0,
|
||||
});
|
||||
|
||||
const result = await pipeline.searchAsync('sci-fi like Dune', 5);
|
||||
|
||||
expect(result).toEqual([{ title: 'New Find', year: 2020, tmdbId: 6, reason: 'matches query' }]);
|
||||
expect(logger.warn).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ err: expect.any(Error) }),
|
||||
'Could not fetch library; exclusion may be incomplete'
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -101,4 +101,11 @@ describe('ApiClient', () => {
|
||||
const client = new TestClient('https://api.example.com');
|
||||
await expect(client.getThing(1)).rejects.toBeInstanceOf(ApiError);
|
||||
});
|
||||
|
||||
it('omits the query string (which may carry secrets) from the ApiError message', async () => {
|
||||
fetchMock.mockResolvedValue({ ok: false, status: 500, text: async () => 'boom' });
|
||||
|
||||
const client = new TestClient('https://api.example.com');
|
||||
await expect(client.getThing(1)).rejects.toThrow('GET https://api.example.com/things/1 returned 500');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -60,6 +60,44 @@ describe('LlmTool.runAsync', () => {
|
||||
);
|
||||
});
|
||||
|
||||
it('falls back to the constructor-provided defaultModel when the prompt config omits model', async () => {
|
||||
const getCompiled = vi
|
||||
.fn()
|
||||
.mockResolvedValue({ name: 'greet', version: 1, config: {}, messages: [{ role: 'user', content: 'hi' }] });
|
||||
const promptService = { getCompiled } as unknown as PromptService;
|
||||
const chat = vi.fn().mockResolvedValue({ content: 'hi' });
|
||||
const llmClient = { chat } as unknown as LlmClient;
|
||||
|
||||
const { ctx } = makeCtx();
|
||||
const tool = new GreetTool(promptService, llmClient, 'llama3');
|
||||
|
||||
await tool.runAsync({ who: 'alice' }, ctx);
|
||||
|
||||
expect(chat).toHaveBeenCalledWith(
|
||||
[{ role: 'user', content: 'hi' }],
|
||||
{ model: 'llama3', temperature: undefined, maxTokens: undefined }
|
||||
);
|
||||
});
|
||||
|
||||
it('falls back to gpt-4o-mini when no defaultModel is given and the prompt config omits model', async () => {
|
||||
const getCompiled = vi
|
||||
.fn()
|
||||
.mockResolvedValue({ name: 'greet', version: 1, config: {}, messages: [{ role: 'user', content: 'hi' }] });
|
||||
const promptService = { getCompiled } as unknown as PromptService;
|
||||
const chat = vi.fn().mockResolvedValue({ content: 'hi' });
|
||||
const llmClient = { chat } as unknown as LlmClient;
|
||||
|
||||
const { ctx } = makeCtx();
|
||||
const tool = new GreetTool(promptService, llmClient);
|
||||
|
||||
await tool.runAsync({ who: 'alice' }, ctx);
|
||||
|
||||
expect(chat).toHaveBeenCalledWith(
|
||||
[{ role: 'user', content: 'hi' }],
|
||||
{ model: 'gpt-4o-mini', temperature: undefined, maxTokens: undefined }
|
||||
);
|
||||
});
|
||||
|
||||
it('ends the generation with error metadata and rethrows when the LLM call fails', async () => {
|
||||
const promptService = {
|
||||
getCompiled: vi.fn().mockResolvedValue({ name: 'greet', version: 1, config: {}, messages: [] }),
|
||||
|
||||
Reference in New Issue
Block a user