Repository navigation
fix(mcp): ניקיון שאריות #3470 — toc כדגל רגיל, סירובים שנכנסים בתקציב, והפניה במקום העתק (#3472) - #3490
Merged
Conversation
…, והפניה במקום העתק (#3472) - WARN-002: toc חוזר ל-bool רגיל כמו outline; StrictBool נמחק, והנימוק כתוב ליד StrictInt ובמצבי הקצה ("true" מגיע לכלי כמחרוזת, ולכן strict על דגל רק חוסם). רשת isinstance ב-file_read_request_error נשארה. - ck_read_mode קורא דגל באותה ולידציה של הכלי (TypeAdapter(bool)): outline="false" תויג outline על קריאה מלאה — באג שקיים מלפני כן, ו-toc הצטרף אליו. null שנשלח לדגל נספר כקריאה שנדחתה. - SUGG-002: כל סירוב שנושא file עובר fit_refusal — אותו _fit_lists, שלמד לרדת רמה אחת (file.tags, עם file.tags_truncated). ב-section_not_found/ambiguous_section התגיות נחתכות ראשונות. תשובה מוצלחת אינה חותכת מטא-דאטה. השורש בכותבים: #3489. - SUGG-008: max בסירובי התקציב הוא OUTPUT_BYTE_BUDGET, ו-bytes כולל את המעטפת. ב-section_too_large ה-bytes הוא עכשיו מה שההחלטה מדדה (נמצא בסף: bytes<=max גם ב-main). - SUGG-001: section של רווחים ארוך מהתקרה הוא section_too_long, כמו בכלי התיעוד. - WARN-003 + YAGNI-002: toc ו-section נפתחים בהוראה להעביר file_id; section מפנה לכלל של codekeeper_docs_get_section במקום להעתיק אותו (tools/list: 40,626 ← 38,348 בתים). - SUGG-003/004/005/006/009/011: הערות ו-docstrings מדויקים, סדר אחד למצבי הקריאה (FILE_READ_MODES), _file_meta דרך _strip_heavy, וטסט ל-hint שחסר. Closes #3472 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JJoa5Kwh5Tfvtg5UJ7ECwm
|
ⓘ 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. |
![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.
![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 6 days and 9 hours by commenting @sourcery-ai review. Upgrade to get a review now.
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
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 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 (10)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughהשינויים מתאמים את ולידציית מצבי הקריאה ואת האנליטיקה, מרחיבים התאמת תשובות לתקציב בתים, ומעדכנים את הטיפול במטא־נתונים בסירובים. תיעוד הכלים והבדיקות מתעדכנים בהתאם. Changesקריאת סעיפים בקובץ שמור
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to No actionable issue remains; the change is mergeable after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The read and refusal changes warrant design review, but the examined paths do not show a new route to another user’s files. An existing limit remains important: unusually large file names or descriptions may still make a refusal exceed its advertised byte budget. Retained concerns Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
Contributor
Reviewer's GuideThis PR completes the MCP file-section cleanup by making Sequence diagram for budget-aware section refusalssequenceDiagram
participant Client
participant GetFile as codekeeper_get_file
participant Backend
participant Docs as docs_handlers.answer_section
participant Fitter as fit_refusal
participant Lists as _fit_lists
Client->>GetFile: Request toc or section
GetFile->>Backend: Read saved Markdown and metadata
Backend->>Docs: answer_section(..., refusal_cuts)
Docs->>Fitter: fit_refusal(refusal, refusal_cuts)
Fitter->>Lists: _fit_lists(..., floor=True)
Lists-->>Fitter: Refusal within budget with file.tags_truncated
Fitter-->>Docs: Budget-aware refusal
Docs-->>GetFile: Refusal with fitted metadata
GetFile-->>Client: Response within OUTPUT_BYTE_BUDGET
Flow diagram for normalized toc validation and analyticsflowchart TD
A[Raw toc argument] --> B{"TypeAdapter(bool)"}
B -->|True| C[Read heading map]
B -->|False| D[Read full file]
B -->|Rejected value| E[Validation refusal]
C --> F[ck_read_mode: outline]
D --> G[ck_read_mode: full]
E --> H[ck_read_mode: outline]
I[No toc key] --> J[Use other read modes]
J --> K[ck_read_mode from FILE_READ_MODES]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
⏱️ Performance report(No performance test durations collected. Mark tests with |
Contributor
📖 Documentation PreviewThe documentation has been built successfully!
To view locally:
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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.
✨ תיאור קצר
tocמקבל דגל כמחרוזת, כמוoutline; כל סירוב שנושא את המטא-דאטה של הקובץ נכנס בתקציב הבתים, בוויתור מסומן על התגיות; ותיאור הפרמטרsectionשלcodekeeper_get_fileמפנה לכלל ההתאמה שלcodekeeper_docs_get_sectionבמקום להעתיק אותו.📦 שינויים עיקריים
פירוט נקודות:
tocהואboolרגיל, כמוoutline, ו-StrictBoolנמחק."true","yes"ו-1נותנים את המפה;"false","no"ו-0את הקובץ המלא;"maybe",2,nullו-""עדיין נדחים בשגיאת ולידציה. למה לא strict: המחרוזת"true"מגיעה לכלי כמו שנשלחה —pre_parse_json(mcp 1.28.1) מפענח אותה ל-True, ו-Trueהואint, ולכן הוא זורק את הפענוח — כך ש-strict על דגל לא מונע שום קריאה שגויה, רק חוסם לקוח ששולח דגלים כמחרוזות (עד היוםtoc="true"נדחה בזמן ש-outline="true"עבד). ב-linesזה הפוך: שם strict מונע[true, 5]שנקרא כשורות 1 עד 5, ולכןStrictInt/StrictLinesנשארים. הנימוק כתוב לידStrictInt, במצבי הקצה שלmcp-file-sections, ובמוסכמה ב-Handoff — כדי ש-StrictBoolלא ייווצר שוב. רשתisinstanceב-file_read_request_errorנשארה, לקורא שעוקף את הסכימה.outline— ו-tocרק מצטרף אליו.ck_read_modeבדק אמת-שקר של פייתון, שבה מחרוזת לא ריקה היא אמת, ולכןoutline="false"— שהכלי קורא כקובץ מלא — תויגoutline. זה לא "תופעת לוואי" של WARN-002: הבאג קיים ב-main ב-codekeeper_get_repo_file, ו-tocהיה נכנס אליו ברגע שהפסיק להיות strict. עכשיו הקולבק קורא דגל באותה ולידציה של הכלי (TypeAdapter(bool)ברמת המודול; נקרא במקור של pydantic 2.12.3 / pydantic-core 2.41.4 שה-validator נבנה במלואו בבנאי ושאין מצב שמשתנה בקריאה — K15): מה שהכלי קוראtrue—outline;false— לפי שאר הפרמטרים; ומה שהכלי דוחה — כוללnullשנשלח —outline, כמו זוג מצבים שנדחה (הקריאה לא קראה תוכן). מפתח שלא נשלח אינו בקשה.fileנכנס בתקציב. עד היום תגית ענקית (אין תקרה על מספר התגיות ועל האורך שלהן) שלחה סירוב של כ-270KB, מעלOUTPUT_BYTE_BUDGET, בלי סימן. עכשיו כל סירוב של מצבי הסעיף יוצא דרךfit_refusal— אותו_fit_listsשחותך את המפה ואת ההצעות, לא פונקציה חדשה — ש-file.tagsנחתכות בו מהסוף, עםfile.tags_truncated: true. ב-section_not_foundוב-ambiguous_sectionהתגיות נחתכות ראשונות, לפני המפה, ההצעות והמועמדים. תשובה מוצלחת לעולם אינה חותכת מטא-דאטה. מחרוזות (תיאור, שם) אינן נחתכות — השורש בכותבים נפתח בנפרד: שדות מטא-דאטה של קובץ שחסומים אצל חלק מהכותבים בלבד — תגיות ותיאור (derived-field-added-to-one-writer) #3489._fit_listsלמד לרדת רמה אחת (("file", "tags")), כשעלות כל פריט נמדדת בעומק שבו הוא יושב (list_item_cost(item, depth=...),nonempty_list_bytes(depth)). כפי שנדרש באישור: טסט שמריץ את אותם קלטים דרך הצורה החדשה בעומק 0 ודרך עותק קפוא של_fit_listsמ-main, על כל תקציב בית-בית, ומשווה החלטה-החלטה — ושלוש מוטציות בחישוב העומק מפילות אותו.maxבסירובי התקציב הואOUTPUT_BYTE_BUDGETעצמו (256,000), ו-bytesכולל את המקום ל-found/status, כך ש-bytes > maxהוא בדיוק ההחלטה שנפלה. עד היוםmaxהיה 255,964 במפה ו-255,960 בסעיף — מספר שלא הופיע בשום תיעוד.sectionשל רווחים בלבד שארוך מ-4,096 תווים היהempty_sectionב-codekeeper_get_fileו-section_too_longב-codekeeper_docs_get_section— שני כלים שעונים באותה פונקציה, ושתי שגיאות לאותו קלט. עכשיו "קודם אורך, אחר כך תוכן" גם כאן:section_too_long, עם המטא-דאטה של הקובץ. רווחים קצרים —empty_sectionכמו קודם.file_id. שני התיאורים (tocו-section) נפתחים במשפט שקריאות המשך (סעיף, העמוד הבא,linesעלline_range) צריכות להעבירfile_id=file.id— ההוראה נגמרת בתו 103, בתוך 120 התווים שלקוח מקצר אליהם. לפיfile_nameכל קריאה קוראת את הגרסה האחרונה, וגרסה ששמר סשן אחר באמצע נותנת עמוד שנפתח באמצע מילה. גם בעמוד התיעוד.sectionב-get_fileשיבץ את_SECTION_PARAM_DOCכמו שהוא (כ-3,000 תווים בכלtools/list). עכשיו יש הפניה לפרמטרsectionשלcodekeeper_docs_get_section(השם נשתל מ-SECTION_TOOL_NAME), ונשאר רק מה שייחודי לקובץ שמור. התיאור ירד מ-5,254 ל-2,516 תווים, ו-tools/list(בלי כלי האדמין) מ-40,626 ל-38,348 בתים. טסט אוכף את שני הקצוות: ההפניה בשם, והכלי האח רשום באותו שרת עם הכלל. תיאור הכלי עצמו לא השתנה (1,317 תווים).ANSWER_TOO_LARGEאומרת מה הסירוב נושא (בליrequested); ה-docstring שלdocs_handlersמונה את שלושת הכלים שעונים דרכו; סדר מצבי הקריאה כתוב במקום אחד (FILE_READ_MODES) — הזוגות שנדחים והתווית באנליטיקס נגזרים ממנו;_file_metaמסיר שדות כבדים דרך_strip_heavyולא בעותק;LoadedDocumentמתעד את שתי צורות ההקשר; וטסט שה-hintאינו ב-section_not_found/ambiguous_section.section_too_largeדיווחbytesשקטן מ-maxבסף ההחלטה.🧪 בדיקות
Unit
Integration
Manual
טסט לכל תיקון, וכל אחד עם מוטציה: 26 מוטציות, כולן נופלות (הרצה על עותק ב-
git worktreeנפרד, לא בעץ העבודה). הרשימה כוללת: עומק ב-_fit_lists(3), עלות לפי עומק (2),bytesבלי המעטפת (2), מדידתsection_too_large,maxהישן,_FILE_REFUSAL_CUTS = (),not_markdownבליrefuse, תגיות אחרי המפה, תשובה מוצלחת שחותכת תגיות,fit_refusalבליfloor,tocחזרה ל-strict, אנליטיקס לפי אמת-שקר,nullכ"לא נשלח", עותק של הסדר באנליטיקס, זוגות בסדר אחר, SUGG-001/006/011,file_idבסוף התיאור, ושלוש מוטציות של ההפניה (שיבוץ מחדש, בלי שם הכלי, העתקת משפט התקרה).T2 — הטסטים החדשים על main: כל טסט של תיקון נופל שם. שלושה עוברים על main בכוונה, וזה כתוב ב-docstring שלהם: האורקל של עומק 0 (שקילות), ה-
hintשחסר (SUGG-011 מקבע החלטה קיימת), ותשובה מוצלחת שאינה חותכת מטא-דאטה (גבול התיקון).bytes > maxבכל סירוב, כפי שנדרש: ארבעת הסירובים של התקציב, כל אחד גם בסף ההחלטה —bytes == budget + 1, ותו אחד פחות בתיאור כבר אין סירוב. העוגן הזה הוא מה שמפיל את המוטציה "bytesבלי המעטפת" (בלעדיו הטסט היה מעגלי — הוא נכתב כך בהתחלה, והמוטציה שרדה).כל טסטי ה-MCP:
tests/test_mcp_*.py— 1,667 עוברים, 36 מדולגים (כולם "דורש מונגו אמיתי", קיימים ב-main). ועוד שלוש ריצות של טסטים מחוץ ל-MCP שקוראים מ-docs/או מייבאים את המודולים ששונו (862, 800 ו-189 עוברים, עם חפיפה ביניהן) — אין נפילה. flake8 (הבדיקה החוסמת ב-CI) נקי; mypy — אין שגיאה חדשה מול main; ruff — אין ממצא חדש מול main.אימות מול המציאות (לקוח MCP אמיתי):
ClientSessionמעל JSON-RPC בזיכרון, מולbuild_mcpעםProductionBackendועם האינסטרומנטציה האמיתית של PostHog (send=False). נמדד מה שהלקוח מקבל ומה ש-PostHog היה שולח:toc="true"← מפה,"false"← קובץ מלא,null/"maybe"← שגיאת ולידציה;ck_read_modeשלtoc="false"←full(ב-main:outline); סירובים עם תגית ענקית — 442 עד 1,628 בתים כפי שהתקבלו (ב-main: ~270,500);codekeeper_docs_get_sectionמופיע באותוtools/listשהלקוח (לא-אדמין) מקבל, כלומר ההפניה מגיעה לכלי שהוא רואה.אפס-דיף:
scripts/docs_section_zero_diff.py, main מול הענף, אותו קורפוס לשתי ההרצות: docs — 6,138 רשומות, זהות בית-בית; amir-bug-patterns (1be1b03) — 5,523 רשומות, זהות בית-בית (יציאה 1 בשני העצים — הצפוי לפי הדוקסטרינג של הסקריפט, כי בקורפוס הזה יש כותרות עם מזהים).codekeeper_get_fileבמצביtoc/section, סוללה מקבילה של 73 מקרים (הקלטים של הטסטים של feat(mcp): קריאה לפי סעיף בקובץ Markdown שמור — toc ו-section ב-codekeeper_get_file (PR ג) #3470, ועוד הקלטים של הדיפים הצפויים): 56 זהים בית-בית, והשאר — בטבלה הבאה.הדיפים — ארבעה נכתבו מראש, לפני שהסוללה רצה (לפי תוספת א' באישור), ומה שיצא בפועל:
empty_section←section_too_longmax= 256,000;bytesגדל ב-36 (מפה) / 40 (סעיף)answer_too_large— בדיוק כך.section_too_large—maxכך, אבלbytesגדל ב-42 / 45 ולא ב-40 → דיף חמישי, ממצא (למטה)"true"/"yes"/1← המפה,"false"/"no"/0← הקובץ המלא (ב-main: שגיאה)"maybe"ו-2— זהים (שגיאה בשני העצים)file.tags: []ו-tags_truncated, "וכל השאר זהה"not_markdown— בדיוק כך. ב-section_not_foundוב-ambiguous_section— לא "כל השאר זהה": ב-main התשובה הייתהanswer_too_large; בענף היאsection_not_foundעם המפה וההצעות /ambiguous_sectionעם המועמדים, והתגיות ריקות ומסומנות (ראו "סטיות")🧭 סטיות מהתוכנית, ומה שנמצא בדרך
section_too_largeמדד אתbytesעל צורה אחרת מזו שההחלטה מדדה. התגלה בטסט שלbytes > maxשנדרש באישור, כשהעוגן בסף הוסף: בסף ההחלטה הסירוב אמרbytesקטן מ-max— ב-main 255,957 מול 255,960, ובענף (לפני התיקון) 255,997 מול 256,000. השורש:_fit_pageמחליט על העמוד הריק כש-next_offset/remaining_charsבערכם המקסימלי ועוד התו הראשון, אבל החזיר את גודל העמוד אחרי שהשדות קיבלו ערך סופי (next_offset: 0) ובלי התו. תוקן במקור:bytesהוא עכשיו בדיוק מה שהושווה לתקציב — "כמה היה צריך העמוד הקטן ביותר שנושא תוכן". ההחלטה עצמה לא השתנתה (אותו סף בדיוק, נמדד), רק המספר שמדווח; חל על שלושת הכלים שעונים על סעיף. בסף — כל ארבעת הסירובים אומרים עכשיוbytes = 256,001.section_not_found/ambiguous_section. זו התוצאה של ההחלטה המאושרת ("תגיות נחתכות ראשונות"): כשהתגיות הן מה שלא נכנס, הוויתור עליהן מחזיר את התשובה האמיתית לשאלה במקום סירוב כללי. זה נכון, אבל לא ניסחתי את זה בטקסט שנכתב מראש — ולכן זה מופיע כאן כסטייה, ולא כהסבר בדיעבד.nullשנשלח לדגל באנליטיקס. התוכנית אמרה "לקרוא דגל כמו pydantic". בזמן המימוש התברר שהקולבק כן רואה את ההבדל בין מפתח חסר ל-nullשנשלח (build_tool_call_requestב-posthog 7.45.3 מעביר את הארגומנטים הגולמיים), והכלי דוחהnullבדגל. לכןnullבדגל מתויג כקריאה שנדחתה (outline) — וב-main הוא תויגfull.nullב-query/section/linesהוא ברירת המחדל שלהם, ונשאר "לא נשלח".StrictInt("strict על טיפוסי הפרמטרים החדשים, ולא רק int/list[int]") נוסחה מחדש: אחרי ההחלטה היא סתרה את ההערה שמתחתיה._fit_listsדגל*_truncatedיכול לעלות יותר מהפריט שהוא מחליף, ולכן ייתכןanswer_too_largeבזמן שחלק מהרשימה בלי דגל היה נכנס. שולי — עשרות בתים מהתקציב — ומחוץ להיקף.📝 סוג שינוי
_fit_lists— אפס-דיף בעומק 0)✅ צ'קליסט
docs/mcp-server.rst(mcp-file-sections,mcp-docs-section,mcp-read-mode, פתרון תקלות) ו-docs/whats-new.rst(ארבע רשומות תחת 2026-09-29). בלי עמוד חדש ובליtoctree, ולכן בלי בנייה מקומית — RTD יבדוקdocs/whats-new.rstdocs/doc-authoring.rst(וגםdocs/versioning-stable-anchors.rst, ו-docs/mcp-server.rstבכל הסעיפים שהשינוי נוגע בהם) | המשפט: "אם המספר יכול להשתנות בלי שאף שורת קוד תשתנה — הוא ספירת מופעים, ואין לכתוב אותו." (בגללו נמחקה ספירה של דגלים מהערה)מסמכים נוספים שעיינתי בהם: ב-Ck —
Ck-MCP-Handoff.md(בפרט "הערך שהכלי מקבל אינו תמיד הערך שנשלח" ו"דוקסטרינג אומר גם מה לא נעשה"),ידע-משיחת-התכנון-MCP.mdסעיף 15, ו-Markdown-Syntax.mdלפני העריכה ב-Ck. ב-amir-bug-patterns, לפי הטריגרים:state-record-without-state-change,derived-field-added-to-one-writer,external-input-isinstance+ U3, U1,BY-STACK/external-sdk.md, H6,line-number-coupling,TESTING-PATTERNS.md+claude-md-snippets/testing.md,widened-exception-scope,silent-truncation-at-sink,prose-restates-code-fact, R6, K11,silent-fallback-to-worse-path, K15 +lazy-init-guard-publish-order.🧩 השפעות/סיכונים
tocכמחרוזת או כמספר עובד (עד היום שגיאה); סירובים עם תגיות ענקיות קטנים ונכנסים בתקציב, עם דגל;max/bytesבסירובי התקציב;sectionשל רווחים מעל 4,096 תווים; תיאורי הפרמטרים (ההוראה עלfile_id, וההפניה ל-codekeeper_docs_get_section). כל תשובה אחרת — זהה בית-בית (אפס-דיף למעלה).ck_read_modeמשתנה רק לדגלים שהכלי קורא אחרת מפייתון ("false","no","0"...) ול-nullשנשלח לדגל. לפי ה-rst, הדשבורדck_mcp_navigation_cost_v2סופר רק אתcodekeeper_get_repo_file, כך שהעמודה שלoutline_readsשם עשויה לרדת מעט — כי היא כבר לא תספור קריאות מלאות.get_file— מתי לחזור לשיבוץ כתוב בהערה ב-server.py.🔗 קישורים
docs/mcp-server.rst—mcp-file-sections,mcp-read-mode🧯 סיכון / החזרה לאחור (Rollback)
git revertלקומיט אחד. אין מיגרציה, אין שינוי במסד ואין משתני סביבה. ההחזרה מחזירה אתStrictBool(ו-toc="true"שוב נדחה), את הסירובים הגדולים עם תגיות ענקיות, ואת התווית השגויה שלoutline="false".🤖 Generated with Claude Code
https://claude.ai/code/session_01JJoa5Kwh5Tfvtg5UJ7ECwm