fix(actions): ip_filter validator crashes on dict responses#1950
fix(actions): ip_filter validator crashes on dict responses#1950vidigoat wants to merge 2 commits into
Conversation
Greptile SummaryThis PR fixes a crash in the
|
| Filename | Overview |
|---|---|
| nemoguardrails/actions/validation/base.py | Correct one-line fix switching dict iteration to .items(), with an added isinstance(value, str) guard to protect against non-string values; logic is sound and minimal. |
| tests/test_actions_validation.py | Two well-scoped regression tests added — one for the original crash scenario and one for mixed-type dict values; both cover the fixed code paths directly. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[validate_response wrapper invoked] --> B{ip_filter in validators?}
B -- No --> G[Run other validators and return]
B -- Yes --> C{response is str?}
C -- Yes --> D[filter_ip on whole string]
D --> G
C -- No --> E{response is dict?}
E -- No --> G
E -- Yes --> F[Iterate response.items]
F --> H{value is str?}
H -- No --> F
H -- Yes --> I[filter_ip on value]
I --> F
F -- exhausted --> G
Reviews (2): Last reviewed commit: "Guard non-string dict values in ip_filte..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| elif isinstance(response_value, dict): | ||
| for key, value in response_value: | ||
| for key, value in response_value.items(): | ||
| response_value[key] = filter_ip(value) |
There was a problem hiding this comment.
filter_ip applies a regex substitution to its argument typed as str. If any dict value is not a string (e.g., int, None, a nested dict, or a list), the call on line 103 raises a TypeError at runtime. Since the dict branch was previously unreachable due to the bug, this gap wasn't visible before — but it is now exercisable in production when LangChain tool actions return mixed-type dicts. A isinstance(value, str) guard before calling filter_ip(value) would make the branch robust.
Prompt To Fix With AI
This is a comment left during a code review.
Path: nemoguardrails/actions/validation/base.py
Line: 101-103
Comment:
**Non-string dict values will still crash**
`filter_ip` applies a regex substitution to its argument typed as `str`. If any dict value is not a string (e.g., `int`, `None`, a nested `dict`, or a `list`), the call on line 103 raises a `TypeError` at runtime. Since the dict branch was previously unreachable due to the bug, this gap wasn't visible before — but it is now exercisable in production when LangChain tool actions return mixed-type dicts. A `isinstance(value, str)` guard before calling `filter_ip(value)` would make the branch robust.
How can I resolve this? If you propose a fix, please make it concise.
📝 WalkthroughWalkthroughThe PR fixes a bug in the ChangesIP Filter Dictionary Iteration Fix
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🤖 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 `@nemoguardrails/actions/validation/base.py`:
- Around line 101-103: The loop that replaces dict values with
filter_ip(response_value[key]) can crash for non-string values; update the
branch that iterates response_value.items() to first check the value type: if
isinstance(value, str) call filter_ip(value), elif isinstance(value, dict)
iterate/recursively process that dict, elif isinstance(value, list/tuple)
iterate and process elements similarly, otherwise leave the value unchanged—use
the existing filter_ip for strings and recursion for nested structures so
response_value assignment only happens for safe types (reference response_value
and filter_ip to find the 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f266b0cb-b2e0-40f8-97f4-469de933badb
📒 Files selected for processing (2)
nemoguardrails/actions/validation/base.pytests/test_actions_validation.py
| elif isinstance(response_value, dict): | ||
| for key, value in response_value: | ||
| for key, value in response_value.items(): | ||
| response_value[key] = filter_ip(value) |
There was a problem hiding this comment.
Handle non-string dict values before applying filter_ip.
This still crashes when a dict value is not a string (e.g., int/bool/nested dict), because re.sub expects a string-like input. Guard value types before filtering.
Suggested fix
elif isinstance(response_value, dict):
for key, value in response_value.items():
- response_value[key] = filter_ip(value)
+ if isinstance(value, str):
+ response_value[key] = filter_ip(value)
+ else:
+ response_value[key] = value📝 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.
| elif isinstance(response_value, dict): | |
| for key, value in response_value: | |
| for key, value in response_value.items(): | |
| response_value[key] = filter_ip(value) | |
| elif isinstance(response_value, dict): | |
| for key, value in response_value.items(): | |
| if isinstance(value, str): | |
| response_value[key] = filter_ip(value) | |
| else: | |
| response_value[key] = value |
🤖 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 `@nemoguardrails/actions/validation/base.py` around lines 101 - 103, The loop
that replaces dict values with filter_ip(response_value[key]) can crash for
non-string values; update the branch that iterates response_value.items() to
first check the value type: if isinstance(value, str) call filter_ip(value),
elif isinstance(value, dict) iterate/recursively process that dict, elif
isinstance(value, list/tuple) iterate and process elements similarly, otherwise
leave the value unchanged—use the existing filter_ip for strings and recursion
for nested structures so response_value assignment only happens for safe types
(reference response_value and filter_ip to find the code).
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Thanks @greptile-apps and @coderabbitai — good catch. |
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✨ |
|
The fix looks correct and well-guarded. The Two minor observations for consideration:
Both are edge cases — the core fix is solid. Thanks for the quick follow-up on the Tip: You can customize Greptile's behavior for this repo with |
vidigoat
changed the title
validate_response(validators=["ip_filter"]) crashed with 'ValueError: too many values to unpack (expected 2)' whenever the decorated action returned a dict. The dict branch iterated 'for key, value in response_value' which iterates the dict's keys (str), unpacking each key into (key, value) and raising for any key longer than two characters. This validator is applied to the LangChain safetool actions in nemoguardrails/integrations/langchain/actions/safetools.py, so any of them returning a dict response hit the crash on valid input. The dict branch had no test coverage, which is why it shipped undetected. Use .items() to iterate key/value pairs. Adds a regression test. Signed-off-by: Vidit Patankar <vidit.patankar16@gmail.com>
Addresses review feedback (greptile P2, coderabbit): filter_ip runs re.sub on its argument, which raises TypeError on a non-str. A dict response with int/bool/nested-dict values would still crash after the .items() fix. Only filter string values; leave others untouched. Adds a mixed-value-dict regression test. Signed-off-by: Vidit Patankar <vidit.patankar16@gmail.com>
vidigoat
force-pushed
the
fix-ip-filter-dict-iteration
branch
from
2b7b931 to
8c86bde
Compare
|
Thanks @Pouyanpi! Per the merge-guidance bot, I've signed both commits and force-pushed — they now show as verified on GitHub, and the signing requirement is satisfied. The force-push dismissed your earlier approval, but the diff is unchanged (commit-signing only, no content changes). Could you re-approve when you get a moment? 🙏 |
2 participants
Problem
@validate_response(validators=["ip_filter"])crashes withValueError: too many values to unpack (expected 2)whenever the decorated action returns adict.nemoguardrails/actions/validation/base.py:Iterating a dict yields its keys (strings). Unpacking each key into
(key, value)raisesValueErrorfor any key longer than two characters. This validator is applied to the LangChain safetool actions innemoguardrails/integrations/langchain/actions/safetools.py, so any of them returning a dict response hits the crash on otherwise-valid input. Thedictbranch had no test coverage, so the bug shipped undetected.Fix
Verification
Added
test_ip_filter_on_dict_responsecovering a dict response containing an IP.ruff checkandruff format --checkpass on both changed files (matches the repo's pre-commit config, ruff v0.14.6).Summary by CodeRabbit