Task/refactor 2 #36

OriNachum · 2025-06-14T00:20:31Z

Chat completions validated

Getting for Responses:
2025-06-14 00:19:07,537 - api_adapter - ERROR - Error in proxy endpoint: cannot access local variable 'response' where it is not associated with a value
INFO: 127.0.0.1:33502 - "POST /chat/completions HTTP/1.1" 500 Internal Server Error
2025-06-14 00:19:08,392 - api_adapter - ERROR - Error in proxy endpoint: cannot access local variable 'response' where it is not associated with a value
INFO: 127.0.0.1:33502 - "POST /chat/completions HTTP/1.1" 500 Internal Server Error

Looks like it broke - I'll fix before merge

src/open_responses_server/api_controller.py

src/open_responses_server/chat_completions_service.py

+                        logger.error(f"Error during chat completions stream proxy: {e}")
+                        yield f"data: {json.dumps({'error': str(e)})}\n\n".encode()
+
+                return StreamingResponse(stream_proxy(), media_type="text/event-stream")


To fix the issue, the exception message (str(e)) should be replaced with a generic error message when sending responses to external users. The detailed exception information should continue to be logged server-side for debugging purposes. This ensures that sensitive information is not exposed while maintaining the ability to diagnose issues internally.

Specifically:

Replace str(e) in the yield statement on line 146 with a generic error message, such as "An internal error occurred during streaming.".

Ensure that the detailed exception information (e) is logged using logger.error() for internal debugging.

…ect of type CallToolResult is not JSON serializable

src/open_responses_server/api_controller.py

+                    yield f"data: {json.dumps({'type': 'error', 'error': {'message': str(e)}})}\n\n"
+
+            return StreamingResponse(
+                stream_response(),


To fix the issue, the exception message (str(e)) should not be included in the response sent to the user. Instead, a generic error message should be returned, while the detailed stack trace is logged on the server for debugging purposes. This approach ensures that sensitive information is not exposed to external users while still allowing developers to diagnose issues.

Steps to implement the fix:

Replace the str(e) in the response payload on line 255 with a generic error message, such as "An internal error has occurred.".

Ensure that the detailed exception message is logged using logger.error for debugging purposes.

Verify that the fix does not alter the application's functionality beyond securing the error handling.

sonarqubecloud · 2025-06-18T09:58:02Z

Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)
3.5% Duplication on New Code (required ≤ 3%)
C Reliability Rating on New Code (required ≥ A)
C Security Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Copilot

Pull Request Overview

Refactors the monolithic server into a modular structure, reorganizes startup scripts and Docker entrypoint, and updates test tooling to use the new /v1/chat/completions path.

Modularized core logic into common/ (config, llm_client, mcp_manager), models/, and dedicated service modules.
Updated CLI, shell scripts, start.py, and Dockerfile to launch the new server_entrypoint.
Consolidated Pydantic models in responses_models.py and streamlined tool injection and execution.

Reviewed Changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
test_tools/call_chat_completions_proxy_mcp.sh	Updated proxy script to call `/v1/chat/completions` and capture output
test_tools/call_chat_completions_proxy.sh	Changed endpoint path to `/v1/chat/completions`
start.py	Added a bash startup script (invokes CLI)
src/open_responses_server/server_entrypoint.py	New entrypoint module for running the FastAPI app via uvicorn
src/open_responses_server/models/responses_models.py	Consolidated and added all response-related Pydantic models
src/open_responses_server/common/mcp_manager.py	Introduced `MCPManager` and `MCPServer` for tool caching/execution
src/open_responses_server/common/llm_client.py	Added `LLMClient` for managing the async HTTP client
src/open_responses_server/common/config.py	Centralized environment variables and logging setup
src/open_responses_server/cli.py	Updated imports and uvicorn invocation to use the new entrypoint
src/open_responses_server/chat_completions_service.py	New service to handle streaming and non‐streaming chat completions with tool support
docs/testing-guide.md	Updated test guidance to mock `server_entrypoint`
Dockerfile	Updated container entrypoint to launch `server_entrypoint:app`

Comments suppressed due to low confidence (2)

Copilot · 2025-06-18T10:18:50Z

start.py

@@ -0,0 +1,3 @@
+#!/bin/bash
+
+uv run ./src/open_responses_server/cli.py start


