fix: guard against empty/filtered LLM responses in core LLM paths#1682
Conversation
Add null checks before accessing choices[0].message in four files: - openai_client.py: sync/async tool-call paths and responses-API path - guardrails.py: check/aguard result extraction - rag.py: query_rag and aquery_rag answer extraction - code_agent.py: generate/review/explain/refactor/fix methods
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds systematic validation of LLM responses across four core modules: CodeAgent, OpenAI client, Guardrails, and RAG. Each module now detects empty or filtered responses before accessing message content, either by raising ChangesLLM Response Validation Hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
![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 introduces robust error handling for LLM responses across several modules, including code generation, OpenAI client interactions, guardrails, and RAG queries. The changes primarily add checks to ensure that response.choices and response.choices[0].message are present before accessing their attributes, raising a ValueError if the response is empty or filtered. The review feedback correctly identifies that these checks should be further extended to verify that message.content is not None. This is critical to prevent TypeError during JSON parsing in the guardrails module and to ensure compliance with string return type hints in the agent and RAG modules.
| if not response.choices or response.choices[0].message is None: | ||
| raise ValueError("LLM returned empty or filtered response") |
There was a problem hiding this comment.
If response.choices[0].message.content is None, json.loads(result_text) will raise a TypeError. The guardrail check should ensure that the content is present before attempting to parse it as JSON.
| if not response.choices or response.choices[0].message is None: | |
| raise ValueError("LLM returned empty or filtered response") | |
| if not response.choices or response.choices[0].message is None or response.choices[0].message.content is None: | |
| raise ValueError("LLM returned empty or filtered response") |
| if not response.choices or response.choices[0].message is None: | ||
| raise ValueError("LLM returned empty or filtered response") |
There was a problem hiding this comment.
If response.choices[0].message.content is None, json.loads(result_text) will raise a TypeError. The guardrail check should ensure that the content is present before attempting to parse it as JSON.
| if not response.choices or response.choices[0].message is None: | |
| raise ValueError("LLM returned empty or filtered response") | |
| if not response.choices or response.choices[0].message is None or response.choices[0].message.content is None: | |
| raise ValueError("LLM returned empty or filtered response") |
| if not response.choices or response.choices[0].message is None: | ||
| raise ValueError("LLM returned empty or filtered response") |
There was a problem hiding this comment.
The check should also include a check for response.choices[0].message.content being None. If content is None, the subsequent line will assign None to code, which violates the str return type hint of the method and may cause issues downstream.
| if not response.choices or response.choices[0].message is None: | |
| raise ValueError("LLM returned empty or filtered response") | |
| if not response.choices or response.choices[0].message is None or response.choices[0].message.content is None: | |
| raise ValueError("LLM returned empty or filtered response") |
| if not response.choices or response.choices[0].message is None: | ||
| raise ValueError("LLM returned empty or filtered response") |
There was a problem hiding this comment.
The check should also include a check for response.choices[0].message.content being None. If content is None, the subsequent line will assign None to code, which violates the str return type hint of the method and may cause issues downstream.
| if not response.choices or response.choices[0].message is None: | |
| raise ValueError("LLM returned empty or filtered response") | |
| if not response.choices or response.choices[0].message is None or response.choices[0].message.content is None: | |
| raise ValueError("LLM returned empty or filtered response") |
| if not response.choices or response.choices[0].message is None: | ||
| raise ValueError("LLM returned empty or filtered response") |
There was a problem hiding this comment.
The check should also include a check for response.choices[0].message.content being None. If content is None, the subsequent line will assign None to review, which violates the str return type hint of the method and may cause issues downstream.
| if not response.choices or response.choices[0].message is None: | |
| raise ValueError("LLM returned empty or filtered response") | |
| if not response.choices or response.choices[0].message is None or response.choices[0].message.content is None: | |
| raise ValueError("LLM returned empty or filtered response") |
| if not response.choices or response.choices[0].message is None: | ||
| raise ValueError("LLM returned empty or filtered response") |
There was a problem hiding this comment.
The check should also include a check for response.choices[0].message.content being None. If content is None, the subsequent line will assign None to explanation, which violates the str return type hint of the method and may cause issues downstream.
| if not response.choices or response.choices[0].message is None: | |
| raise ValueError("LLM returned empty or filtered response") | |
| if not response.choices or response.choices[0].message is None or response.choices[0].message.content is None: | |
| raise ValueError("LLM returned empty or filtered response") |
| if not response.choices or response.choices[0].message is None: | ||
| raise ValueError("LLM returned empty or filtered response") |
There was a problem hiding this comment.
The check should also include a check for response.choices[0].message.content being None. If content is None, the subsequent line will assign None to refactored, which violates the str return type hint of the method and may cause issues downstream.
| if not response.choices or response.choices[0].message is None: | |
| raise ValueError("LLM returned empty or filtered response") | |
| if not response.choices or response.choices[0].message is None or response.choices[0].message.content is None: | |
| raise ValueError("LLM returned empty or filtered response") |
| if not response.choices or response.choices[0].message is None: | ||
| raise ValueError("LLM returned empty or filtered response") |
There was a problem hiding this comment.
The check should also include a check for response.choices[0].message.content being None. If content is None, the subsequent line will assign None to fixed, which violates the str return type hint of the method and may cause issues downstream.
| if not response.choices or response.choices[0].message is None: | |
| raise ValueError("LLM returned empty or filtered response") | |
| if not response.choices or response.choices[0].message is None or response.choices[0].message.content is None: | |
| raise ValueError("LLM returned empty or filtered response") |
| ) | ||
|
|
||
| answer = response.choices[0].message.content if response.choices else "" | ||
| answer = response.choices[0].message.content if response.choices and response.choices[0].message is not None else "" |
There was a problem hiding this comment.
If response.choices[0].message.content is None, answer will be assigned None. Since RAGResult expects a string for the answer field, it's safer to default to an empty string.
| answer = response.choices[0].message.content if response.choices and response.choices[0].message is not None else "" | |
| answer = (response.choices[0].message.content if response.choices and response.choices[0].message is not None else "") or "" |
| ) | ||
|
|
||
| answer = response.choices[0].message.content if response.choices else "" | ||
| answer = response.choices[0].message.content if response.choices and response.choices[0].message is not None else "" |
There was a problem hiding this comment.
If response.choices[0].message.content is None, answer will be assigned None. Since RAGResult expects a string for the answer field, it's safer to default to an empty string.
| answer = response.choices[0].message.content if response.choices and response.choices[0].message is not None else "" | |
| answer = (response.choices[0].message.content if response.choices and response.choices[0].message is not None else "") or "" |
Greptile SummaryThis PR adds null/empty guards before
Confidence Score: 3/5Half of the new guards are defeated by existing exception handlers and do not protect the paths they target; merging as-is leaves guardrails and the Responses API path with the same exposure they had before. The guards in code_agent.py and rag.py are straightforward and correct. However, in guardrails.py both new checks raise ValueError inside a try/except that converts any exception to a passing guardrail result — filtered content still passes unchanged, which is the wrong outcome for a safety component. In openai_client.py the two Responses-API guards raise ValueError inside a try/except that logs a warning and falls through to Chat Completions, so the caller never sees the error. Two of the four targeted files end up no safer than before. openai_client.py (Responses API try/except blocks swallow the new ValueError) and guardrails.py (fail-safe except handler converts the new ValueError to a passing guardrail result). Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[LLM call returns response] --> B{choices empty or message is None?}
B -- No --> C[Access choices0.message - OK]
B -- Yes via code_agent.py --> D[raise ValueError - propagates to caller - CORRECT]
B -- Yes via rag.py --> E[return empty string - CORRECT]
B -- Yes via openai_client.py Responses API --> F[raise ValueError]
F --> G[Caught by except Exception in Responses API try block]
G --> H[Logs warning, falls back to Chat Completions - Error never reaches caller - BUG]
B -- Yes via guardrails.py --> I[raise ValueError]
I --> J[Caught by except Exception fail-safe handler]
J --> K[Returns GuardrailResult passed=True - Filtered content silently passes - BUG]
Reviews (1): Last reviewed commit: "fix: guard against empty/filtered LLM re..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if not final_response.choices or final_response.choices[0].message is None: | ||
| raise ValueError("LLM returned empty or filtered response") |
There was a problem hiding this comment.
Both new guards in the Responses API path (sync at line 910 and async at line 1172) sit inside a try block whose except Exception as e: handler at line 935 (and 1202 async) catches all exceptions, logs a warning, and falls through to the Chat Completions path. When the LLM returns empty/filtered choices, raise ValueError(...) is immediately caught, the fallback kicks in, and the caller never sees the error — the exact opposite of the intended behavior.
The same handler was originally meant to recover from network/API failures in the Responses API, not from empty responses. The guard should either be placed after the try/except block (on final_response after it has been returned), or the except handler should re-raise on ValueError.
| if not response.choices or response.choices[0].message is None: | ||
| raise ValueError("LLM returned empty or filtered response") |
There was a problem hiding this comment.
The new check raises ValueError inside the try block at line 77. The broad except Exception as e: at line 102 (and its async counterpart) catches it and returns GuardrailResult(passed=True, ...) — the "fail safe" path. When the LLM returns filtered or empty content, the guardrail silently passes the input through unchanged, which is the wrong behavior for a safety guard. The fix doesn't alter the observable outcome at all compared to the pre-existing IndexError/AttributeError being caught there. To actually block on empty/filtered responses, the check needs to return GuardrailResult(passed=False, violations=["LLM returned empty or filtered response"], ...) directly rather than relying on raise.
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job Architecture Review & Fix Implementation ✅Todo List:
Phase 1: Architecture Review ✅Protocol-driven: ✅ Changes are in implementation files, not core protocols Phase 2: Critical Issues Fixed ✅🔧 Architectural Flaws Fixed (Greptile P1)Problem: Guards were defeated by exception handlers, making them no-ops
🔧 Content Validation Enhanced (Gemini High/Medium Priority)Problem: Missing checks for
Phase 3: Final Verdict ✅✅ APPROVED - All Critical Issues Resolved Summary of Changes Made:
What Was Fixed:
Architecture Compliance:
The PR is now architecturally sound and ready for merge. |
2 participants
What
Adds null checks before accessing
choices[0].messagein four files:src/praisonai-agents/praisonaiagents/llm/openai_client.py— sync tool-call path, async tool-call path, responses-API path (3 locations)src/praisonai/praisonai/capabilities/guardrails.py—check()andaguard()result extraction (2 locations)src/praisonai/praisonai/capabilities/rag.py—query_rag()andaquery_rag()answer extraction (2 locations)src/praisonai-agents/praisonaiagents/agent/code_agent.py— generate/review/explain/refactor/fix methods (6 locations)Why
The OpenAI SDK can return two silent failure modes that crash silently at these lines:
IndexError—choicesis an empty list ([]) when the provider returns no completionsAttributeError—choices[0].messageisNonewhen content is filtered (e.g. Gemini returns HTTP 200 withPROHIBITED_CONTENTfinish reason). Anyor ""fallback never fires because the crash occurs on.contentfirst.Fix pattern
For raise-on-failure (agent methods):
For return-empty (RAG queries):
Detected by pact static analysis.
Summary by CodeRabbit