fix: restore error handling in deliberate execution service#298
fix: restore error handling in deliberate execution service#298ArthurCRodrigues merged 2 commits intomainfrom
Conversation
The except block was accidentally removed during the asset injection refactor, causing unhandled exceptions (e.g. S3 asset not found) to propagate as raw 500 errors instead of structured SYSTEM_ERROR responses.
There was a problem hiding this comment.
Pull request overview
Restores structured error handling in the Deliberate Code Execution (DCE) service so unexpected failures (e.g., during asset resolution/injection or sandbox operations) return SYSTEM_ERROR results instead of propagating as unhandled 500s.
Changes:
- Reintroduced a broad
exceptblock in the DCE service to catch unexpected exceptions. - Logs the exception with traceback and returns a
DeliberateCodeExecutionResponsecontaining aSYSTEM_ERRORresult.
| return DeliberateCodeExecutionResponse( | ||
| results=[ | ||
| DeliberateCodeExecutionResult( | ||
| output="", | ||
| category=ResponseCategory.SYSTEM_ERROR, | ||
| error_message=f"Execution failed: {str(e)}", | ||
| execution_time=0.0 | ||
| ) | ||
| ] | ||
| ) |
There was a problem hiding this comment.
On exception this returns a response with exactly one result, which breaks the service’s normal contract of returning one result per provided test case (and can confuse clients when request.test_cases has length > 1). Consider building the results list to match the number of requested test cases (e.g., len(request.test_cases or [[]])), or preserving any already-computed results and appending SYSTEM_ERROR entries for the remaining cases.
| output="", | ||
| category=ResponseCategory.SYSTEM_ERROR, | ||
| error_message=f"Execution failed: {str(e)}", | ||
| execution_time=0.0 |
There was a problem hiding this comment.
The response error_message includes the raw exception string. Since this endpoint appears to be unauthenticated and potentially student-facing, this can leak internal details (e.g., S3 bucket/key names, filesystem paths). Safer approach: log the full exception (as you already do) but return a generic client message (optionally include a correlation/request ID) unless an explicit debug/internal flag is enabled.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Done in fb48eab. The raw exception string is no longer included in the response — the client now receives the generic message "An unexpected error occurred. Please try again later." while the full exception details continue to be logged server-side. Also addressed the first reviewer comment in the same commit: the results list is now sized to match len(request.test_cases) (or 1 when none are provided), keeping the response contract intact across all error scenarios.
… test cases Agent-Logs-Url: https://github.com/webtech-network/autograder/sessions/f9b1d561-a098-4f12-b1b6-85aaee79515b Co-authored-by: ArthurCRodrigues <167308923+ArthurCRodrigues@users.noreply.github.com>
The except block was accidentally removed during the asset injection refactor, causing unhandled exceptions (e.g. S3 asset not found) to propagate as raw 500 errors instead of structured SYSTEM_ERROR responses.