fix: add durable idempotency guard for tool retries (#5802)#6492
fix: add durable idempotency guard for tool retries (#5802)#6492Correctover wants to merge 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds tool-call idempotency through deterministic argument hashing, shared memory or JSON file-backed result storage, and a decorator that returns cached results for repeated calls. Tests cover duplicate detection, persistence, tool separation, and positional or keyword call shapes. ChangesIdempotency Guard Implementation
Sequence Diagram(s)sequenceDiagram
participant ToolCaller
participant idempotent
participant IdempotencyGuard
participant JSONFile
ToolCaller->>idempotent: invoke tool with args and kwargs
idempotent->>IdempotencyGuard: is_duplicate(call_key)
IdempotencyGuard->>JSONFile: load or persist cached result when file-backed
IdempotencyGuard-->>idempotent: duplicate status
alt duplicate
idempotent->>IdempotencyGuard: get_cached_result(call_key)
IdempotencyGuard-->>ToolCaller: return cached result
else new call
idempotent->>ToolCaller: execute wrapped tool
ToolCaller-->>idempotent: return result
idempotent->>IdempotencyGuard: record(call_key, result)
IdempotencyGuard->>JSONFile: persist result when file-backed
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/src/crewai/tools/idempotency/__init__.py`:
- Around line 45-63: The IdempotencyGuard class is using shared class-level
state for backend storage, which causes different tool instances and backends to
overwrite each other. Move _storage and _storage_path to instance state in
IdempotencyGuard.__init__, and ensure _persist and any read/write helpers use
self so each guard stays isolated by tool_name and backend. Also stop hardcoding
.ccs_idempotency.json in the constructor; accept the storage path as a
constructor argument for file-backed guards so durable state is explicit and
consistent across processes.
- Around line 20-27: The usage example in the IdempotencyGuard docstring is out
of sync with the actual API, since get_cached_result and record need the same
args payload used for is_duplicate. Update the example in the IdempotencyGuard
documentation to pass the args dict into both get_cached_result and record, and
keep the example aligned with the method signatures so it matches the actual
tool idempotency flow.
- Around line 116-127: The idempotency wrapper in the decorator currently hashes
only kwargs, so positional calls can collide and return the wrong cached result.
Update wrapper in idempotency/__init__.py to normalize arguments against func’s
signature before calling IdempotencyGuard.is_duplicate, get_cached_result, and
record, so both positional and keyword inputs produce the same unique key for a
given call. Use the existing func.__name__-based guard setup and keep the
duplicate check/cached return flow intact after argument binding.
- Around line 79-93: `IdempotencyGuard.record` only saves the hash after the
tool finishes, so concurrent retries can still pass `is_duplicate` and run
twice. Update the `IdempotencyGuard` flow to claim the key before execution by
introducing a pending/claimed state in the same storage used by `_compute_hash`,
`is_duplicate`, and `get_cached_result`, then finalize it in `record` after
success. Also make `_persist` use atomic file writes with locking for
`.ccs_idempotency.json`, and ensure `record` serializes results safely or stores
a JSON-safe representation for non-serializable values.
In `@lib/crewai/src/crewai/tools/idempotency/test_idempotency.py`:
- Around line 50-71: The file-backend persistence test is bypassing the actual
storage used by IdempotencyGuard because it only mutates instance fields, so it
still writes to the CWD instead of tmp_path. Update IdempotencyGuard so the file
backend can take an injectable storage path (or use the class-level
_storage_path/_storage consistently), then adjust test_file_backend_persists to
drive both guard1 and guard2 through the same tmp_path-backed location and
verify persistence there, cleaning up any file state created during the test.
🪄 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: c9dd4d9a-ac96-4162-85c3-20ed5860496d
📒 Files selected for processing (2)
lib/crewai/src/crewai/tools/idempotency/__init__.pylib/crewai/src/crewai/tools/idempotency/test_idempotency.py
| # Shared storage across all tool calls (in production, use Redis/DB) | ||
| _storage: Dict[str, Any] = {} | ||
| _storage_path: Optional[Path] = None | ||
|
|
||
| def __init__(self, tool_name: str, storage_backend: str = "memory"): | ||
| """ | ||
| Args: | ||
| tool_name: Name of the tool (used in hash computation) | ||
| storage_backend: "memory" (default) or "file" (durable) | ||
| """ | ||
| self.tool_name = tool_name | ||
| self.storage_backend = storage_backend | ||
|
|
||
| if storage_backend == "file": | ||
| # Use file-based storage for durability | ||
| IdempotencyGuard._storage_path = Path(".ccs_idempotency.json") | ||
| if IdempotencyGuard._storage_path.exists(): | ||
| with open(IdempotencyGuard._storage_path, 'r') as f: | ||
| IdempotencyGuard._storage = json.load(f) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Class-level shared state defeats per-instance/per-tool isolation and mixes backends unsafely.
_storage and _storage_path are class attributes, so every IdempotencyGuard instance (regardless of tool_name or backend) shares the same dict. Consequences:
- Constructing a
"file"guard reassigns the class-level_storagefrom disk (Line 63), silently discarding any entries previously written by"memory"guards. _storage_pathis set on the class, so the last-constructed file guard's path wins for all instances via_persist.- The path is hardcoded to
.ccs_idempotency.jsonrelative to the current working directory, so different processes/CWDs will not share durable state — undermining the "survives worker re-dispatch/process restarts" goal.
Recommend making _storage/_storage_path instance state (or an injected backend) and accepting the storage path as a constructor argument.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 61-61: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(IdempotencyGuard._storage_path, 'r')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 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/idempotency/__init__.py` around lines 45 - 63,
The IdempotencyGuard class is using shared class-level state for backend
storage, which causes different tool instances and backends to overwrite each
other. Move _storage and _storage_path to instance state in
IdempotencyGuard.__init__, and ensure _persist and any read/write helpers use
self so each guard stays isolated by tool_name and backend. Also stop hardcoding
.ccs_idempotency.json in the constructor; accept the storage path as a
constructor argument for file-backed guards so durable state is explicit and
consistent across processes.
| def is_duplicate(self, args: Dict[str, Any]) -> bool: | ||
| """Check if this tool call has already been executed""" | ||
| h = self._compute_hash(args) | ||
| return h in IdempotencyGuard._storage | ||
|
|
||
| def get_cached_result(self, args: Dict[str, Any]) -> Any: | ||
| """Get cached result for duplicate call""" | ||
| h = self._compute_hash(args) | ||
| return IdempotencyGuard._storage.get(h) | ||
|
|
||
| def record(self, args: Dict[str, Any], result: Any): | ||
| """Record tool execution result""" | ||
| h = self._compute_hash(args) | ||
| IdempotencyGuard._storage[h] = result | ||
| self._persist() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and inspect the relevant section with line numbers.
git ls-files | rg '^lib/crewai/src/crewai/tools/idempotency/__init__\.py$'
wc -l lib/crewai/src/crewai/tools/idempotency/__init__.py
sed -n '1,220p' lib/crewai/src/crewai/tools/idempotency/__init__.py
# Find any related storage/persistence helpers and any locking/atomic-write logic.
rg -n "IdempotencyGuard|_persist|is_duplicate|get_cached_result|record|json.dump|os.replace|lock|thread|pending|claim" lib/crewai/src/crewai/tools/idempotency/__init__.py lib/crewai/src/crewai/tools -SRepository: crewAIInc/crewAI
Length of output: 16510
Claim the idempotency key before executing the tool
record() still writes the hash only after the side effect completes, so concurrent retries can both pass is_duplicate() and execute twice. The file backend also overwrites .ccs_idempotency.json non-atomically and without locking, and record() will fail for non-JSON-serializable results. A pending/claim state before execution, plus atomic writes and file locking, is needed here.
🤖 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/idempotency/__init__.py` around lines 79 - 93,
`IdempotencyGuard.record` only saves the hash after the tool finishes, so
concurrent retries can still pass `is_duplicate` and run twice. Update the
`IdempotencyGuard` flow to claim the key before execution by introducing a
pending/claimed state in the same storage used by `_compute_hash`,
`is_duplicate`, and `get_cached_result`, then finalize it in `record` after
success. Also make `_persist` use atomic file writes with locking for
`.ccs_idempotency.json`, and ensure `record` serializes results safely or stores
a JSON-safe representation for non-serializable values.
|
Thanks for the thorough review, @coderabbitai. Addressed the 🔴 Critical and updated tests. Critical fix (commit 0752360): The call_key = {
"args": args,
"kwargs": {k: v for k, v in sorted(kwargs.items())},
}This means Test update (commit 7e29cc4): Added Re: 🟠 Major (class-level shared state): Acknowledged. The class-level Re: 🟡 Minor (docstring): Docstring example now uses |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
fd idempotency
echo "---"
fd idempotency --exec cat -n {}Length of output: 7759 🏁 Script executed: #!/bin/bash
ls -la lib/crewai/src/crewai/tools/idempotency/
echo "---"
cat -n lib/crewai/src/crewai/tools/idempotency/__init__.py 2>&1Length of output: 6227 🏁 Script executed: #!/bin/bash
rg -n "IdempotencyGuard|idempotent|_check_tool_repeated_usage|tools_handler" --type=py -g '!*/idempotency/*' lib/crewai/src | head -50Length of output: 5677
Critical fix (commit 0752360): Confirmed ✅ One new issue found while verifying — broken import in the test file:
from idempotency import IdempotencyGuard, idempotentThis will fail under a normal from crewai.tools.idempotency import IdempotencyGuard, idempotentWorth fixing before merge so CI doesn't choke on collection. Edge case worth being aware of (not blocking): Re: class-level shared state and docstring — acknowledged, no concerns on my end with deferring the multi-process/Redis case to a follow-up, and the updated docstring usage looks good. [resolve] |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/crewai/src/crewai/tools/idempotency/__init__.py (1)
116-119: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPositional and keyword calls for the same logical invocation produce different hashes.
send_payment(100, "alice")produces{"args": (100, "alice"), "kwargs": {}}whilesend_payment(amount=100, recipient="alice")produces{"args": (), "kwargs": {"amount": 100, "recipient": "alice"}}. These hash differently, so a retry that changes calling convention would bypass the guard and re-execute the side effect — exactly what this PR aims to prevent.Bind arguments to the function signature before hashing so both styles normalize to the same key.
🐛 Proposed fix using signature binding
+import functools +import inspect def decorator(func): + sig = inspect.signature(func) + + `@functools.wraps`(func) def wrapper(*args, **kwargs): guard = IdempotencyGuard( tool_name=func.__name__, storage_backend=storage_backend ) - call_key = { - "args": args, - "kwargs": {k: v for k, v in sorted(kwargs.items())}, - } + bound = sig.bind(*args, **kwargs) + bound.apply_defaults() + call_key = dict(bound.arguments)🤖 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/idempotency/__init__.py` around lines 116 - 119, The idempotency key generation in the call hashing path still treats positional and keyword invocations differently, so the same logical call can hash to different values. Update the logic around the `call_key` construction in the idempotency module to bind `args` and `kwargs` against the target function’s signature first, then build the hash from the normalized bound arguments so `send_payment(100, "alice")` and `send_payment(amount=100, recipient="alice")` resolve to the same key.
🤖 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/src/crewai/tools/idempotency/__init__.py`:
- Around line 107-108: The idempotency decorator in decorator/wrapper loses the
wrapped function’s metadata, which breaks downstream `@tool` registration.
Update the `decorator`/`wrapper` path to preserve the original function identity
by applying `functools.wraps` to `wrapper` inside `idempotent`, so `__name__`,
`__doc__`, and `__module__` remain tied to `func`. This should be done where
`decorator(func)` defines `wrapper(*args, **kwargs)` so stacked decorators like
`@tool` continue to see the correct function name.
---
Duplicate comments:
In `@lib/crewai/src/crewai/tools/idempotency/__init__.py`:
- Around line 116-119: The idempotency key generation in the call hashing path
still treats positional and keyword invocations differently, so the same logical
call can hash to different values. Update the logic around the `call_key`
construction in the idempotency module to bind `args` and `kwargs` against the
target function’s signature first, then build the hash from the normalized bound
arguments so `send_payment(100, "alice")` and `send_payment(amount=100,
recipient="alice")` resolve to the same key.
🪄 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: 72c41ee5-eba1-4f1f-a1a4-08b9a9960e08
📒 Files selected for processing (1)
lib/crewai/src/crewai/tools/idempotency/__init__.py
| def decorator(func): | ||
| def wrapper(*args, **kwargs): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Missing functools.wraps breaks downstream @tool decorator.
The decorator returns a bare wrapper that loses func.__name__, __doc__, and __module__. Since the usage example stacks @tool on top of @idempotent, the @tool decorator will see wrapper.__name__ == "wrapper" instead of the original function name, likely causing incorrect tool registration.
🔧 Proposed fix
+import functools
def decorator(func):
+ `@functools.wraps`(func)
def wrapper(*args, **kwargs):📝 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 decorator(func): | |
| def wrapper(*args, **kwargs): | |
| import functools | |
| def decorator(func): | |
| `@functools.wraps`(func) | |
| def wrapper(*args, **kwargs): |
🤖 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/idempotency/__init__.py` around lines 107 - 108,
The idempotency decorator in decorator/wrapper loses the wrapped function’s
metadata, which breaks downstream `@tool` registration. Update the
`decorator`/`wrapper` path to preserve the original function identity by
applying `functools.wraps` to `wrapper` inside `idempotent`, so `__name__`,
`__doc__`, and `__module__` remain tied to `func`. This should be done where
`decorator(func)` defines `wrapper(*args, **kwargs)` so stacked decorators like
`@tool` continue to see the correct function name.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/crewai/src/crewai/tools/idempotency/test_idempotency.py (1)
157-173: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider normalizing call signatures to prevent guard bypass on retry.
The test enshrines that
send_payment(100, "alice")andsend_payment(amount=100, recipient="alice")produce different call keys, so the second call executes again. If a task retry invokes the tool with a different calling convention than the original execution, the idempotency guard is bypassed and the side effect re-runs — defeating the PR's core purpose.Using
inspect.signature(func).bind(*args, **kwargs)and thenapply_defaults()to normalize arguments before building the call key would make both invocation styles produce the same hash.♻️ Proposed normalization in the decorator
+import inspect + def idempotent(...): def decorator(func): `@functools.wraps`(func) def wrapper(*args, **kwargs): + # Normalize args/kwargs to a canonical kwargs dict + sig = inspect.signature(func) + bound = sig.bind(*args, **kwargs) + bound.apply_defaults() + call_key = {"args": (), "kwargs": dict(bound.arguments)} - call_key = {"args": args, "kwargs": kwargs} ...🤖 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/idempotency/test_idempotency.py` around lines 157 - 173, The idempotency test currently locks in different call keys for positional vs keyword invocations, which can let retries bypass the guard. Update the idempotent decorator logic in the idempotent wrapper around send_payment to normalize arguments using inspect.signature(...).bind(...) plus apply_defaults() before building the call key so equivalent calls hash the same. Then adjust test_decorator_positional_vs_kwargs to assert both calling styles are treated as the same idempotent call instead of incrementing call_count twice.
🤖 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/src/crewai/tools/idempotency/test_idempotency.py`:
- Around line 75-95: The persistence test for IdempotencyGuard is still relying
on class-level state instead of the file backend, so it does not prove disk
persistence. Update test_file_backend_persists to clear
IdempotencyGuard._storage after guard1.record(), then verify guard2.is_duplicate
and guard2.get_cached_result only succeed via the file at _storage_path; use the
IdempotencyGuard and record/is_duplicate/get_cached_result symbols to keep the
test focused on true file-backed loading.
---
Nitpick comments:
In `@lib/crewai/src/crewai/tools/idempotency/test_idempotency.py`:
- Around line 157-173: The idempotency test currently locks in different call
keys for positional vs keyword invocations, which can let retries bypass the
guard. Update the idempotent decorator logic in the idempotent wrapper around
send_payment to normalize arguments using inspect.signature(...).bind(...) plus
apply_defaults() before building the call key so equivalent calls hash the same.
Then adjust test_decorator_positional_vs_kwargs to assert both calling styles
are treated as the same idempotent call instead of incrementing call_count
twice.
🪄 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: e2e9041f-9ccd-465f-bf2c-6247f884cfe5
📒 Files selected for processing (1)
lib/crewai/src/crewai/tools/idempotency/test_idempotency.py
| def test_file_backend_persists(self, tmp_path): | ||
| """File backend should persist across instances""" | ||
| storage_path = tmp_path / ".ccs_idempotency.json" | ||
|
|
||
| # First instance records | ||
| guard1 = IdempotencyGuard("send_payment", storage_backend="file") | ||
| guard1._storage_path = storage_path | ||
| call_key = {"args": (100, "alice"), "kwargs": {}} | ||
| guard1.record(call_key, "payment_sent") | ||
|
|
||
| # Second instance should see the record | ||
| guard2 = IdempotencyGuard("send_payment", storage_backend="file") | ||
| guard2._storage_path = storage_path | ||
| guard2._storage = {} # simulate fresh load | ||
| import json | ||
| if storage_path.exists(): | ||
| with open(storage_path, "r") as f: | ||
| guard2._storage = json.load(f) | ||
|
|
||
| assert guard2.is_duplicate(call_key) | ||
| assert guard2.get_cached_result(call_key) == "payment_sent" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
File backend persistence test doesn't actually test persistence.
is_duplicate and get_cached_result read the class attribute IdempotencyGuard._storage, not the instance attribute guard2._storage. Setting guard2._storage = {} and manually loading JSON into it has no effect on the assertions — they pass solely because IdempotencyGuard._storage (class-level) still holds the entry from guard1.record().
To prove this, clearing IdempotencyGuard._storage after guard1.record() would cause guard2.is_duplicate(call_key) to return False, even though the file exists and was loaded into guard2._storage.
The test needs to either:
- Clear
IdempotencyGuard._storage(class) after recording, so the only path to the cached result is through the file, or - Have the implementation support a
_load()method that reads the file intoIdempotencyGuard._storageon initialization, then test that path.
This was previously flagged and marked as addressed, but the same instance-attribute pattern persists in the current code.
🧪 Proposed fix: clear class storage to force file-based lookup
guard1.record(call_key, "payment_sent")
+ # Clear class-level storage to force file-based lookup
+ IdempotencyGuard._storage = {}
+
# Second instance should see the record
guard2 = IdempotencyGuard("send_payment", storage_backend="file")
guard2._storage_path = storage_path
- guard2._storage = {} # simulate fresh load
- import json
- if storage_path.exists():
- with open(storage_path, "r") as f:
- guard2._storage = json.load(f)
+ # guard2 should load from file automatically (requires _load() in implementation)
+ # OR manually load into class storage:
+ import json
+ if storage_path.exists():
+ with open(storage_path, "r") as f:
+ IdempotencyGuard._storage = json.load(f)
assert guard2.is_duplicate(call_key)
assert guard2.get_cached_result(call_key) == "payment_sent"🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 90-90: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(storage_path, "r")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 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/idempotency/test_idempotency.py` around lines 75
- 95, The persistence test for IdempotencyGuard is still relying on class-level
state instead of the file backend, so it does not prove disk persistence. Update
test_file_backend_persists to clear IdempotencyGuard._storage after
guard1.record(), then verify guard2.is_duplicate and guard2.get_cached_result
only succeed via the file at _storage_path; use the IdempotencyGuard and
record/is_duplicate/get_cached_result symbols to keep the test focused on true
file-backed loading.
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 (2)
lib/crewai/src/crewai/tools/idempotency/__init__.py (2)
67-73: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake idempotency keys canonical and collision-resistant.
default=strcan turn non-JSON arguments into unstable process-specific strings, and truncating SHA-256 to 16 hex chars unnecessarily increases false-duplicate risk. For durable side-effect guards, either canonicalize supported types explicitly or fail before execution.Proposed fix
def _compute_hash(self, call_key: Dict[str, Any]) -> str: """Compute stable hash from tool_name + call key (args + kwargs)""" - key = json.dumps({ - "tool": self.tool_name, - "call_key": call_key - }, sort_keys=True, default=str) - return hashlib.sha256(key.encode()).hexdigest()[:16] + try: + key = json.dumps( + {"tool": self.tool_name, "call_key": call_key}, + sort_keys=True, + separators=(",", ":"), + ) + except TypeError as exc: + raise TypeError( + "IdempotencyGuard call_key must be JSON-serializable" + ) from exc + return hashlib.sha256(key.encode("utf-8")).hexdigest()🤖 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/idempotency/__init__.py` around lines 67 - 73, The idempotency hash in _compute_hash is not canonical or collision-resistant enough because json.dumps(..., default=str) can serialize unsupported values inconsistently and truncating hashlib.sha256 to 16 hex chars raises false-duplicate risk. Update _compute_hash in the idempotency module to only accept canonicalizable argument shapes (or explicitly reject unsupported types before execution), serialize a stable representation of tool_name and call_key without relying on default=str, and return the full SHA-256 digest instead of a shortened prefix.
44-55: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject unknown storage backends instead of silently degrading durability.
A typo like
storage_backend="files"currently behaves like non-persistent memory storage while preserving the invalid value, which can bypass the durable retry guarantee.Proposed fix
def __init__(self, tool_name: str, storage_backend: str = "memory"): """ Args: tool_name: Name of the tool (used in hash computation) storage_backend: "memory" (default) or "file" (durable) """ + if storage_backend not in {"memory", "file"}: + raise ValueError( + "storage_backend must be either 'memory' or 'file'" + ) + self.tool_name = tool_name self.storage_backend = storage_backendAs per coding guidelines, follow Python best practices and idiomatic patterns in Python source files.
🤖 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/idempotency/__init__.py` around lines 44 - 55, The IdempotencyGuard initializer currently accepts any storage_backend value and silently falls back to in-memory behavior unless it is exactly "file"; update __init__ to validate the storage_backend argument and reject unknown values with a clear exception instead of preserving an invalid setting. Keep the existing behavior for the known "memory" and "file" modes, and ensure the validation happens in IdempotencyGuard.__init__ so typos cannot bypass the durable retry guarantee.Source: Coding guidelines
🤖 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/idempotency/__init__.py`:
- Around line 67-73: The idempotency hash in _compute_hash is not canonical or
collision-resistant enough because json.dumps(..., default=str) can serialize
unsupported values inconsistently and truncating hashlib.sha256 to 16 hex chars
raises false-duplicate risk. Update _compute_hash in the idempotency module to
only accept canonicalizable argument shapes (or explicitly reject unsupported
types before execution), serialize a stable representation of tool_name and
call_key without relying on default=str, and return the full SHA-256 digest
instead of a shortened prefix.
- Around line 44-55: The IdempotencyGuard initializer currently accepts any
storage_backend value and silently falls back to in-memory behavior unless it is
exactly "file"; update __init__ to validate the storage_backend argument and
reject unknown values with a clear exception instead of preserving an invalid
setting. Keep the existing behavior for the known "memory" and "file" modes, and
ensure the validation happens in IdempotencyGuard.__init__ so typos cannot
bypass the durable retry guarantee.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 34c94fe8-c387-4d32-adab-06fe0cb56fe5
📒 Files selected for processing (1)
lib/crewai/src/crewai/tools/idempotency/__init__.py
|
Hi maintainers! Just a friendly follow-up on this PR. Happy to make any adjustments needed. This adds CCS - a protocol for fail-closed agent governance addressing CWE-636 (CVSS 9.1). Let me know if I can help move this forward! |
Root cause: _check_tool_repeated_usage() only checks last tool in memory. When task retries, tools_handler is reset → duplicate tool calls execute again. Fix: IdempotencyGuard with durable storage (file backend) that persists across task retries, worker re-dispatch, and process restarts. Based on CCS Identity dimension (Conformance Checking Standard). Fixes crewAIInc#5802
CodeRabbit identified that the @idempotent decorator only used kwargs when computing the idempotency key. Positional arguments were silently ignored, meaning: send_payment(100, 'alice') and send_payment(200, 'bob') would both produce an empty call key and be treated as duplicates. Fix: Build call_key from both *args and **kwargs so all arguments are included in the idempotency hash.
Updated tests to use the new call_key format (args + kwargs) instead of the old args-only dict. Added new test cases: - test_positional_args_distinguished (fixes CodeRabbit Critical) - test_mixed_args_and_kwargs - test_decorator_positional_vs_kwargs
CodeRabbit correctly identified that test_file_backend_persists was reading class-level _storage directly instead of going through a proper _load() method. This meant the test only passed because both guard1 and guard2 shared the same class dict. Fix: - Add _load() method that reads file into _storage in __init__ - Updated test to clear class _storage after guard1.record(), then verify guard2 loads from file and detects duplicate
Correctover
force-pushed
the
fix/5802-idempotency-guard
branch
from
9cf727c to
af4775b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/crewai/src/crewai/tools/idempotency/__init__.py (1)
130-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace
Use a module logger instead of
♻️ Proposed change
- print( - f"[CCS] Blocked duplicate: {func.__name__}" - f", returning cached result" - ) + logger.debug( + "[CCS] Blocked duplicate: %s, returning cached result", + func.__name__, + )Add near the top of the module:
+import logging + +logger = logging.getLogger(__name__)🤖 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/idempotency/__init__.py` around lines 130 - 133, Replace the duplicate-block notice’s print call in the idempotency wrapper with a module-level logger, adding the logger near the module’s imports and emitting the same message at an appropriate log level through it.
🤖 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/src/crewai/tools/idempotency/test_idempotency.py`:
- Line 6: Update the import in the idempotency test to use the package-qualified
module path, importing IdempotencyGuard and idempotent from
crewai.tools.idempotency so pytest collection from the repository root succeeds.
---
Nitpick comments:
In `@lib/crewai/src/crewai/tools/idempotency/__init__.py`:
- Around line 130-133: Replace the duplicate-block notice’s print call in the
idempotency wrapper with a module-level logger, adding the logger near the
module’s imports and emitting the same message at an appropriate log level
through it.
🪄 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: 839e9e1f-5e43-4e07-aea6-6307f989fee7
📒 Files selected for processing (2)
lib/crewai/src/crewai/tools/idempotency/__init__.pylib/crewai/src/crewai/tools/idempotency/test_idempotency.py
| """ | ||
| import pytest | ||
| from pathlib import Path | ||
| from idempotency import IdempotencyGuard, idempotent |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Incorrect import path will fail pytest at repo root.
from idempotency import ... only resolves if this directory is on sys.path. Under normal repository-root collection the module is crewai.tools.idempotency, so this import raises ModuleNotFoundError and the whole test file fails to collect.
🐛 Proposed fix
-from idempotency import IdempotencyGuard, idempotent
+from crewai.tools.idempotency import IdempotencyGuard, idempotent📝 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.
| from idempotency import IdempotencyGuard, idempotent | |
| from crewai.tools.idempotency import IdempotencyGuard, idempotent |
🤖 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/idempotency/test_idempotency.py` at line 6,
Update the import in the idempotency test to use the package-qualified module
path, importing IdempotencyGuard and idempotent from crewai.tools.idempotency so
pytest collection from the repository root succeeds.
1 participant
Fixes #5802 — Tool re-execution on task retry has no idempotency guard.
Root Cause
_check_tool_repeated_usage()intool_usage.py:728only checkslast_used_toolin memory. When task retries,tools_handleris reset → duplicate tool calls execute again → duplicate payments/emails/trades.Fix
Added
IdempotencyGuard— durable idempotency layer that persists across task retries, worker re-dispatch, and process restarts.Each tool call gets a stable hash (
tool_name + arguments). If hash exists in durable storage, returns cached result.Usage
Tests
8/8 passing — 100% duplicate detection, zero false positives.
Performance
Based on CCS Identity dimension.
Validation: https://gist.github.com/Correctover/d79fe2d2ff05e181023cbdd1c673bcc6