feat: add builtin tool of llm firewall#271
Conversation
config.yaml.full
Outdated
| url: #mcp sse/streamable-http url | ||
| api_key: #mcp api key | ||
| # [optional] for Volcengine LLM Firewall https://www.volcengine.com/product/LLM-FW | ||
| llm_firewall: |
There was a problem hiding this comment.
llm_firewall-》llm_shield,
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new LLM Firewall plugin for content moderation and security filtering that integrates with Volcengine's LLM Firewall service to provide real-time content moderation for LLM requests.
- Introduces
LLMFirewallPluginclass that analyzes user inputs for various risks before allowing requests to reach the language model - Adds configuration support for the LLM Firewall plugin with AppID configuration
- Updates documentation with usage examples and configuration instructions
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| veadk/tools/builtin_tools/llm_firewall.py | New plugin implementation with content moderation logic, history building, and risk detection |
| docs/content/5.tools/1.builtin-tools.md | Documentation update with LLMFirewallPlugin usage example and configuration notes |
| config.yaml.full | Configuration template addition for LLM Firewall app_id |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Defaults to 50. | ||
|
|
||
| Raises: | ||
| ValueError: If required environment variables are missing |
There was a problem hiding this comment.
The docstring states that ValueError is raised if required environment variables are missing, but the init method doesn't actually raise this exception. The getenv() function will raise ValueError if the environment variable is not found (based on the veadk.utils.misc implementation), but this happens inside getenv, not explicitly in init. The documentation should clarify that the ValueError comes from the getenv() calls for VOLCENGINE_ACCESS_KEY, VOLCENGINE_SECRET_KEY, and TOOL_LLM_FIREWALL_APP_ID.
| ValueError: If required environment variables are missing | |
| ValueError: If required environment variables are missing. This exception is raised by the | |
| `getenv` calls for `VOLCENGINE_ACCESS_KEY`, `VOLCENGINE_SECRET_KEY`, and | |
| `TOOL_LLM_FIREWALL_APP_ID`. |
| decision = getattr(result, "decision", None) | ||
| decision_type = getattr(decision, "decision_type", None) | ||
| risk_info = getattr(result, "risk_info", None) | ||
| if int(decision_type) == 2 and risk_info: |
There was a problem hiding this comment.
Potential exception if decision_type is None. The code calls int(decision_type) without checking if decision_type is None first. If decision_type is None (which can happen when getattr returns None), this will raise a TypeError. Consider adding a null check: if decision_type is not None and int(decision_type) == 2 and risk_info:
| if int(decision_type) == 2 and risk_info: | |
| if decision_type is not None and int(decision_type) == 2 and risk_info: |
|
|
||
| for content in recent_contents: | ||
| parts = getattr(content, "parts", []) | ||
| if parts and hasattr(parts[0], "text"): |
There was a problem hiding this comment.
Potential IndexError if parts list is empty. While line 152 checks if parts, it assumes parts[0] exists when checking hasattr(parts[0], \"text\"). If parts is a truthy but empty container, this will raise an IndexError. The condition should be if parts and len(parts) > 0 and hasattr(parts[0], \"text\") or simplified to check the length explicitly.
| if parts and hasattr(parts[0], "text"): | |
| if parts and len(parts) > 0 and hasattr(parts[0], "text"): |
| risk_reasons_list = list(risk_reasons) | ||
|
|
||
| # Generate blocking response | ||
| reason_text = ( | ||
| ", ".join(risk_reasons_list) | ||
| if risk_reasons_list |
There was a problem hiding this comment.
The conversion of set to list at line 260 is unnecessary since the set is only used once for joining. You can directly join the set: reason_text = \", \".join(risk_reasons) if risk_reasons else \"security policy violation\". This simplifies the code and avoids the intermediate variable.
| risk_reasons_list = list(risk_reasons) | |
| # Generate blocking response | |
| reason_text = ( | |
| ", ".join(risk_reasons_list) | |
| if risk_reasons_list | |
| # Generate blocking response | |
| reason_text = ( | |
| ", ".join(risk_reasons) | |
| if risk_reasons |
| except Exception as e: | ||
| logger.error(f"LLM Firewall request failed: {e}") | ||
| return None |
There was a problem hiding this comment.
The broad Exception catch at line 224 makes it difficult to diagnose and handle different failure scenarios. Consider catching more specific exceptions (e.g., network errors, timeout errors, API errors) and logging them with appropriate severity levels. This would also help distinguish between transient failures and configuration issues.
cuericlee
left a comment
There was a problem hiding this comment.
add additional call back
before_agent_callback=governance.before_agent_callback,
after_agent_callback=governance.after_agent_callback,
| name="robot", | ||
| description="A robot can help user.", | ||
| instruction="Talk with user friendly.", | ||
| before_agent_callback=content_safety.before_agent_callback, |
| after_model_callback=content_safety.after_model_callback, | ||
| before_tool_callback=content_safety.before_tool_callback, | ||
| after_tool_callback=content_safety.after_tool_callback, | ||
| after_agent_callback=content_safety.after_agent_callback |
No description provided.