Conversation
Added APIStatusError handling to manage transient errors during API calls.
p-
requested review from
JarLob,
kevinbackhouse,
m-y-mo and
sylwia-budzynska
as code owners
There was a problem hiding this comment.
Pull request overview
This PR adds special handling for HTTP 499 status errors from the OpenAI API, treating them as transient errors that should be retried similar to timeout errors.
Changes:
- Imports
APIStatusErrorfrom the openai library to catch API status errors - Adds retry logic for 499 status code errors (client closed request/upstream cancelled)
- Updates error logging message from "Max retries for APITimeoutError reached" to the more generic "Max API retries reached"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except APIStatusError as e: | ||
| # Retry transient “client closed request / upstream cancelled” style errors | ||
| if getattr(e, "status_code", None) != 499: | ||
| raise # propagate non-499 errors | ||
| # 499: retry | ||
| if not max_retry: | ||
| logging.error(f"Max API retries reached") | ||
| raise | ||
| max_retry -= 1 |
There was a problem hiding this comment.
The new APIStatusError exception handler should be added to the outer exception handling block (lines 361-381) to provide consistent error handling and user feedback. Currently, if a non-499 APIStatusError is raised and propagated from the inner try-except block, it won't be caught by any outer handler, potentially resulting in an unhandled exception. Consider adding an exception handler for APIStatusError similar to the existing handlers for BadRequestError and APITimeoutError.
There was a problem hiding this comment.
Yea probably worth adding a handler in the outer scope like APITimeoutError
| except APIStatusError as e: | ||
| # Retry transient “client closed request / upstream cancelled” style errors | ||
| if getattr(e, "status_code", None) != 499: | ||
| raise # propagate non-499 errors | ||
| # 499: retry | ||
| if not max_retry: | ||
| logging.error(f"Max API retries reached") | ||
| raise | ||
| max_retry -= 1 |
There was a problem hiding this comment.
The new APIStatusError retry logic for 499 status codes lacks test coverage. Since the repository includes tests for other functionality (test_api_endpoint_config.py, test_cli_parser.py, test_yaml_parser.py), consider adding tests to verify that 499 errors are properly retried and that non-499 APIStatusErrors are propagated correctly.
|
smoke test |
Deployment Triggered 🚀p-, started a branch deployment to smoketest (branch: You can watch the progress here 🔗 Details{
"type": "branch",
"environment": {
"name": "smoketest",
"url": null
},
"deployment": {
"timestamp": "2026-01-20T16:27:06.876Z",
"logs": "https://github.com/GitHubSecurityLab/seclab-taskflow-agent/actions/runs/21179132080"
},
"git": {
"branch": "p--handle-499",
"commit": "c38b939e7ecb6d588fae9f5f5e223a346feca4cc",
"verified": true,
"committer": "web-flow",
"html_url": "https://github.com/GitHubSecurityLab/seclab-taskflow-agent/commit/c38b939e7ecb6d588fae9f5f5e223a346feca4cc"
},
"context": {
"actor": "p-",
"noop": false,
"fork": false,
"comment": {
"created_at": "2026-01-20T16:26:48Z",
"updated_at": "2026-01-20T16:26:48Z",
"body": "smoke test",
"html_url": "https://github.com/GitHubSecurityLab/seclab-taskflow-agent/pull/132#issuecomment-3773810578"
}
},
"parameters": {
"raw": null,
"parsed": null
}
} |
Deployment Results ✅p- successfully deployed branch Details{
"status": "success",
"environment": {
"name": "smoketest",
"url": null
},
"deployment": {
"id": 3666688678,
"timestamp": "2026-01-20T16:27:08.716Z",
"logs": "https://github.com/GitHubSecurityLab/seclab-taskflow-agent/actions/runs/21179132080",
"duration": 2
},
"git": {
"branch": "p--handle-499",
"commit": "c38b939e7ecb6d588fae9f5f5e223a346feca4cc",
"verified": true
},
"context": {
"actor": "p-",
"noop": false,
"fork": false
},
"reviews": {
"count": 1,
"decision": "APPROVED"
},
"parameters": {
"raw": null,
"parsed": null
}
} |
|
smoke test |
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except APIStatusError as e: | ||
| await render_model_output(f"** 🤖❗ API Status Error: {e}\n", | ||
| async_task=async_task, | ||
| task_id=task_id) | ||
| logging.error(f"API Status Error: Status={e.status_code}, Response={e.response}") |
There was a problem hiding this comment.
The APIStatusError exception handler is placed before the more specific exception types (BadRequestError, APITimeoutError, RateLimitError) in the exception hierarchy. Since these specific exceptions inherit from APIStatusError, this catch block will intercept them before they reach their intended handlers above (lines 372-381). The APIStatusError handler should be moved to the end of the exception chain, after all the more specific handlers.
3 participants
No description provided.