Repository navigation
fix: תיאור codekeeper_get_repo_file נחתך אצל הלקוח — פיצול לתיאורי פרמטרים + טסט תקרה - #3388
Conversation
…טליין ו-symbol= — לא הגיע לסוכן התיאור הגיע ל-2,482 תווים: פי 3.8 מהכלי השני בגודלו (652) ופי שמונה מהחציון (303). נמדד שהוא מגיע לסוכן חתוך באמצע המשפט על RST ועל symbol=, כלומר שני פיצ'רים עבדו ואף לקוח לא קרא עליהם — בדיוק מחלקת הכשל שהמשפטים האלה נכתבו כדי למנוע. **העברה ולא מחיקה.** כל משפט בתיאור נולד מבאג אמיתי ויש עליו טסט, ולכן הפירוט על השפות עבר ל-Field(description=...) של הפרמטר outline והפירוט על הסינון לזה של symbol — שניהם יושבים ב-inputSchema ואינם מתחרים על תקציב התיאור. סך שלושת השדות גדל (2,711 מול 2,482), כלומר אף לקח לא נמחק; תיאור הכלי ירד ל-1,125. **טסט תקרה על כל 29 הכלים**, כי זו מחלקת בעיה ולא מופע. הוא רץ על _tool_manager.list_tools() ולא על mcp.list_tools(): האחרון מסנן את כלי האדמין ומחזיר 22, ושבעת החסרים כוללים את codekeeper_get_repo_file עצמו — טסט על התצוגה המסוננת היה ירוק בלי לכסות את המקרה היחיד שהפיל אותנו. **page/per_page היה הפיצ'ר היחיד בתיאור בלי אף טסט**, ולכן נשמט בטיוטה הראשונה של הפיצול בלי שאף בדיקה שמה לב. הוחזר עם טסט: עמוד ראשון שנראה כמו כל המפה הוא כשל שקט, כי OUTLINE_PER_PAGE_DEFAULT הוא 100 ו-486 הסימבולים שנמדדו על הקובץ הצפוף בקורפוס הם חמישה עמודים. assertion הסדר (symbol="@media" לפני 500KB) איבדה משמעות — השניים אינם באותה מחרוזת והסכנה נמנעת מבנית — והוחלפה בטסט שאוכף את שרשרת הגילוי: שתיאור הכלי מפנה לפרמטרים, ושהם באמת נושאים את הפירוט. נוספו: בדיקת ה-escape ב-services.backup\_service (עד כה נבדק רק דרך symbol="backup_service", שעובר גם על טקסט שאיבד את הלוכסן), האיסור על "only the RST label targets" חל על שלושת השדות, וסריקת lines=[...] כוללת את תיאורי הפרמטרים כדי שהכיסוי לא ייפול בשקט. אומת מול הגרסאות המוצמדות (mcp==1.28.1, pydantic==2.12.3, posthog==7.45.3): התיאורים נשלחים במלואם ב-inputSchema עם ריצת בקרה שבלי Field השדה חוזר None; הזרקת context של PostHog אינה דורסת אותם; ארבע מוטציות מפילות את הטסטים המתאימים. מה שלא נמדד הוא מה שלקוח **מציג** מהם — ראו האזהרה ב-_TOOL_DESCRIPTION_MAX_CHARS. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TbDVRRR8NxB8FtttvLpc1M
|
ⓘ 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. |
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 5 days and 11 hours by commenting @sourcery-ai review. Upgrade to get a review now.
Reviewer's GuideThis PR prevents client truncation from hiding File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reachedNext included review available in 28 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)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughהכלי Changesתיאורי פרמטרים בכלי MCP
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~12 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The tool metadata change is ready to merge; no concrete current-head risk was identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 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 |
**הקונפליקטים היו סמיכות ולא חפיפה.** ב-mcp_server/server.py שני הצדדים הוסיפו בלוק חדש מיד אחרי _RANGE_DOC — אני _OUTLINE_PARAM_DOC ו- _SYMBOL_PARAM_DOC, ו-main את _build_query_doc — ואין ביניהם הגיון משותף, אז שתי התוספות נשארו. ב-docs/whats-new.rst שני ערכים תחת אותו תאריך, ושניהם נשארו. _RANGE_DOC לא נגע. **והממצא שגדול מהקונפליקט: טסט התקרה תפס כלי שני, יומיים אחרי שנכתב.** #3385 הוסיף query ל-codekeeper_get_file עם תיאור בן 1,376 תווים שצורף לתיאור הכלי, והביא אותו מ-349 ל-1,726 — מעל הסף. זה בדיוק מה שהופך את התקרה ממופע בודד למחלקת בעיה, ולכן התרופה זהה ולא חריגה: _QUERY_DOC עבר ל-Field(description=...) של query, ותיאור הכלי ירד ל-464 עם משפט שמפנה לפרמטר. מנגנון השתלת הקבועים ב-_build_query_doc נשמר כמו שהוא, ולכן test_the_tool_description_takes_its_ceilings_from_the_constants לא נגע. **השוואה שיטתית משפט-משפט מול הנוסח הישן, בשני הכלים**, העלתה שהפסוקית "symbol= filters on that full name" נשמטה בסבב הקודם. היא נושאת מידע — הסינון על השם המנוקד השלם ולא על החלק האחרון, ולכן symbol="method" מוצא גם Class.method — והוחזרה. אחרי התיקון: אפס מושגים אבודים בשני הכלים. **ומבחן מוטציה מצא assertion שלא הייתה מסוגלת ליפול:** הבדיקה שהכלי מפנה ל-query נוסחה כ-"query" in description, והיא עברה גם אחרי מחיקת ההפניה — כי התיאור נושא ממילא את הדוגמה query="...". הוחלפה בבדיקה על ההפניה לשם הפרמטר, והמוטציה נתפסת עכשיו. test_the_tool_schema_declares_the_new_parameters_compatibly דרש ש-query יהיה בעל אותה צורה בדיוק כמו file_name, ו-description שבר את ההשוואה. נמדד שההפרש היחיד הוא המפתח description: anyOf ו-default זהים בית-בית ו-required הוא None. כלומר תאימות הטיפוס והאופציונליות — שתי הטענות של הטסט — נשארות נאכפות, ו-description הוחרג באותו היגיון שבו title כבר מוחרג: תצוגה ולא חוזה. אימות: כל הסוויטות של ה-MCP ירוקות, ruff ו-flake8 נקיים על הקבצים שהשתנו, אף כלי אינו מעל 1,400 (הגבוה הוא get_repo_file ב-1,125), ושלוש מוטציות מפילות את הטסטים המתאימים — Field ריק של query, הסרת ההפניה, ותיאור מעל התקרה. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TbDVRRR8NxB8FtttvLpc1M
🧯 Dangerous deletes guard reportPolicy: see .cursorrules — dangerous deletions are blocked unless wrapped safely. Summary:
Flagged findings (file:line:snippet): Excluded matches (by path pattern) |
סבב 2 — יישוב קונפליקטים, ו-
|
| כלי | לפני | אחרי |
|---|---|---|
codekeeper_get_repo_file |
2,482 | 1,125 + outline 1,011 + symbol 591 |
codekeeper_get_file |
1,726 | 464 + query 1,376 |
אף כלי אינו מעל 1,400.
שני דברים שהאימות השיטתי מצא
(1) פסוקית שנשמטה בסבב הקודם. ההשוואה משפט-משפט מול הנוסח הישן — שנעשתה הפעם על שני הכלים — העלתה ש-symbol= filters on that **full name** נעלם. הוא נושא מידע: הסינון הוא על השם המנוקד השלם ולא על החלק האחרון שלו, ולכן symbol="method" מוצא גם Class.method. הוחזר. אחרי התיקון: אפס מושגים אבודים בשני הכלים.
(2) assertion שלא הייתה מסוגלת ליפול. הבדיקה שהכלי מפנה ל-query נוסחה כ-"query" in description — והמוטציה שמחקה את ההפניה עברה אותה בשקט, כי התיאור נושא ממילא את הדוגמה query="...". היא הוחלפה בבדיקה על ההפניה לשם הפרמטר, והמוטציה נתפסת עכשיו. זה בדיוק מה שמבחן מוטציה קיים בשבילו.
טסט של main שנדרש התאמה
test_the_tool_schema_declares_the_new_parameters_compatibly דרש ש-query יהיה בעל אותה צורה בדיוק כמו file_name, ו-description שבר את ההשוואה. נמדד שההפרש היחיד הוא המפתח description: anyOf (מחרוזת או null) ו-default זהים בית-בית, ו-required בסכימה הוא None. כלומר שתי הטענות של הטסט — תאימות הטיפוס והאופציונליות — נשארות נאכפות. description הוחרג באותו היגיון שבו title כבר מוחרג שם: תצוגה ולא חוזה, ולקוח קיים אינו נשבר ממנו. הנימוק והמדידה כתובים בדוקסטרינג.
אימות
כל סוויטות ה-MCP ירוקות (כולל test_mcp_file_query.py החדש), ruff ו-flake8 נקיים על הקבצים שהשתנו. שלוש מוטציות מפילות את הטסטים המתאימים: Field ריק של query, הסרת ההפניה, ותיאור מעל התקרה.
הסיכון הפתוח מהסבב הראשון עומד בעינו — מה שנמדד הוא שהשרת שולח את תיאורי הפרמטרים במלואם, לא מה שלקוח מציג מהם. האימות שחסר: לבקש מסוכן ב-Claude Code להדפיס מילולית מה הוא רואה עבור outline, symbol ו-query.
Generated by Claude Code
⏱️ Performance report(No performance test durations collected. Mark tests with |
📖 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! |
✨ תיאור קצר
התיאור של
codekeeper_get_repo_fileהגיע ל-2,482 תווים — פי 3.8 מהכלי השני בגודלו (652) ופי שמונה מהחציון (303) — ונמדד שהוא מגיע לסוכן חתוך באמצע המשפט על RST ועלsymbol=. כלומר שני פיצ'רים עבדו ואף לקוח לא קרא עליהם, בדיוק מחלקת הכשל שהמשפטים האלה נכתבו כדי למנוע.הפתרון הוא העברה ולא מחיקה: הפירוט על השפות עבר ל-
Field(description=...)של הפרמטרoutline, והפירוט על הסינון לזה שלsymbol. סך שלושת השדות גדל (2,711 מול 2,482) — אף לקח לא נמחק — ותיאור הכלי ירד ל-1,125.📦 שינויים עיקריים
פירוט:
mcp_server/server.py— שני קבועים חדשים,_OUTLINE_PARAM_DOCו-_SYMBOL_PARAM_DOC, מחוברים לפרמטרים דרךAnnotated[..., Field(description=...)]. תיאור הכלי מקוצר ל-1,125 ומפנה לשני הפרמטרים._RANGE_DOCלא נגעתי בו — הוא משותף ל-codekeeper_get_fileו-docs/mcp-server.rstמחייב שהשניים יישארו סימטריים.mcp_server/outline.py— הערת "מקור האמת" מעל_SCANNERSנוקבת עכשיו בתיאור הפרמטרoutlineולא בתיאור הכלי.tests/test_mcp_server_build.py— עשר assertions הועברו לשדה שבו הטקסט יושב עכשיו; שני טסטים חדשים; שלוש הרחבות כיסוי.docs/— הערה ב-mcp-server.rstשאומרת איפה הפירוט גר בקוד, וערך ב-whats-new.rst.page/per_page— פיצ'ר בלי שומר"and page/per_page walk long files"היה המשפט היחיד בתיאור שלא היה עליו אף טסט, ולכן הוא נשמט בטיוטה הראשונה של הפיצול בלי שאף בדיקה שמה לב. הוחזר עם assertion. זה לא שולי:OUTLINE_PER_PAGE_DEFAULTהוא 100, ולכן 486 הסימבולים שנמדדו על הקובץ הצפוף בקורפוס מתפרסים על חמישה עמודים — סוכן שלא יודע עלpageרואה עמוד אחד וחושב שראה הכול.assertion הסדר
assert description.index('symbol="@media"') < description.index("500KB")הגנה על כך שהפילטר לא ייקרא כמדיניות גודל. אחרי הפיצול השניים אינם באותה מחרוזת כלל — הסכנה נמנעת מבנית והטענה לא הייתה יכולה לרוץ. הוחלפה בטסט שאוכף את שרשרת הגילוי: שתיאור הכלי מפנה לפרמטרים, ושהם באמת נושאים את הפירוט. הכשל המקורי ("פיצ'ר שאף אחד לא קורא לו הוא פיצ'ר שאינו קיים") חוזר אחרת אחרי הפיצול — סוכן שקורא רק את תיאור הכלי לא יֵדע ש-symbol=קיים.שלוש הרחבות כיסוי
services.backup\_serviceנבדק עכשיו ישירות. עד כה הוא נבדק רק דרךsymbol="backup_service"— טענה שעוברת גם על טקסט שאיבד את הלוכסן, כלומר בדיוק מה שקורה בהעתקה דרך שכבת escaping. הדוגמה הייתה הופכת לשקר שקט."only the RST label targets"חל עכשיו על שלושת השדות ולא על אחד.lines=[...]כוללת את תיאורי הפרמטרים, אחרת דוגמה שתיכתב שם לא הייתה נבדקת.🧪 בדיקות
435 טסטים עוברים, אפס כשלים:
test_mcp_server_build·test_mcp_analytics_privacy·test_mcp_outline·test_mcp_docs_handlers·test_mcp_repo_handlers.ruffו-flake8נקיים.ארבע מוטציות — כל אחת מפילה את הטסט שאמור לתפוס אותה (הרצה ראשונה של אחת מהן החזירה
ERRORולאFAILED, כלומר ה-sedשבר תחביר במקום שהטסט יתפוס; חזרתי עליה נקי עם אימותast.parse):page/per_page←paginationtest_the_description_tells_the_agent_which_languages_have_a_mapFieldשלsymbol← מחרוזת ריקהtest_the_description_says_symbol_works_on_the_non_python_namesbackup\_servicetest_the_description_says_symbol_works_on_the_non_python_namestest_no_tool_description_exceeds_the_truncation_budgetאומת מול הגרסאות המוצמדות ב-
requirements/base.txt—mcp==1.28.1,pydantic==2.12.3,posthog==7.45.3. (התקנה ראשונה הביאהmcp 2.2.0; אימות מול גרסה שגויה היה חסר ערך, והוחלף.) דרךrequest_handlers[ListToolsRequest]עם ההזרקה האמיתית של PostHog פעילה:contextהוזרק,get_more_toolsנוסף, שני תיאורי הפרמטרים שרדו במלואם, והסכמה עוברת round-trip של JSON בית-בית — כולל{% block %},<script>,@mediaוה-escape.🧩 השפעות/סיכונים
_TOOL_DESCRIPTION_MAX_CHARSובהערה שמעל שני הקבועים, כדי שלא תיקרא כאן יותר ממה שנמדד.האימות שחסר: לבקש מסוכן ב-Claude Code להדפיס מילולית את מה שהוא רואה עבור
outlineו-symbol.תופעת לוואי מוצהרת:
$mcp_tool_descriptionנמצא ברשימת ההיתר ב-analytics.py:133, ולכן המאפיין שנשלח ל-PostHog יתקצר — תיאורי הפרמטרים לא ייכללו בו. אין assert על תוכנו.טסט התקרה — ממצא ששווה לשים לב אליו בריוויו: הוא רץ על
_tool_manager.list_tools()ולא עלmcp.list_tools().AdminAwareFastMCPמסנן את כלי האדמין כשאין request context, ומחזיר 22 כלים מול 29 — ושבעת החסרים כוללים אתcodekeeper_get_repo_fileעצמו. טסט על התצוגה המסוננת היה ירוק בלי לכסות את המקרה היחיד שהפיל אותנו.📝 סוג שינוי
✅ צ'קליסט
docs/mcp-server.rst| המשפט: "הסיומות הנתמכות הן.pyו-.pyiלפייתון…"סטיות מהתוכנית, לשקיפות: התוכנית ציינה ארבעה מקומות לעדכון ב-
docs/mcp-server.rstואתmcp_server/README.mdשורות 50 ו-84. בפועל בדקתי ומצאתי שאין שם אף טענה על מבנה התיאור — רק על התנהגות, וההתנהגות לא זזה. לכן הוספתי שם רק הערה אחת שאומרת איפה הפירוט גר בקוד, ולא נגעתי ב-README.🤖 Generated with Claude Code
https://claude.ai/code/session_01TbDVRRR8NxB8FtttvLpc1M
Generated by Claude Code
Summary by Sourcery
Keep MCP tool guidance discoverable by moving detailed file, outline, symbol, and query documentation into parameter descriptions while enforcing a safe tool-description length.
Bug Fixes:
Enhancements:
Documentation:
Tests: