feat: implement deterministic tool permission gating for agents (#6232)#6274
feat: implement deterministic tool permission gating for agents (#6232)#6274Jacopos311 wants to merge 19 commits into
Conversation
… protocol and addressing CodeRabbit feedback
…t guardrail none checks
📝 WalkthroughWalkthroughThe PR adds ChangesMimir Memory Backend
Agent Capability and Approval Enforcement
Task Token Usage Tracking
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
note over Agent,ToolUsage: Capability & Approval Gate
end
participant Agent
participant ToolUsage
participant BaseTool
participant Human
Agent->>ToolUsage: _tool_calling(tool_string)
ToolUsage->>ToolUsage: parse tool_calling
ToolUsage->>BaseTool: check required_capability
alt agent.capabilities missing required_capability
ToolUsage-->>Agent: ToolUsageError("Security Violation")
else capability present or not required
ToolUsage->>Agent: check require_approval_for list
alt tool in require_approval_for
ToolUsage->>Human: _ask_human_approval(tool.name)
alt Human approves
Human-->>ToolUsage: approved
ToolUsage-->>Agent: tool_calling
else Human denies
ToolUsage-->>Agent: ToolUsageError("Execution cancelled by human operator.")
end
else not in require_approval_for
ToolUsage-->>Agent: tool_calling
end
end
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![corridor-security[bot]](https://avatars.githubusercontent.com/in/1177339?s=60&v=4){kind=small}
There was a problem hiding this comment.
Security Issues
- Broken Access Control: Tool capability checks fail open for tools without
required_capability, which is the default forBaseTool, allowing prompt-injected agents to execute unannotated sensitive tools. - Manual Approval Bypass: Tools listed in
require_approval_forare allowed if no approval handler is present, so approval-required execution can proceed without human approval.
Summary: This PR adds capability and approval gating around tool calls and introduces a Mimir memory backend using subprocess execution.
Risk: High risk. The new permission and approval gates are fail-open in security-critical tool execution paths, allowing realistic prompt-injection or untrusted task input to bypass intended tool restrictions.
Recommendations:
- Enforce fail-closed behavior for missing
required_capabilityor missing approval handlers before any tool execution. - Require every tool to declare an explicit capability, with explicit allowlisting for intentionally unrestricted tools.
| approved = self._ask_human_approval(tool.name) | ||
| if not approved: | ||
| return ToolUsageError("Execution cancelled by human operator.") | ||
| return tool_calling |
There was a problem hiding this comment.
The approval check is fail-open when no approval callback is present:
if tool.name in require_approval_for or (hasattr(tool, "required_capability") and tool.required_capability in require_approval_for):
if hasattr(self, "_ask_human_approval"):
approved = self._ask_human_approval(tool.name)
if not approved:
return ToolUsageError("Execution cancelled by human operator.")If _ask_human_approval is not implemented or configured, execution falls through to return tool_calling, so tools explicitly listed in require_approval_for run without human approval. A prompt-injection attacker can exploit this to trigger an approval-required tool whenever the runtime lacks that handler.
Remediation: Fail closed when approval is required but no approval handler is available, and only return tool_calling after an explicit approval decision.
For more details, see the finding in Corridor.
Provide feedback: Reply with whether this is a valid vulnerability or false positive to help improve Corridor's accuracy.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/crewai/src/crewai/task.py (2)
694-705:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
token_usageis written toTaskOutput, but the output contract does not define it.Line 704 and Line 833 pass
token_usage, butTaskOutput(inlib/crewai/src/crewai/tasks/task_output.py) has no declaredtoken_usagefield andto_dict()does not include it. This breaks the cross-file contract and makes token tracking non-portable downstream.Suggested contract fix
--- a/lib/crewai/src/crewai/tasks/task_output.py +++ b/lib/crewai/src/crewai/tasks/task_output.py @@ class TaskOutput(BaseModel): @@ messages: list[LLMMessage] = Field( description="Messages of the task", default_factory=list ) + token_usage: dict[str, Any] | int | float | None = Field( + default=None, + description="Token usage consumed while producing this task output.", + ) @@ def to_dict(self) -> dict[str, Any]: @@ if self.json_dict: output_dict.update(self.json_dict) elif self.pydantic: output_dict.update(self.pydantic.model_dump()) + if self.token_usage is not None: + output_dict["token_usage"] = self.token_usage return output_dictAlso applies to: 823-834
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai/src/crewai/task.py` around lines 694 - 705, The TaskOutput class in lib/crewai/src/crewai/tasks/task_output.py does not define a token_usage field, but line 704 in task.py passes token_usage=token_delta when instantiating TaskOutput. Add a token_usage field to the TaskOutput class definition and ensure it is included in the to_dict() method so the field is properly exposed in the output contract and can be accessed downstream.
1353-1385:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuardrail retries undercount token usage by reusing stale
current_token_usage.After each retry
agent.execute_task/agent.aexecute_taskcall, the newTaskOutputis rebuilt withtoken_usage=current_token_usage(Line 1384, Line 1497) without adding retry-call delta. This drops token consumption from guardrail retries and underreports task cost.Suggested accumulation fix (sync + async)
@@ - result = agent.execute_task( + retry_tokens_before = self._get_agent_token_usage(agent) + result = agent.execute_task( task=self, context=context, tools=tools, ) + retry_tokens_after = self._get_agent_token_usage(agent) + retry_delta = self._calculate_token_delta( + retry_tokens_before, retry_tokens_after + ) + merged_token_usage = self._merge_token_usage( + current_token_usage, retry_delta + ) @@ - token_usage=current_token_usage, + token_usage=merged_token_usage, )@@ - result = await agent.aexecute_task( + retry_tokens_before = self._get_agent_token_usage(agent) + result = await agent.aexecute_task( task=self, context=context, tools=tools, ) + retry_tokens_after = self._get_agent_token_usage(agent) + retry_delta = self._calculate_token_delta( + retry_tokens_before, retry_tokens_after + ) + merged_token_usage = self._merge_token_usage( + current_token_usage, retry_delta + ) @@ - token_usage=current_token_usage, + token_usage=merged_token_usage, )Also applies to: 1466-1498
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai/src/crewai/task.py` around lines 1353 - 1385, The token usage from guardrail retries is not being accumulated because current_token_usage is not updated after each agent.execute_task() call during retry attempts. To fix this, after each agent.execute_task() call (and similarly for agent.aexecute_task() in the async version), calculate the delta in token usage from the agent and add it to the current_token_usage variable before the TaskOutput is constructed with token_usage=current_token_usage. This ensures that token consumption from all retry attempts is properly accumulated and reflected in the final task output.
🧹 Nitpick comments (3)
lib/crewai/src/crewai/memory/storage/factory.py (1)
56-60: 💤 Low valueBroad
TypeErrorcatch may mask unrelated errors inside the factory.Catching
TypeErrorto detect signature incompatibility could also swallowTypeErrorexceptions raised from within the factory's implementation (e.g., from internal type mismatches). Consider inspecting the exception or usinginspect.signatureto check parameter support before calling.♻️ Alternative approach using signature inspection
import inspect factory = _factory if factory is not None: sig = inspect.signature(factory) if 'config' in sig.parameters: custom_backend = factory(spec, config=config) else: custom_backend = factory(spec) if custom_backend is not None: return custom_backend🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai/src/crewai/memory/storage/factory.py` around lines 56 - 60, The try-except block catching TypeError to determine if the factory function supports a config parameter is too broad and can mask actual TypeErrors occurring inside the factory implementation. Instead of using exception handling, import the inspect module and use inspect.signature to check if the factory function has a 'config' parameter before calling it. Check if 'config' is in the sig.parameters dictionary, and call factory(spec, config=config) if the parameter exists, otherwise call factory(spec). This approach is more explicit about detecting signature incompatibility and prevents masking genuine errors from within the factory implementation.lib/crewai/src/crewai/memory/storage/mimir_storage.py (1)
49-54: 💤 Low valueQuery validation is incomplete—only leading hyphen is blocked.
While blocking queries starting with
-prevents basic flag injection, other injection vectors remain:
- Queries containing shell metacharacters if ever interpolated
- Queries with embedded newlines or null bytes
- Very long queries that could cause DoS
Since
query_stris passed directly tosubprocess.runas a list element (not shell-interpolated), the current validation is adequate for flag injection. However, consider adding length limits for robustness.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai/src/crewai/memory/storage/mimir_storage.py` around lines 49 - 54, The _validate_inputs method currently validates query strings only for leading hyphens, but lacks length limit validation which could allow DoS attacks. Add a maximum length check for the query parameter to prevent excessively long strings from being processed. Implement this by adding a conditional check after the existing hyphen validation that raises a ValueError if the query exceeds a reasonable maximum length threshold (e.g., 10000 characters).Source: Linters/SAST tools
lib/crewai/src/crewai/tools/base_tool.py (1)
153-157: ⚡ Quick winRemove the duplicate
args_schemadeclaration.Lines 148-152 and Lines 153-157 define the same field twice; the second silently overwrites the first and adds noise.
♻️ Proposed cleanup
- args_schema: type[PydanticBaseModel] = Field( - default=_ArgsSchemaPlaceholder, - validate_default=True, - description="The schema for the arguments that the tool accepts.", - ) args_schema: type[PydanticBaseModel] = Field( default=_ArgsSchemaPlaceholder, validate_default=True, description="The schema for the arguments that the tool accepts.", )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai/src/crewai/tools/base_tool.py` around lines 153 - 157, The `args_schema` field is defined twice in the base_tool class, with the second definition overwriting the first. Remove the duplicate `args_schema` field declaration that uses Field with default=_ArgsSchemaPlaceholder, validate_default=True, and the description parameter. Keep only one definition of the `args_schema` field to eliminate the redundant code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/crewai/README.md`:
- Line 559: The README documentation at line 559 incorrectly describes how
CrewAI integrates with Mimir. Currently it states that CrewAI "communicates with
Mimir using MCP via standard I/O subprocesses," but the actual MimirStorage
implementation uses direct subprocess.run() calls to execute the mimir CLI
binary, not the Model Context Protocol. Update the documentation text to
accurately reflect that CrewAI invokes the mimir CLI directly via subprocess
calls rather than through MCP integration, removing any reference to MCP to
avoid misleading users about the integration mechanism.
- Around line 571-576: The Crew class instantiation in the example includes a
storage parameter with MimirStorage that is not supported by the Crew class.
Remove the line storage=MimirStorage(config=mimir_config) from the Crew
constructor call in the example, or if task output persistence is intended,
replace it with the correct parameter name and approach that the Crew class
actually supports. Verify the correct way to configure storage/persistence for
the Crew class and update the example accordingly.
In `@lib/crewai/src/crewai/memory/storage/mimir_storage.py`:
- Around line 56-63: The save method is attempting to access record.value on
line 59, but the MemoryRecord class defines the text field as content, not
value. This will cause an AttributeError at runtime. Change the line that reads
value_str = str(record.value) to instead use value_str = str(record.content) to
access the correct field from the MemoryRecord schema.
- Around line 141-144: In the MemoryRecord instantiation within the
formatted_results construction, change the parameter name from
`value=content_text` to `content=content_text` to match the correct field name
expected by the MemoryRecord constructor, which will prevent validation errors
when creating the MemoryRecord instance.
- Around line 89-98: The search method in mimir_storage.py has a signature that
does not conform to the StorageBackend protocol. Change the first parameter from
query: Any to query_embedding: list[float] to match the protocol requirement for
vector similarity search. Update the limit parameter default from None to 10,
and update the min_score parameter default from None to 0.0 to align with
protocol expectations. Additionally, review the extra parameters like
scope_prefix, categories, and metadata_filter to determine if they should be
removed from the signature or if they need special handling within the
implementation.
- Line 12: Fix three issues in the mimir_storage.py file: First, change the
import statement from `crewai.memory.storage.interface` to `crewai.memory.types`
to correctly import the `MemoryRecord` class. Second, update all field accesses
and assignments for `MemoryRecord` from `value` to `content` - specifically,
change `record.value` to `record.content` where records are accessed, and change
the `value=` parameter to `content=` where `MemoryRecord` instances are created.
Third, update the `search()` method signature to match the `StorageBackend`
protocol by replacing the `query: Any` parameter with `query_embedding:
list[float]` as the first parameter.
In `@lib/crewai/src/crewai/tools/base_tool.py`:
- Around line 158-161: The required_capability field defined in the base_tool.py
is not being propagated when tools are converted to their structured format,
which bypasses capability gating enforcement. When ToolUsage._tool_calling
evaluates the capability on the selected structured tool object, it cannot find
the required_capability field. Update the structured tool conversion path to
ensure the required_capability field is carried forward from the base tool to
the structured tool object so that capability enforcement remains intact across
all execution paths.
In `@lib/crewai/src/crewai/tools/tool_usage.py`:
- Line 862: The approval check condition at the location comparing tool.name
against require_approval_for is not using sanitize_tool_name() for consistency
with other comparisons throughout the file (as seen in lines 146-147, 305-308,
536-539, 708-710). Wrap tool.name with sanitize_tool_name() when comparing it
against require_approval_for to ensure consistent tool name matching and prevent
mismatches if the approval list contains sanitized or non-sanitized names.
- Around line 848-857: The code directly accesses self.agent.role in the error
message on line 854 without a null check, but the constructor allows agent to be
None, which will cause an AttributeError. Although line 855 safely checks if
self.agent exists before accessing verbose, this guard comes after the error
message construction. Use getattr with a default value or add a null check guard
before line 854 to safely access self.agent.role, similar to how
getattr(self.agent, "capabilities", None) is safely used on line 851.
- Around line 859-866: The human-in-the-loop approval feature is non-functional
because the _ask_human_approval method referenced in the hasattr check does not
exist in the ToolUsage class. To fix this, you need to either implement the
_ask_human_approval method in the ToolUsage class to handle the actual approval
logic (taking the tool name as a parameter and returning a boolean), or
integrate with an existing approval mechanism such as tools_handler or an
injected callback that can be used to retrieve user approval. Ensure that the
method is properly defined before the hasattr check will work correctly and the
human-in-the-loop gating logic becomes functional.
---
Outside diff comments:
In `@lib/crewai/src/crewai/task.py`:
- Around line 694-705: The TaskOutput class in
lib/crewai/src/crewai/tasks/task_output.py does not define a token_usage field,
but line 704 in task.py passes token_usage=token_delta when instantiating
TaskOutput. Add a token_usage field to the TaskOutput class definition and
ensure it is included in the to_dict() method so the field is properly exposed
in the output contract and can be accessed downstream.
- Around line 1353-1385: The token usage from guardrail retries is not being
accumulated because current_token_usage is not updated after each
agent.execute_task() call during retry attempts. To fix this, after each
agent.execute_task() call (and similarly for agent.aexecute_task() in the async
version), calculate the delta in token usage from the agent and add it to the
current_token_usage variable before the TaskOutput is constructed with
token_usage=current_token_usage. This ensures that token consumption from all
retry attempts is properly accumulated and reflected in the final task output.
---
Nitpick comments:
In `@lib/crewai/src/crewai/memory/storage/factory.py`:
- Around line 56-60: The try-except block catching TypeError to determine if the
factory function supports a config parameter is too broad and can mask actual
TypeErrors occurring inside the factory implementation. Instead of using
exception handling, import the inspect module and use inspect.signature to check
if the factory function has a 'config' parameter before calling it. Check if
'config' is in the sig.parameters dictionary, and call factory(spec,
config=config) if the parameter exists, otherwise call factory(spec). This
approach is more explicit about detecting signature incompatibility and prevents
masking genuine errors from within the factory implementation.
In `@lib/crewai/src/crewai/memory/storage/mimir_storage.py`:
- Around line 49-54: The _validate_inputs method currently validates query
strings only for leading hyphens, but lacks length limit validation which could
allow DoS attacks. Add a maximum length check for the query parameter to prevent
excessively long strings from being processed. Implement this by adding a
conditional check after the existing hyphen validation that raises a ValueError
if the query exceeds a reasonable maximum length threshold (e.g., 10000
characters).
In `@lib/crewai/src/crewai/tools/base_tool.py`:
- Around line 153-157: The `args_schema` field is defined twice in the base_tool
class, with the second definition overwriting the first. Remove the duplicate
`args_schema` field declaration that uses Field with
default=_ArgsSchemaPlaceholder, validate_default=True, and the description
parameter. Keep only one definition of the `args_schema` field to eliminate the
redundant code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1d343761-6d8e-436b-8d21-51d78686923e
📒 Files selected for processing (7)
lib/crewai/README.mdlib/crewai/src/crewai/agents/agent_builder/base_agent.pylib/crewai/src/crewai/memory/storage/factory.pylib/crewai/src/crewai/memory/storage/mimir_storage.pylib/crewai/src/crewai/task.pylib/crewai/src/crewai/tools/base_tool.pylib/crewai/src/crewai/tools/tool_usage.py
| 👉 [Perseus-Computing-LLC/mimir](https://github.com/Perseus-Computing-LLC/mimir) | ||
|
|
||
| #### Setup | ||
| Since CrewAI communicates with Mimir using MCP via standard I/O subprocesses, you must ensure the `mcp` Python package is installed (automatically handled by CrewAI dependencies). |
There was a problem hiding this comment.
Documentation incorrectly describes the integration mechanism.
The text states CrewAI "communicates with Mimir using MCP via standard I/O subprocesses," but the actual MimirStorage implementation uses direct subprocess.run() calls to the mimir CLI binary—not the Model Context Protocol. This could confuse users expecting MCP integration.
📝 Suggested correction
-Since CrewAI communicates with Mimir using MCP via standard I/O subprocesses, you must ensure the `mcp` Python package is installed (automatically handled by CrewAI dependencies).
+CrewAI communicates with Mimir by invoking the `mimir` CLI binary via subprocess. Ensure the binary is installed and available in your PATH.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/crewai/README.md` at line 559, The README documentation at line 559
incorrectly describes how CrewAI integrates with Mimir. Currently it states that
CrewAI "communicates with Mimir using MCP via standard I/O subprocesses," but
the actual MimirStorage implementation uses direct subprocess.run() calls to
execute the mimir CLI binary, not the Model Context Protocol. Update the
documentation text to accurately reflect that CrewAI invokes the mimir CLI
directly via subprocess calls rather than through MCP integration, removing any
reference to MCP to avoid misleading users about the integration mechanism.
| crew = Crew( | ||
| agents=[...], | ||
| tasks=[...], | ||
| memory=True, | ||
| storage=MimirStorage(config=mimir_config) | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Crew constructor signature for storage parameter
echo "=== Crew class definition ==="
ast-grep --pattern 'class Crew($_) {
$$$
def __init__($$$) {
$$$
}
$$$
}'
echo "=== Search for storage parameter in Crew ==="
rg -n "storage" --type py -C 3 -g "*crew*.py" | head -80Repository: crewAIInc/crewAI
Length of output: 4179
🏁 Script executed:
cat lib/crewai/src/crewai/crew.py | head -300 | tail -150Repository: crewAIInc/crewAI
Length of output: 6396
🏁 Script executed:
rg -n "def __init__" lib/crewai/src/crewai/crew.py -A 50 | head -100Repository: crewAIInc/crewAI
Length of output: 42
🏁 Script executed:
sed -n '180,350p' lib/crewai/src/crewai/crew.pyRepository: crewAIInc/crewAI
Length of output: 6873
🏁 Script executed:
sed -n '320,400p' lib/crewai/src/crewai/crew.pyRepository: crewAIInc/crewAI
Length of output: 3109
Remove or correct the storage parameter in the Crew example.
The Crew class does not accept a storage parameter. The example should be corrected to remove storage=MimirStorage(config=mimir_config) or use the correct parameter name/approach if task output persistence is intended.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/crewai/README.md` around lines 571 - 576, The Crew class instantiation in
the example includes a storage parameter with MimirStorage that is not supported
by the Crew class. Remove the line storage=MimirStorage(config=mimir_config)
from the Crew constructor call in the example, or if task output persistence is
intended, replace it with the correct parameter name and approach that the Crew
class actually supports. Verify the correct way to configure storage/persistence
for the Crew class and update the example accordingly.
|
|
||
| from crewai.memory.storage.backend import StorageBackend | ||
| # CodeRabbit Fix: Direct import to fail-fast and avoid masking integration issues | ||
| from crewai.memory.storage.interface import MemoryRecord # type: ignore |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify where MemoryRecord is actually defined
echo "=== Checking for MemoryRecord definition ==="
rg -n "class MemoryRecord" --type py
echo "=== Checking if interface module exists ==="
fd -t f "interface.py" --full-path "memory/storage"Repository: crewAIInc/crewAI
Length of output: 341
🏁 Script executed:
#!/bin/bash
# Find the mimir_storage.py file and check its imports
echo "=== Finding mimir_storage.py ==="
fd -t f "mimir_storage.py" --exec cat -n {}
echo "=== Checking for interface.py in memory directory ==="
fd -t f "interface.py" --full-path
echo "=== Checking all files in memory directory ==="
find . -path "*/memory/*" -name "*.py" -type f | head -20Repository: crewAIInc/crewAI
Length of output: 8483
🏁 Script executed:
#!/bin/bash
# Check MemoryRecord definition
echo "=== MemoryRecord definition in types.py ==="
sed -n '20,50p' lib/crewai/src/crewai/memory/types.py
echo "=== StorageBackend protocol definition ==="
cat lib/crewai/src/crewai/memory/storage/backend.py
echo "=== Check if interface.py exists anywhere ==="
find . -name "interface.py" -type fRepository: crewAIInc/crewAI
Length of output: 8308
🏁 Script executed:
#!/bin/bash
echo "=== Check storage/__init__.py ==="
cat lib/crewai/src/crewai/memory/storage/__init__.py
echo "=== Check memory/__init__.py ==="
cat lib/crewai/src/crewai/memory/__init__.pyRepository: crewAIInc/crewAI
Length of output: 1516
Fix import path and MemoryRecord field usage.
The import at line 12 references crewai.memory.storage.interface, which does not exist—MemoryRecord is defined in crewai.memory.types. Additionally, MemoryRecord has a content field, not value: line 59 accesses record.value (should be record.content), and line 143 creates an instance with value= parameter (should be content=). Finally, the search() method signature does not match the StorageBackend protocol, which expects query_embedding: list[float] as the first parameter, not query: Any.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/crewai/src/crewai/memory/storage/mimir_storage.py` at line 12, Fix three
issues in the mimir_storage.py file: First, change the import statement from
`crewai.memory.storage.interface` to `crewai.memory.types` to correctly import
the `MemoryRecord` class. Second, update all field accesses and assignments for
`MemoryRecord` from `value` to `content` - specifically, change `record.value`
to `record.content` where records are accessed, and change the `value=`
parameter to `content=` where `MemoryRecord` instances are created. Third,
update the `search()` method signature to match the `StorageBackend` protocol by
replacing the `query: Any` parameter with `query_embedding: list[float]` as the
first parameter.
| def save(self, records: List[MemoryRecord]) -> None: | ||
| """Saves a list of MemoryRecords conforming to the StorageBackend protocol.""" | ||
| for record in records: | ||
| value_str = str(record.value) | ||
|
|
||
| # Generate a persistent deterministic hash key using hashlib MD5 | ||
| hash_suffix = hashlib.md5(value_str.encode('utf-8')).hexdigest()[:12] | ||
| key = f"memory_{hash_suffix}" |
There was a problem hiding this comment.
MemoryRecord has a content field, not value—this will fail at runtime.
Per the MemoryRecord schema in crewai/memory/types.py, the text field is content, not value. Line 59 accesses record.value which will raise an AttributeError.
🐛 Proposed fix
def save(self, records: List[MemoryRecord]) -> None:
"""Saves a list of MemoryRecords conforming to the StorageBackend protocol."""
for record in records:
- value_str = str(record.value)
+ value_str = str(record.content)
# Generate a persistent deterministic hash key using hashlib MD5
hash_suffix = hashlib.md5(value_str.encode('utf-8')).hexdigest()[:12]📝 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.
| def save(self, records: List[MemoryRecord]) -> None: | |
| """Saves a list of MemoryRecords conforming to the StorageBackend protocol.""" | |
| for record in records: | |
| value_str = str(record.value) | |
| # Generate a persistent deterministic hash key using hashlib MD5 | |
| hash_suffix = hashlib.md5(value_str.encode('utf-8')).hexdigest()[:12] | |
| key = f"memory_{hash_suffix}" | |
| def save(self, records: List[MemoryRecord]) -> None: | |
| """Saves a list of MemoryRecords conforming to the StorageBackend protocol.""" | |
| for record in records: | |
| value_str = str(record.content) | |
| # Generate a persistent deterministic hash key using hashlib MD5 | |
| hash_suffix = hashlib.md5(value_str.encode('utf-8')).hexdigest()[:12] | |
| key = f"memory_{hash_suffix}" |
🧰 Tools
🪛 ast-grep (0.43.0)
[warning] 61-61: Do not use insecure functions
Context: hashlib.md5(value_str.encode('utf-8'))
Note: [CWE-327] [CWE-328].
(insecure-hash-functions)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/crewai/src/crewai/memory/storage/mimir_storage.py` around lines 56 - 63,
The save method is attempting to access record.value on line 59, but the
MemoryRecord class defines the text field as content, not value. This will cause
an AttributeError at runtime. Change the line that reads value_str =
str(record.value) to instead use value_str = str(record.content) to access the
correct field from the MemoryRecord schema.
| def search( | ||
| self, | ||
| query: Any, | ||
| limit: Optional[int] = None, | ||
| scope_prefix: Optional[str] = None, | ||
| categories: Optional[List[str]] = None, | ||
| min_score: Optional[float] = None, | ||
| metadata_filter: Optional[Dict[str, Any]] = None, | ||
| **kwargs | ||
| ) -> List[Tuple[MemoryRecord, float]]: |
There was a problem hiding this comment.
Method signature does not conform to StorageBackend protocol.
The StorageBackend.search() protocol expects query_embedding: list[float] as the first positional argument for vector similarity search, but this implementation accepts query: Any. The parameter order and defaults also differ (limit defaults to 10 in protocol, None here; min_score defaults to 0.0, not None).
This will cause type errors and unexpected behavior when the memory system passes an embedding vector.
🐛 Proposed fix to align with protocol
def search(
self,
- query: Any,
- limit: Optional[int] = None,
+ query_embedding: list[float],
scope_prefix: Optional[str] = None,
categories: Optional[List[str]] = None,
- min_score: Optional[float] = None,
metadata_filter: Optional[Dict[str, Any]] = None,
+ limit: int = 10,
+ min_score: float = 0.0,
**kwargs
) -> List[Tuple[MemoryRecord, float]]:
- """Searches memories and returns a list of (MemoryRecord, score) tuples."""
- query_str = query if isinstance(query, str) else str(query)
+ """Searches memories and returns a list of (MemoryRecord, score) tuples.
+
+ Note: query_embedding is converted to a string representation for the mimir CLI.
+ """
+ # Convert embedding to string for CLI (mimir may need different handling)
+ query_str = " ".join(str(x) for x in query_embedding[:10]) # or appropriate conversion
- actual_limit = limit if limit is not None else 3
+ actual_limit = limitYou'll need to verify how the mimir CLI expects search queries—if it requires text rather than embeddings, this backend may need a different integration approach (e.g., storing text alongside embeddings and using text-based search).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/crewai/src/crewai/memory/storage/mimir_storage.py` around lines 89 - 98,
The search method in mimir_storage.py has a signature that does not conform to
the StorageBackend protocol. Change the first parameter from query: Any to
query_embedding: list[float] to match the protocol requirement for vector
similarity search. Update the limit parameter default from None to 10, and
update the min_score parameter default from None to 0.0 to align with protocol
expectations. Additionally, review the extra parameters like scope_prefix,
categories, and metadata_filter to determine if they should be removed from the
signature or if they need special handling within the implementation.
|
|
||
| # Construct official MemoryRecord instances | ||
| record = MemoryRecord(value=content_text, metadata=meta) | ||
| formatted_results.append((record, score)) |
There was a problem hiding this comment.
MemoryRecord constructor uses wrong field name.
MemoryRecord expects content= not value=. This will raise a validation error.
🐛 Proposed fix
# Construct official MemoryRecord instances
- record = MemoryRecord(value=content_text, metadata=meta)
+ record = MemoryRecord(content=content_text, metadata=meta)
formatted_results.append((record, score))📝 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.
| # Construct official MemoryRecord instances | |
| record = MemoryRecord(value=content_text, metadata=meta) | |
| formatted_results.append((record, score)) | |
| # Construct official MemoryRecord instances | |
| record = MemoryRecord(content=content_text, metadata=meta) | |
| formatted_results.append((record, score)) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/crewai/src/crewai/memory/storage/mimir_storage.py` around lines 141 -
144, In the MemoryRecord instantiation within the formatted_results
construction, change the parameter name from `value=content_text` to
`content=content_text` to match the correct field name expected by the
MemoryRecord constructor, which will prevent validation errors when creating the
MemoryRecord instance.
| required_capability: Optional[str] = Field( | ||
| default=None, | ||
| description="The specific capability required to execute this tool." | ||
| ) |
There was a problem hiding this comment.
required_capability is not propagated to structured tools, so gating can be bypassed.
Line 158 introduces required_capability, but the structured conversion path does not carry it forward. ToolUsage._tool_calling evaluates capability on the selected structured tool object, so this drops enforcement on common execution paths.
🔧 Proposed fix
@@
def to_structured_tool(self) -> CrewStructuredTool:
"""Convert this tool to a CrewStructuredTool instance."""
self._set_args_schema()
structured_tool = CrewStructuredTool(
name=self.name,
description=self.description,
args_schema=self.args_schema,
func=self._run,
result_as_answer=self.result_as_answer,
max_usage_count=self.max_usage_count,
current_usage_count=self.current_usage_count,
cache_function=self.cache_function,
)
structured_tool._original_tool = self
+ setattr(structured_tool, "required_capability", self.required_capability)
return structured_tool
@@
return cls(
name=getattr(tool, "name", "Unnamed Tool"),
description=getattr(tool, "description", ""),
func=tool.func,
args_schema=args_schema,
+ required_capability=getattr(tool, "required_capability", None),
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/crewai/src/crewai/tools/base_tool.py` around lines 158 - 161, The
required_capability field defined in the base_tool.py is not being propagated
when tools are converted to their structured format, which bypasses capability
gating enforcement. When ToolUsage._tool_calling evaluates the capability on the
selected structured tool object, it cannot find the required_capability field.
Update the structured tool conversion path to ensure the required_capability
field is carried forward from the base tool to the structured tool object so
that capability enforcement remains intact across all execution paths.
| if tool and hasattr(tool, "required_capability"): | ||
| required_cap = tool.required_capability | ||
| if required_cap: | ||
| agent_caps = getattr(self.agent, "capabilities", None) or [] | ||
| # Block execution if the agent lacks the required capability | ||
| if required_cap not in agent_caps: | ||
| error_msg = f"Security Violation: Agent '{self.agent.role}' lacks the required capability '{required_cap}' to execute tool '{tool.name}'." | ||
| if self.agent and self.agent.verbose: | ||
| PRINTER.print(content=error_msg, color="red") | ||
| return ToolUsageError(error_msg) |
There was a problem hiding this comment.
self.agent.role access will raise AttributeError when agent is None.
The constructor allows agent to be None (line 92), and getattr(self.agent, "capabilities", None) on line 851 safely handles this. However, line 854 directly accesses self.agent.role without a null check, which will crash before reaching the guarded verbose check on line 855.
🐛 Proposed fix
# Block execution if the agent lacks the required capability
if required_cap not in agent_caps:
- error_msg = f"Security Violation: Agent '{self.agent.role}' lacks the required capability '{required_cap}' to execute tool '{tool.name}'."
- if self.agent and self.agent.verbose:
+ agent_role = getattr(self.agent, "role", "unknown") if self.agent else "unknown"
+ error_msg = f"Security Violation: Agent '{agent_role}' lacks the required capability '{required_cap}' to execute tool '{tool.name}'."
+ if self.agent and getattr(self.agent, "verbose", False):
PRINTER.print(content=error_msg, color="red")
return ToolUsageError(error_msg)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/crewai/src/crewai/tools/tool_usage.py` around lines 848 - 857, The code
directly accesses self.agent.role in the error message on line 854 without a
null check, but the constructor allows agent to be None, which will cause an
AttributeError. Although line 855 safely checks if self.agent exists before
accessing verbose, this guard comes after the error message construction. Use
getattr with a default value or add a null check guard before line 854 to safely
access self.agent.role, similar to how getattr(self.agent, "capabilities", None)
is safely used on line 851.
| # Human-in-the-loop gating / Manual approval check | ||
| if tool and self.agent: | ||
| require_approval_for = getattr(self.agent, "require_approval_for", None) or [] | ||
| if tool.name in require_approval_for or (hasattr(tool, "required_capability") and tool.required_capability in require_approval_for): | ||
| if hasattr(self, "_ask_human_approval"): | ||
| approved = self._ask_human_approval(tool.name) | ||
| if not approved: | ||
| return ToolUsageError("Execution cancelled by human operator.") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify _ask_human_approval is not defined anywhere in the codebase
echo "=== Searching for _ask_human_approval definition ==="
rg -n "_ask_human_approval" --type pyRepository: crewAIInc/crewAI
Length of output: 345
🏁 Script executed:
# First, find the ToolUsage class definition and examine the file structure
cd lib/crewai/src/crewai/tools && wc -l tool_usage.pyRepository: crewAIInc/crewAI
Length of output: 79
🏁 Script executed:
# Read the area around the code in question (lines 859-866) with more context
sed -n '840,880p' lib/crewai/src/crewai/tools/tool_usage.pyRepository: crewAIInc/crewAI
Length of output: 2595
🏁 Script executed:
# Search for class ToolUsage definition and check if _ask_human_approval is defined anywhere in the file
grep -n "class ToolUsage\|def _ask_human_approval" lib/crewai/src/crewai/tools/tool_usage.pyRepository: crewAIInc/crewAI
Length of output: 116
🏁 Script executed:
# Check if ToolUsage has a parent class and look for _ask_human_approval in any parent files
head -50 lib/crewai/src/crewai/tools/tool_usage.py | grep -E "^from|^import|^class ToolUsage"Repository: crewAIInc/crewAI
Length of output: 898
🏁 Script executed:
# Read the ToolUsage class definition starting from line 73
sed -n '73,150p' lib/crewai/src/crewai/tools/tool_usage.pyRepository: crewAIInc/crewAI
Length of output: 3000
🏁 Script executed:
# Search for any setattr, dynamic assignment, or method that might add _ask_human_approval
rg -n "setattr|_ask_human_approval\s*=|def.*ask.*human" lib/crewai/src/crewai/tools/tool_usage.pyRepository: crewAIInc/crewAI
Length of output: 42
🏁 Script executed:
# Search the entire codebase for _ask_human_approval including non-Python files
rg -n "_ask_human_approval"Repository: crewAIInc/crewAI
Length of output: 292
Human approval gate is dead code—_ask_human_approval method does not exist.
The hasattr(self, "_ask_human_approval") check on line 863 will always evaluate to False because this method is never defined in the ToolUsage class or anywhere in the codebase. The human-in-the-loop approval feature is non-functional.
Implement the _ask_human_approval method or wire this to an existing approval mechanism (e.g., via tools_handler or an injected callback).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/crewai/src/crewai/tools/tool_usage.py` around lines 859 - 866, The
human-in-the-loop approval feature is non-functional because the
_ask_human_approval method referenced in the hasattr check does not exist in the
ToolUsage class. To fix this, you need to either implement the
_ask_human_approval method in the ToolUsage class to handle the actual approval
logic (taking the tool name as a parameter and returning a boolean), or
integrate with an existing approval mechanism such as tools_handler or an
injected callback that can be used to retrieve user approval. Ensure that the
method is properly defined before the hasattr check will work correctly and the
human-in-the-loop gating logic becomes functional.
| # Human-in-the-loop gating / Manual approval check | ||
| if tool and self.agent: | ||
| require_approval_for = getattr(self.agent, "require_approval_for", None) or [] | ||
| if tool.name in require_approval_for or (hasattr(tool, "required_capability") and tool.required_capability in require_approval_for): |
There was a problem hiding this comment.
Tool name comparison should use sanitize_tool_name() for consistency.
Throughout this file (e.g., lines 146-147, 305-308, 536-539, 708-710), tool names are compared using sanitize_tool_name(). The approval check compares raw tool.name against require_approval_for, which may cause mismatches if the list contains sanitized names or vice versa.
🔧 Proposed fix
- if tool.name in require_approval_for or (hasattr(tool, "required_capability") and tool.required_capability in require_approval_for):
+ sanitized_name = sanitize_tool_name(tool.name)
+ sanitized_approval_list = [sanitize_tool_name(n) for n in require_approval_for]
+ if sanitized_name in sanitized_approval_list or (hasattr(tool, "required_capability") and tool.required_capability in require_approval_for):📝 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.
| if tool.name in require_approval_for or (hasattr(tool, "required_capability") and tool.required_capability in require_approval_for): | |
| sanitized_name = sanitize_tool_name(tool.name) | |
| sanitized_approval_list = [sanitize_tool_name(n) for n in require_approval_for] | |
| if sanitized_name in sanitized_approval_list or (hasattr(tool, "required_capability") and tool.required_capability in require_approval_for): |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/crewai/src/crewai/tools/tool_usage.py` at line 862, The approval check
condition at the location comparing tool.name against require_approval_for is
not using sanitize_tool_name() for consistency with other comparisons throughout
the file (as seen in lines 146-147, 305-308, 536-539, 708-710). Wrap tool.name
with sanitize_tool_name() when comparing it against require_approval_for to
ensure consistent tool name matching and prevent mismatches if the approval list
contains sanitized or non-sanitized names.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/crewai/src/crewai/tools/base_tool.py (1)
150-171:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMerge conflict artifacts break module loading.
Lines 155 and 171 contain bare branch names (
feat/agent-permission-gatingandmain) that are remnants of an unresolved git merge. These will raiseNameErrorat import time. Additionally,args_schemais defined twice (lines 150-154 and 156-160).Remove the conflict markers and duplicate field definition to restore valid Python syntax.
🐛 Proposed fix
args_schema: type[PydanticBaseModel] = Field( default=_ArgsSchemaPlaceholder, validate_default=True, description="The schema for the arguments that the tool accepts.", ) - feat/agent-permission-gating - args_schema: type[PydanticBaseModel] = Field( - default=_ArgsSchemaPlaceholder, - validate_default=True, - description="The schema for the arguments that the tool accepts.", - ) required_capability: Optional[str] = Field( default=None, description="The specific capability required to execute this tool." ) result_schema: type[PydanticBaseModel] | None = Field( default=None, validate_default=True, description="The schema for the output that the tool returns.", ) - main `@field_serializer`("args_schema", when_used="json")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai/src/crewai/tools/base_tool.py` around lines 150 - 171, Remove the merge conflict artifacts and duplicate field definitions in the BaseTool class. Delete the second occurrence of the args_schema field definition along with the bare branch name identifiers (feat/agent-permission-gating and main) that appear between field definitions. Keep only one args_schema field definition and retain the newly added required_capability and result_schema fields to ensure valid Python syntax and proper module loading.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@lib/crewai/src/crewai/tools/base_tool.py`:
- Around line 150-171: Remove the merge conflict artifacts and duplicate field
definitions in the BaseTool class. Delete the second occurrence of the
args_schema field definition along with the bare branch name identifiers
(feat/agent-permission-gating and main) that appear between field definitions.
Keep only one args_schema field definition and retain the newly added
required_capability and result_schema fields to ensure valid Python syntax and
proper module loading.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b04965bd-05e5-4823-8d0f-5ba383a47b02
📒 Files selected for processing (2)
lib/crewai/src/crewai/tools/base_tool.pylib/crewai/src/crewai/tools/tool_usage.py
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/crewai/src/crewai/tools/tool_usage.py
|
Capability gating solves an important problem: "Can this agent use this tool?" One thing we've run into in production is that access control and execution control tend to become separate concerns over time. An agent may legitimately have access to a payment, database, or deployment tool, but the actual action can still require additional checks based on context (amount, environment, risk level, approvals, etc.). Feels like capability gating is a strong foundation, with policy-driven execution decisions sitting as a complementary layer on top. Curious how others are handling that distinction. |
3 participants
Implements deterministic tool permission gating based on agent capabilities to structurally resolve the prompt injection and permission bypass issue #6232.
Summary by CodeRabbit