The command uv run is not a standard invocation and will likely fail. Consider replacing it with uvicorn open_responses_server.cli:app --reload or invoking the CLI via python -m open_responses_server.cli start.

Suggested change

uv run ./src/open_responses_server/cli.py start

python -m open_responses_server.cli start

Copilot · 2025-06-18T10:18:51Z

start.py

@@ -0,0 +1,3 @@
+#!/bin/bash


[nitpick] This file uses a bash shebang but has a .py extension. Either rename it to .sh or switch the shebang to a Python interpreter (e.g., #!/usr/bin/env python3).

Copilot · 2025-06-18T10:18:51Z

src/open_responses_server/chat_completions_service.py

+                            arguments = json.loads(function_call.get("arguments", "{}"))
+                            result = await mcp_manager.execute_mcp_tool(tool_name, arguments)
+                            logger.info(f"Executed tool {tool_name} with result: {result}")
+                            #result is serlized as: "meta=None content=[TextContent(type='text', text="[{'name': 'listings'}]", annotations=None)] isError=False"


Typo in comment: serlized should be serialized.

Suggested change

#result is serlized as: "meta=None content=[TextContent(type='text', text="[{'name': 'listings'}]", annotations=None)] isError=False"

#result is serialized as: "meta=None content=[TextContent(type='text', text="[{'name': 'listings'}]", annotations=None)] isError=False"

Copilot · 2025-06-18T10:18:52Z

src/open_responses_server/common/mcp_manager.py

+        for server in self.mcp_servers:
+            try:
+                # No need to list tools again, just try to execute
+                # A better approach would be to cache which server has which tool
+                # For now, we'll keep the logic of checking before calling
+                tools = await server.list_tools()
+                if any(tool.get("name") == tool_name for tool in tools):
+                    logger.info(f"Executing tool {tool_name} on server {server.name}")
+                    return await server.execute_tool(tool_name, arguments)
+            except Exception:
+                logger.warning(f"Error checking/executing tool on server {server.name}: {traceback.format_exc()}")
+                continue
+        raise RuntimeError(f"Tool '{tool_name}' not found on any active MCP server")


Calling list_tools() on every execute_mcp_tool invocation can be expensive. Consider caching server-to-tool mappings to avoid repeated RPC calls.

Suggested change

for server in self.mcp_servers:

try:

# No need to list tools again, just try to execute

# A better approach would be to cache which server has which tool

# For now, we'll keep the logic of checking before calling

tools = await server.list_tools()

if any(tool.get("name") == tool_name for tool in tools):

logger.info(f"Executing tool {tool_name} on server {server.name}")

return await server.execute_tool(tool_name, arguments)

except Exception:

logger.warning(f"Error checking/executing tool on server {server.name}: {traceback.format_exc()}")

continue

raise RuntimeError(f"Tool '{tool_name}' not found on any active MCP server")

server = self.tool_to_server_map.get(tool_name)

if server:

try:

logger.info(f"Executing tool {tool_name} on server {server.name}")

return await server.execute_tool(tool_name, arguments)

except Exception:

logger.warning(f"Error executing tool {tool_name} on server {server.name}: {traceback.format_exc()}")

raise RuntimeError(f"Error executing tool '{tool_name}' on server {server.name}")

raise RuntimeError(f"Tool '{tool_name}' not found on any active MCP server")

raise RuntimeError(f"Tool '{tool_name}' not found on any active MCP server")

Copilot · 2025-06-18T10:18:52Z

src/open_responses_server/common/mcp_manager.py

+    async def shutdown_mcp_servers(self):
+        """Clean up all MCP servers on shutdown."""
+        if self._refresh_task:
+            self._refresh_task.cancel()


Cancelling the _refresh_task without awaiting it may leave the background loop running. Consider awaiting task termination or handling asyncio.CancelledError for a clean shutdown.

Suggested change

self._refresh_task.cancel()

self._refresh_task.cancel()

try:

await self._refresh_task

except asyncio.CancelledError:

logger.info("Refresh task was cancelled cleanly.")

OriNachum and others added 5 commits June 12, 2025 16:45

refactor and chat completions

992dbb8

fix(cli): adapt to refactor

fd1e59a

fix(mcp): load servers from configured file location

6d89ab1

fix(mcp): chat completions

a3471da

fix(mcp): response fastapi

15c4203

github-advanced-security bot found potential problems Jun 14, 2025

View reviewed changes

src/open_responses_server/api_controller.py Fixed Show fixed Hide fixed

src/open_responses_server/chat_completions_service.py Fixed Show fixed Hide fixed

fix(mcp): run mcp on chat completions

631f8e3

github-advanced-security bot found potential problems Jun 14, 2025

View reviewed changes

OriNachum and others added 7 commits June 15, 2025 10:19

fix(mcp): api_adapter - ERROR - Error executing tool list_tables: Obj…

0da7d7c

…ect of type CallToolResult is not JSON serializable

add back server.py

8132bec

refactor plan

a675c7a

Milestone: refactor finised

ca5b992

refactor log

ae60688

easy testing

368e8b9

fix(responses): correctly apply server

7de46a9

github-advanced-security bot found potential problems Jun 18, 2025

View reviewed changes

OriNachum added 3 commits June 18, 2025 08:36

fix chat completions

aab409c

add log

31feaff

test fixes

b6544b9

OriNachum marked this pull request as ready for review June 18, 2025 10:14

OriNachum requested a review from Copilot June 18, 2025 10:14

OriNachum merged commit c4c91ba into main Jun 18, 2025
7 of 11 checks passed

OriNachum deleted the task/refactor-2 branch June 18, 2025 10:17

Copilot AI reviewed Jun 18, 2025

View reviewed changes

@@ -145,3 +145,3 @@
                                     logger.error(f"Error during chat completions stream proxy: {e}")
-                                    yield f"data: {json.dumps({'error': str(e)})}\n\n".encode()
+                                    yield f"data: {json.dumps({'error': 'An internal error occurred during streaming.'})}\n\n".encode()
@@ -152,3 +152,3 @@
                         async def error_stream():
-                            yield f"data: {json.dumps({'error': str(e)})}\n\n".encode()
+                            yield f"data: {json.dumps({'error': 'An internal error occurred during streaming.'})}\n\n".encode()
                         return StreamingResponse(error_stream(), media_type="text/event-stream", status_code=500)

@@ -254,3 +254,3 @@
                                 logger.error(f"Error in stream_response: {str(e)}")
-                                yield f"data: {json.dumps({'type': 'error', 'error': {'message': str(e)}})}\n\n"
+                                yield f"data: {json.dumps({'type': 'error', 'error': {'message': 'An internal error has occurred.'}})}\n\n"
@@ -268,3 +268,3 @@
                         status_code=500,
-                        detail=f"Error processing request: {str(e)}"
+                        detail="An internal error has occurred."
                     )

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Task/refactor 2 #36

