diff --git a/FEATURE_SUGGESTIONS/FEATURE_MCP_CLAUDE_INTEGRATION.md b/FEATURE_SUGGESTIONS/FEATURE_MCP_CLAUDE_INTEGRATION.md index 4909f9278..69bb0ab18 100644 --- a/FEATURE_SUGGESTIONS/FEATURE_MCP_CLAUDE_INTEGRATION.md +++ b/FEATURE_SUGGESTIONS/FEATURE_MCP_CLAUDE_INTEGRATION.md @@ -1,6 +1,10 @@ -# חיבור Claude.ai ל‑CodeKeeper דרך MCP — מסמך תכנון +# חיבור Claude.ai ל‑CodeKeeper דרך MCP — תכנון ולקחי מימוש -> **סטטוס:** פאזות 0 (PAT) **+** 1 (OAuth 2.1 ל‑Claude.ai) **+** 3 (כתיבה: save + edit/append חלקיים) **+** ד' (דפדפן ריפו לאדמין, קריאה בלבד) **+** פתקים דביקים (list/create/update) **מומשו** — ראו `mcp_server/`. מחיקה (קבצים ופתקים) וכתיבת אוספים עדיין בתכנון. +> **איך לקרוא את המסמך:** סעיפים 1–13 הם התכנון המקורי (נשמרו כפי שהיו, עם סימוני +> סטטוס). **סעיף 14 הוא מה שגילינו אחרי שהתחלנו לכתוב קוד** — אם אתם מממשים שרת MCP +> בפרויקט אחר, התחילו משם: כל פריט שם עלה בסבב תיקון אמיתי. +> +> **סטטוס:** פאזות 0 (PAT) **+** 1 (OAuth 2.1 ל‑Claude.ai) **+** 3 (כתיבה: save + edit/append חלקיים) **+** ד' (דפדפן ריפו לאדמין, קריאה בלבד) **+** פתקים דביקים (list/create/update) **+** `docs_get_section` (קריאת תיעוד RST, ציבורי) **מומשו** — ראו `mcp_server/` (18 כלים). מחיקה (קבצים ופתקים) וכתיבת אוספים עדיין בתכנון. > **ענף פיתוח:** `claude/mcp-codekeeper-webapp-ldnzsg` > **מתי להשתמש:** לפני מימוש חיבור MCP; מסמך זה הוא מקור האמת לתכנון. > **ראו גם:** `mcp_server/README.md` (שימוש), [CodeBot – Project Docs](https://amirbiron.github.io/CodeBot/), `CLAUDE.md` (מדיניות מחייבת). @@ -375,7 +379,183 @@ db.delete_file(user_id, file_name) # מחיקה רכה (recycle --- -## 14. מקורות +## 14. מה התכנון לא צפה — לקחים מהמימוש + +הסעיפים 1–13 הם התכנון. הסעיף הזה הוא מה שגילינו **אחרי** שהתחלנו לכתוב קוד. אם אתם +מממשים שרת MCP בפרויקט אחר, זה החלק שיחסוך לכם את הזמן — כל פריט כאן עלה לנו בסבב תיקון. + +המצב בפועל: 18 כלים, `mcp_server/` עם 17 מודולים, ארבע פאזות בפרודקשן. + +### 14.1 ל‑SDK אין scopes פר‑כלי — האכיפה היא בגוף הכלי + +זו ההפתעה הראשונה, והיא משנה איך בונים את השרת. ה‑MCP Python SDK יודע לאמת טוקן ולתת +לכם `ctx` עם ה‑scopes שלו, אבל **אין מנגנון שאומר "הכלי הזה דורש scope write"**. הרישום של +כלי הוא דקורטור, לא הצהרת הרשאה. + +המשמעות: כל כלי כותב חייב לקרוא בעצמו, בשורה הראשונה שלו, לבדיקה. אצלנו: + +```python +@mcp.tool(name="codekeeper_save_file", ...) +def save_file(ctx: Context, file_name: str, code: str, ...) -> dict: + require_write(ctx) # דוחה טוקן קריאה-בלבד לפני כל נגיעה בנתונים + return handlers.save_file(backend, user_id=current_user_id(ctx), ...) +``` + +אצלנו זה עשרה מקומות: חמישה כלים קוראים `require_write`, וחמישה קוראים `require_admin`. +אין נקודת אכיפה מרכזית אחת — יש עשר נקודות שכל אחת מהן חייבת לזכור בעצמה. + +**הטעות הנפוצה:** להניח שהרישום מגן. הסימפטום: כלי כתיבה עובד עם טוקן קריאה‑בלבד בבדיקה +ידנית. הפתרון: טסט שמנסה כל כלי כותב עם טוקן `read` בלבד ומצפה לדחייה — לא לסמוך על עין. + +### 14.2 הסתרת כלי מ‑tools/list היא UX, לא בקרת גישה + +בנינו תת‑מחלקה של FastMCP שמסננת את כלי הריפו מ‑`list_tools` כשהמשתמש אינו אדמין. זה נוח, +אבל **ה‑dispatcher מריץ גם כלי שאינו ברשימה**. מי שיודע את השם `codekeeper_get_repo_file` +יכול לקרוא לו ישירות והוא ירוץ. + +לכן המבנה אצלנו הוא שתי שכבות נפרדות, ורק אחת מהן היא אבטחה: + +```python +_ADMIN_TOOLS = frozenset({...}) # שכבה 1: הסתרה מהרשימה — נוחות בלבד +# שכבה 2: require_admin(ctx) בשורה הראשונה של כל גוף כלי — זה החסם האמיתי +``` + +השארנו הערה מפורשת בקוד ליד ה‑frozenset ("every one of these also calls require_admin in +its body") כי זה בדיוק סוג הקשר שנשכח בסבב התחזוקה הבא. + +### 14.3 שינוי scopes דורש רישום DCR מחדש + +זה תפס אותנו בפרודקשן. Claude.ai שומר את ה‑scopes שסוכמו ברישום הדינמי (DCR) של +ה‑connector. כשהוספנו את scope ה‑`write`, המשתמשים הקיימים המשיכו לקבל טוקני `read` — +והכלים החדשים נכשלו בלי סיבה נראית לעין. + +אין דרך "לרענן" את זה מהשרת. המשתמש חייב **להסיר את ה‑connector ולהוסיף אותו מחדש**. + +תכננו את ה‑scopes מראש, גם אם לא תממשו את כולם ביום הראשון. הוספת scope אחרי שיש +משתמשים היא שינוי שובר‑תאימות מבחינתם. + +### 14.4 גשר זהות חתום — איך מחברים OAuth לזהות קיימת + +התכנון אמר "מסך ה‑authorize משתמש בסשן הטלגרם הקיים" (סעיף 6, פאזה 1). זה נכון, אבל +בפועל צריך פתרון לבעיה שלא הופיעה בתכנון: **ה‑OAuth רץ בשירות ה‑MCP, והסשן חי בוובאפ.** +שני תהליכים, שני שירותי Render, אין ביניהם זיכרון משותף. + +הפתרון: assertion זהות חתום, קצר‑מועד, שעובר בין השירותים. הוובאפ מזהה את המשתמש +(סשן טלגרם), חותם `{user_id, txn, exp}` בסוד המשותף, ומעביר לשירות ה‑MCP; שם מאמתים +את החתימה ומציגים מסך consent. תוקף חמש דקות — מספיק לאשר, קצר מכדי להיות שימושי אם דלף. + +זה בדיוק הדפוס של `webapp_tokens` שכבר היה בפרויקט (סעיף 2.3), רק בכיוון ההפוך. אם יש +לכם כבר גשר טוקנים בין שירותים — השתמשו באותו דפוס במקום להמציא אחד. + +### 14.5 כלי שמקבל מזהה משאב הוא IDOR עד שהוכח אחרת + +הלקח הכי חד מכל הסבב הזה. הוספנו כלי **ציבורי** (`docs_get_section`) שמקבל פרמטר +`repo` אופציונלי. הוא נראה תמים: ברירת המחדל היא הריפו שלנו, והוא רק קורא קבצי `.rst`. + +הבעיה: ה‑backend אימת רק את ה**פורמט** של שם הריפו, לא את ההרשאה. כלומר כל משתמש +מאומת יכול היה להעביר `repo="SomeOtherRepo"` ולקרוא ממנו — **בדיוק מה שהשער האדמין +של פאזה ד' נועד למנוע.** כלי ציבורי אחד עקף את כל מודל ההרשאות של דפדפן הריפו. + +התיקון הוא allowlist מפורש, ודחייה לפני שנוגעים ב‑backend: + +```python +def _resolve_docs_repo(repo: str | None) -> str | None: + allowed = _allowed_docs_repos() # CSV מ-MCP_DOCS_REPO + r = (repo or "").strip() + if not r: + return allowed[0] # ברירת מחדל + return r if r in allowed else None # None ⇒ repo_not_allowed +``` + +**הכלל להכללה:** כל פרמטר שהוא מזהה של משאב (`repo`, `tenant`, `project`, `org`) בכלי +שאינו מוגן — הוא IDOR עד שיש לו allowlist. ואם משנים משתנה סביבה לגבול אבטחה, תעדו +אותו ככזה: `MCP_DOCS_REPO` מתועד אצלנו כ"רשימת ריפואים מותרים", לא כ"שם הריפו". + +### 14.6 נתיבים: לנרמל בשכבת ה‑handler, לא לסמוך על ה‑backend + +באותו סבב מצאנו שהכלי העביר גם `webapp/config.rst` (מחוץ ל‑`docs/`) וגם `../../etc/passwd`. +ה‑backend אמנם חוסם traversal, אבל להסתמך עליו זה להעמיד שכבת אבטחה אחת בלבד. + +הכלל: לנרמל עם `posixpath.normpath` ואז לוודא שהתוצאה **מתחילה בקידומת המותרת**. סדר +הפעולות חשוב — נרמול לפני הבדיקה, אחרת `docs/../secrets` עובר. + +```python +norm = posixpath.normpath(p) +if not norm.startswith("docs/"): + return None # גם traversal, גם נתיב מוחלט, גם קלט עם slash מחוץ ל-docs/ +``` + +### 14.7 תקציב טוקנים הוא דרישה תפקודית, לא אופטימיזציה + +`docs/environment-variables.rst` אצלנו הוא 77KB ומעל 2200 שורות. כדי לקרוא ערך אחד מטבלת +משתני הסביבה, מודל היה צריך למשוך את כולו — ולשרוף חלון הקשר שלם על שורה אחת. + +מכאן נולד `docs_get_section`: מחזיר **סקשן בודד** מקובץ RST במקום קובץ שלם. שני דברים +שכדאי להעתיק מהעיצוב שלו: + +**ראשית, כלי חיפוש שמחזיר מספר שורה כמעט חסר ערך בתיעוד.** מה שרלוונטי הוא באיזה +*סעיף* אתה נמצא, לא באיזו שורה. אם אתם בונים כלי קריאה לתיעוד — חשבו במונחי סקשנים. + +**שנית, לעולם אל תחזירו למודל רק "לא נמצא".** מודל שקיבל `{"error": "not_found"}` תקוע: +הוא לא רואה את הקובץ ולא יודע מה כן קיים. אצלנו כל תשובה מחזירה ערך שמיש: + +| מצב | מה מוחזר | +|------|-----------| +| בלי `section` | עץ הכותרות המלא (הכלי משמש גם לניווט) | +| `section` לא נמצא | TOC מלא + הצעות קרובות (difflib) | +| כותרת מופיעה פעמיים | כל המועמדים עם breadcrumb — בלי לנחש | +| נמצא | הסקשן + שכנים + תת‑סקשנים לניווט המשך | + +זה מוסיף אולי שלושים שורות קוד וחוסך למודל סיבוב שלם של ניחושים. + +### 14.8 תיאור הכלי הוא ממשק, לא תיעוד + +מודל בוחר כלי לפי התיאור. אם יש לכם שני כלים חופפים — `get_repo_file` שמחזיר קובץ שלם +ו‑`docs_get_section` שמחזיר חלק — התיאור חייב להגיד **מתי להעדיף את מי**, אחרת המודל +יבחר את הראשון שנראה לו מתאים וימשוך 77KB. + +נסחו את התיאור כהוראה, לא כהגדרה: "השתמש בזה במקום `get_repo_file` כשצריך חלק ממסמך +RST — מחזיר סקשן בודד עם ניווט במקום קובץ שלם; בלי `section` מחזיר עץ כותרות." + +### 14.9 פארסר עצמאי, לא לוגיקה בתוך ה‑handler + +את פענוח ה‑RST הוצאנו ל‑`services/rst_parser.py` — מודול טהור בלי שום תלות ב‑MCP. זה +נראה כמו over‑engineering לכלי אחד, אבל שתי סיבות מצדיקות את זה: + +הראשונה, אפשר לבדוק אותו על קבצי RST אמיתיים מהריפו בלי להרים שרת MCP. הטסטים שלנו +קוראים את `docs/environment-variables.rst` האמיתי ומוודאים שמקרי הקצה מטופלים — +למשל ששורת `===` **בתוך** בלוק קוד לא מזוהה ככותרת. + +השנייה, כלים עתידיים (חיפוש בתיעוד, שליפת טבלת env-vars כדאטה מובנית) משתמשים באותו +פארסר. הלוגיקה שקשה לכתוב נכון נכתבת פעם אחת. + +מקרי הקצה שתפסו אותנו ב‑RST, אם אתם מפרסרים משהו דומה: רמות כותרת נקבעות **דינמית** +לפי סדר ההופעה של תווי הקישוט בקובץ (לא `===` = תמיד H1); כותרת חייבת להיות בעמודה 0, +אחרת כל טבלה מוזחת נראית ככותרת; ובלוקי literal אחרי `::` מכילים שורות שנראות בדיוק +כמו קישוט כותרת. + +### 14.10 השירות צריך לסנכרן את הדיסק של עצמו + +פאזה ד' קוראת מ‑bare mirrors על הדיסק. בתכנון זה נראה כמו פרט תפעולי, אבל בפרודקשן +התברר: שירות ה‑MCP הוא **שירות Render נפרד עם דיסק נפרד**. ה‑sync שרץ בוובאפ מעדכן את +הדיסק של הוובאפ — לא של ה‑MCP. + +התוצאה: ה‑MCP הגיש גרסאות ישנות של קבצים, בלי שום שגיאה. הוספנו worker +(`mcp_server/repo_autosync.py`) שרץ בתוך שירות ה‑MCP ומושך `git fetch` לדיסק שלו, עם +`MCP_REPO_AUTOSYNC_INTERVAL` (ברירת מחדל 300 שניות). + +**הכלל:** אם השירות שלכם קורא ממצב על דיסק שגורם אחר מייצר — ודאו מי בדיוק מעדכן את +העותק שהשירות הזה רואה. "יש sync" זו לא תשובה; השאלה היא סנכרון של *איזה* דיסק. + +### 14.11 מה שעדיין לא מומש + +כדי שהמסמך יישאר מדויק: מחיקת קבצים (`delete_file`), כתיבה לאוספים, מחיקת פתקים +ותזכורות — כולם נשארו בתכנון. `get_version_diff` ו‑Resources (סעיף 5.2) גם הם לא מומשו. +זה לא פער שדורש תיקון; זה היקף שנבחר במודע. + +--- + +## 15. מקורות - [Claude Help Center — Get started with custom connectors using remote MCP](https://support.claude.com/en/articles/11175166-get-started-with-custom-connectors-using-remote-mcp) - [Claude Docs — Authentication for connectors](https://claude.com/docs/connectors/building/authentication) - [CodeBot – Project Docs](https://amirbiron.github.io/CodeBot/)