Conversation
…i malicious sino a parafraseo y el segundo guard si malicious te lleva a fallback_final sino termina
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughRewires the agent graph to replace fallback_inicial/generator/fallback_final with a single Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Files/areas to focus review on:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR attempts to modify the agent workflow graph routing logic to implement a dual guard check system. The changes aim to route malicious requests to fallback nodes at two different points in the workflow: after the initial guard check and after the generator node. However, the implementation contains critical bugs that prevent the workflow from functioning correctly.
- Modified the first guard node's conditional routing to send malicious requests to
fallback_inicialinstead of ending immediately, and valid requests toparafraseoinstead offallback_inicial - Removed the direct edge from
fallback_inicialtoparafraseo(since routing now handles this conditionally) - Changed the flow after
generatorto route back toguardfor a second check instead of going tofallback_final - Attempted to add a second conditional routing from
guardafter the generator completes
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| workflow.add_conditional_edges( | ||
| "fallback_final", | ||
| route_after_fallback_final, | ||
| "guard", | ||
| route_after_guard, | ||
| { | ||
| "risky": END, # End with error if risky | ||
| "continue": END, # End with final_response if valid | ||
| # Note: Final LLM is called within fallback_final node | ||
| "malicious": "fallback_inicial", # go to fallback_final if malicious | ||
| "continue": END, # if there's no error ends | ||
| }, | ||
| ) |
There was a problem hiding this comment.
This is a duplicate conditional edge definition for the "guard" node. Lines 58-65 already define conditional edges for "guard" with route_after_guard. In LangGraph, adding a second conditional edge to the same node will overwrite the first one, meaning the routing defined at lines 58-65 will be ignored and only this second definition will be active. This creates a logical error where the workflow cannot reach the parafraseo node at all, since the first guard check is effectively removed.
RAGManager/app/agents/graph.py
Outdated
| "risky": END, # End with error if risky | ||
| "continue": END, # End with final_response if valid | ||
| # Note: Final LLM is called within fallback_final node | ||
| "malicious": "fallback_inicial", # go to fallback_final if malicious |
There was a problem hiding this comment.
The comment says "go to fallback_final if malicious" but the code routes to "fallback_inicial". Based on the PR description indicating the second guard should route to fallback_final if malicious, this appears to be a copy-paste error. The routing should likely be to "fallback_final" not "fallback_inicial".
| "malicious": "fallback_inicial", # go to fallback_final if malicious | |
| "malicious": "fallback_final", # go to fallback_final if malicious |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
RAGManager/app/agents/graph.py (2)
58-65: Duplicate conditional edges from "guard" node will cause the second definition to overwrite this one.LangGraph's
add_conditional_edgeson the same source node (lines 58-65 and 81-88) will result in the second call overwriting the first. This means the routing defined here ("malicious": "fallback_inicial","continue": "parafraseo") will be lost, and only the routing from lines 81-88 will be active.To implement two distinct guard checks (one before parafraseo and one after generator), you need two separate guard nodes with different names (e.g.,
guard_inicialandguard_final).
23-32: Docstring is outdated and doesn't reflect the new flow.The docstring still describes the old routing (e.g., "generator -> fallback_final" on line 31, guard routing to "END" on line 26). Update it to match the intended new workflow to avoid confusion for future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
RAGManager/app/agents/graph.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
RAGManager/app/agents/graph.py (1)
RAGManager/app/agents/routing.py (1)
route_after_guard(6-20)
⏰ 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: Upload results
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
RAGManager/app/agents/graph.py (2)
24-35:fallback_inicialis a dead-end in the graph; addfallback_inicial -> parafraseo.
guard_inicialcan route tofallback_inicial, but there’s no outgoing edge defined here; execution will stop there.@@ # parafraseo -> retriever workflow.add_edge("parafraseo", "retriever") + + # fallback_inicial -> parafraseo + workflow.add_edge("fallback_inicial", "parafraseo")Also applies to: 60-79
80-91:fallback_finalis also a dead-end; addfallback_final -> END(or conditional edges).
guard_finalcan route tofallback_final, butfallback_finalhas no outgoing edge here.@@ workflow.add_conditional_edges( "guard_final", route_after_guard_final, { "risky": "fallback_final", # go to fallback_final if PII detected "continue": END, # if there's no error ends }, ) + + # fallback_final -> END + workflow.add_edge("fallback_final", END)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
RAGManager/app/agents/graph.py(4 hunks)RAGManager/app/agents/nodes/__init__.py(1 hunks)RAGManager/app/agents/nodes/guard_final.py(1 hunks)RAGManager/app/agents/nodes/guard_inicial.py(1 hunks)RAGManager/app/agents/routing.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
RAGManager/app/agents/nodes/guard_final.py (1)
RAGManager/app/agents/state.py (1)
AgentState(6-46)
RAGManager/app/agents/nodes/guard_inicial.py (1)
RAGManager/app/agents/state.py (1)
AgentState(6-46)
RAGManager/app/agents/routing.py (1)
RAGManager/app/agents/state.py (1)
AgentState(6-46)
RAGManager/app/agents/nodes/__init__.py (4)
RAGManager/app/agents/nodes/guard_final.py (1)
guard_final(25-77)RAGManager/app/agents/nodes/guard_inicial.py (1)
guard_inicial(24-75)RAGManager/app/agents/nodes/parafraseo.py (1)
parafraseo(6-32)RAGManager/app/agents/nodes/retriever.py (1)
retriever(6-32)
🔇 Additional comments (2)
RAGManager/app/agents/nodes/__init__.py (1)
8-23: Exports look consistent with the new nodes.RAGManager/app/agents/graph.py (1)
83-91: Verify terminal output on the “continue -> END” path (likelyfinal_responseis unset).
If callers expectfinal_response(perAgentState), ending right afterguard_finalmay return onlygenerated_response. Ensure the API contract is consistent (e.g., setfinal_response = generated_responseon the safe path or add a small finalize node).
| # Initialize Guard with DetectPII validator | ||
| # Note: The validator must be installed via: guardrails hub install hub://guardrails/detect_pii | ||
| # Also requires: presidio-analyzer and presidio-anonymizer | ||
| # Common PII entities: EMAIL_ADDRESS, PHONE_NUMBER, PERSON, LOCATION, CREDIT_CARD, SSN, etc. | ||
| # For full list see: https://microsoft.github.io/presidio/ | ||
| _guard = Guard().use( | ||
| DetectPII( | ||
| pii_entities=["EMAIL_ADDRESS", "PHONE_NUMBER", "PERSON", "LOCATION", "CREDIT_CARD", "SSN"], | ||
| on_fail="noop", # Don't raise exceptions, handle via state flags | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check repository structure and find dependency files
find . -type f \( -name "requirements*.txt" -o -name "setup.py" -o -name "pyproject.toml" -o -name "Pipfile" \) | head -20Repository: ucudal/reto-xmas-2025-goland-ia-backend
Length of output: 139
🏁 Script executed:
# Examine the full guard_final.py file to see the actual code context
fd "guard_final.py" --type fRepository: ucudal/reto-xmas-2025-goland-ia-backend
Length of output: 125
🏁 Script executed:
# Search for any logging statements that might expose PII
rg "logger|print|logging" RAGManager/app/agents/nodes/ -A 3 -B 1Repository: ucudal/reto-xmas-2025-goland-ia-backend
Length of output: 6963
🏁 Script executed:
# Check RAGManager/pyproject.toml for dependency versions
cat RAGManager/pyproject.tomlRepository: ucudal/reto-xmas-2025-goland-ia-backend
Length of output: 1735
🏁 Script executed:
# Read the full guard_final.py file to see context
cat -n RAGManager/app/agents/nodes/guard_final.pyRepository: ucudal/reto-xmas-2025-goland-ia-backend
Length of output: 3525
Fix import-time initialization risks and dependency specification; remove PII exposure in logging.
- Guard/DetectPII imports at module level (lines 5-6) will crash app at import if Guardrails or hub validator aren't installed
- Presidio packages (presidio-analyzer, presidio-anonymizer) mentioned in comments (line 14) are missing from pyproject.toml dependencies; only "guardrails-ai>=0.5.10" is specified without version pinning
- Line 67 logs first 100 characters of generated_response which defeats PII detection by exposing the PII in logs:
logger.warning(f"PII detected in generated response: {generated_response[:100]}...") - Exception handler (lines 69-75) fails open by setting is_risky=False on any error, bypassing detection entirely
Defer Guard initialization to function scope, pin Guardrails and add missing Presidio dependencies to pyproject.toml, and remove response snippet from warning log (log detection fact only, not content).
🤖 Prompt for AI Agents
RAGManager/app/agents/nodes/guard_final.py lines ~12-22 (and related locations
5-6, 14, 67, 69-75): the module currently imports and initializes
Guard/DetectPII at import time, logs a snippet of generated_response (exposing
PII), and has an exception handler that fails open; fix by deferring import and
Guard/DetectPII initialization into the function that performs validation so
missing packages won’t crash at import, update pyproject.toml to pin
guardrails-ai to a specific compatible version and add presidio-analyzer and
presidio-anonymizer as dependencies, change the logger.warning to only note that
PII was detected without including any response content, and modify the
exception handling to fail closed (treat errors as risky or re-raise) and log
the exception details (not the response) so detection cannot be bypassed on
errors.
RAGManager/app/agents/routing.py
Outdated
| def route_after_guard_final(state: AgentState) -> str: | ||
| """ | ||
| Route after Guard Final node validation. | ||
|
|
||
| Determines the next step based on whether the generated response was flagged as risky (PII detected). | ||
|
|
||
| Args: | ||
| state: Current agent state | ||
|
|
||
| Returns: | ||
| "risky" if PII is detected (is_risky=True), "continue" otherwise | ||
| """ | ||
| if state.get("is_risky", False): | ||
| return "risky" | ||
| return "continue" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optional: dedupe is_risky routing helper.
route_after_guard_final() and route_after_fallback_final() are identical; consider a single helper to avoid drift.
🤖 Prompt for AI Agents
In RAGManager/app/agents/routing.py around lines 23 to 37, the functions
route_after_guard_final and route_after_fallback_final are identical; replace
both with a single shared helper (e.g., route_after_final(state) or
route_after_risk_check(state)) and update all callers to use that helper, then
remove the duplicate function to avoid drift; ensure the new helper preserves
the same behavior (returns "risky" when state.get("is_risky", False) is true,
otherwise "continue") and run tests to verify no call sites were missed.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
RAGManager/app/agents/nodes/guard_final.py(1 hunks)RAGManager/app/agents/nodes/guard_inicial.py(4 hunks)RAGManager/app/core/config.py(2 hunks)RAGManager/pyproject.toml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
RAGManager/app/agents/nodes/guard_inicial.py (2)
RAGManager/app/agents/nodes/guard_final.py (1)
_get_guard(21-34)RAGManager/app/agents/state.py (1)
AgentState(6-46)
🪛 Ruff (0.14.8)
RAGManager/app/agents/nodes/guard_final.py
18-18: Remove quotes from type annotation
Remove quotes
(UP037)
21-21: Remove quotes from type annotation
Remove quotes
(UP037)
🔇 Additional comments (8)
RAGManager/app/core/config.py (1)
1-2: LGTM! Well-documented security configuration.The new
guardrails_fail_closedsetting is properly structured with:
- Appropriate default (False for backward compatibility)
- Clear documentation explaining fail-closed vs fail-open behavior
- Proper Field constraints and type hints
This gives operators explicit control over the security posture when Guardrails validation fails.
Also applies to: 43-48
RAGManager/app/agents/nodes/guard_inicial.py (3)
15-27: LGTM! Lazy initialization properly implemented.The singleton pattern with
_get_guard()correctly:
- Defers Guard creation until first use (prevents import-time crashes)
- Uses global for singleton caching
- Configures DetectJailbreak with appropriate settings
- Sets
on_fail="noop"for manual validation result checkingThis addresses the past review concern about import-time initialization risks.
54-74: LGTM! Validation logic is correct and privacy-safe.The validation flow properly:
- Validates using the lazy-loaded guard
- Checks
validation_passedflag correctly- Sets appropriate state flags (
is_malicious,error_message)- Logs only metadata (prompt length) without exposing potentially sensitive content
This addresses the past review concern about logging user prompt snippets.
76-89: LGTM! Exception handling implements configurable fail-safe behavior.The exception handler correctly:
- Logs errors with full stack trace for debugging
- Consults
settings.guardrails_fail_closedfor behavior control- Implements fail-closed (treat as malicious) when configured
- Defaults to fail-open for backward compatibility
- Includes clear comments explaining the security trade-offs
This addresses the past review concern about adding configurable fail-safe behavior.
RAGManager/app/agents/nodes/guard_final.py (4)
4-4: LGTM! Lazy initialization with proper TYPE_CHECKING pattern.The implementation correctly:
- Uses
TYPE_CHECKINGto avoid import-time crashes- Defers Guard/DetectPII imports to
_get_guard()runtime- Configures DetectPII with appropriate PII entities
- Uses
on_fail="noop"for manual validation checkingNote on static analysis hints: The quotes on line 18 are necessary because
Guardis only imported underTYPE_CHECKING. The quote on line 21 could be removed, but it's a minor style issue and doesn't affect functionality.Also applies to: 8-9, 18-34
61-82: LGTM! Validation logic is correct and privacy-safe.The PII detection flow properly:
- Validates generated response using the lazy-loaded guard
- Checks
validation_passedflag correctly- Sets appropriate state flags (
is_risky,error_message)- Logs only metadata (response length) without exposing detected PII
This addresses the past review concern about never logging detected PII content.
84-89: Consider making fail behavior consistent with guard_inicial.The exception handler correctly implements fail-closed behavior (sets
is_risky=Trueon errors), addressing the past review concern. However,guard_inicialuses the configurablesettings.guardrails_fail_closedsetting, whileguard_finalalways fails closed.For PII detection, fail-closed is the safer default. However, for consistency, consider whether
guard_finalshould also respect theguardrails_fail_closedsetting, or if the current hardcoded fail-closed behavior is intentional due to the higher sensitivity of output PII detection.If consistency is desired, apply this diff:
except Exception as e: - # Fail-closed: If validation fails due to error, treat as risky - # This prevents bypassing PII detection through errors logger.error("Error during PII detection", exc_info=True) - updated_state["is_risky"] = True - updated_state["error_message"] = f"PII detection error: {str(e)}" + + # Check if fail-closed mode is enabled + if settings.guardrails_fail_closed: + # Fail-closed: treat errors as risky to prevent bypassing detection + updated_state["is_risky"] = True + updated_state["error_message"] = f"PII detection error: {str(e)}" + else: + # Fail-open: allow responses when Guardrails fails (backward compatibility) + updated_state["is_risky"] = False + updated_state["error_message"] = NoneNote: Given the sensitivity of PII in outputs, you may want to keep the current fail-closed behavior regardless of the setting. Document this decision if that's the case.
55-59: LGTM! Empty response handling is correct.Treating empty responses as safe is appropriate since they cannot contain PII, and this is consistent with how
guard_inicialhandles empty prompts.
Germanadjemian
left a comment
Germanadjemian
left a comment
There was a problem hiding this comment.
Request changes in Graph.py:
This is a duplicate conditional edge definition for the "guard" node. Lines 58-65 already define conditional edges for "guard" with route_after_guard. In LangGraph, adding a second conditional edge to the same node will overwrite the first one, meaning the routing defined at lines 58-65 will be ignored and only this second definition will be active. This creates a logical error where the workflow cannot reach the parafraseo node at all, since the first guard check is effectively removed.
Germanadjemian
left a comment
Germanadjemian
left a comment
There was a problem hiding this comment.
hacer cambios marcados en guar_final.py
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
RAGManager/app/agents/nodes/agent_host.py (2)
4-9: Remove unused imports.
UUID,uuid4,settings,SessionLocal,ChatMessage, andChatSessionare no longer used after the DB-backed logic was removed.Apply this diff:
"""Nodo 1: Agent Host - Entry point that saves initial context.""" import logging -from uuid import UUID, uuid4 from app.agents.state import AgentState -from app.core.config import settings -from app.core.database_connection import SessionLocal -from app.models.chat import ChatMessage, ChatSession logger = logging.getLogger(__name__)
15-30: Update the stale docstring.The docstring describes PostgreSQL session creation, message persistence, and retrieval that no longer happen. Update it to reflect the current behavior (extracting initial context from the last message).
RAGManager/app/agents/nodes/parafraseo.py (1)
25-29: Remove the stale TODO comment.The paraphrasing logic has been implemented below, so this TODO block is no longer applicable.
- # TODO: Implement paraphrasing logic - # This should: - # 1. Use an LLM or paraphrasing model to rephrase the text - # 2. Improve clarity, adjust tone, or format as needed - # 3. Set paraphrased_text with the result - # Paraphrase the last message using historyRAGManager/app/agents/nodes/context_builder.py (1)
34-36: Remove unusedupdated_stateandparaphrasedvariables.
updated_state = state.copy()andparaphrased = state.get("paraphrased_text", "")are extracted but never used in the new implementation. This is dead code.- updated_state = state.copy() - paraphrased = state.get("paraphrased_text", "") chunks = state.get("relevant_chunks", [])RAGManager/app/agents/state.py (1)
7-12: Update docstring to reflect the actual base class.The docstring mentions "TypedDict" but the class now inherits from
MessagesState. Update for accuracy:""" State schema for the agent graph. - This TypedDict defines all the state variables that flow through + This class extends MessagesState and defines all the state variables that flow through the LangGraph nodes during agent execution. """RAGManager/app/agents/graph.py (1)
53-60: Critical: Duplicate conditional edges on "guard" node will break routing.LangGraph's
add_conditional_edgescalled twice on the same node ("guard") will cause the second definition (lines 72-79) to overwrite the first (lines 53-60). This means:
- The first guard check will use
{malicious: fallback, continue: END}instead of{malicious: fallback, continue: parafraseo}- The
parafraseonode will never be reached, breaking the entire workflowYou need separate guard nodes for initial and final validation. Based on
guard_inicial.pyin this PR, rename the guards accordingly:from app.agents.nodes import ( agent_host, context_builder, fallback, - guard, + guard_inicial, + guard_final, # You'll need to create/import this parafraseo, retriever, ) +from app.agents.routing import route_after_guard ... - workflow.add_node("guard", guard) + workflow.add_node("guard_inicial", guard_inicial) + workflow.add_node("guard_final", guard_final) ... - workflow.add_edge("agent_host", "guard") + workflow.add_edge("agent_host", "guard_inicial") workflow.add_conditional_edges( - "guard", + "guard_inicial", route_after_guard, { "malicious": "fallback", "continue": "parafraseo", }, ) ... - workflow.add_edge("context_builder", "guard") + workflow.add_edge("context_builder", "guard_final") workflow.add_conditional_edges( - "guard", + "guard_final", route_after_guard, # Or a different routing function for final guard { "malicious": "fallback", "continue": END, }, )Also applies to: 71-79
♻️ Duplicate comments (1)
RAGManager/app/agents/graph.py (1)
20-29: Docstring describes non-existent nodes and incorrect flow.The docstring references
generator(Nodo 7) which is no longer in the graph, and the described flow doesn't match the actual implementation:""" Create and configure the LangGraph agent graph. The graph implements the following flow: 1. START -> agent_host (Nodo 1) - 2. agent_host -> guard (Nodo 2) - 3. guard -> [conditional] -> fallback (Nodo 3) or END - 4. fallback -> parafraseo (Nodo 4) + 2. agent_host -> guard_inicial (Nodo 2) + 3. guard_inicial -> [conditional] -> fallback or parafraseo + 4. parafraseo (Nodo 4) 5. parafraseo -> retriever (Nodo 5) 6. retriever -> context_builder (Nodo 6) - 7. context_builder -> generator (Nodo 7) - 8. generator -> fallback (Nodo 8) - 9. fallback -> [conditional] -> END (with final_response) or END (with error) + 7. context_builder -> guard_final + 8. guard_final -> [conditional] -> fallback or END + 9. fallback -> END Returns: Configured StateGraph instance ready for execution """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
RAGManager/app/agents/graph.py(4 hunks)RAGManager/app/agents/nodes/__init__.py(1 hunks)RAGManager/app/agents/nodes/agent_host.py(1 hunks)RAGManager/app/agents/nodes/context_builder.py(2 hunks)RAGManager/app/agents/nodes/fallback.py(1 hunks)RAGManager/app/agents/nodes/fallback_final.py(0 hunks)RAGManager/app/agents/nodes/fallback_inicial.py(0 hunks)RAGManager/app/agents/nodes/generator.py(0 hunks)RAGManager/app/agents/nodes/guard_inicial.py(5 hunks)RAGManager/app/agents/nodes/parafraseo.py(2 hunks)RAGManager/app/agents/routing.py(1 hunks)RAGManager/app/agents/state.py(2 hunks)
💤 Files with no reviewable changes (3)
- RAGManager/app/agents/nodes/fallback_inicial.py
- RAGManager/app/agents/nodes/fallback_final.py
- RAGManager/app/agents/nodes/generator.py
🧰 Additional context used
🧬 Code graph analysis (3)
RAGManager/app/agents/nodes/guard_inicial.py (2)
RAGManager/app/agents/nodes/guard_final.py (1)
_get_guard(21-34)RAGManager/app/agents/state.py (1)
AgentState(6-47)
RAGManager/app/agents/graph.py (3)
RAGManager/app/agents/nodes/fallback.py (1)
fallback(16-41)RAGManager/app/agents/state.py (1)
AgentState(6-47)RAGManager/app/agents/routing.py (1)
route_after_guard(6-20)
RAGManager/app/agents/nodes/fallback.py (1)
RAGManager/app/agents/state.py (1)
AgentState(6-47)
🔇 Additional comments (6)
RAGManager/app/agents/routing.py (1)
6-20: LGTM!The routing function correctly checks the
is_maliciousflag with a safe default. The removal ofroute_after_fallback_finaladdresses the previous duplicate code concern.RAGManager/app/agents/nodes/__init__.py (1)
5-17: LGTM!The package exports are correctly updated to reflect the consolidated
fallbacknode, replacing the previousfallback_inicial,generator, andfallback_finalexports.RAGManager/app/agents/nodes/guard_inicial.py (2)
15-27: Lazy initialization pattern correctly implemented.The
_get_guard()function properly implements lazy initialization, addressing the previous review comment about preventing startup failures.
72-75: Logging and fail-closed behavior correctly implemented.The logging now avoids PII leakage by only logging prompt length, and the configurable fail-closed/fail-open behavior via
settings.guardrails_fail_closedaddresses previous review concerns.Also applies to: 77-89
RAGManager/app/agents/nodes/context_builder.py (1)
7-7: > Likely an incorrect or invalid review comment.RAGManager/app/agents/state.py (1)
22-22: Remove this suggestion; the proposed syntax will not work with TypedDict.The
is_maliciousandis_riskyfields are safely initialized before use due to the execution order of the graph—the guard nodes always set these flags regardless of code path. Adding default values with= Falsesyntax is not supported by TypedDict (which is whatMessagesStateis) and would not provide runtime defaults. The existing defensive.get("is_malicious", False)in the routing function provides extra safety if needed.Likely an incorrect or invalid review comment.
| llm = ChatOpenAI( | ||
| model="gpt-5-nano", | ||
| ) |
There was a problem hiding this comment.
Invalid model name gpt-5-nano.
Same issue as in parafraseo.py - this model doesn't exist and will cause a runtime error.
llm = ChatOpenAI(
- model="gpt-5-nano",
+ model="gpt-4o-mini",
)📝 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.
| llm = ChatOpenAI( | |
| model="gpt-5-nano", | |
| ) | |
| llm = ChatOpenAI( | |
| model="gpt-4o-mini", | |
| ) |
🤖 Prompt for AI Agents
RAGManager/app/agents/nodes/fallback.py around lines 11-13: the ChatOpenAI
instantiation uses an invalid model name "gpt-5-nano" which will raise at
runtime; change it to a valid model (e.g., "gpt-4o-mini" or your project’s
configured default/ENV model variable) and make it consistent with parafraseo.py
(use the same valid model or centralize the model name into a config/env var and
reference that here).
| Returns: | ||
| error_message |
There was a problem hiding this comment.
Docstring and return value inconsistency.
The docstring states it returns error_message, but the function returns {"messages": [error_message]}. Additionally, the AgentState schema has an error_message field that isn't being set. Consider updating the return to set error_message for downstream nodes that may rely on it.
error_message = llm.invoke(messages)
- return {"messages": [error_message]}
+ return {"messages": [error_message], "error_message": error_message.content}Also applies to: 41-41
🤖 Prompt for AI Agents
In RAGManager/app/agents/nodes/fallback.py around lines 27-28 (and also at line
41), the docstring claims the function returns error_message but the function
actually returns {"messages": [error_message]} and doesn't set
AgentState.error_message; update the function to (1) set the
AgentState.error_message field to the error string before returning, (2) return
a structure that matches the docstring (or update the docstring to reflect
returning both error_message and messages), and (3) ensure downstream callers
expect AgentState.error_message — i.e., assign state.error_message =
error_message and return either {"error_message": error_message, "messages":
[error_message]} or change the docstring to match the current return shape
consistently at both locations.
| messages = [ | ||
| SystemMessage( | ||
| content="Your job is to generate an error message in user's language for the user explaining the database doesn't have the information to respond what the user asked" | ||
| ) | ||
| ] + state["messages"] | ||
| error_message = llm.invoke(messages) |
There was a problem hiding this comment.
System prompt is misleading for malicious input handling.
The prompt instructs the LLM to say "the database doesn't have the information," but this node handles malicious prompts. This misleads users about why their request failed. Consider a prompt that politely declines without revealing detection logic.
messages = [
SystemMessage(
- content="Your job is to generate an error message in user's language for the user explaining the database doesn't have the information to respond what the user asked"
+ content="Your job is to generate a polite error message in the user's language explaining that you cannot process this request. Do not reveal that it was flagged as malicious."
)
] + state["messages"]🤖 Prompt for AI Agents
In RAGManager/app/agents/nodes/fallback.py around lines 35-40, the SystemMessage
currently instructs the LLM to claim "the database doesn't have the information"
which is misleading for malicious or disallowed inputs; change the system prompt
to instead instruct the model to politely decline disallowed requests without
revealing detection logic (e.g., apologize, state it cannot assist with that
request, offer safe alternatives or resources), ensure the refusal is phrased in
the user's language, and keep the rest of the message flow intact so the node
returns a polite, non-revealing decline for malicious prompts.
| last_message = state["messages"][-1] | ||
| prompt = last_message.content if last_message else "" |
There was a problem hiding this comment.
Guard against empty messages list to prevent IndexError.
If state["messages"] is empty, state["messages"][-1] will raise an IndexError. Add a defensive check:
- last_message = state["messages"][-1]
- prompt = last_message.content if last_message else ""
+ messages = state.get("messages", [])
+ if not messages:
+ updated_state["is_malicious"] = False
+ updated_state["error_message"] = None
+ return updated_state
+ last_message = messages[-1]
+ prompt = getattr(last_message, "content", "") or ""Using getattr also guards against messages that might not have a .content attribute.
🤖 Prompt for AI Agents
In RAGManager/app/agents/nodes/guard_inicial.py around lines 46-47, the code
directly indexes state["messages"][-1] which will raise IndexError if the list
is empty; change it to first check that state.get("messages") is a non-empty
list and then read the last item safely, e.g., verify messages and use
getattr(last_message, "content", "") to extract content, assigning an empty
string when messages is empty or the message lacks a content attribute.
| from langchain_core.messages import SystemMessage | ||
| from langchain_openai import ChatOpenAI | ||
|
|
||
| llm = ChatOpenAI(model="gpt-5-nano") |
There was a problem hiding this comment.
Invalid model name gpt-5-nano.
gpt-5-nano is not a valid OpenAI model. This will cause a runtime error when invoking the LLM. Use a valid model such as gpt-4o-mini or gpt-3.5-turbo.
-llm = ChatOpenAI(model="gpt-5-nano")
+llm = ChatOpenAI(model="gpt-4o-mini")📝 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.
| llm = ChatOpenAI(model="gpt-5-nano") | |
| llm = ChatOpenAI(model="gpt-4o-mini") |
🤖 Prompt for AI Agents
In RAGManager/app/agents/nodes/parafraseo.py around line 7, the ChatOpenAI
instantiation uses an invalid model name `gpt-5-nano`; replace it with a
supported model (for example `gpt-4o-mini` or `gpt-3.5-turbo`) by updating the
model parameter to a valid OpenAI model string, and ensure any other code or
config that depends on model-specific options is adjusted accordingly.
| response = llm.invoke(messages) | ||
| updated_state = state.copy() # Create a copy of the state to update | ||
| updated_state["paraphrased_text"] = response.content |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add error handling for LLM invocation.
If llm.invoke() fails (e.g., network error, rate limit), the exception will propagate unhandled. Consider wrapping with try/except and setting an appropriate error state.
🤖 Prompt for AI Agents
In RAGManager/app/agents/nodes/parafraseo.py around lines 39-41, the call to
llm.invoke(messages) is unprotected and will raise uncaught exceptions; wrap the
invocation in a try/except block that catches broad LLM/network errors, log the
exception, and update a copied state with error details (e.g.,
updated_state["error"]="paraphrase_failed" and
updated_state["error_message"]=str(exc)) before returning or re-raising as
appropriate so the caller can handle failure gracefully.
|
@NicoWagner2005 I've opened a new pull request, #28, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@NicoWagner2005 I've opened a new pull request, #29, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
RAGManager/app/agents/nodes/context_builder.py (2)
34-36: Remove dead code and complete the implementation.
updated_stateis created but never used.paraphrasedis extracted but never utilized, despite the TODO comments and docstring indicating that the enriched query should combineparaphrased_textwithrelevant_chunks.Either complete the implementation to use
paraphrasedin the context, or remove the dead code.- updated_state = state.copy() - paraphrased = state.get("paraphrased_text", "") chunks = state.get("relevant_chunks", [])Or incorporate
paraphrasedinto the prompt:system_content = f"""You are a helpful assistant. Use the following context to answer the user's question. If the answer is not in the context, say you don't know. +User's query: {paraphrased} + Context: {context_section}"""
11-24: Docstring does not match implementation.The docstring states the function returns "Updated state with enriched_query and primary_response set," but the function actually returns
{"messages": [response]}. Update the docstring to reflect the actual behavior.""" Context Builder node - Enriches query with retrieved context. This node: 1. Takes paraphrased text and relevant chunks 2. Builds an enriched query combining both 3. Calls Primary LLM with the enriched query 4. Gets response from Primary LLM Args: state: Agent state containing paraphrased_text and relevant_chunks Returns: - Updated state with enriched_query and primary_response set + Dict with 'messages' key containing the LLM response """
♻️ Duplicate comments (1)
RAGManager/app/agents/nodes/context_builder.py (1)
4-7: Move LLM initialization to lazy loading to prevent startup failures.Instantiating
ChatOpenAIat module scope can crash at import time if the API key is missing or the model is unavailable. Use lazy initialization as recommended in the previous review.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
RAGManager/app/agents/nodes/context_builder.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
RAGManager/app/agents/nodes/context_builder.py (1)
RAGManager/app/agents/state.py (1)
AgentState(6-47)
⏰ 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: Agent
| Context: | ||
| {context_section}""" | ||
|
|
||
| messages = [SystemMessage(content=system_content)] + state["messages"] |
There was a problem hiding this comment.
Use defensive access for state["messages"] to avoid KeyError.
Direct dictionary access will raise KeyError if messages is not present in the state. Use .get() with a default empty list for consistency with other state access patterns in this function.
- messages = [SystemMessage(content=system_content)] + state["messages"]
+ messages = [SystemMessage(content=system_content)] + state.get("messages", [])📝 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.
| messages = [SystemMessage(content=system_content)] + state["messages"] | |
| messages = [SystemMessage(content=system_content)] + state.get("messages", []) |
🤖 Prompt for AI Agents
In RAGManager/app/agents/nodes/context_builder.py around line 47, the code
directly accesses state["messages"] which can raise KeyError if messages is
missing; change to use state.get("messages", []) so it falls back to an empty
list (preserving existing behavior and matching other defensive access patterns
in this function), and ensure the resulting messages variable is a list before
concatenation.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Updated state with is_risky and error_message set | ||
| """ | ||
| updated_state = state.copy() | ||
| generated_response = state.get("generated_response", "") |
There was a problem hiding this comment.
The guard_final node is trying to access "generated_response" from state, but the context_builder node now returns only a messages dict without setting "generated_response". The workflow needs to extract the response content from the last message in state["messages"] instead. Update this to use the last message's content: generated_response = state["messages"][-1].content if state.get("messages") else ""
| generated_response = state.get("generated_response", "") | |
| generated_response = state["messages"][-1].content if state.get("messages") else "" |
| ) | ||
| system_message_content = ( | ||
| "Your job is to generate an error message in user's language for the user " | ||
| "explaining the database doesn't have the information to respond what the user asked" |
There was a problem hiding this comment.
Grammatical error in the error message. "what the user asked" should be "what the user asked for" or "the user's question".
| "explaining the database doesn't have the information to respond what the user asked" | |
| "explaining the database doesn't have the information to answer the user's question" |
RAGManager/pyproject.toml
Outdated
| "typing-extensions>=4.15.0", | ||
| "uvicorn>=0.38.0", | ||
| "guardrails-ai>=0.5.10", | ||
| "guardrails-ai>=0.6.2", |
There was a problem hiding this comment.
The dependency "langchain-openai" is missing from the dependencies list. This package is required for ChatOpenAI usage in multiple nodes (parafraseo.py, context_builder.py, fallback.py). Add "langchain-openai>=0.1.0" to the dependencies list.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
RAGManager/app/agents/nodes/fallback.py (3)
1-1: Update docstring to reflect unified fallback purpose.The module docstring still refers only to "Fallback Inicial" but this node now serves as a general fallback handling both initial malicious prompts (from
guard_inicial) and risky responses (fromguard_final).Apply this diff:
-"""Nodo 3: Fallback Inicial - Initial fallback processing.""" +"""Nodo 3: Fallback - Unified fallback processing for malicious prompts and risky responses."""
11-13: Invalid model name will cause runtime failure.The model
"gpt-5-nano"does not exist in OpenAI's model catalog and will raise an error when invoked.Apply this diff:
llm = ChatOpenAI( - model="gpt-5-nano", + model="gpt-4o-mini", )
28-29: Docstring and return value mismatch.The docstring claims the function returns
error_message, but it actually returns{"messages": [error_message]}. Additionally, theAgentStateschema includes anerror_messagefield that should be populated but isn't.Update the return statement to also set the
error_messagefield:error_message = llm.invoke(messages) - return {"messages": [error_message]} + return { + "messages": [error_message], + "error_message": error_message.content + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
RAGManager/app/agents/nodes/fallback.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
RAGManager/app/agents/nodes/fallback.py (1)
RAGManager/app/agents/state.py (1)
AgentState(6-47)
| else: | ||
| logger.info( | ||
| "Fallback triggered: Generic fallback (no risky/malicious flag)" | ||
| ) | ||
| system_message_content = ( | ||
| "Your job is to generate an error message in user's language for the user " | ||
| "explaining the database doesn't have the information to respond what the user asked" | ||
| ) |
There was a problem hiding this comment.
Generic fallback uses misleading error message.
The generic fallback branch (when neither is_risky nor is_malicious is set) uses the same misleading message as the malicious case, claiming "the database doesn't have the information." For a true generic fallback scenario, this explanation may be confusing or inaccurate.
Consider a more neutral message for unexpected fallback scenarios:
else:
logger.info(
"Fallback triggered: Generic fallback (no risky/malicious flag)"
)
system_message_content = (
- "Your job is to generate an error message in user's language for the user "
- "explaining the database doesn't have the information to respond what the user asked"
+ "Your job is to generate a polite error message in the user's language "
+ "explaining that the request cannot be completed at this time and offering to help with something else"
)🤖 Prompt for AI Agents
In RAGManager/app/agents/nodes/fallback.py around lines 53 to 60, the generic
fallback branch logs and sets a system_message_content that incorrectly states
"the database doesn't have the information," which is misleading for
non-malicious/unflagged fallbacks; change the generic fallback to use a neutral,
user-facing message (e.g., apologize and state we cannot generate a confident
response or ask for clarification) and update the logger message accordingly so
it reflects an unexpected/unhandled fallback rather than implying missing
database content.
| messages = [ | ||
| SystemMessage(content=system_message_content) | ||
| ] + state["messages"] | ||
|
|
||
| error_message = llm.invoke(messages) | ||
| return {"messages": [error_message]} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add error handling for LLM invocation in fallback node.
The llm.invoke() call has no error handling. Since this is the fallback node (a last-resort error handler), if the LLM call fails due to network issues, API errors, or the invalid model name, the entire fallback will crash. Consider wrapping the invocation in try/except and returning a hardcoded fallback message on failure.
messages = [
SystemMessage(content=system_message_content)
] + state["messages"]
- error_message = llm.invoke(messages)
+ try:
+ error_message = llm.invoke(messages)
+ except Exception as e:
+ logger.error(f"LLM invocation failed in fallback node: {e}")
+ from langchain_core.messages import AIMessage
+ error_message = AIMessage(
+ content="I'm sorry, but I cannot process your request at this time. Please try again later."
+ )
+
return {"messages": [error_message]}📝 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.
| messages = [ | |
| SystemMessage(content=system_message_content) | |
| ] + state["messages"] | |
| error_message = llm.invoke(messages) | |
| return {"messages": [error_message]} | |
| messages = [ | |
| SystemMessage(content=system_message_content) | |
| ] + state["messages"] | |
| try: | |
| error_message = llm.invoke(messages) | |
| except Exception as e: | |
| logger.error(f"LLM invocation failed in fallback node: {e}") | |
| from langchain_core.messages import AIMessage | |
| error_message = AIMessage( | |
| content="I'm sorry, but I cannot process your request at this time. Please try again later." | |
| ) | |
| return {"messages": [error_message]} |
🤖 Prompt for AI Agents
In RAGManager/app/agents/nodes/fallback.py around lines 62-67, the
llm.invoke(messages) call is unprotected and can raise (network/API/model)
errors causing the fallback node to crash; wrap the llm.invoke call in a
try/except that catches Exception, log the exception (or attach a minimal safe
error string), and return a deterministic fallback response (e.g. a single
assistant/SystemMessage with a hardcoded apology/temporary-fallback text) in the
same {"messages": [...]} shape so the pipeline continues even if the LLM call
fails.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@NicoWagner2005 I've opened a new pull request, #30, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 2. Generates an error_message from llm to show the user | ||
|
|
||
| Args: | ||
| state: Agent state containing the prompt or initial context | ||
|
|
||
| Returns: | ||
| error_message |
There was a problem hiding this comment.
The docstring states the function returns "error_message", but the actual return statement returns a dictionary with a "messages" key containing the LLM response. The docstring should accurately describe the return value format to match the implementation.
| 2. Generates an error_message from llm to show the user | |
| Args: | |
| state: Agent state containing the prompt or initial context | |
| Returns: | |
| error_message | |
| 2. Generates an error message from llm to show the user | |
| Args: | |
| state: Agent state containing the prompt or initial context | |
| Returns: | |
| dict: A dictionary with a "messages" key containing a list with the generated error message from the LLM. |
| ) | ||
|
|
||
|
|
||
| # TO DO: implementar clase nodo fallback y inicializar el llm en el init |
There was a problem hiding this comment.
Comment contains "TO DO" which should be written as "TODO" (one word, no space) for consistency with standard TODO comment conventions.
| # TO DO: implementar clase nodo fallback y inicializar el llm en el init | |
| # TODO: implementar clase nodo fallback y inicializar el llm en el init |
| finally: | ||
| if db is not None: | ||
| db.close() | ||
| initial_message = state["messages"][-1] |
There was a problem hiding this comment.
The code accesses state["messages"][-1] without first checking if the messages list is empty. If state["messages"] is an empty list, this will raise an IndexError. Add a check to ensure the messages list is not empty before accessing the last element.
| initial_message = state["messages"][-1] | |
| initial_message = state["messages"][-1] if state["messages"] else None |
|
|
||
| def context_builder(state: AgentState) -> AgentState: | ||
|
|
||
| def context_builder(state: AgentState) -> dict: |
There was a problem hiding this comment.
The function signature declares return type as "dict" but the docstring still states "Returns: Updated state with enriched_query and primary_response set". The docstring should be updated to reflect that the function now returns a dictionary with a "messages" key containing the LLM response, not the fields mentioned in the old documentation.
| from langchain_core.messages import SystemMessage | ||
| from langchain_openai import ChatOpenAI | ||
|
|
||
| llm = ChatOpenAI(model="gpt-5-nano") |
There was a problem hiding this comment.
The LLM is initialized at module level, which means it will be created immediately when the module is imported, even if the parafraseo function is never called. This could waste resources during startup or testing. Consider using lazy initialization with a function like _get_llm() similar to the pattern used in guard_inicial.py and guard_final.py.
| llm = ChatOpenAI( | ||
| model="gpt-5-nano", | ||
| ) |
There was a problem hiding this comment.
The LLM is initialized at module level, which means it will be created immediately when the module is imported, even if the fallback function is never called. This could waste resources during startup or testing. Consider using lazy initialization with a function like _get_llm() similar to the pattern used in guard_inicial.py and guard_final.py.
| llm = ChatOpenAI( | ||
| model="gpt-5-nano", | ||
| ) | ||
|
|
||
|
|
||
| # TO DO: implementar clase nodo fallback y inicializar el llm en el init | ||
| def fallback(state: AgentState) -> AgentState: | ||
| """ | ||
| Fallback node - Performs fallback processing. | ||
|
|
||
| This node: | ||
| 1. Alerts about malicious prompt or PII detection | ||
| 2. Generates an error_message from llm to show the user | ||
|
|
||
| Args: | ||
| state: Agent state containing the prompt or initial context | ||
|
|
||
| Returns: | ||
| error_message | ||
| """ | ||
|
|
||
| # Check for PII/Risky content (from guard_final) | ||
| if state.get("is_risky"): | ||
| logger.warning( | ||
| "Defensive check triggered: PII/Risky content detected in response" | ||
| ) | ||
| system_message_content = ( | ||
| "Your job is to generate an error message in user's language explaining " | ||
| "that the response cannot be provided because it contains sensitive or private information." | ||
| ) | ||
|
|
||
| # Check for Malicious prompt (from guard_inicial) | ||
| elif state.get("is_malicious"): | ||
| logger.warning( | ||
| "Defensive check triggered: Malicious prompt detected" | ||
| ) | ||
| system_message_content = ( | ||
| "Your job is to generate an error message in user's language for the user " | ||
| "explaining the database doesn't have the information to answer the user's question" | ||
| ) | ||
|
|
||
| # Generic Fallback (neither risky nor malicious) | ||
| else: | ||
| logger.info( | ||
| "Fallback triggered: Generic fallback (no risky/malicious flag)" | ||
| ) | ||
| system_message_content = ( | ||
| "Your job is to generate an error message in user's language for the user " | ||
| "explaining the database doesn't have the information to answer the user's question" | ||
| ) | ||
|
|
||
| messages = [ | ||
| SystemMessage(content=system_message_content) | ||
| ] + state["messages"] | ||
|
|
||
| error_message = llm.invoke(messages) | ||
| return {"messages": [error_message]} | ||
|
|
There was a problem hiding this comment.
The TODO comment suggests implementing a class for the fallback node and initializing the LLM in init. This is a valid concern since the current module-level LLM initialization is inconsistent with the lazy initialization pattern used in guard_inicial.py and guard_final.py. Consider addressing this TODO before merging, as it impacts the maintainability and consistency of the codebase.
| llm = ChatOpenAI( | |
| model="gpt-5-nano", | |
| ) | |
| # TO DO: implementar clase nodo fallback y inicializar el llm en el init | |
| def fallback(state: AgentState) -> AgentState: | |
| """ | |
| Fallback node - Performs fallback processing. | |
| This node: | |
| 1. Alerts about malicious prompt or PII detection | |
| 2. Generates an error_message from llm to show the user | |
| Args: | |
| state: Agent state containing the prompt or initial context | |
| Returns: | |
| error_message | |
| """ | |
| # Check for PII/Risky content (from guard_final) | |
| if state.get("is_risky"): | |
| logger.warning( | |
| "Defensive check triggered: PII/Risky content detected in response" | |
| ) | |
| system_message_content = ( | |
| "Your job is to generate an error message in user's language explaining " | |
| "that the response cannot be provided because it contains sensitive or private information." | |
| ) | |
| # Check for Malicious prompt (from guard_inicial) | |
| elif state.get("is_malicious"): | |
| logger.warning( | |
| "Defensive check triggered: Malicious prompt detected" | |
| ) | |
| system_message_content = ( | |
| "Your job is to generate an error message in user's language for the user " | |
| "explaining the database doesn't have the information to answer the user's question" | |
| ) | |
| # Generic Fallback (neither risky nor malicious) | |
| else: | |
| logger.info( | |
| "Fallback triggered: Generic fallback (no risky/malicious flag)" | |
| ) | |
| system_message_content = ( | |
| "Your job is to generate an error message in user's language for the user " | |
| "explaining the database doesn't have the information to answer the user's question" | |
| ) | |
| messages = [ | |
| SystemMessage(content=system_message_content) | |
| ] + state["messages"] | |
| error_message = llm.invoke(messages) | |
| return {"messages": [error_message]} | |
| class FallbackNode: | |
| """ | |
| Fallback node - Performs fallback processing. | |
| This node: | |
| 1. Alerts about malicious prompt or PII detection | |
| 2. Generates an error_message from llm to show the user | |
| """ | |
| def __init__(self): | |
| self.llm = ChatOpenAI( | |
| model="gpt-5-nano", | |
| ) | |
| def __call__(self, state: AgentState) -> AgentState: | |
| """ | |
| Args: | |
| state: Agent state containing the prompt or initial context | |
| Returns: | |
| error_message | |
| """ | |
| # Check for PII/Risky content (from guard_final) | |
| if state.get("is_risky"): | |
| logger.warning( | |
| "Defensive check triggered: PII/Risky content detected in response" | |
| ) | |
| system_message_content = ( | |
| "Your job is to generate an error message in user's language explaining " | |
| "that the response cannot be provided because it contains sensitive or private information." | |
| ) | |
| # Check for Malicious prompt (from guard_inicial) | |
| elif state.get("is_malicious"): | |
| logger.warning( | |
| "Defensive check triggered: Malicious prompt detected" | |
| ) | |
| system_message_content = ( | |
| "Your job is to generate an error message in user's language for the user " | |
| "explaining the database doesn't have the information to answer the user's question" | |
| ) | |
| # Generic Fallback (neither risky nor malicious) | |
| else: | |
| logger.info( | |
| "Fallback triggered: Generic fallback (no risky/malicious flag)" | |
| ) | |
| system_message_content = ( | |
| "Your job is to generate an error message in user's language for the user " | |
| "explaining the database doesn't have the information to answer the user's question" | |
| ) | |
| messages = [ | |
| SystemMessage(content=system_message_content) | |
| ] + state["messages"] | |
| error_message = self.llm.invoke(messages) | |
| return {"messages": [error_message]} |
| last_message = state["messages"][-1] | ||
| prompt = last_message.content if last_message else "" |
There was a problem hiding this comment.
The code accesses state["messages"][-1] without first checking if the messages list is empty. If state["messages"] is an empty list, this will raise an IndexError. Add a check to ensure the messages list is not empty before accessing the last element.
| last_message = state["messages"][-1] | |
| prompt = last_message.content if last_message else "" | |
| if state["messages"]: | |
| last_message = state["messages"][-1] | |
| prompt = last_message.content if last_message else "" | |
| else: | |
| last_message = None | |
| prompt = "" |
| from langchain_core.messages import SystemMessage | ||
| from langchain_openai import ChatOpenAI | ||
|
|
||
| llm = ChatOpenAI(model="gpt-5-nano") |
There was a problem hiding this comment.
The LLM is initialized at module level, which means it will be created immediately when the module is imported, even if the context_builder function is never called. This could waste resources during startup or testing. Consider using lazy initialization with a function like _get_llm() similar to the pattern used in guard_inicial.py and guard_final.py.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@NicoWagner2005 I've opened a new pull request, #31, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
RAGManager/app/agents/nodes/agent_host.py (1)
14-26: Outdated docstring describes removed functionality.The docstring still references PostgreSQL session handling, message persistence, and retrieval (steps 2-5), but the implementation now only extracts
initial_contextfrom the last message. Update it to reflect the simplified behavior.""" Agent Host node - Entry point for the agent flow. This node: - 1. Receives the initial prompt and optional chat_session_id - 2. Creates or retrieves chat session from PostgreSQL - 3. Saves the user's message to the chat session - 4. Retrieves all chat messages for the session - 5. Prepares state for validation + 1. Receives incoming messages + 2. Extracts the initial context from the last message Args: - state: Agent state containing the user prompt and optional chat_session_id + state: Agent state containing the messages list Returns: - Updated state with chat_session_id, chat_messages, and initial_context set + Updated state with initial_context set """
♻️ Duplicate comments (8)
RAGManager/app/agents/nodes/parafraseo.py (3)
8-16: Good: Lazy initialization pattern implemented.The lazy initialization pattern addresses the past concern about module-level LLM creation wasting resources during startup or testing. This is an improvement.
However, the invalid model name
gpt-5-nanoat line 15 remains unaddressed from the previous review.
48-48: Missing error handling for LLM invocation.The call to
_get_llm().invoke(messages)remains unprotected and will raise uncaught exceptions on network errors or rate limits. This issue was flagged in the previous review.
49-52: Return value inconsistency with other workflow nodes.The function returns the entire
updated_stateobject instead of a dict with just the updated fields. This inconsistency with other nodes (likecontext_builder) was noted in the previous review, which suggested returning{"paraphrased_text": response.content}for consistency with the graph's expectation of partial state updates.RAGManager/app/agents/nodes/context_builder.py (1)
54-54: Use defensive access forstate["messages"].Direct dictionary access will raise
KeyErrorifmessagesis not present. This was flagged in a previous review.- messages = [SystemMessage(content=system_content)] + state["messages"] + messages = [SystemMessage(content=system_content)] + state.get("messages", [])RAGManager/app/agents/nodes/fallback.py (3)
19-21: Invalid model namegpt-5-nano.This was flagged in a previous review. The model
gpt-5-nanodoes not exist and will cause a runtime error._llm = ChatOpenAI( - model="gpt-5-nano", + model="gpt-4o-mini", )
55-58: Misleading error messages for malicious prompt handling.The system prompts for both
is_maliciousand generic fallback cases claim "the database doesn't have the information," which is inaccurate. This was flagged in previous reviews. The malicious case should not reveal detection logic, and the generic case should use a neutral message.Also applies to: 65-68
74-75: Add error handling for LLM invocation.This was flagged in a previous review. Since this is the fallback node (last-resort error handler), if the LLM call fails, the entire fallback crashes. Wrap in try/except with a hardcoded fallback message.
RAGManager/app/agents/nodes/guard_final.py (1)
52-59: Consider returning only updated fields for MessagesState reducer pattern.Creating a full copy of the state and modifying it works, but MessagesState supports a reducer pattern where nodes return only the changed fields. This approach is more efficient and aligns with the framework's design.
Refactor to return only the updated fields:
- updated_state = state.copy() - generated_response = state["messages"][-1].content if state.get("messages") and len(state["messages"]) > 0 else "" + generated_response = state["messages"][-1].content if state.get("messages") and len(state["messages"]) > 0 else "" if not generated_response: # Empty response is considered safe - updated_state["is_risky"] = False - updated_state["error_message"] = None - return updated_state + return {"is_risky": False, "error_message": None}Apply the same pattern to the rest of the function (lines 69-70, 74-78, 88-89).
Based on learnings, this aligns with MessagesState patterns used elsewhere in the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
RAGManager/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
RAGManager/app/agents/nodes/agent_host.py(1 hunks)RAGManager/app/agents/nodes/context_builder.py(3 hunks)RAGManager/app/agents/nodes/fallback.py(1 hunks)RAGManager/app/agents/nodes/guard_final.py(1 hunks)RAGManager/app/agents/nodes/guard_inicial.py(1 hunks)RAGManager/app/agents/nodes/parafraseo.py(2 hunks)RAGManager/pyproject.toml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
RAGManager/app/agents/nodes/context_builder.py (3)
RAGManager/app/agents/state.py (1)
AgentState(6-47)RAGManager/app/agents/nodes/fallback.py (1)
_get_llm(15-22)RAGManager/app/agents/nodes/parafraseo.py (1)
_get_llm(11-16)
RAGManager/app/agents/nodes/guard_inicial.py (2)
RAGManager/app/agents/state.py (1)
AgentState(6-47)RAGManager/app/agents/nodes/guard_final.py (1)
_get_guard(21-34)
RAGManager/app/agents/nodes/parafraseo.py (3)
RAGManager/app/agents/state.py (1)
AgentState(6-47)RAGManager/app/agents/nodes/context_builder.py (1)
_get_llm(11-16)RAGManager/app/agents/nodes/fallback.py (1)
_get_llm(15-22)
RAGManager/app/agents/nodes/guard_final.py (2)
RAGManager/app/agents/state.py (1)
AgentState(6-47)RAGManager/app/agents/nodes/guard_inicial.py (1)
_get_guard(17-27)
🪛 Ruff (0.14.8)
RAGManager/app/agents/nodes/guard_final.py
18-18: Remove quotes from type annotation
Remove quotes
(UP037)
21-21: Remove quotes from type annotation
Remove quotes
(UP037)
🔇 Additional comments (11)
RAGManager/app/agents/nodes/parafraseo.py (2)
3-4: LGTM!The imports are appropriate for the paraphrase implementation using LangChain.
42-46: LGTM!The system instruction clearly defines the paraphrasing task, and the message construction correctly prepends the system instruction to the conversation history.
RAGManager/app/agents/nodes/agent_host.py (1)
27-33: LGTM - Empty messages guard is in place.The ternary check on line 28 properly handles empty message lists, and the
initial_message.content if initial_message else ""on line 30 handles the None case. This addresses the previous IndexError concern.RAGManager/pyproject.toml (2)
12-21: Dependency updates look good.The additions address previous review feedback:
langchain-openai>=0.1.0enables ChatOpenAI usageguardrails-ai>=0.6.2updates from the outdated 0.5.10- Presidio packages pinned to
2.2.360for reproducibility
6-6: Remove or clarify the Python version requirement if possible.
requires-python = ">=3.14"is now a valid requirement as Python 3.14 has been released (October 2025) and is stable. However, ensure this minimum version is intentional—if the project can support Python 3.12 or 3.13, consider broadening the requirement to>=3.12or>=3.13for wider compatibility.Likely an incorrect or invalid review comment.
RAGManager/app/agents/nodes/context_builder.py (1)
3-16: Lazy LLM initialization pattern is good.The
_get_llm()pattern correctly defers ChatOpenAI instantiation until first use, preventing import-time failures. This is consistent with other nodes.RAGManager/app/agents/nodes/guard_inicial.py (4)
17-27: Lazy Guard initialization is well implemented.The
_get_guard()pattern correctly defers Guard instantiation, preventing import-time crashes if the validator isn't installed. Configuration is properly pulled from settings.
46-57: Empty messages handling is correct.The guard against empty messages list properly prevents
IndexErrorand treats empty prompts as safe. This addresses the previous review feedback.
76-79: Good - logging uses metadata only, no PII leakage.The warning log uses
extra={"prompt_len": len(prompt)}instead of logging prompt content, addressing the previous privacy concern.
81-94: Configurable fail-safe behavior is properly implemented.The exception handling now respects
settings.guardrails_fail_closedto choose between fail-closed (treat errors as malicious) and fail-open (allow through) behavior. This addresses the previous review feedback about making this configurable.RAGManager/app/agents/nodes/guard_final.py (1)
61-91: LGTM! Previous critical security issues have been addressed.The validation and exception handling logic is now correct:
✓ PII logging fixed (lines 79-82): Logs only metadata (
response_len), not response content
✓ Fail-closed exception handling (lines 84-90): Setsis_risky=Trueon errors to prevent bypassing PII detection
✓ Proper error propagation: Usesexc_info=Truefor detailed logging and includes exception details inerror_messageThe implementation correctly prevents PII exposure while maintaining robust error handling.
| messages = [ | ||
| SystemMessage(content=system_message_content) | ||
| ] + state["messages"] |
There was a problem hiding this comment.
Use defensive access for state["messages"].
Direct dictionary access state["messages"] will raise KeyError if the key is missing. Use .get() with a default for consistency with other state access patterns.
messages = [
SystemMessage(content=system_message_content)
- ] + state["messages"]
+ ] + state.get("messages", [])📝 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.
| messages = [ | |
| SystemMessage(content=system_message_content) | |
| ] + state["messages"] | |
| messages = [ | |
| SystemMessage(content=system_message_content) | |
| ] + state.get("messages", []) |
🤖 Prompt for AI Agents
In RAGManager/app/agents/nodes/fallback.py around lines 70-72, the code accesses
state["messages"] directly which can raise KeyError; replace that direct access
with defensive access using state.get("messages", []) (or wrap with list(...) /
ensure it's a list) so the messages concatenation becomes stable and consistent
with other state access patterns.
| # Also requires: presidio-analyzer and presidio-anonymizer | ||
| # Common PII entities: EMAIL_ADDRESS, PHONE_NUMBER, PERSON, LOCATION, CREDIT_CARD, SSN, etc. | ||
| # For full list see: https://microsoft.github.io/presidio/ | ||
| _guard: "Guard | None" = None |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove unnecessary quotes from type annotation.
The quotes around "Guard | None" are unnecessary when using the TYPE_CHECKING pattern. Modern Python (3.10+) with proper conditional imports supports the union syntax without quotes.
Apply this diff:
-_guard: "Guard | None" = None
+_guard: Guard | None = None🧰 Tools
🪛 Ruff (0.14.8)
18-18: Remove quotes from type annotation
Remove quotes
(UP037)
🤖 Prompt for AI Agents
In RAGManager/app/agents/nodes/guard_final.py around line 18, the type
annotation uses unnecessary quotes: replace the quoted annotation "_guard:
\"Guard | None\" = None" with an unquoted annotation (e.g. _guard: Guard | None
= None) and ensure Guard is only imported inside a TYPE_CHECKING block or
otherwise conditionally to avoid runtime import issues.
| _guard: "Guard | None" = None | ||
|
|
||
|
|
||
| def _get_guard() -> "Guard": |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove unnecessary quotes from return type annotation.
Similar to line 18, the quotes around "Guard" in the return type are unnecessary with the TYPE_CHECKING pattern.
Apply this diff:
-def _get_guard() -> "Guard":
+def _get_guard() -> Guard:🧰 Tools
🪛 Ruff (0.14.8)
21-21: Remove quotes from type annotation
Remove quotes
(UP037)
🤖 Prompt for AI Agents
In RAGManager/app/agents/nodes/guard_final.py around line 21, the return type
annotation uses quoted "Guard" which is unnecessary when using the TYPE_CHECKING
import pattern; remove the quotes so the signature reads def _get_guard() ->
Guard: and ensure the Guard symbol is imported under an if TYPE_CHECKING: block
(or otherwise available at runtime) to keep static typing intact.
| _guard = Guard().use( | ||
| DetectPII( | ||
| pii_entities=["EMAIL_ADDRESS", "PHONE_NUMBER", "PERSON", "LOCATION", "CREDIT_CARD", "SSN"], | ||
| on_fail="noop", # Don't raise exceptions, handle via state flags | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Pull PII configuration from settings for consistency and flexibility.
The PII entity list and on_fail behavior are hardcoded, unlike guard_inicial which uses settings.guardrails_jailbreak_threshold and settings.guardrails_device. Hardcoding these values creates maintenance burden and configuration inconsistency across guards.
Consider adding settings to the config module (e.g., guardrails_pii_entities, guardrails_pii_on_fail) and importing them here:
from app.core.config import settings
_guard = Guard().use(
DetectPII(
pii_entities=settings.guardrails_pii_entities,
on_fail=settings.guardrails_pii_on_fail,
)
)This approach would:
- Align with the pattern established in
guard_inicial.py - Allow runtime configuration without code changes
- Centralize security policy settings
🤖 Prompt for AI Agents
In RAGManager/app/agents/nodes/guard_final.py around lines 28 to 32, the
DetectPII guard is using a hardcoded list of pii_entities and a hardcoded
on_fail value which causes configuration inconsistency with other guards; update
the code to import settings from app.core.config and replace the hardcoded
values with settings.guardrails_pii_entities and settings.guardrails_pii_on_fail
(add those keys to the config if missing), ensuring the guard is created like
Guard().use(DetectPII(pii_entities=settings.guardrails_pii_entities,
on_fail=settings.guardrails_pii_on_fail)) so PII behavior is configurable at
runtime.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 14 comments.
Comments suppressed due to low confidence (1)
RAGManager/app/agents/nodes/guard_inicial.py:50
- Variable last_message is not used.
last_message = None
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
RAGManager/app/agents/routing.py
Outdated
|
|
||
|
|
||
| def route_after_fallback_final(state: AgentState) -> str: | ||
| def route_after_guard_final(state: AgentState) -> str: |
There was a problem hiding this comment.
The function name 'route_after_guard_final' is inconsistent with 'route_after_guard' (which doesn't use 'inicial' suffix). For consistency with the node naming convention where guard_inicial and guard_final are explicit, consider keeping the naming pattern. However, since 'route_after_guard' routes after 'guard_inicial', the asymmetry is acceptable. Consider adding a comment clarifying that 'route_after_guard' routes after 'guard_inicial' for better clarity.
| # Fail-closed: If validation fails due to error, treat as risky | ||
| # This prevents bypassing PII detection through errors | ||
| logger.error("Error during PII detection", exc_info=True) | ||
| updated_state["is_risky"] = True | ||
| updated_state["error_message"] = f"PII detection error: {str(e)}" | ||
|
|
There was a problem hiding this comment.
The guard_final node always fails closed on exceptions (treating validation errors as risky), but guard_inicial respects the guardrails_fail_closed configuration setting. This inconsistency could lead to unexpected behavior. Consider using the same fail-closed/fail-open logic from guard_inicial here as well, controlled by the guardrails_fail_closed setting, to provide consistent error handling behavior across both guard nodes.
| # Fail-closed: If validation fails due to error, treat as risky | |
| # This prevents bypassing PII detection through errors | |
| logger.error("Error during PII detection", exc_info=True) | |
| updated_state["is_risky"] = True | |
| updated_state["error_message"] = f"PII detection error: {str(e)}" | |
| # Respect guardrails_fail_closed setting for consistency with guard_inicial | |
| fail_closed = state.get("guardrails_fail_closed", True) | |
| logger.error("Error during PII detection", exc_info=True) | |
| if fail_closed: | |
| updated_state["is_risky"] = True | |
| updated_state["error_message"] = f"PII detection error: {str(e)}" | |
| else: | |
| updated_state["is_risky"] = False | |
| updated_state["error_message"] = None |
|
|
||
| from app.agents.state import AgentState | ||
|
|
||
| _llm: ChatOpenAI | None = None | ||
|
|
||
|
|
||
| def _get_llm() -> ChatOpenAI: | ||
| """Lazy initialization of LLM instance.""" | ||
| global _llm | ||
| if _llm is None: | ||
| _llm = ChatOpenAI(model="gpt-5-nano") |
There was a problem hiding this comment.
The lazy initialization of the LLM instance using a global variable is not thread-safe. In a multi-threaded environment (like FastAPI with uvicorn), multiple threads could simultaneously check '_llm is None' and create multiple ChatOpenAI instances, potentially causing race conditions. Consider using threading.Lock or a thread-safe singleton pattern to ensure only one LLM instance is created.
| from app.agents.state import AgentState | |
| _llm: ChatOpenAI | None = None | |
| def _get_llm() -> ChatOpenAI: | |
| """Lazy initialization of LLM instance.""" | |
| global _llm | |
| if _llm is None: | |
| _llm = ChatOpenAI(model="gpt-5-nano") | |
| import threading | |
| from app.agents.state import AgentState | |
| _llm: ChatOpenAI | None = None | |
| _llm_lock = threading.Lock() | |
| def _get_llm() -> ChatOpenAI: | |
| """Lazy initialization of LLM instance (thread-safe).""" | |
| global _llm | |
| if _llm is None: | |
| with _llm_lock: | |
| if _llm is None: | |
| _llm = ChatOpenAI(model="gpt-5-nano") |
| def _get_llm() -> ChatOpenAI: | ||
| """Lazy initialization of LLM instance.""" | ||
| global _llm | ||
| if _llm is None: | ||
| _llm = ChatOpenAI( | ||
| model="gpt-5-nano", | ||
| ) | ||
| return _llm |
There was a problem hiding this comment.
The lazy initialization of the LLM instance using a global variable is not thread-safe. In a multi-threaded environment (like FastAPI with uvicorn), multiple threads could simultaneously check '_llm is None' and create multiple ChatOpenAI instances, potentially causing race conditions. Consider using threading.Lock or a thread-safe singleton pattern to ensure only one LLM instance is created.
|
|
||
| from app.agents.state import AgentState | ||
|
|
||
| _llm: ChatOpenAI | None = None | ||
|
|
||
|
|
||
| def _get_llm() -> ChatOpenAI: | ||
| """Lazy initialization of LLM instance.""" | ||
| global _llm | ||
| if _llm is None: | ||
| _llm = ChatOpenAI(model="gpt-4o-mini") |
There was a problem hiding this comment.
The lazy initialization of the LLM instance using a global variable is not thread-safe. In a multi-threaded environment (like FastAPI with uvicorn), multiple threads could simultaneously check '_llm is None' and create multiple ChatOpenAI instances, potentially causing race conditions. Consider using threading.Lock or a thread-safe singleton pattern to ensure only one LLM instance is created.
| from app.agents.state import AgentState | |
| _llm: ChatOpenAI | None = None | |
| def _get_llm() -> ChatOpenAI: | |
| """Lazy initialization of LLM instance.""" | |
| global _llm | |
| if _llm is None: | |
| _llm = ChatOpenAI(model="gpt-4o-mini") | |
| import threading | |
| from app.agents.state import AgentState | |
| _llm: ChatOpenAI | None = None | |
| _llm_lock = threading.Lock() | |
| def _get_llm() -> ChatOpenAI: | |
| """Lazy initialization of LLM instance (thread-safe).""" | |
| global _llm | |
| if _llm is None: | |
| with _llm_lock: | |
| if _llm is None: | |
| _llm = ChatOpenAI(model="gpt-4o-mini") |
| def _get_guard() -> Guard: | ||
| global _guard | ||
| if _guard is None: | ||
| _guard = Guard().use( | ||
| DetectJailbreak( | ||
| threshold=settings.guardrails_jailbreak_threshold, | ||
| device=settings.guardrails_device, | ||
| on_fail="noop", | ||
| ) | ||
| ) | ||
| return _guard |
There was a problem hiding this comment.
The lazy initialization of the Guard instance using a global variable is not thread-safe. In a multi-threaded environment (like FastAPI with uvicorn), multiple threads could simultaneously check '_guard is None' and create multiple Guard instances, potentially causing race conditions. Consider using threading.Lock or a thread-safe singleton pattern to ensure only one Guard instance is created.
| "continue": END, # End with final_response if valid | ||
| # Note: Final LLM is called within fallback_final node | ||
| "risky": "fallback", # go to fallback if risky (PII detected) | ||
| "continue": END, # if there's no error ends |
There was a problem hiding this comment.
The comment contains a grammatical error. It should read "if there's no error, ends" or better yet "End if no error is detected".
| "continue": END, # if there's no error ends | |
| "continue": END, # End if no error is detected |
RAGManager/app/agents/nodes/guard.py
Outdated
| """Guard node - alias to guard_inicial for backward compatibility.""" | ||
|
|
||
| import logging | ||
| from app.agents.nodes.guard_inicial import guard_inicial as guard | ||
|
|
||
| from guardrails import Guard | ||
| from guardrails.hub import DetectJailbreak | ||
|
|
||
| from app.agents.state import AgentState | ||
| from app.core.config import settings | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| # Initialize Guard with DetectJailbreak validator | ||
| # Note: The validator must be installed via: guardrails hub install hub://guardrails/detect_jailbreak | ||
| _guard = Guard().use( | ||
| DetectJailbreak( | ||
| threshold=settings.guardrails_jailbreak_threshold, | ||
| device=settings.guardrails_device, | ||
| on_fail="noop", # Don't raise exceptions, handle via state flags | ||
| ) | ||
| ) | ||
|
|
||
|
|
||
| def guard(state: AgentState) -> AgentState: | ||
| """ | ||
| Guard node - Validates user input for malicious content using Guardrails DetectJailbreak. | ||
|
|
||
| This node: | ||
| 1. Validates the prompt using Guardrails DetectJailbreak validator | ||
| 2. Sets is_malicious flag if jailbreak attempt is detected | ||
| 3. Sets error_message if malicious content is detected | ||
|
|
||
| Args: | ||
| state: Agent state containing the prompt | ||
|
|
||
| Returns: | ||
| Updated state with is_malicious and error_message set | ||
| """ | ||
| updated_state = state.copy() | ||
| prompt = state.get("prompt", "") | ||
|
|
||
| if not prompt: | ||
| # Empty prompt is considered safe | ||
| updated_state["is_malicious"] = False | ||
| updated_state["error_message"] = None | ||
| return updated_state | ||
|
|
||
| try: | ||
| # Validate the prompt using Guardrails | ||
| validation_result = _guard.validate(prompt) | ||
|
|
||
| # Check if validation passed | ||
| # The validator returns ValidationResult with outcome | ||
| # If validation fails, outcome will indicate failure | ||
| if validation_result.validation_passed: | ||
| updated_state["is_malicious"] = False | ||
| updated_state["error_message"] = None | ||
| logger.debug("Prompt passed jailbreak detection") | ||
| else: | ||
| # Jailbreak detected | ||
| updated_state["is_malicious"] = True | ||
| updated_state["error_message"] = ( | ||
| "Jailbreak attempt detected. Your request contains content that violates security policies." | ||
| ) | ||
| logger.warning(f"Jailbreak attempt detected in prompt: {prompt[:100]}...") | ||
|
|
||
| except Exception as e: | ||
| # If validation fails due to error, log it but don't block the request | ||
| # This is a safety measure - if Guardrails fails, we allow the request | ||
| # but log the error for monitoring | ||
| logger.error(f"Error during jailbreak detection: {e}") | ||
| updated_state["is_malicious"] = False | ||
| updated_state["error_message"] = None | ||
|
|
||
| return updated_state | ||
| __all__ = ["guard"] |
There was a problem hiding this comment.
This backward compatibility wrapper file imports guard_inicial as guard but doesn't expose it properly for the aliasing pattern shown. The all export is correct, but the import statement should explicitly rename it during import: 'from app.agents.nodes.guard_inicial import guard_inicial as guard' makes the aliasing clearer. The current approach creates an intermediate reference that may be confusing.
| # 1. Combine paraphrased_text with relevant_chunks into enriched_query | ||
| # 2. Format the query appropriately (e.g., with system prompts, context sections) | ||
| # 3. Call Primary LLM with the enriched query | ||
| # 4. Store the LLM response in primary_response |
There was a problem hiding this comment.
The TODO comment references outdated implementation steps that mention "primary_response" which is no longer part of the new workflow. Since the function now directly returns messages from the LLM, this TODO comment should be removed or updated to reflect the actual implementation.
| # 4. Store the LLM response in primary_response |
| def _get_guard() -> "Guard": | ||
| """Lazy initialization of Guard with DetectPII validator to prevent import-time crashes.""" | ||
| global _guard | ||
| if _guard is None: | ||
| from guardrails import Guard | ||
| from guardrails.hub import DetectPII | ||
|
|
||
| _guard = Guard().use( | ||
| DetectPII( | ||
| pii_entities=["EMAIL_ADDRESS", "PHONE_NUMBER", "PERSON", "LOCATION", "CREDIT_CARD", "SSN"], | ||
| on_fail="noop", # Don't raise exceptions, handle via state flags | ||
| ) | ||
| ) | ||
| return _guard |
There was a problem hiding this comment.
The lazy initialization of the Guard instance using a global variable is not thread-safe. In a multi-threaded environment (like FastAPI with uvicorn), multiple threads could simultaneously check '_guard is None' and create multiple Guard instances, potentially causing race conditions. Consider using threading.Lock or a thread-safe singleton pattern to ensure only one Guard instance is created.
…ack, no hace falta separarlos en 2
NicoWagner2005
force-pushed
the
feature/fallback_inicial
branch
from
ec0b13b to
e869bed
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
RAGManager/app/agents/state.py (1)
3-16: VerifyMessagesStateimport/API and required-field strictness.
GivenAgentState(MessagesState), confirmfrom langgraph.graph import MessagesStateis correct for your pinned LangGraph version, and consider making workflow-populated flags optional (or ensure every node initializes them) to avoid “missing key” states at runtime.In the LangGraph version used by this repo, is `MessagesState` available at `langgraph.graph.MessagesState`, and is subclassing it to add additional TypedDict fields the recommended pattern?
♻️ Duplicate comments (5)
RAGManager/app/agents/nodes/parafraseo.py (1)
4-41: Fix invalid model + avoid module-scope LLM init; add defensivemessagesaccess and invoke error handling.from app.agents.state import AgentState from langchain_core.messages import SystemMessage from langchain_openai import ChatOpenAI +import threading +import logging + +logger = logging.getLogger(__name__) + +_llm: ChatOpenAI | None = None +_llm_lock = threading.Lock() + +def _get_llm() -> ChatOpenAI: + global _llm + if _llm is None: + with _llm_lock: + if _llm is None: + # TODO: prefer settings/env for model name + _llm = ChatOpenAI(model="gpt-4o-mini") + return _llm - -llm = ChatOpenAI(model="gpt-5-nano") @@ - messages = [SystemMessage(content=system_instruction)] + state["messages"] + messages = [SystemMessage(content=system_instruction)] + state.get("messages", []) - response = llm.invoke(messages) - updated_state = state.copy() # Create a copy of the state to update - updated_state["paraphrased_text"] = response.content - - return updated_state + try: + response = _get_llm().invoke(messages) + return {"paraphrased_text": response.content} + except Exception: + logger.exception("parafraseo llm.invoke failed") + return {"paraphrased_text": None, "error_message": "Paraphrasing failed. Please try again."}What are the valid OpenAI model names supported by `langchain_openai.ChatOpenAI` in the versions used by this repo (and is `gpt-4o-mini` valid there)?RAGManager/app/agents/nodes/context_builder.py (1)
4-52: Fix invalid model, return type mismatch, and unsafestate["messages"]access; update docstring to match behavior.from app.agents.state import AgentState from langchain_core.messages import SystemMessage from langchain_openai import ChatOpenAI +import threading +import logging + +logger = logging.getLogger(__name__) -llm = ChatOpenAI(model="gpt-5-nano") +_llm: ChatOpenAI | None = None +_llm_lock = threading.Lock() + +def _get_llm() -> ChatOpenAI: + global _llm + if _llm is None: + with _llm_lock: + if _llm is None: + _llm = ChatOpenAI(model="gpt-4o-mini") + return _llm -def context_builder(state: AgentState) -> AgentState: +def context_builder(state: AgentState) -> dict: @@ - messages = [SystemMessage(content=system_content)] + state["messages"] + messages = [SystemMessage(content=system_content)] + state.get("messages", []) @@ - response = llm.invoke(messages) + try: + response = _get_llm().invoke(messages) + except Exception: + logger.exception("context_builder llm.invoke failed") + return {"messages": []} return {"messages": [response]}RAGManager/app/agents/nodes/agent_host.py (1)
28-32: Guard against missing/emptymessagesbefore indexing.
state["messages"][-1]can raiseKeyError/IndexError. Also consider returning only the changed keys (partial update) for consistency with other nodes.def agent_host(state: AgentState) -> AgentState: @@ - updated_state = state.copy() - initial_message = state["messages"][-1] - updated_state["initial_context"] = ( - initial_message.content if initial_message else "" - ) - - return updated_state + messages = state.get("messages", []) + initial_context = messages[-1].content if messages else "" + return {"initial_context": initial_context}RAGManager/app/agents/nodes/fallback.py (1)
11-41: Fallback must not crash: fix invalid model, avoid module-scope init, defensivemessages, and handlellm.invokefailures.
Also, the current system prompt is misleading for malicious/risky handling and the docstring/return shape don’t match.import logging +import threading from app.agents.state import AgentState -from langchain_core.messages import SystemMessage +from langchain_core.messages import SystemMessage, AIMessage from langchain_openai import ChatOpenAI logger = logging.getLogger(__name__) -llm = ChatOpenAI( - model="gpt-5-nano", -) +_llm: ChatOpenAI | None = None +_llm_lock = threading.Lock() + +def _get_llm() -> ChatOpenAI: + global _llm + if _llm is None: + with _llm_lock: + if _llm is None: + _llm = ChatOpenAI(model="gpt-4o-mini") + return _llm -# TO DO: implementar clase nodo fallback y inicializar el llm en el init +# TODO: implementar clase nodo fallback y inicializar el llm en el init def fallback(state: AgentState) -> AgentState: @@ - messages = [ - SystemMessage( - content="Your job is to generate an error message in user's language for the user explaining the database doesn't have the information to respond what the user asked" - ) - ] + state["messages"] - error_message = llm.invoke(messages) - return {"messages": [error_message]} + system_content = ( + "Generate a brief, polite refusal in the user's language. " + "Do not mention policy checks or detection mechanisms." + ) + messages = [SystemMessage(content=system_content)] + state.get("messages", []) + try: + error_msg = _get_llm().invoke(messages) + except Exception: + logger.exception("fallback llm.invoke failed") + error_msg = AIMessage(content="Sorry, I can’t help with that request.") + return {"messages": [error_msg], "error_message": getattr(error_msg, "content", None)}In the versions used by this repo, what exceptions can `langchain_openai.ChatOpenAI.invoke()` raise and what is the recommended retry/timeout pattern?RAGManager/app/agents/graph.py (1)
20-30: Split the guard node into distinct nodes for initial and final validation, and fix the docstring to match actual graph edges.The graph has two
add_conditional_edges("guard", ...)calls with conflicting routing mappings — the first routing (toparafraseo) becomes unreachable. Rename these to separate nodes (guard_initialandguard_final) so each can have its own conditional routing logic. Also update the docstring: it incorrectly mentions a"generator"node (which doesn't exist) and falsely claimsgenerator -> fallbackedges andfallback -> [conditional]routing; in reality,fallbackunconditionally edges toEND.@@ - workflow.add_node("guard", guard) + workflow.add_node("guard_initial", guard) + workflow.add_node("guard_final", guard) @@ - workflow.add_edge("agent_host", "guard") + workflow.add_edge("agent_host", "guard_initial") @@ workflow.add_conditional_edges( - "guard", + "guard_initial", route_after_guard, { "malicious": "fallback", "continue": "parafraseo", }, ) @@ - workflow.add_edge("context_builder", "guard") + workflow.add_edge("context_builder", "guard_final") @@ workflow.add_conditional_edges( - "guard", + "guard_final", route_after_guard, { "malicious": "fallback", "continue": END, }, )Update the docstring (lines 20–29) to reflect the actual graph: remove references to the non-existent
generatornode and correctfallback's unconditional edge toEND.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
RAGManager/app/agents/graph.py(4 hunks)RAGManager/app/agents/nodes/__init__.py(1 hunks)RAGManager/app/agents/nodes/agent_host.py(1 hunks)RAGManager/app/agents/nodes/context_builder.py(2 hunks)RAGManager/app/agents/nodes/fallback.py(1 hunks)RAGManager/app/agents/nodes/fallback_final.py(0 hunks)RAGManager/app/agents/nodes/fallback_inicial.py(0 hunks)RAGManager/app/agents/nodes/generator.py(0 hunks)RAGManager/app/agents/nodes/guard.py(1 hunks)RAGManager/app/agents/nodes/parafraseo.py(2 hunks)RAGManager/app/agents/routing.py(0 hunks)RAGManager/app/agents/state.py(2 hunks)
💤 Files with no reviewable changes (4)
- RAGManager/app/agents/nodes/generator.py
- RAGManager/app/agents/routing.py
- RAGManager/app/agents/nodes/fallback_inicial.py
- RAGManager/app/agents/nodes/fallback_final.py
🧰 Additional context used
🧬 Code graph analysis (3)
RAGManager/app/agents/nodes/fallback.py (1)
RAGManager/app/agents/state.py (1)
AgentState(6-46)
RAGManager/app/agents/graph.py (3)
RAGManager/app/agents/nodes/fallback.py (1)
fallback(16-41)RAGManager/app/agents/nodes/guard.py (1)
guard(24-76)RAGManager/app/agents/routing.py (1)
route_after_guard(6-20)
RAGManager/app/agents/nodes/__init__.py (4)
RAGManager/app/agents/nodes/fallback.py (1)
fallback(16-41)RAGManager/app/agents/nodes/guard.py (1)
guard(24-76)RAGManager/app/agents/nodes/parafraseo.py (1)
parafraseo(10-43)RAGManager/app/agents/nodes/retriever.py (1)
retriever(6-32)
🔇 Additional comments (1)
RAGManager/app/agents/nodes/__init__.py (1)
5-17: Exports look consistent with the new singlefallbacknode.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
RAGManager/app/agents/nodes/guard.py
Outdated
| """ | ||
| updated_state = state.copy() | ||
| prompt = state.get("prompt", "") | ||
| last_message = state["messages"][-1] |
There was a problem hiding this comment.
The code accesses state["messages"][-1] without checking if the messages list exists or is non-empty, which could raise an IndexError if the state is not properly initialized. While the subsequent check handles empty prompt strings, it doesn't protect against a missing or empty messages list. Add a defensive check for the messages list before accessing it.
| last_message = state["messages"][-1] | |
| messages = state.get("messages") | |
| last_message = messages[-1] if isinstance(messages, list) and messages else None |
| paraphrased = state.get("paraphrased_text", "") | ||
| chunks = state.get("relevant_chunks", []) | ||
|
|
||
| # Build enriched query | ||
| context_section = "\n\n".join(chunks) if chunks else "" | ||
| enriched_query = f"{paraphrased}\n\nContext:\n{context_section}" if context_section else paraphrased | ||
| updated_state["enriched_query"] = enriched_query | ||
| # Build enriched query with context | ||
| context_section = "\n\n".join(chunks) if chunks else "No relevant context found." | ||
|
|
||
| system_content = f"""You are a helpful assistant. Use the following context to answer the user's question. | ||
| If the answer is not in the context, say you don't know. | ||
|
|
||
| Context: | ||
| {context_section}""" | ||
|
|
||
| messages = [SystemMessage(content=system_content)] + state["messages"] |
There was a problem hiding this comment.
The function uses state.get("paraphrased_text", "") but this value is set by the parafraseo node and should always exist at this point in the workflow. However, it's never used in the actual LLM invocation - only the messages from state["messages"] and the context are used. This means the paraphrasing step is effectively ignored, which is likely a bug. Either use the paraphrased text in the query, or reconsider if the paraphrasing step is needed.
| 3. guard -> [conditional] -> fallback (Nodo 3) or END | ||
| 4. fallback -> parafraseo (Nodo 4) | ||
| 5. parafraseo -> retriever (Nodo 5) | ||
| 6. retriever -> context_builder (Nodo 6) | ||
| 7. context_builder -> generator (Nodo 7) | ||
| 8. generator -> fallback_final (Nodo 8) | ||
| 9. fallback_final -> [conditional] -> END (with final_response) or END (with error) | ||
| 8. generator -> fallback (Nodo 8) | ||
| 9. fallback -> [conditional] -> END (with final_response) or END (with error) |
There was a problem hiding this comment.
The documentation is outdated and does not match the actual workflow implementation. According to the code, after guard detects a malicious prompt, it goes to fallback and then END, not "fallback (Nodo 3) or END". The documentation should reflect that fallback is always a node that terminates the flow.
| """Nodo 3: Fallback Inicial - Initial fallback processing.""" | ||
|
|
There was a problem hiding this comment.
The module docstring incorrectly states "Nodo 3: Fallback Inicial" when this node is now a unified fallback that can be called from different points in the workflow (both after initial guard and after context_builder). The docstring should be updated to reflect the new unified purpose.
| """Nodo 3: Fallback Inicial - Initial fallback processing.""" | |
| """Unified fallback node - Handles fallback processing from multiple workflow points. | |
| This node can be invoked after the initial guard or after the context builder, | |
| providing a consistent fallback mechanism across the workflow. | |
| """ |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
RAGManager/app/agents/nodes/agent_host.py (1)
31-44:prompt = state["messages"][-1]is unsafe (IndexError) and wrong type (BaseMessage), and you persist it as DB text.
This will crash on empty messages, and even when present you’re storing the message object intoChatMessage.message/initial_contextinstead of its.content. (This is also the previously-reported empty-messages issue, plus a new correctness bug.)updated_state = state.copy() - prompt = state["messages"][-1] + messages = state.get("messages") or [] + last_message = messages[-1] if messages else None + prompt = last_message.content if last_message else "" @@ new_message = ChatMessage( session_id=chat_session.id, sender="user", message=prompt, ) @@ - updated_state["initial_context"] = prompt + updated_state["initial_context"] = prompt(After this change,
promptis guaranteedstr, and empty messages won’t raise.)Also applies to: 78-83, 113-117
RAGManager/app/agents/nodes/fallback.py (2)
11-13:model="gpt-5-nano"will likely fail at runtime — use a valid/configured model.
This is the same issue already flagged in prior threads. Centralize the model name in config/env and reuse it here.Is `gpt-5-nano` a valid model identifier for ChatOpenAI (langchain_openai) as of December 2025? If not, what are the correct current model names?
15-41: Harden fallback: avoidKeyError/empty messages, fix misleading system prompt, seterror_message, and wrapllm.invokein try/except.
This consolidates multiple already-raised concerns that are still present (docstring/return mismatch,state["messages"], misleading “database doesn’t have info”, and fallback crashing if the LLM call fails)."""Nodo 3: Fallback - Handles fallback processing from multiple workflow points.""" import logging +import threading from app.agents.state import AgentState +from app.core.config import settings -from langchain_core.messages import SystemMessage +from langchain_core.messages import SystemMessage, AIMessage from langchain_openai import ChatOpenAI logger = logging.getLogger(__name__) -llm = ChatOpenAI( - model="gpt-5-nano", -) +_llm: ChatOpenAI | None = None +_llm_lock = threading.Lock() + +def _get_llm() -> ChatOpenAI: + global _llm + if _llm is None: + with _llm_lock: + if _llm is None: + model = getattr(settings, "openai_model", "gpt-4o-mini") + _llm = ChatOpenAI(model=model) + return _llm -# TO DO: implementar clase nodo fallback y inicializar el llm en el init +# TODO: implementar clase nodo fallback y inicializar el llm en el init def fallback(state: AgentState) -> AgentState: @@ - logger.warning( - "Defensive check triggered: Malicious prompt detected" - ) + logger.warning("Fallback handler triggered") - messages = [ - SystemMessage( - content="Your job is to generate an error message in user's language for the user explaining the database doesn't have the information to respond what the user asked" - ) - ] + state["messages"] - error_message = llm.invoke(messages) - return {"messages": [error_message]} + prior_messages = list(state.get("messages") or []) + system_message = SystemMessage( + content=( + "Generate a short, polite refusal in the user's language. " + "Do not mention security checks, jailbreak detection, or private/sensitive data. " + "Offer to help with a different request." + ) + ) + messages = [system_message] + prior_messages + + try: + error_msg = _get_llm().invoke(messages) + error_text = getattr(error_msg, "content", "") + except Exception: + logger.exception("LLM invocation failed in fallback node") + error_msg = AIMessage(content="Sorry, I can’t help with that request right now.") + error_text = error_msg.content + + return {"messages": [error_msg], "error_message": error_text}Note: if you keep
def fallback(...) -> AgentStatewhile returning partial updates, you must makeAgentStatenon-total / useNotRequired(seestate.pycomment).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
RAGManager/app/agents/nodes/agent_host.py(1 hunks)RAGManager/app/agents/nodes/fallback.py(1 hunks)RAGManager/app/agents/nodes/guard.py(2 hunks)RAGManager/app/agents/state.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
RAGManager/app/agents/nodes/fallback.py (1)
RAGManager/app/agents/state.py (1)
AgentState(6-50)
| updated_state = state.copy() | ||
| prompt = state.get("prompt", "") | ||
| messages = state.get("messages", []) | ||
| last_message = messages[-1] if messages else None | ||
| prompt = last_message.content if last_message else "" | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good shift to messages-based prompt + length-only logging; harden messages type and exception logging.
If state["messages"] can be non-list/None, treat it as empty. Also log exceptions with stack trace for ops.
updated_state = state.copy()
- messages = state.get("messages", [])
+ messages = state.get("messages")
+ if not isinstance(messages, list):
+ messages = []
last_message = messages[-1] if messages else None
prompt = last_message.content if last_message else ""
@@
- logger.error(f"Error during jailbreak detection: {e}")
+ logger.exception("Error during jailbreak detection")Also applies to: 67-75
| from langgraph.graph import MessagesState | ||
|
|
||
|
|
||
| class AgentState(TypedDict): | ||
| class AgentState(MessagesState): |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
langgraph.graph.MessagesState TypedDict optional fields best practices
💡 Result:
Short best-practices for optional fields in langgraph.graph.MessagesState (TypedDict)
-
Use PEP 655 tools to express “may be absent” vs “may be None”:
- Mark keys that may be omitted with typing.NotRequired (or make the whole TypedDict total=False and use Required for the few keys that must exist). Do NOT use Optional[T] to mean “may be omitted” because Optional means T | None. [1][2]
-
For langgraph message state specifically:
- Keep the messages key typed as a sequence/list of message objects (e.g., Sequence[BaseMessage] or list[MessageLikeRepresentation]) and annotate it with the reducer (add_messages / operator.add) as shown in the langgraph docs. If messages must always exist, keep it required; if it can be absent, mark it NotRequired. [5][6]
-
If a field can be present but accept None, express that as type | None (or Optional[T] if you prefer), separate from NotRequired which controls presence. Example:
- name: NotRequired[str | None] # may be omitted; if present may be None. [1]
-
Prefer explicit totality for readability:
- Use class Foo(TypedDict) with explicit NotRequired/Required for mixed keys, or class Foo(TypedDict, total=False) + Required[...] for the inverse common case. This keeps one type declaration and is supported by type checkers (PEP 655). [1][3]
-
Keep runtime behavior and static typing aligned:
- TypedDict annotations are enforced by static checkers only. If your runtime needs to validate presence/type, add runtime checks or pydantic/dataclass wrappers; don’t rely on TypedDict at runtime. [3]
Sources
- PEP 655: Required / NotRequired for TypedDict fields. [1]
- PEP 589 TypedDict / total semantics. [3]
- langgraph / LangGraph docs and examples (MessagesState, Annotated usage, message typing). [5][6]
🏁 Script executed:
# First, let me inspect the full AgentState implementation (lines 14-50)
cd RAGManager && find . -name "state.py" -path "*/agents/*" -type fRepository: ucudal/reto-xmas-2025-goland-ia-backend
Length of output: 104
🏁 Script executed:
# Check if there's a pyproject.toml or requirements file to see langgraph version
cd RAGManager && find . -name "pyproject.toml" -o -name "requirements.txt" -o -name "requirements*.txt" | head -5Repository: ucudal/reto-xmas-2025-goland-ia-backend
Length of output: 99
🏁 Script executed:
cat RAGManager/pyproject.toml | grep -A 20 -B 2 langgraphRepository: ucudal/reto-xmas-2025-goland-ia-backend
Length of output: 567
🏁 Script executed:
cat -n RAGManager/app/agents/state.pyRepository: ucudal/reto-xmas-2025-goland-ia-backend
Length of output: 2086
🌐 Web query:
langgraph MessagesState source code definition TypedDict
💡 Result:
The MessagesState TypedDict in LangGraph is a small prebuilt state that defines a single key "messages" typed as a list of AnyMessage and annotated to use the add_messages reducer. Example source/definition shown in the docs:
from typing_extensions import TypedDict
from typing import Annotated
from langchain_core.messages import AnyMessage
from langgraph.graph.message import add_messages
class MessagesState(TypedDict):
messages: Annotated[list[AnyMessage], add_messages]
(Uses the add_messages reducer so message lists are appended/updated rather than overwritten.) [1][2]
Sources:
- LangGraph docs — Graph API / MessagesState. [1]
- LangGraph tutorial / examples showing MessagesState TypedDict. [2]
🏁 Script executed:
# Check if there are any imports or usage patterns that suggest how these fields are set
grep -r "is_malicious\|is_risky" RAGManager/app/agents/ --include="*.py" -B 2 -A 2Repository: ucudal/reto-xmas-2025-goland-ia-backend
Length of output: 3386
🏁 Script executed:
# Look for node implementations to understand which fields are conditionally set
find RAGManager/app/agents -name "*.py" -type f | head -10Repository: ucudal/reto-xmas-2025-goland-ia-backend
Length of output: 469
🏁 Script executed:
# Check fallback.py to see how is_risky is set
grep -A 10 -B 5 "is_risky" RAGManager/app/agents/nodes/fallback.pyRepository: ucudal/reto-xmas-2025-goland-ia-backend
Length of output: 65
Use NotRequired to mark fields that nodes may not set on every execution.
AgentState extends MessagesState (a TypedDict) but marks fields like is_malicious and is_risky as required when they are only set conditionally by specific nodes. Since LangGraph nodes return partial state updates, these fields must be marked NotRequired. The codebase already uses defensive .get("is_malicious", False) in routing logic, confirming fields may be absent.
Apply NotRequired to all fields set conditionally by nodes:
is_malicious: NotRequired[bool](guard node only)is_risky: NotRequired[bool](fallback_final node only)prompt,initial_context,chat_messages,adjusted_text,paraphrased_text,relevant_chunks,enriched_query,primary_response,generated_response,final_response— allNotRequiredsince set by intermediate nodes, not guaranteed at init
Keep messages required (inherited from MessagesState) and chat_session_id, user_id as NotRequired[str | None] if they may be absent in message-first flows.
nodo condicional guard te lleva a fallback_inicial si malicious sino a parafraseo y el segundo guard si malicious te lleva a fallback_final sino termina