OpenAI conversation: Fixing Failure API response #330
Conversation
WalkthroughRefactors payload construction in backend/app/api/routes/responses.py: callbacks (success/error) now send plain dicts with merged additional data; internal error responses use ResponsesAPIResponse.failure_response; /responses auth/config error path returns get_additional_data directly. No public signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Responses as /responses (async)
participant Processor
participant Webhook
Client->>Responses: POST /responses
alt Auth/Config error
Responses-->>Client: {success:false, data:get_additional_data(...), error:<msg>, metadata:null}
else Accepted
Responses-->>Client: {success:true, data:{status,message,additional_data:...}}
Responses->>Processor: enqueue/process
alt Processing error
Processor-->>Webhook: {success:false, data:{... merged with get_additional_data(...)}, error:<msg>, metadata:null}
else Processing success
Processor-->>Webhook: {success:true, data:{... merged with get_additional_data(...)}, error:null, metadata:null}
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
backend/app/api/routes/responses.py (1)
257-257: Inconsistent data payload structure between sync and async responses.Line 257 adds additional data fields directly to the
_APIResponseobject using**get_additional_data(request_dict), but the sync endpoint (line 477) no longer includes this additional data. This creates inconsistency between the two endpoints.For consistency, either both endpoints should include additional data or neither should. Apply this diff to remove the additional data spread from the async endpoint to match the sync endpoint:
- **get_additional_data(request_dict),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/api/routes/responses.py(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/api/routes/responses.py (3)
backend/app/api/routes/threads.py (2)
send_callback(37-49)get_additional_data(128-134)backend/app/utils.py (2)
APIResponse(27-48)failure_response(40-48)backend/app/api/routes/collections.py (1)
WebHookCallback(147-170)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (3)
backend/app/api/routes/responses.py (3)
326-326: LGTM: Consistent metadata usage for additional data.The refactoring correctly moves additional data to the
datafield in error responses, maintaining consistency with the overall error response structure.
392-395: LGTM: Proper use of metadata field for additional data.The refactoring correctly moves additional data from the
datafield to themetadatafield usingAPIResponse.failure_response, which is more semantically appropriate for auxiliary request information.
489-491: LGTM: Consistent error response structure.The error handling properly uses
metadatafor additional data while keeping the main error message in theerrorfield, maintaining consistency with theAPIResponsepattern.
|
|
||
| # Send callback with webhook-specific response format | ||
| send_callback( | ||
| request.callback_url, | ||
| { | ||
| "success": False, | ||
| "data": get_additional_data(request_dict), | ||
| "error": error_message, | ||
| "metadata": None, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Critical logic error: Success case callback is replaced with error case callback.
The successful response callback (lines 244-259) is being overwritten by the error case callback. This means that successful API responses will never be sent to the callback URL, only error responses will be sent.
The success callback code appears to be missing. Apply this diff to restore the missing success callback:
+ # Send success callback
+ if request.callback_url:
+ logger.info(
+ f"[process_response] Sending success callback to URL: {request.callback_url}, assistant={mask_string(request.assistant_id)}, project_id={project_id}"
+ )
+ send_callback(request.callback_url, callback_response.model_dump())
+ logger.info(
+ f"[process_response] Success callback sent, assistant={mask_string(request.assistant_id)}, project_id={project_id}"
+ )
+
+ return
+
except openai.OpenAIError as e:
error_message = handle_openai_error(e)
logger.error(
f"[process_response] OpenAI API error during response processing: {error_message}, project_id={project_id}",
exc_info=True,
)
tracer.log_error(error_message, response_id=request.response_id)
request_dict = request.model_dump()
- tracer.flush()
+ tracer.flush()
- if request.callback_url:
- logger.info(
- f"[process_response] Sending callback to URL: {request.callback_url}, assistant={mask_string(request.assistant_id)}, project_id={project_id}"
- )
+ if request.callback_url:
+ logger.info(
+ f"[process_response] Sending error callback to URL: {request.callback_url}, assistant={mask_string(request.assistant_id)}, project_id={project_id}"
+ )
- # Send callback with webhook-specific response format
- send_callback(
- request.callback_url,
- {
- "success": False,
- "data": get_additional_data(request_dict),
- "error": error_message,
- "metadata": None,
- },
- )
- logger.info(
- f"[process_response] Callback sent successfully, assistant={mask_string(request.assistant_id)}, project_id={project_id}"
- )
+ # Send callback with webhook-specific response format
+ send_callback(
+ request.callback_url,
+ {
+ "success": False,
+ "data": get_additional_data(request_dict),
+ "error": error_message,
+ "metadata": None,
+ },
+ )
+ logger.info(
+ f"[process_response] Error callback sent, assistant={mask_string(request.assistant_id)}, project_id={project_id}"
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Send callback with webhook-specific response format | |
| send_callback( | |
| request.callback_url, | |
| { | |
| "success": False, | |
| "data": get_additional_data(request_dict), | |
| "error": error_message, | |
| "metadata": None, | |
| }, | |
| ) | |
| # Send success callback | |
| if request.callback_url: | |
| logger.info( | |
| f"[process_response] Sending success callback to URL: {request.callback_url}, assistant={mask_string(request.assistant_id)}, project_id={project_id}" | |
| ) | |
| send_callback(request.callback_url, callback_response.model_dump()) | |
| logger.info( | |
| f"[process_response] Success callback sent, assistant={mask_string(request.assistant_id)}, project_id={project_id}" | |
| ) | |
| return | |
| except openai.OpenAIError as e: | |
| error_message = handle_openai_error(e) | |
| logger.error( | |
| f"[process_response] OpenAI API error during response processing: {error_message}, project_id={project_id}", | |
| exc_info=True, | |
| ) | |
| tracer.log_error(error_message, response_id=request.response_id) | |
| request_dict = request.model_dump() | |
| tracer.flush() | |
| if request.callback_url: | |
| logger.info( | |
| f"[process_response] Sending error callback to URL: {request.callback_url}, assistant={mask_string(request.assistant_id)}, project_id={project_id}" | |
| ) | |
| # Send callback with webhook-specific response format | |
| send_callback( | |
| request.callback_url, | |
| { | |
| "success": False, | |
| "data": get_additional_data(request_dict), | |
| "error": error_message, | |
| "metadata": None, | |
| }, | |
| ) | |
| logger.info( | |
| f"[process_response] Error callback sent, assistant={mask_string(request.assistant_id)}, project_id={project_id}" | |
| ) |
🤖 Prompt for AI Agents
In backend/app/api/routes/responses.py around lines 275 to 285, the error-case
send_callback is overwriting the earlier success-case callback so successful
responses never get sent; restore the missing success callback by re-inserting
the original send_callback call used for successful responses (the one that
sends success=True, the actual response data, and metadata) before the error
handling path, and ensure the error-path only runs on exceptions so it sends the
failure payload (success=False) without replacing the success callback.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
backend/app/api/routes/responses.py (2)
89-91: Add the missingfailure_responseclassmethod toResponsesAPIResponse.The code on line 268 calls
ResponsesAPIResponse.failure_response(error=error_message), but this method is not defined in theResponsesAPIResponseclass. According to the AI summary, this method was added but it's missing from the implementation.Apply this diff to add the missing method:
class ResponsesAPIResponse(APIResponse[_APIResponse]): - pass + @classmethod + def failure_response(cls, error: str) -> "ResponsesAPIResponse": + """Create a failure response with error message.""" + return cls(success=False, data=None, error=error, metadata=None)The base
APIResponseclass inbackend/app/utils.pyhas a similarfailure_responsemethod that you can reference for the implementation pattern.
98-99: Fix variable reference error inget_file_search_results.The function incorrectly references
resultsinstead oftool_call.resultswhen creatingFileResultChunkobjects. This creates an infinite loop that will cause aRecursionError.Apply this diff to fix the variable reference:
def get_file_search_results(response): results: list[FileResultChunk] = [] for tool_call in response.output: if tool_call.type == "file_search_call": results.extend( - [FileResultChunk(score=hit.score, text=hit.text) for hit in results] + [FileResultChunk(score=hit.score, text=hit.text) for hit in tool_call.results] ) return results
♻️ Duplicate comments (1)
backend/app/api/routes/responses.py (1)
259-270: Success callback is still missing from the success path.The past review comment correctly identified that the success callback is missing. After the success case (lines 244-258), the code should send a success callback before reaching the except block. Currently, only the error callback (lines 272-293) will be sent regardless of success or failure.
Apply this diff to add the missing success callback:
) ) + + # Send success callback + if request.callback_url: + logger.info( + f"[process_response] Sending success callback to URL: {request.callback_url}, assistant={mask_string(request.assistant_id)}, project_id={project_id}" + ) + callback_data = callback_response.model_dump() + send_callback( + request.callback_url, + { + "success": callback_data.get("success", False), + "data": { + **(callback_data.get("data") or {}), + **get_additional_data(request_dict), + }, + "error": callback_data.get("error"), + "metadata": None, + }, + ) + logger.info( + f"[process_response] Success callback sent, assistant={mask_string(request.assistant_id)}, project_id={project_id}" + ) + + tracer.flush() + return + except openai.OpenAIError as e: error_message = handle_openai_error(e) logger.error( f"[process_response] OpenAI API error during response processing: {error_message}, project_id={project_id}", exc_info=True, ) tracer.log_error(error_message, response_id=request.response_id) request_dict = request.model_dump() callback_response = ResponsesAPIResponse.failure_response(error=error_message) - - tracer.flush() - - if request.callback_url: - logger.info( - f"[process_response] Sending callback to URL: {request.callback_url}, assistant={mask_string(request.assistant_id)}, project_id={project_id}" - ) - - # Send callback with webhook-specific response format - callback_data = callback_response.model_dump() - send_callback( - request.callback_url, - { - "success": callback_data.get("success", False), - "data": { - **(callback_data.get("data") or {}), - **get_additional_data(request_dict), - }, - "error": callback_data.get("error"), - "metadata": None, - }, - ) - logger.info( - f"[process_response] Callback sent successfully, assistant={mask_string(request.assistant_id)}, project_id={project_id}" - ) + + # Send error callback + if request.callback_url: + logger.info( + f"[process_response] Sending error callback to URL: {request.callback_url}, assistant={mask_string(request.assistant_id)}, project_id={project_id}" + ) + callback_data = callback_response.model_dump() + send_callback( + request.callback_url, + { + "success": callback_data.get("success", False), + "data": { + **(callback_data.get("data") or {}), + **get_additional_data(request_dict), + }, + "error": callback_data.get("error"), + "metadata": None, + }, + ) + logger.info( + f"[process_response] Error callback sent, assistant={mask_string(request.assistant_id)}, project_id={project_id}" + ) + + tracer.flush()
🧹 Nitpick comments (1)
backend/app/api/routes/responses.py (1)
329-334: Consider using consistent error response structure.The error response structure here differs from the one used in the synchronous endpoint (lines 404-409) which uses
APIResponseclass. Consider using the same response class for consistency.Apply this diff for consistency:
- return { - "success": False, - "error": "OpenAI API key not configured for this organization.", - "data": additional_data if additional_data else None, - "metadata": None, - } + return APIResponse( + success=False, + data=additional_data if additional_data else None, + error="OpenAI API key not configured for this organization.", + metadata=None, + ).model_dump()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/api/routes/responses.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/api/routes/responses.py (2)
backend/app/utils.py (1)
failure_response(40-48)backend/app/api/routes/threads.py (2)
send_callback(37-49)get_additional_data(128-134)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (1)
backend/app/api/routes/responses.py (1)
278-290: Verify webhook payload backward compatibility in responses.pyThe async
/responseshandler now builds the callback payload as:payload["data"] = { **(callback_data.get("data") or {}), **get_additional_data(request_dict), }Any key in
get_additional_data(request_dict)that matches a key incallback_data["data"]will overwrite the original value.• File:
backend/app/api/routes/responses.py(lines 278–290)
• Async exclusion: assistant_id, callback_url, response_id, question
• Sync exclusion: model, instructions, vector_store_ids, max_num_results, temperatureI did not find any tests in
backend/app/tests/api/routesthat exercisesend_callbackfor the responses endpoints (there are tests for threads but none for responses).Please:
- Confirm that your external webhook consumers handle the merged payload structure.
- Verify there are no overlapping field names between
callback_data["data"]and the remaining request fields.- Add unit tests (e.g. a
test_responses_*.py) to coversend_callback()’s payload shape and merge behavior.
1 participant
Summary
Target issue is #317
Explain the motivation for making this change. What existing problem does the pull request solve?
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
Summary by CodeRabbit