feat(mcp): implement Phase D — admin-only repo-browser tools - #3183
Merged
Conversation
מימוש דפדפן הריפו ב-MCP (קריאה בלבד, אדמין בלבד), לפי סעיף 13 במסמך התכנון: - 4 כלים חדשים מעל ה-Repo Sync Engine הקיים (ללא לוגיקת persistence חדשה): codekeeper_list_repos / list_repo_tree (עימוד+סינון) / get_repo_file (מעטפת יציבה: ok/binary/too_large + not_found/path_denied) / search_repo (snippets). - שער אדמין fail-closed: require_admin ב-auth.py (config.ADMIN_USER_IDS בלבד, בלי מפלט CHATOPS_ALLOW_ALL_IF_NO_ADMINS; רשימה ריקה = אף אחד) בגוף כל כלי, ו-AdminAwareFastMCP שמסתיר את הכלים מ-tools/list ללא-אדמין (הסתרה = UX, החסימה האמיתית בגוף). - מדיניות סינון סודות מחייבת (repo_policy.py): חסימה/השמטה/דילוג של .env*, *.pem, id_rsa* וכו' בכל שלושת המשטחים; נרמול נתיבים case-insensitive על נתיב מלא ו-basename; fail-closed על שגיאה פנימית; הרחבה דרך MCP_REPO_DENYLIST_EXTRA. - תקרות שרת + clamp (repo_handlers.py): limit 50/200, per_page 200/1000, max_results 50/100, תקציב פלט 256KB עם truncated. - הערת הביקורת של המשתמש #1: אינדקס unique ל-repo_metadata נוצר כחלק מהפאזה — גם ב-scripts/create_repo_indexes.py וגם best-effort באתחול ה-backend. - הערת הביקורת של המשתמש #2: כשל קריאה בזמן sync פעיל מחזיר {"error": "sync_in_progress", "retry_after": 30} (בדיקת sync_jobs.running) — כדי שהמודל הקורא יחזור אחרי המתנה ולא יסיק שהקובץ לא קיים. - תיעוד: README (כולל הערת פריסה — דיסק פר-שירות ב-Render + REPO_MIRROR_PATH), environment-variables.rst + config_inspector (MCP_REPO_DENYLIST_EXTRA חדש), ועדכון סטטוס פאזה ד' במסמך התכנון (כולל retry_after וסגירת פער 6). טסטים: 61 חדשים (policy/backend/handlers/require_admin/visibility) — 145 עוברים סה"כ; black/ruff/flake8 נקי; אומת גם שער האדמין בשרשרת המלאה. עיינתי בתיעוד: https://amirbiron.github.io/CodeBot/ Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WNFuSyshwpRcxozVZEui5K
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
🧯 Dangerous deletes guard reportPolicy: see .cursorrules — dangerous deletions are blocked unless wrapped safely. Summary:
Flagged findings (file:line:snippet): Excluded matches (by path pattern) |
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughנוסף דפדפן ריפוזיטוריז לקריאה בלבד ב-MCP, הכולל ארבעה כלים אדמיניים, הרשאות fail-closed, denylist לנתיבים רגישים, mirrors מקומיים, autosync, חיפוש, פאג’ינציה וטיפול ב־ Changesדפדפן ריפוזיטוריז ואבטחת גישה
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AdminClient
participant AdminAwareFastMCP
participant RepoHandlers
participant RepoBackend
participant RepoMirror
participant RepoAutosync
AdminClient->>AdminAwareFastMCP: tools/list או קריאת כלי repo
AdminAwareFastMCP->>RepoHandlers: העברת קלט לאחר בדיקת אדמין
RepoHandlers->>RepoBackend: list_tree/get_file/search
RepoBackend->>RepoMirror: קריאת mirror או חיפוש
RepoAutosync->>RepoMirror: clone או fetch לפי SHA
RepoMirror-->>RepoBackend: תוצאות או שגיאת קריאה
RepoBackend-->>RepoHandlers: תוצאת קריאה או sync_in_progress
RepoHandlers-->>AdminAwareFastMCP: מעטפת כלי
AdminAwareFastMCP-->>AdminClient: תגובת MCP
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
⏱️ Performance report(No performance test durations collected. Mark tests with |
📖 Documentation PreviewThe documentation has been built successfully!
To view locally:
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/create_repo_indexes.py (1)
20-22: 🗄️ Data Integrity & Integration | 🔵 Trivialשימו לב לסיכון מיגרציה: יצירת אינדקס unique תיכשל אם קיימים כבר
repo_nameכפולים.אם באוסף
repo_metadataיש כבר מסמכים עםrepo_nameכפול (מצטבר לפני הפיצ'ר), הרצת השורה הזו בסביבת פרודקשן תיכשל עם שגיאת duplicate key ותעצור את הסקריפט. מומלץ לבצע בדיקה מקדימה (למשלaggregateעם$groupעלrepo_name+$matchעלcount > 1) ולדווח/לנקות כפילויות לפני יצירת האינדקס, כדי לא להיתקע בזמן דיפלוי.🤖 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 `@scripts/create_repo_indexes.py` around lines 20 - 22, Update the index-creation flow around db.repo_metadata.create_index to detect duplicate repo_name values first, using aggregation grouped by repo_name and filtering groups with count greater than one. Report or clean the duplicates before attempting the unique index, and prevent index creation from proceeding when unresolved duplicates remain.
🤖 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 `@mcp_server/repo_backend.py`:
- Around line 154-176: Validate and normalize the page and per_page inputs
before calculating the slice in the repository listing handler: convert them
safely inside the existing error-handling flow, reject non-numeric values, and
clamp page to at least 1 and per_page to the supported positive limit. Use the
normalized page_i and per_page_i for slicing and for the returned "page" and
"per_page" fields, preserving the existing error response behavior for invalid
input.
- Around line 241-265: Update the result handling around the rows construction
and return payload so exceeding max_results sets truncated to true when
additional policy-allowed matches exist beyond the cap. Compute total from the
policy-filtered results before applying the max_results slice, ensuring it
reflects available results after is_denied filtering rather than the backend
total; preserve the byte-budget truncation behavior and add coverage in
test_search_caps_filters_and_snippets for both fields.
---
Nitpick comments:
In `@scripts/create_repo_indexes.py`:
- Around line 20-22: Update the index-creation flow around
db.repo_metadata.create_index to detect duplicate repo_name values first, using
aggregation grouped by repo_name and filtering groups with count greater than
one. Report or clean the duplicates before attempting the unique index, and
prevent index creation from proceeding when unresolved duplicates remain.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 96737f55-c8a7-435c-be19-c0e64dd5a09e
📒 Files selected for processing (16)
FEATURE_SUGGESTIONS/FEATURE_MCP_CLAUDE_INTEGRATION.mddocs/environment-variables.rstmcp_server/README.mdmcp_server/app.pymcp_server/auth.pymcp_server/repo_backend.pymcp_server/repo_handlers.pymcp_server/repo_policy.pymcp_server/server.pyscripts/create_repo_indexes.pyservices/config_inspector_service.pytests/test_mcp_repo_backend.pytests/test_mcp_repo_handlers.pytests/test_mcp_repo_policy.pytests/test_mcp_require_admin.pytests/test_mcp_server_build.py
…ls, index dup-guard תיקוני code review על מימוש פאזה ד' (כל שלושת הממצאים תקפים): - list_tree: נרמול הגנתי של page/per_page גם בשכבת ה-backend (ה-handler כבר עושה clamp, אבל המתודה ציבורית): קלט לא-מספרי ⇒ ברירת מחדל, רצפה של 1, ותקרת TREE_PER_PAGE_MAX — הערכים המנורמלים משמשים גם לחיתוך וגם בתשובה (בלי slicing שלילי ובלי ValueError). - search: total משקף עכשיו את מספר ההתאמות המותרות לאחר סינון המדיניות (ולא את ה-total הגולמי של המנוע שכולל נתיבים חסומים), ו-truncated נדלק גם כשקיימות התאמות מותרות מעבר ל-max_results (בנוסף לקיטום תקציב-הבייטים ולדגל המנוע). - create_repo_indexes.py: בדיקת כפילויות repo_name (aggregation) לפני יצירת ה-unique index — כפילות מדווחת ומדלגת במקום להפיל את הסקריפט באמצע ריצת שאר האינדקסים. טסטים: עודכן test_search_caps_filters_and_snippets (total+truncated), נוספו טסט אי-קיטום מתחת לתקרה וטסט נרמול קלט לעץ. 147 עוברים; black/ruff/flake8 נקי. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WNFuSyshwpRcxozVZEui5K
הפיכת עדכון ה-mirrors בשירות ה-MCP לאוטומטי לגמרי, באותו דפוס כמו הוובאפ (thread רקע דמון בתוך אותו שירות — לא cron ולא שירות Render נוסף): - mcp_server/repo_autosync.py: לולאת רענון שמנצלת את מה שכבר קורה — הוובאפ מקבל את ה-webhook מ-GitHub ואחרי כל sync כותב last_synced_sha ל-Mongo המשותף. הלולאה משווה את ה-SHA המקומי ומריצה fetch כשיש סטייה; ריפו שקיים ב-repo_metadata אך חסר בדיסק המקומי משוכפל אוטומטית מ-repo_url (init_mirror אידמפוטנטי) — אין יותר initial_import ידני בצד ה-MCP. - זרימה: merge ל-main → webhook לוובאפ → SHA ב-Mongo → ה-MCP מזהה ומושך. - בזמן clone/fetch מקומי הכלים מחזירים sync_in_progress+retry_after (is_refreshing משולב ב-RepoBackend._sync_running לצד תור ה-jobs של הוובאפ). - שליטה: MCP_REPO_AUTOSYNC (ברירת מחדל פעיל, 0 מכבה), MCP_REPO_AUTOSYNC_INTERVAL (ברירת מחדל 300ש', רצפה 30). מותנע מ-create_app עם עטיפת שגיאות — כשל בהתנעה לא מפיל את השירות. - תיעוד: README (זרימה + ENV: צריך GITHUB_TOKENS לפרטיים, לא צריך GITHUB_WEBHOOK_SECRET ב-MCP), environment-variables.rst + config_inspector. טסטים: 10 חדשים ל-refresh_once (clone-חסר, דילוג SHA-זהה, fetch על סטייה/חוסר ודאות, בליעת שגיאות + ניקוי דגל, is_refreshing בזמן פעולה, kill-switch, רצפת interval) + טסט backend ל-sync_in_progress ברענון מקומי. 157 עוברים; לינט נקי. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WNFuSyshwpRcxozVZEui5K
עמוד תיעוד חדש docs/mcp-server.rst (משולב ב-toctree תחת האינטגרציות): - מה זה נותן + ארכיטקטורה בקצרה (in-process, user_id מהטוקן, Smart Projection). - טבלאות כל 12 הכלים — 8 כלי משתמש + 4 כלי אדמין (דפדפן הריפו). - אימות והרשאות: PAT/OAuth מאוחדים, שכבות read/write/אדמין. - הפעלה צעד-אחר-צעד: שירות Render → מצב PAT → מצב OAuth (כולל אילו ENV על איזה שירות) → דפדפן הריפו (דיסק, ADMIN_USER_IDS, GITHUB_TOKENS; בלי webhook secret). - חיבור משלושת הלקוחות (Claude.ai / Code / Desktop) עם פקודות מוכנות להעתקה. - autosync (זרימת merge→webhook→SHA→fetch), מדיניות סינון הסודות, התנהגות sync_in_progress/retry_after, עקרונות אבטחה, וטבלת פתרון תקלות. בנוסף: תיקון docstring ב-database/collections_manager.py (get_tags_metadata) — רשימת ההגדרות השבורה הייתה שתי האזהרות היחידות בבניית Sphinx; אחרי התיקון הבנייה כולה על 0 אזהרות (חובה לפי מדיניות ה-RTD fail_on_warning). אומת: בנייה מלאה exit 0 עם 0 אזהרות; doc8 נקי על העמוד החדש. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WNFuSyshwpRcxozVZEui5K
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@mcp_server/repo_autosync.py`:
- Around line 115-117: Prevent Git secrets from reaching logs by adding a shared
sanitization helper and applying it to res.get("message") before logging
failures from init_mirror at mcp_server/repo_autosync.py lines 115-117 and
fetch_updates at mcp_server/repo_autosync.py line 131. Ensure token-bearing Git
URLs are redacted while preserving safe diagnostic details.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b518fced-d800-43c2-816e-495650495e74
📒 Files selected for processing (12)
database/collections_manager.pydocs/environment-variables.rstdocs/index.rstdocs/mcp-server.rstmcp_server/README.mdmcp_server/app.pymcp_server/repo_autosync.pymcp_server/repo_backend.pyscripts/create_repo_indexes.pyservices/config_inspector_service.pytests/test_mcp_repo_autosync.pytests/test_mcp_repo_backend.py
🚧 Files skipped from review as they are similar to previous changes (6)
- mcp_server/README.md
- mcp_server/app.py
- services/config_inspector_service.py
- scripts/create_repo_indexes.py
- mcp_server/repo_backend.py
- tests/test_mcp_repo_backend.py
ממצא ביקורת: לוגי הכשל של autosync מדפיסים message שמגיע מתלות חיצונית (init_mirror/fetch_updates) ועלול להכיל URL של git עם טוקן. אימות מול הקוד: המנוע עצמו כבר מסנן את ה-stderr במקור (_run_git_command → _sanitize_output, כולל ה-URL עם הטוקן), כך שלא נמצאה דליפה חיה בנתיבים הקיימים — אבל אנחנו מלוגגים ערכים מתלות duck-typed, ולפי CLAUDE.md אסור שסודות יגיעו ללוגים בכלל. לכן נוסף helper משותף `_redact`: - מסיר credentials מ-userinfo של URL (https://user:token@host → https://***@host) - מסיר צורות טוקן של GitHub (ghp_/gho_/ghu_/ghs_/ghr_/github_pat_) - fail-closed: שגיאה פנימית ⇒ placeholder, לעולם לא הטקסט הגולמי - מוחל על שני אתרי הלוג (clone failed / fetch failed) טסטים: יחידה ל-_redact (URL עם credentials, טוקן חשוף, שימור אבחון תקין, None) + טסט caplog שמוכיח שנתיב הכשל לא מדליף טוקן ללוג. 159 עוברים; לינט נקי. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WNFuSyshwpRcxozVZEui5K
16 tasks
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.
מימוש דפדפן הריפו ב-MCP (קריאה בלבד, אדמין בלבד), לפי סעיף 13 במסמך התכנון:
טסטים: 61 חדשים (policy/backend/handlers/require_admin/visibility) — 145 עוברים סה"כ; black/ruff/flake8 נקי; אומת גם שער האדמין בשרשרת המלאה.
עיינתי בתיעוד: https://amirbiron.github.io/CodeBot/
Claude-Session: https://claude.ai/code/session_01WNFuSyshwpRcxozVZEui5K
תבנית Pull Request
✨ תיאור קצר
📦 שינויים עיקריים
פירוט נקודות (רשימת תבליטים):
🧪 בדיקות
🧪 בדיקות נדרשות ב‑PR
📝 סוג שינוי
דוגמאות Conventional Commits
✅ צ'קליסט
services/register_jobs.py(כולל Callback/Trigger להפעלה ידנית — למשלcallback_name/trigger_funcלפי המבנה) כדי שיופיעו בדשבורדdocs/environment-variables.rstוגםservices/config_inspector_service.pydocs/webapp/theming_and_css.rst+FEATURE_SUGGESTIONS/theme_matrix.md🧩 השפעות/סיכונים
🔗 קישורים
🧯 סיכון / החזרה לאחור (Rollback)
Summary by CodeRabbit
sync_in_progressעםretry_afterשל 30 שניות.