Repository navigation
fix(mcp): מחרוזת שנראית כמו JSON מגיעה לכלי כמחרוזת — pre_parse_json מדלג על פרמטרי str (#3471) - #3485
Merged
Conversation
…מדלג על פרמטרי str (#3471) ה-SDK מריץ FuncMetadata.pre_parse_json לפני הוולידציה: כל מחרוזת שנשלחת לפרמטר שאינו מוצהר str בדיוק עוברת json.loads, ו-None, רשימה או מילון מחליפים אותה (נמדד על mcp 1.28.1). בפרמטר str | None המחרוזת "null" נהייתה "לא נשלח" — query="null" ו-section="null" החזירו את הקובץ המלא — ו-"[1, 2]" ו-'{"a": 1}' נדחו כ"לא מחרוזת". AdminAwareFastMCP.add_tool, שכבר עוטף כל כלי ב-_offload_to_thread, מחליף עכשיו את מטא-הדאטה של הארגומנטים ב-_RawStringMetadata: פרמטר שה-annotation שלו כולל str (לבד, או לצד None/int/float/bool) יוצא מהפענוח, וכל השאר נשאר אצל ה-SDK — lines ו-items ממשיכים לקבל JSON כמחרוזת (Claude Desktop). str לצד רשימה/מילון/tuple/set/מודל, Any, או צורה לא מוכרת — TypeError ברישום, והכלי מוסר. ההכרעה נגזרת מ-FieldInfo.annotation, אותו שדה ש-pre_parse_json קורא, ולא מרשימת שמות. tests/test_mcp_pre_parse_json.py עובר על כל הכלים הרשומים (הפרמטרים נגזרים מהסכימה המוצהרת), בקרה שלילית, קצה-לקצה עם גוף אמיתי, ריצה עם PostHog דלוק ב-session אמיתי, הכלל ברישום, והתפר של ה-SDK. על 5d7fdc2: 43 נופלות, 12 בקרות עוברות. 13 מוטציות על עותק — כולן נתפסות. תיעוד: שתי השורות על "null" נמחקו מ-mcp-server.rst, ההסבר על pre_parse_json בסעיף content_changed עודכן, ו-whats-new. ההערה ב-build_mcp שטענה ש-PostHog אינו נוגע בסכימה תוקנה. ארגומנט שאינו בסכימה — #3484. Closes #3471 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MEsSHNcaSFuF5dKwQkQcLC
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Sorry @amirbiron, you've used your own review budget of 250,000 diff characters for the last 7 days.
You can request another review in 15 hours and 9 minutes by commenting @sourcery-ai review. Upgrade to get a review now.
Contributor
|
Warning Review limit reachedNext included review available in 5 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Contributor
Reviewer's GuideThe PR fixes MCP argument corruption by selectively bypassing the SDK’s JSON pre-parser for parameters whose annotations accept strings, while preserving decoding for list/object-oriented parameters. It validates unsupported annotation shapes and metadata changes at registration, adds extensive end-to-end and mutation-oriented regression coverage, and updates related documentation. Sequence diagram for preserving raw MCP string argumentssequenceDiagram
participant Client
participant MCP as AdminAwareFastMCP
participant Tool as MCP Tool
participant SDK as FuncMetadata
Client->>MCP: call_tool(name, arguments)
MCP->>Tool: invoke tool
Tool->>SDK: call_fn_with_arg_validation(arguments)
SDK->>SDK: pre_parse_json(arguments)
Note over SDK: _RawStringMetadata removes raw string keys first
SDK-->>Tool: validate and call with original string values
Tool-->>Client: tool result
Flow diagram for MCP argument annotation decisionsflowchart TD
A[Tool registration] --> B[Read arg_model.model_fields]
B --> C{Annotation includes str?}
C -->|No| D[Keep SDK JSON pre-parse]
C -->|Yes| E{Only scalar alternatives?}
E -->|str, None, int, float, bool| F[Mark field and alias as raw string]
E -->|Container or model| G[Reject registration with TypeError]
E -->|Any, object, or unknown shape| G
F --> H[Replace fn_metadata with _RawStringMetadata]
D --> I[Register tool]
H --> I
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
🧯 Dangerous deletes guard reportPolicy: see .cursorrules — dangerous deletions are blocked unless wrapped safely. Summary:
Flagged findings (file:line:snippet): Excluded matches (by path pattern) |
Contributor
⏱️ Performance report(No performance test durations collected. Mark tests with |
Contributor
📖 Documentation PreviewThe documentation has been built successfully!
To view locally:
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (5 files)
Reviewed by nemotron-3-ultra-550b-a55b:free · Input: 0 · Output: 0 · Cached: 0 |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
amirbiron
pushed a commit
that referenced
this pull request
Sep 29, 2026
…הלקוח ממיר אותו לפני השליחה אחרי ש-#3485 עלה לייצור, query="null" ו-section="null" מהלקוחות האלה עדיין מחזירים את הקובץ המלא, וקובץ ששמו null אינו נמצא לפי file_name. השרת תקין: המחרוזת פשוט לא מגיעה אליו. בפרמטר שהסכימה שלו מתירה null, הלקוח ממיר את המילה null ל-null של JSON עוד לפני שהבקשה יוצאת. נמדד ב-29.09.2026 מול claude-code 2.1.284 ו-Anthropic/ClaudeAI 1.0.0: - Claude Code: ה-input של קריאת הכלי בתמליל הוא {"query": null}, {"section": null}, {"file_name": null}, {"content": null}. בפרמטר שהסכימה שלו string בלבד (create_note, save_file.description) המילה יוצאת כמחרוזת, ו-[1, 2] / {"a": 1} יוצאים כמחרוזות גם לפרמטר שמתיר null. "null" עם מירכאות יוצא עם המירכאות — אין דרך לשלוח את המילה לבדה. - claude.ai: ck_read_mode ב-PostHog נגזר מהארגומנטים כמו שהגיעו לשרת. כיול על Claude Code: {"query": null} תויג full, ומחרוזת תויגה query. שתי הקריאות של claude.ai ל-query="null" תויגו full. השורות על "null" שהוסרו ב-#3485 מ-mcp-file-sections ומטבלת "פתרון תקלות" חוזרות, עם הסיבה הנכונה ועם העקיפה: כותרת ששמה null נקראת ב-lines לפי ה-line_range מהמפה, וקובץ ששמו null לפי file_id — שתיהן נבדקו מול השרת. ורשומה ב-whats-new. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MEsSHNcaSFuF5dKwQkQcLC
12 of 28 tasks
amirbiron
added a commit
that referenced
this pull request
Sep 29, 2026
…הלקוח ממיר אותו לפני השליחה (#3488) אחרי ש-#3485 עלה לייצור, query="null" ו-section="null" מהלקוחות האלה עדיין מחזירים את הקובץ המלא, וקובץ ששמו null אינו נמצא לפי file_name. השרת תקין: המחרוזת פשוט לא מגיעה אליו. בפרמטר שהסכימה שלו מתירה null, הלקוח ממיר את המילה null ל-null של JSON עוד לפני שהבקשה יוצאת. נמדד ב-29.09.2026 מול claude-code 2.1.284 ו-Anthropic/ClaudeAI 1.0.0: - Claude Code: ה-input של קריאת הכלי בתמליל הוא {"query": null}, {"section": null}, {"file_name": null}, {"content": null}. בפרמטר שהסכימה שלו string בלבד (create_note, save_file.description) המילה יוצאת כמחרוזת, ו-[1, 2] / {"a": 1} יוצאים כמחרוזות גם לפרמטר שמתיר null. "null" עם מירכאות יוצא עם המירכאות — אין דרך לשלוח את המילה לבדה. - claude.ai: ck_read_mode ב-PostHog נגזר מהארגומנטים כמו שהגיעו לשרת. כיול על Claude Code: {"query": null} תויג full, ומחרוזת תויגה query. שתי הקריאות של claude.ai ל-query="null" תויגו full. השורות על "null" שהוסרו ב-#3485 מ-mcp-file-sections ומטבלת "פתרון תקלות" חוזרות, עם הסיבה הנכונה ועם העקיפה: כותרת ששמה null נקראת ב-lines לפי ה-line_range מהמפה, וקובץ ששמו null לפי file_id — שתיהן נבדקו מול השרת. ורשומה ב-whats-new. Claude-Session: https://claude.ai/code/session_01MEsSHNcaSFuF5dKwQkQcLC Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
✨ תיאור קצר
לפני הוולידציה ה-SDK של
mcpמריץFuncMetadata.pre_parse_json: כל מחרוזת שנשלחת לפרמטר שאינו מוצהרstrבדיוק עוברתjson.loads, ו-None, רשימה או מילון מחליפים אותה. בפרמטרstr | Noneזה היה באג שקט:"null"נהיה "לא נשלח", ולכןquery="null"ו-section="null"החזירו את הקובץ המלא. התיקון יושב בתפר שכבר קיים —AdminAwareFastMCP.add_tool— ומוציא מהפענוח כל פרמטר שה-annotation שלו כוללstr, כך שהמחרוזת מגיעה לכלי כמו שנשלחה.Closes #3471
📦 שינויים עיקריים
קוד (Backend)
בוט טלגרם
מסד נתונים/מיגרציות
תיעוד (docs/)
DevOps/CI/CD
mcp_server/server.py—AdminAwareFastMCP.add_toolמאתר את הכלי שה-SDK יצר זה עתה (השם שלא היה רשום לפני הקריאה — בלי לגזור מחדש את כלל השמות של ה-SDK), ומחליף אתtool.fn_metadataב-_RawStringMetadata: תת-מחלקה שלFuncMetadataשמוציאה את מפתחות המחרוזת לפני שה-SDK מפענח, ומעבירה את כל השאר אליו._offload_to_thread,_declares_writeו-tool.fnלא השתנו.הכלל (
_keeps_raw_string) נגזר מ-FieldInfo.annotation— אותו שדה ש-pre_parse_jsonקורא — ולא מרשימת שמות, ומכריע במפורש על כל צורה:str, לבד או לצדNone/int/float/bool← מדלגים על הפענוח.str← ה-SDK ממשיך כרגיל (lines,items— Claude Desktop שולח רשימות כמחרוזות JSON).strלצד רשימה/מילון/tuple/set/מודל,Any/object, או צורה שהכלל לא מכיר ←TypeErrorברישום, עם הסיבה, והכלי מוסר — כמו כלי כתיבה אסינכרוני.BaseModel, למשלjson).FuncMetadata←TypeError, כדי שמחלקה חדשה של ה-SDK לא תאבד בהעתקה את מה שהיא מוסיפה.ההערה ב-
build_mcpטענה ש-PostHog "no tool schema is touched" — בסתירה ל-instrument_mcp_server, שמזריקcontextלכל סכימה. תוקנה.docs/mcp-server.rst— נמחקו שתי השורות שתיעדו את"null"(במצבי הקצה שלmcp-file-sections, ובטבלת "פתרון תקלות"), והפסקה עלpre_parse_jsonבסעיףcontent_changedעודכנה: מעכשיו זה נכון לכל פרמטר שמקבל מחרוזת.docs/whats-new.rst— רשומה ל-2026-09-29.tests/test_mcp_content_sha256.py— רק ה-docstring: המוטציהcode: str | Noneהפילה שם שלוש בדיקות לפני התיקון, ועכשיו עוברת (נמדד בשני הכיוונים). הטסט עצמו לא השתנה.🧪 בדיקות
send=False)tests/test_mcp_pre_parse_json.py(55 בדיקות), כולן דרךcall_tool:"null","[1, 2]"ו-'{"a": 1}'כמו שנשלחו. הפרמטרים נגזרים מהסכימה שמוצהרת ללקוח (קוד של pydantic) ולא מהכלל שנבדק, ולכן הרשימה אינה טאוטולוגית. המספר 28 לא מקובע; נבדק רק שרשימת האישו מוכלת במה שכוסה.lines="[5, 10]"ב-get_fileוב-get_repo_file, ו-itemsכמחרוזת JSON ב-read_batch, עדיין מפוענחים.query="null"מחפש את המילה;"[1, 2]"/'{"a": 1}'הם חיפוש מילולי; כותרת ששמהnullנקראת בשמה; קובץ ששמוnullנמצא;docs_get_section(section="null")מחזיר סעיף ולא מפה;update_note(content="null")כותב את המילה — נבדק במצב שהגיע לשכבת הכתיבה, לא בתשובת הכלי.build_mcpאמיתי, לקוחPosthogאמיתי, וכל הקריאות ב-session אמיתי. נבדק ש-contextמופיע כחובה בכל סכימה (כלומר אלו הסכימות של הייצור), שהמחרוזות מגיעות כמו שנשלחו, ושה-contextהמוזרק לא מגיע לכלי.str | list[str](ועוד תשע צורות) מפיל את הרישום עם הסיבה הנכונה, והכלי אינו רשום אחרי הסירוב.requirements/base.txt= המותקנת; ה-SDK עדיין קורא ל-self.pre_parse_json(נספר); וה-SDK לבדו עדיין ממיר"null"ל-None(אם יתקנו upstream — הטסט ייפול, וזה הסימן לבדוק אם השכבה עדיין נחוצה).ריצת בקרה על הקוד שלפני התיקון (
5d7fdc2, ב-worktree נפרד): 43 נופלות, 12 עוברות. ה-12 הן בדיוק מה שאמור לעבור בשני המצבים: הבקרה השלילית,strבדיוק, שני שומרי ה-SDK, ובדיקת ההכלה של רשימת האישו. הנפילה בטסט הפרמטרי היא 28 שורות שלsent 'null', the tool received None— הבאג כלשונו.מוטציות (על עותק ב-
git worktree, כל אחת החלפה מדויקת אחת):list/dict/מודלstrלצד container לא מסורבAnyמאבד את הסירוב המפורש שלוadd_toolלא מחליף מטא-דאטהstr | Noneנחשב טקסטAnnotatedלא מקולףtyping.Unionלא מטופלstr | list[str] | Noneבכלי אמיתי (codekeeper_list_versions)TypeErrorנוקב בכלי ובפרמטרשאר הסוויטה (
-n auto --dist=loadscope, כמו ב-CI, Python 3.11):tests/test_infrastructure.py::TestEnvironmentTools, ובודקות ש-isort/autopep8ב-PATH. אומת שהן נופלות זהה על הבסיס, ועוברות כשה-venv ב-PATH — סביבתי, לא קשור.typingו-pydantic באופן שזהה בשתי הגרסאות, אבל לא הרצתי.🧪 בדיקות נדרשות ב‑PR
📝 סוג שינוי
✅ צ'קליסט
docs/whats-new.rst)docs/mcp-server.rst| המשפט: "הקולבק מקבל את מילון הארגומנטים הגולמי — לפני ש-pydantic מסלק ממנו מפתחות שאינם בסכימה של הכלי"עיון ב-CodeBot – Project Docs:
AI-MAP.md; ב-docs/mcp-server.rst— "מצבי הקצה" שלmcp-file-sections, "פתרון תקלות", "טביעת אצבע לתוכן" › "תשובות הכתיבה:content_changed", "מדידת שימוש" › "לכידת הכוונה (context)" ו"מצב הקריאה (ck_read_mode)";docs/doc-authoring.rst(לפני עריכת ה-rst — כולל "ספירות בפרוזה");docs/versioning-stable-anchors.rst(לא נמחק ולא שונה אף עוגן). מחוץ לריפו (Ck):Ck-MCP-Handoff.md— "מודל הריצה של הכלים" כולו, "מוסכמות" כולו, "הערך שהכלי מקבל אינו תמיד הערך שנשלח", "שער הפרטיות", "run_in_executorאינו מעתיקcontextvars";לקחי-RST-להרחבת-האאוטליין-ל-md.mdסעיפים 10 ו-13.🧩 השפעות/סיכונים
null" מגיעה לכלי כמחרוזת. סוכן ששלח"null"כדי לומר "לא נשלח" (אין סיבה לעשות זאת — משמיטים את הפרמטר, או שולחיםnullשל JSON) יקבל עכשיו חיפוש של המילה.pre_parse_jsonזהה גם ב-mcp1.30.0 וגם ב-2.2.0 (נקרא מהמקור, 29.09) — שדרוג לא מייתר את התיקון. ב-2.xmcp.server.fastmcpמוחלף במודול שזורקModuleNotFoundErrorבכוונה, כלומר שדרוג כזה ייפול בייבוא ולא יחזיר את הבאג בשקט.list_tools()לכל רישום (בעלייה בלבד), ובכל קריאת כלי — שני dict comprehension על הארגומנטים. גוף טקסט גדול (פתק, למשל) כבר לא עוברjson.loads.FastMCP(tools=[...])רושם כלים בלי לעבור ב-add_tool, כך שכלי שיירשם כך יעקוף גם את_offload_to_threadוגם את התיקון הזה. אף אחד לא משתמש בזה היום, ושני טסטים מבניים (test_every_registered_tool_is_a_coroutine_function,test_every_registered_tool_carries_the_string_preserving_metadata) תופסים את זה ב-build_mcp.🔗 קישורים
secton="K11") עדיין נזרק בשקט. נמדד כאן דרךcall_tool, עם PostHog ובלעדיו, ונפתח כאישו נפרד כי התיקון צריך להתחשב ב-contextשמוזרק רק בייצור.🧯 סיכון / החזרה לאחור (Rollback)
🤖 Generated with Claude Code
https://claude.ai/code/session_01MEsSHNcaSFuF5dKwQkQcLC
Generated by Claude Code
Summary by Sourcery
Preserve text arguments at the MCP boundary while retaining JSON parsing for collection-oriented parameters.
Bug Fixes:
Enhancements:
Documentation:
Tests: