fix(langchain): coerce None in format_store_content - #2561
Conversation
Memory/tool callers may pass None nl/sql before validation. Bare sql.strip() AttributeError aborted the store envelope.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
ChangesStored content formatting
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
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 |
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/_format.py`:
- Around line 120-125: Update the format_store_content signature so nl and sql
are annotated as str | None, matching the existing None-handling behavior in
nl_text and sql_text; leave tags and the function implementation unchanged.
In `@sdk/wren-langchain/tests/unit/test_format_store_content_none.py`:
- Around line 5-19: Update the _PATH resolution block in
test_format_store_content_none.py so the primary path is assigned once and the
fallback remains conditional on !_PATH.exists(). Remove the duplicate
unconditional _PATH assignment and retain a single working resolution flow for
both layouts.
- Around line 25-33: Strengthen test_none_sql and test_none_nl_and_tags to
assert the complete normalized string returned by format_store_content,
verifying that None inputs are rendered as empty strings rather than the literal
"None". Preserve the existing input combinations and use the exact expected
output for each case.
🪄 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
Run ID: 624db374-ba08-43da-96a5-1503f807c04b
📒 Files selected for processing (2)
sdk/wren-langchain/src/wren_langchain/_format.pysdk/wren-langchain/tests/unit/test_format_store_content_none.py
| def format_store_content(nl: str, sql: str, tags: list[str] | None) -> str: | ||
| """One-liner ``Stored: "<nl>" → <sql preview> (N tags)``.""" | ||
| sql_preview = sql.strip().split("\n")[0] | ||
| # Memory/tool callers may pass None for sql/nl before validation; bare | ||
| # ``sql.strip()`` raised AttributeError and aborted the store envelope. | ||
| nl_text = "" if nl is None else str(nl) | ||
| sql_text = "" if sql is None else str(sql) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the type annotations with the new None behavior.
nl and sql now intentionally accept None, but their annotations still declare str. Update the signature so type checkers and callers see the supported contract.
Proposed fix
-def format_store_content(nl: str, sql: str, tags: list[str] | None) -> str:
+def format_store_content(
+ nl: str | None, sql: str | None, tags: list[str] | None
+) -> str:📝 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 format_store_content(nl: str, sql: str, tags: list[str] | None) -> str: | |
| """One-liner ``Stored: "<nl>" → <sql preview> (N tags)``.""" | |
| sql_preview = sql.strip().split("\n")[0] | |
| # Memory/tool callers may pass None for sql/nl before validation; bare | |
| # ``sql.strip()`` raised AttributeError and aborted the store envelope. | |
| nl_text = "" if nl is None else str(nl) | |
| sql_text = "" if sql is None else str(sql) | |
| def format_store_content( | |
| nl: str | None, sql: str | None, tags: list[str] | None | |
| ) -> str: | |
| """One-liner ``Stored: "<nl>" → <sql preview> (N tags)``.""" | |
| # Memory/tool callers may pass None for sql/nl before validation; bare | |
| # ``sql.strip()`` raised AttributeError and aborted the store envelope. | |
| nl_text = "" if nl is None else str(nl) | |
| sql_text = "" if sql is None else str(sql) |
🤖 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/src/wren_langchain/_format.py` around lines 120 - 125,
Update the format_store_content signature so nl and sql are annotated as str |
None, matching the existing None-handling behavior in nl_text and sql_text;
leave tags and the function implementation unchanged.
| def test_none_sql(): | ||
| out = _mod.format_store_content("q", None, None) | ||
| assert "Stored:" in out | ||
| assert "q" in out | ||
|
|
||
|
|
||
| def test_none_nl_and_tags(): | ||
| out = _mod.format_store_content(None, "SELECT 1", None) | ||
| assert "SELECT 1" in out |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the normalized output, not only that formatting succeeds.
These tests would still pass if None were rendered as the literal string "None". Assert the complete output to lock in the intended empty-string behavior.
Proposed test assertions
def test_none_sql():
out = _mod.format_store_content("q", None, None)
- assert "Stored:" in out
- assert "q" in out
+ assert out == 'Stored: "q" → (0 tags)'
def test_none_nl_and_tags():
out = _mod.format_store_content(None, "SELECT 1", None)
- assert "SELECT 1" in out
+ assert out == 'Stored: "" → SELECT 1 (0 tags)'📝 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 test_none_sql(): | |
| out = _mod.format_store_content("q", None, None) | |
| assert "Stored:" in out | |
| assert "q" in out | |
| def test_none_nl_and_tags(): | |
| out = _mod.format_store_content(None, "SELECT 1", None) | |
| assert "SELECT 1" in out | |
| def test_none_sql(): | |
| out = _mod.format_store_content("q", None, None) | |
| assert out == 'Stored: "q" → (0 tags)' | |
| def test_none_nl_and_tags(): | |
| out = _mod.format_store_content(None, "SELECT 1", None) | |
| assert out == 'Stored: "" → SELECT 1 (0 tags)' |
🤖 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_store_content_none.py` around lines
25 - 33, Strengthen test_none_sql and test_none_nl_and_tags to assert the
complete normalized string returned by format_store_content, verifying that None
inputs are rendered as empty strings rather than the literal "None". Preserve
the existing input combinations and use the exact expected output for each case.
CI lint fails on ruff format --check for the new unit test.
|
Closing as superseded — None coercion in |
1 participant
Summary
Coerce
Nonenl/sql informat_store_contentso store envelopes do not AttributeError on.strip().Motivation
Tool callers can emit store confirmations before fields are fully populated.
Real behavior proof
License
Touches
sdk/wren-langchain/**(Apache-2.0).Summary by CodeRabbit
Bug Fixes
Tests