fix: surface expired MinerU token errors during renovation#446
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves error handling for MinerU authentication failures by detecting invalid or expired tokens and surfacing clear, actionable messages to the user instead of generic network errors. It updates the file parser service and task manager to identify these credential issues and adds comprehensive unit and regression tests. The review feedback suggests enhancing the robustness of the error-parsing helpers by defensively handling potential AttributeError exceptions when reading response bodies or processing non-string error messages.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| except ValueError: | ||
| body_text = (getattr(response, "text", "") or "").strip() |
There was a problem hiding this comment.
If response is not a standard requests.Response object (e.g., a mock or a different exception response), calling response.json() will raise an AttributeError which is not caught by except ValueError:. This can crash the error handling path. Catching AttributeError and defensively converting the text attribute to a string ensures robust error extraction.
| except ValueError: | |
| body_text = (getattr(response, "text", "") or "").strip() | |
| except (ValueError, AttributeError): | |
| body_text = str(getattr(response, "text", "") or "").strip() |
| if status_code in (401, 403): | ||
| return True | ||
|
|
||
| normalized = (text or "").lower() |
There was a problem hiding this comment.
If the API returns a non-string value for msg or message (such as an integer or a list of validation errors), text will not be a string. In that case, (text or "") will evaluate to that non-string object, and calling .lower() on it will raise an AttributeError. Converting the value to a string defensively prevents this potential crash.
| normalized = (text or "").lower() | |
| normalized = str(text or "").lower() |
cb1467b to
987672b
Compare
|
Addressed the Gemini review suggestions in 987672b by defensively handling response-like objects without json() and non-string MinerU error payloads, with regression coverage added. @gemini-code-assist please re-review when available. |
There was a problem hiding this comment.
Code Review
This pull request introduces specialized handling for MinerU authentication errors, ensuring that expired or invalid credentials surface as actionable, user-friendly messages rather than generic network failures. Changes include helper functions for error extraction and detection in file_parser_service.py, task failure propagation in task_manager.py, and comprehensive unit and regression tests. The review feedback suggests catching a broader Exception instead of only ValueError and AttributeError when calling response.json() to safely handle cases where response.json is not callable.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| try: | ||
| body = response.json() | ||
| except (ValueError, AttributeError): |
There was a problem hiding this comment.
Catching only ValueError and AttributeError when calling response.json() can lead to unhandled exceptions such as TypeError if response.json is not callable (e.g., if it is None or a non-callable attribute). Catching a broader Exception ensures that any failure during JSON parsing safely falls back to extracting the error from response.text without bypassing the specific auth error detection.
| try: | |
| body = response.json() | |
| except (ValueError, AttributeError): | |
| try: | |
| body = response.json() | |
| except Exception: |
References
- Use try-except blocks to handle potential errors and provide safe fallbacks.
987672b to
60258f8
Compare
|
Addressed the latest Gemini suggestion in 60258f8 by falling back on any response.json() failure and adding a non-callable json regression test. @gemini-code-assist please re-review when available. |
There was a problem hiding this comment.
Code Review
This pull request introduces robust error handling and detection for MinerU authentication and credential failures. It adds helper functions to parse response errors, detect auth-related terms, and format user-friendly error messages. These checks are integrated into the file upload and polling processes, and task execution is updated to raise errors immediately upon authentication failure. Comprehensive unit and regression tests are also added. The feedback suggests enhancing the authentication failure term list with common English words like 'failed' and 'failure', and truncating raw response text fallbacks to prevent large HTML error pages from cluttering logs and the user interface.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| _MINERU_AUTH_FAILURE_TERMS = ( | ||
| "expired", | ||
| "invalid", | ||
| "unauthorized", | ||
| "forbidden", | ||
| "denied", | ||
| "expire", | ||
| "过期", | ||
| "无效", | ||
| "失败", | ||
| "拒绝", | ||
| ) |
There was a problem hiding this comment.
The _MINERU_AUTH_FAILURE_TERMS tuple is missing common English failure terms such as "failed", "fail", and "failure". If the MinerU API returns an error message like "auth failed" or "token validation failed", it will not be recognized as an authentication error because "failed" is not in the failure terms list (even though "auth" is in _MINERU_AUTH_TERMS).
Adding these terms will make the authentication error detection much more robust.
_MINERU_AUTH_FAILURE_TERMS = (
"expired",
"invalid",
"unauthorized",
"forbidden",
"denied",
"expire",
"failed",
"fail",
"failure",
"过期",
"无效",
"失败",
"拒绝",
)| except Exception: | ||
| body_text = str(getattr(response, "text", "") or "").strip() | ||
| if body_text: | ||
| parts.append(body_text) |
There was a problem hiding this comment.
If the server returns a large HTML error page (e.g., a 502 Bad Gateway or 403 Forbidden page from Nginx/Cloudflare), response.json() will fail, and the code will fall back to reading response.text.
Without truncation, the entire HTML page (which can be tens of kilobytes) will be appended to the error message, logged, and potentially displayed to the user. This can flood the logs and ruin the user interface.
Truncating body_text to a reasonable length (e.g., 500 characters) prevents this issue.
| except Exception: | |
| body_text = str(getattr(response, "text", "") or "").strip() | |
| if body_text: | |
| parts.append(body_text) | |
| except Exception: | |
| body_text = str(getattr(response, "text", "") or "").strip() | |
| if body_text: | |
| if len(body_text) > 500: | |
| body_text = body_text[:500] + "..." | |
| parts.append(body_text) |
60258f8 to
e30aa8f
Compare
|
Addressed the latest Gemini suggestions in the current head by adding failed/fail/failure detection and truncating large response.text fallbacks with regression tests. @gemini-code-assist please re-review when available. |
There was a problem hiding this comment.
Code Review
This pull request introduces robust error handling and detection for MinerU authentication failures, mapping expired or invalid credentials to actionable user-facing messages rather than generic network errors. It includes helper functions for error extraction and pattern matching, integrates these checks into the file upload and polling workflows, and adds comprehensive unit and regression tests. The reviewer suggested improving the error response parser to support list-type values (such as FastAPI/Pydantic validation errors) in the detail field to prevent them from being ignored.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if isinstance(body, dict): | ||
| for key in ("msg", "message", "error", "detail"): | ||
| value = body.get(key) | ||
| if isinstance(value, str) and value.strip(): | ||
| parts.append(value.strip()) | ||
| elif isinstance(value, dict): | ||
| nested = value.get("message") or value.get("msg") or value.get("error") | ||
| if isinstance(nested, str) and nested.strip(): | ||
| parts.append(nested.strip()) |
There was a problem hiding this comment.
When parsing the error response body, if detail is a list (which is the standard format for FastAPI/Pydantic validation errors, e.g., {"detail": [{"msg": "..."}]}), it will be ignored because the code only checks for str and dict types. Surfacing these validation details is highly beneficial for debugging API integration issues.
Consider adding support for list-type values to extract nested error messages.
| if isinstance(body, dict): | |
| for key in ("msg", "message", "error", "detail"): | |
| value = body.get(key) | |
| if isinstance(value, str) and value.strip(): | |
| parts.append(value.strip()) | |
| elif isinstance(value, dict): | |
| nested = value.get("message") or value.get("msg") or value.get("error") | |
| if isinstance(nested, str) and nested.strip(): | |
| parts.append(nested.strip()) | |
| if isinstance(body, dict): | |
| for key in ("msg", "message", "error", "detail"): | |
| value = body.get(key) | |
| if isinstance(value, str) and value.strip(): | |
| parts.append(value.strip()) | |
| elif isinstance(value, dict): | |
| nested = value.get("message") or value.get("msg") or value.get("error") | |
| if isinstance(nested, str) and nested.strip(): | |
| parts.append(nested.strip()) | |
| elif isinstance(value, list): | |
| for item in value: | |
| if isinstance(item, str) and item.strip(): | |
| parts.append(item.strip()) | |
| elif isinstance(item, dict): | |
| nested = item.get("msg") or item.get("message") or item.get("error") | |
| if isinstance(nested, str) and nested.strip(): | |
| parts.append(nested.strip()) |
e30aa8f to
d25a9b6
Compare
|
Addressed the latest Gemini suggestion in the current head by extracting list-style detail messages, with regression coverage. @gemini-code-assist please re-review when available. |
There was a problem hiding this comment.
Code Review
This pull request introduces robust detection and handling for MinerU authentication errors (such as invalid or expired tokens) during file uploading and polling. It adds helper functions to parse error responses, raises immediate errors in the task manager when auth failures occur, and includes comprehensive unit and regression tests. The review feedback suggests enhancing robustness by wrapping fallback text extraction in a nested try-except block and checking that the response object is not null before performing keyword-based auth error detection to prevent false positives during network exceptions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| except Exception: | ||
| body_text = _truncate_mineru_error_text(str(getattr(response, "text", "") or "").strip()) | ||
| if body_text: | ||
| parts.append(body_text) |
There was a problem hiding this comment.
Accessing response.text or converting it to a string can occasionally raise exceptions (e.g., UnicodeDecodeError or custom mock errors in tests). Since this is error-handling/formatting code, it should be extremely robust and never propagate exceptions. Wrapping the fallback text extraction in a nested try-except block prevents any unexpected crashes.
except Exception:
try:
body_text = _truncate_mineru_error_text(str(getattr(response, "text", "") or "").strip())
if body_text:
parts.append(body_text)
except Exception:
passReferences
- Use try-except blocks to handle potential errors and provide safe fallbacks.
| if _looks_like_mineru_auth_error( | ||
| f"{response_text} {str(e)}", | ||
| getattr(response, "status_code", None), | ||
| ): |
There was a problem hiding this comment.
When a network-level exception occurs (e.g., DNS resolution failure, connection timeout, or connection refused), response will be None. In such cases, the exception message str(e) might accidentally contain keywords like "auth" (e.g., in a custom base URL hostname) and "failed", leading to a false positive where a network error is incorrectly classified as a MinerU auth/token error.
To prevent this, we should ensure that response is not None before performing keyword-based auth error detection in the exception handler.
| if _looks_like_mineru_auth_error( | |
| f"{response_text} {str(e)}", | |
| getattr(response, "status_code", None), | |
| ): | |
| if response is not None and _looks_like_mineru_auth_error( | |
| f"{response_text} {str(e)}", | |
| getattr(response, "status_code", None), | |
| ): |
| if _looks_like_mineru_auth_error( | ||
| f"{response_text} {str(e)}", | ||
| getattr(response, "status_code", None), | ||
| ): |
There was a problem hiding this comment.
Similarly, during polling, if a network-level exception occurs and response is None, we should not classify it as a MinerU auth error based on keyword matching of the exception message. Checking response is not None ensures we only flag auth errors when we actually received an HTTP response from the server.
| if _looks_like_mineru_auth_error( | |
| f"{response_text} {str(e)}", | |
| getattr(response, "status_code", None), | |
| ): | |
| if response is not None and _looks_like_mineru_auth_error( | |
| f"{response_text} {str(e)}", | |
| getattr(response, "status_code", None), | |
| ): |
d25a9b6 to
101ee7c
Compare
|
Addressed the latest Gemini suggestions in 101ee7c by making fallback response.text extraction non-throwing and avoiding keyword-based auth classification for pure network errors without a response. Added regressions for both cases. @gemini-code-assist please re-review when available. |
There was a problem hiding this comment.
Code Review
This pull request introduces specific handling for MinerU authentication errors to provide user-friendly error messages instead of generic network errors. It adds helper functions to detect and format MinerU credential failures, integrates these checks into file uploading and polling processes, and ensures that task execution fails immediately upon detecting auth errors. Comprehensive unit and regression tests are also added. The review feedback suggests refining the substring matching in _looks_like_mineru_auth_error to use word boundaries for English terms, preventing false positives such as matching 'tokenize' as 'token'.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def _looks_like_mineru_auth_error(text: str | None, status_code: int | None = None) -> bool: | ||
| if status_code in (401, 403): | ||
| return True | ||
|
|
||
| normalized = str(text or "").lower() | ||
| if not normalized: | ||
| return False | ||
|
|
||
| has_auth_term = any(term in normalized for term in _MINERU_AUTH_TERMS) | ||
| has_failure_term = any(term in normalized for term in _MINERU_AUTH_FAILURE_TERMS) | ||
| return has_auth_term and has_failure_term |
There was a problem hiding this comment.
The current implementation of _looks_like_mineru_auth_error uses simple substring matching (term in normalized) for all terms. This can lead to false positives where non-auth errors are misclassified as auth errors. For example, a document processing error containing the word tokenize or failed to tokenize will match token (from _MINERU_AUTH_TERMS) and failed (from _MINERU_AUTH_FAILURE_TERMS), incorrectly triggering the MinerU token expiration error message.
To prevent this, we should use word boundary matching (\b) for English terms, while keeping substring matching for Chinese terms.
def _looks_like_mineru_auth_error(text: str | None, status_code: int | None = None) -> bool:
if status_code in (401, 403):
return True
normalized = str(text or "").lower()
if not normalized:
return False
def contains_term(text_str: str, terms: tuple[str, ...]) -> bool:
for term in terms:
if any('\\u4e00' <= char <= '\\u9fff' for char in term):
if term in text_str:
return True
else:
if re.search(r'\\b' + re.escape(term) + r'\\b', text_str):
return True
return False
has_auth_term = contains_term(normalized, _MINERU_AUTH_TERMS)
has_failure_term = contains_term(normalized, _MINERU_AUTH_FAILURE_TERMS)
return has_auth_term and has_failure_term
101ee7c to
ecda122
Compare
|
Addressed Gemini latest suggestion in boundary matching by switching auth/failure keyword checks to word-boundary matching for ASCII terms (e.g., avoid tokenizing false positives like "tokenize") and added regression tests covering auth/authentic/tokenize cases. @gemini-code-assist please re-review this commit. |
There was a problem hiding this comment.
Code Review
This pull request introduces robust detection and handling for MinerU authentication errors (such as invalid or expired tokens) in the file parser service and task manager. It maps these failures to clear, actionable user-facing messages rather than generic network errors, and raises immediate errors during page processing to halt execution. Additionally, comprehensive unit and regression tests have been added to verify this behavior. No review comments were provided, and the implementation looks solid.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
1 participant
Summary
Issue mapping
Fixes #420. The issue reports that PPT renovation showed an unclear generic failure when the MinerU key had expired. This PR maps those auth failures at the shared file parser layer and propagates them through the renovation task error_message that the existing frontend already displays.
Decision
I chose to fail-fast only for MinerU credential/auth errors during PPT renovation. Other parse failures keep the existing warning/empty-page behavior to avoid changing unrelated renovation behavior.
Verification
Frontend / browser coverage
No browser pass was run because this is a backend-only error mapping change. The existing frontend already renders task.error_message for failed PPT renovation tasks; the new regression verifies that the backend task error_message contains the specific MinerU Token message.