fix(sdk): coerce non-str tool descriptions in system prompts#2569
fix(sdk): coerce non-str tool descriptions in system prompts#2569Bartok9 wants to merge 5 commits into
Conversation
_build_tools_section used (tool.description or "").strip(), which raises when description is a non-empty non-str (e.g. int mock). Coerce safely in both langchain and pydantic SDKs.
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughBoth SDKs now defensively normalize tool names and descriptions when generating prompts and instructions. Tests cover missing or invalid names, non-string descriptions, and multiline descriptions. ChangesTool metadata rendering
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
sdk/wren-langchain/tests/unit/test_prompt_tool_description.py (1)
27-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for all promised metadata fallbacks.
sdk/wren-langchain/tests/unit/test_prompt_tool_description.py#L27-L38: test absent/invalid names, missing descriptions, and the public prompt-building path.sdk/wren-pydantic/tests/unit/test_instructions_tool_description.py#L30-L38: also add multiline-description coverage and test the public instructions-building 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 `@sdk/wren-langchain/tests/unit/test_prompt_tool_description.py` around lines 27 - 38, Add regression tests for all metadata fallbacks in sdk/wren-langchain/tests/unit/test_prompt_tool_description.py lines 27-38, covering absent or invalid names, missing descriptions, and the public prompt-building path around the existing test_build_tools_section_non_str_description test. Also update sdk/wren-pydantic/tests/unit/test_instructions_tool_description.py lines 30-38 to cover multiline descriptions and the public instructions-building path, while retaining the existing fallback assertions.
🤖 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 `@sdk/wren-langchain/src/wren_langchain/_prompt.py`:
- Around line 212-219: Centralize validated tool-name extraction in the
LangChain prompt builder and reuse that normalized name both for workflow
detection and bullet rendering, rather than accessing tool.name directly. Apply
the same change in sdk/wren-langchain/src/wren_langchain/_prompt.py lines
212-219 and sdk/wren-pydantic/src/wren_pydantic/_instructions.py lines 231-238,
ensuring each build_system_prompt/build_instructions path constructs tool_names
from the normalized fallback-aware name.
---
Nitpick comments:
In `@sdk/wren-langchain/tests/unit/test_prompt_tool_description.py`:
- Around line 27-38: Add regression tests for all metadata fallbacks in
sdk/wren-langchain/tests/unit/test_prompt_tool_description.py lines 27-38,
covering absent or invalid names, missing descriptions, and the public
prompt-building path around the existing
test_build_tools_section_non_str_description test. Also update
sdk/wren-pydantic/tests/unit/test_instructions_tool_description.py lines 30-38
to cover multiline descriptions and the public instructions-building path, while
retaining the existing fallback assertions.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 915419b1-3e9f-47f0-bf48-31c36904a303
📒 Files selected for processing (4)
sdk/wren-langchain/src/wren_langchain/_prompt.pysdk/wren-langchain/tests/unit/test_prompt_tool_description.pysdk/wren-pydantic/src/wren_pydantic/_instructions.pysdk/wren-pydantic/tests/unit/test_instructions_tool_description.py
…ack tests Extract _tool_name() helper in both SDK prompt builders and reuse it for workflow detection (tool_names set) and bullet rendering, so a missing or non-str tool.name is handled consistently everywhere. Add regression tests covering None/empty/numeric names and __name__ fallback.
|
Thanks @coderabbitai — good call. Addressed in the latest push (76bb050):
Ruff clean and unit tests green locally. |
|
Tip For best results, initiate chat on the files or code changes.
🐇✨ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
sdk/wren-langchain/tests/unit/test_prompt_tool_description.py (1)
26-37: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that
Nonedescriptions render as empty.The current assertion also passes if the output contains
`wren_query`: None. Assert the exact rendered line so the coercion behavior is actually covered.🧪 Proposed test adjustment
- assert "`wren_query`:" in out + assert "- `wren_query`: " in out.splitlines()🤖 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 `@sdk/wren-langchain/tests/unit/test_prompt_tool_description.py` around lines 26 - 37, Update test_build_tools_section_non_str_description to assert the exact rendered wren_query line, requiring an empty description after the colon rather than merely checking the tool name. Keep the existing assertions for numeric coercion and newline trimming unchanged.
🤖 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 `@sdk/wren-langchain/src/wren_langchain/_prompt.py`:
- Around line 207-213: Update _tool_name so the __name__ fallback is validated
with the same non-empty-string requirement as name; return "tool" whenever
__name__ is missing, empty, or not a string, ensuring the function always
returns a valid tool name.
---
Nitpick comments:
In `@sdk/wren-langchain/tests/unit/test_prompt_tool_description.py`:
- Around line 26-37: Update test_build_tools_section_non_str_description to
assert the exact rendered wren_query line, requiring an empty description after
the colon rather than merely checking the tool name. Keep the existing
assertions for numeric coercion and newline trimming unchanged.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b5b65888-2c8e-44fa-b3a0-69a9d2c6cd2f
📒 Files selected for processing (4)
sdk/wren-langchain/src/wren_langchain/_prompt.pysdk/wren-langchain/tests/unit/test_prompt_tool_description.pysdk/wren-pydantic/src/wren_pydantic/_instructions.pysdk/wren-pydantic/tests/unit/test_instructions_tool_description.py
🚧 Files skipped from review as they are similar to previous changes (2)
- sdk/wren-pydantic/tests/unit/test_instructions_tool_description.py
- sdk/wren-pydantic/src/wren_pydantic/_instructions.py
Address CodeRabbit review on Canner#2569: - _tool_name now applies the same non-empty-str validation to the __name__ fallback in both langchain and pydantic SDKs, returning "tool" whenever __name__ is missing, empty, or non-str. - Tighten test_build_tools_section_non_str_description to assert the exact rendered line for a None description (empty after colon).
|
Thanks @coderabbitai — both addressed in eb22875:
Ruff clean, unit tests green in both SDKs. |
|
Tip For best results, initiate chat on the files or code changes.
🐇✨ |
Summary
_build_tools_sectionin langchain + pydantic assumedtool.descriptionis 'str|None'.AttributeErroron.strip().License
sdk/**Apache-2.0.Test plan
test_prompt_tool_description.py(1 passed)test_instructions_tool_description.py(1 passed)Summary by CodeRabbit
Bug Fixes
Tests
name, using__name__) and for non-string/multilinedescriptionhandling.