diff --git a/workspaces/lightspeed/.changeset/shiny-foxes-film.md b/workspaces/lightspeed/.changeset/shiny-foxes-film.md new file mode 100644 index 0000000000..fd895ca363 --- /dev/null +++ b/workspaces/lightspeed/.changeset/shiny-foxes-film.md @@ -0,0 +1,5 @@ +--- +'@red-hat-developer-hub/backstage-plugin-lightspeed-backend': patch +--- + +add return statement to /v1/feedback error handler to prevent unhandled error fallthrough diff --git a/workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.test.ts b/workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.test.ts index 53bd5118db..c181b733f8 100644 --- a/workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.test.ts +++ b/workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.test.ts @@ -238,7 +238,7 @@ describe('lightspeed router tests', () => { expect(response.body.error).toBeDefined(); }); - it('should return 500 error when conversation does not exist', async () => { + it('should return upstream status code when conversation does not exist', async () => { const backendServer = await startBackendServer(); const response = await request(backendServer) .put(`/api/lightspeed/v2/conversations/${mockAnotherConversationId}`) @@ -246,7 +246,7 @@ describe('lightspeed router tests', () => { topic_summary: 'new topic', }); - expect(response.statusCode).toEqual(500); + expect(response.statusCode).toEqual(404); expect(response.body.error).toContain('not found'); }); @@ -399,6 +399,40 @@ describe('lightspeed router tests', () => { }); expect(feedbackResponse.statusCode).toEqual(403); }); + + it('should handle upstream server errors properly', async () => { + const backendServer = await startBackendServer(); + rcs.use( + http.post(`${LOCAL_LCS_ADDR}/v1/feedback`, () => { + return new HttpResponse( + JSON.stringify({ + error: { + message: 'Internal server error', + }, + }), + { + status: 500, + headers: { 'Content-Type': 'application/json' }, + }, + ); + }), + ); + + const response = await request(backendServer) + .post('/api/lightspeed/v1/feedback') + .send({ + conversation_id: '12345678-abcd-0000-0123-456789abcdef', + llm_response: 'bar', + sentiment: 1, + user_feedback: 'Great service!', + user_question: 'foo', + }); + + expect(response.statusCode).toEqual(500); + expect(response.body.error).toContain( + 'Error from lightspeed-core server', + ); + }); }); describe('GET /v1/feedback/status', () => { @@ -683,7 +717,7 @@ describe('lightspeed router tests', () => { ); }); - it('returns 500 if unexpected error', async () => { + it('returns upstream status code on error', async () => { const backendServer = await startBackendServer(); const nonExistentModel = 'nonexistent-model'; rcs.use( @@ -707,7 +741,10 @@ describe('lightspeed router tests', () => { provider: 'test-server', query: 'Hello', }); - expect(response.statusCode).toEqual(500); + expect(response.statusCode).toEqual(404); + expect(response.body.error).toContain( + 'Error from lightspeed-core server', + ); }); }); diff --git a/workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.ts b/workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.ts index f102570df3..b415143dff 100644 --- a/workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.ts +++ b/workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.ts @@ -525,10 +525,11 @@ export async function createRouter( const errormsg = `Error from lightspeed-core server: ${errorBody.error?.message || errorBody?.detail?.cause || 'Unknown error'}`; logger.error(errormsg); - // Return a 500 status for any upstream error - response.status(500).json({ + response.status(fetchResponse.status).json({ error: errormsg, }); + + return; } const data = await fetchResponse.json(); @@ -570,7 +571,7 @@ export async function createRouter( const errorBody = await fetchResponse.json(); const errormsg = `Error from lightspeed-core server: ${errorBody.error?.message || errorBody?.detail?.cause || 'Unknown error'}`; logger.error(errormsg); - response.status(500).json({ error: errormsg }); + response.status(fetchResponse.status).json({ error: errormsg }); return; } response.status(fetchResponse.status).json(await fetchResponse.json()); @@ -650,8 +651,7 @@ export async function createRouter( const errormsg = `Error from lightspeed-core server: ${errorBody.error?.message || errorBody?.detail?.cause || 'Unknown error'}`; logger.error(errormsg); - // Return a 500 status for any upstream error - response.status(500).json({ + response.status(fetchResponse.status).json({ error: errormsg, }); @@ -707,8 +707,7 @@ export async function createRouter( const errormsg = `Error from lightspeed-core server: ${errorBody.error?.message || errorBody?.detail?.cause || 'Unknown error'}`; logger.error(errormsg); - // Return a 500 status for any upstream error - response.status(500).json({ + response.status(fetchResponse.status).json({ error: errormsg, }); return;