Task/refactor 2 #36

Uh oh!

OriNachum commented Jun 14, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check warning

Copilot Autofix

Check warning

Copilot Autofix

sonarqubecloud bot commented Jun 18, 2025

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jun 18, 2025

Uh oh!

Copilot AI Jun 18, 2025

Uh oh!

Copilot AI Jun 18, 2025

Uh oh!

Copilot AI Jun 18, 2025

Uh oh!

Copilot AI Jun 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -0,0 +1,3 @@
		#!/bin/bash

		uv run ./src/open_responses_server/cli.py start No newline at end of file

	uv run ./src/open_responses_server/cli.py start
	python -m open_responses_server.cli start

	#result is serlized as: "meta=None content=[TextContent(type='text', text="[{'name': 'listings'}]", annotations=None)] isError=False"
	#result is serialized as: "meta=None content=[TextContent(type='text', text="[{'name': 'listings'}]", annotations=None)] isError=False"

-            self._refresh_task.cancel()
+            self._refresh_task.cancel()
+            try:
+                await self._refresh_task
+            except asyncio.CancelledError:
+                logger.info("Refresh task was cancelled cleanly.")

Task/refactor 2 #36

Task/refactor 2 #36

Uh oh!

Conversation

OriNachum commented Jun 14, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check warning

Uh oh!

Copilot Autofix

Check warning

Uh oh!

Copilot Autofix

sonarqubecloud bot commented Jun 18, 2025

Quality Gate failed

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants