fix(langchain): coerce non-str nl/sql in format_recall_content - #2588
fix(langchain): coerce non-str nl/sql in format_recall_content#2588Bartok9 wants to merge 2 commits into
Conversation
|
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 (1)
WalkthroughA unit test was added to verify ChangesWren LangChain updates
Estimated code review effort: 1 (Trivial) | ~2 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
🤖 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/tests/unit/test_format_malformed.py`:
- Around line 126-128: Update test_format_recall_coerces_non_str_nl_sql to
assert the complete formatted output, including the SQL section and its empty
code fence when sql is None. Use a non-string SQL input as well if the test is
intended to verify SQL coercion, and replace the partial substring assertion
with an exact expected-output assertion.
🪄 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: 9995ca5f-f301-47da-874a-2da53ea146f9
📒 Files selected for processing (1)
sdk/wren-langchain/tests/unit/test_format_malformed.py
| def test_format_recall_coerces_non_str_nl_sql() -> None: | ||
| out = format_recall_content([{"nl": 123, "sql": None}]) | ||
| assert '1. "123"' in out |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the SQL output too.
Line [128] verifies only the NL value, so the test would pass if the SQL fence were omitted or rendered incorrectly. Assert the complete output, including the empty SQL fence required for sql=None; use a non-string SQL value too if coercion is part of this test’s contract.
🤖 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_format_malformed.py` around lines 126 -
128, Update test_format_recall_coerces_non_str_nl_sql to assert the complete
formatted output, including the SQL section and its empty code fence when sql is
None. Use a non-string SQL input as well if the test is intended to verify SQL
coercion, and replace the partial substring assertion with an exact
expected-output assertion.
|
Closing this one — there's no production change in it. The diff touches only nl = row.get("nl_query") or row.get("nl") or ""
sql = row.get("sql_query") or row.get("sql") or ""so If you'd like the malformed-input case covered, a test-only PR adding it to |
|
You're right — |
Summary
nl/sqlvalues informat_recall_contentso recall formatting never embedsTypeErrorrisk when memory rows hold non-strings.Verification
format_recall_content([{"nl": 123, "sql": None}])→1. "123"with empty SQL fence.License
sdk/**Apache-2.0.Summary by CodeRabbit
Bug Fixes
Tests
Documentation