Repository navigation
fix(mcp): מראה בדיסק של שירות ה-MCP בלי רשומה ב-repo_metadata נמחקת בסוף מעבר ה-autosync - #3520
Conversation
…סוף מעבר ה-autosync ריפו שהוסר בממשק (unmirror_repo) נמחק מהדיסק של הוובאפ ומ-Mongo, אבל המראה שלו בדיסק של שירות ה-MCP נשארה לנצח, וכלי הקריאה המשיכו להגיש אותה (codekeeper-plugin, קומיט 6ec18f2). - refresh_once: שלב התאמה (_prune_orphans) אחרי המעבר על הרשומות, על אותה קריאה של repo_metadata; המחיקה דרך delete_mirror (_safe_rmtree). - לא נמחק: כשהשאילתה נכשלה, כשאין אף שם ריפו (עם אזהרה), שם שאינו עובר _validate_repo_name, קישור סמלי, וריפו ש-is_refreshing מדווח עליו. - מראה נספרת ב-pruned רק אחרי ש-mirror_exists מראה שהיא איננה; כשל של delete_mirror נספר ב-errors, והמעבר ממשיך לשאר המראות. - שורת "repo autosync pass" עברה ל-refresh_once, והתנאי שלה כולל pruned. - GitMirrorService.list_mirror_names: מניית המראות בדיסק, משותפת ל-sweep של ה-credentials ולהתאמה (עד עכשיו עותק בתוך ה-sweep). הסיומת .git בקבוע אחד, ש-_get_repo_path קורא גם הוא. - טסטים מול מראות bare אמיתיות ב-tmp_path; תיעוד: mcp-server.rst, README של mcp_server, environment-variables, whats-new. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016LWehkLW2ZDpXQuXuDqh5X
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
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 1 day and 7 hours by commenting @sourcery-ai review. Upgrade to get a review now.
![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.
🧯 Dangerous deletes guard reportPolicy: see .cursorrules — dangerous deletions are blocked unless wrapped safely. Summary:
Flagged findings (file:line:snippet): Excluded matches (by path pattern) |
Reviewer's GuideThe PR makes MCP autosync reconcile the local mirror directory with repository metadata by safely deleting orphaned mirrors after successful passes, adds shared mirror enumeration, records and logs pruning outcomes, tests the safety and failure paths, and documents the behavior. Sequence diagram for MCP autosync orphan pruningsequenceDiagram
participant Autosync as refresh_once
participant Mongo as repo_metadata
participant Mirror as GitMirrorService
participant Disk as Local mirror disk
Autosync->>Mongo: find
alt query fails
Mongo-->>Autosync: failure
Autosync-->>Autosync: return stats
else query succeeds
Mongo-->>Autosync: repo records
Autosync->>Mirror: refresh known repositories
Autosync->>Mirror: list_mirror_names
Mirror->>Disk: enumerate *.git directories
Disk-->>Mirror: local mirror names
Mirror-->>Autosync: on_disk names
loop orphan mirrors
Autosync->>Mirror: is_refreshing(name)
alt safe to delete
Autosync->>Mirror: delete_mirror(name)
Mirror->>Disk: _safe_rmtree
Autosync->>Mirror: mirror_exists(name)
Mirror-->>Autosync: mirror absent
Autosync-->>Autosync: increment pruned
else refreshing, invalid, or symlink
Autosync-->>Autosync: skip mirror
end
end
Autosync-->>Autosync: log repo autosync pass
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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 configuration
📒 Files selected for processing (9)
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ניקוי מראות MCP יתומות
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant refresh_once
participant repo_metadata
participant GitMirrorService
refresh_once->>repo_metadata: שאילתת שמות מאגרים
repo_metadata-->>refresh_once: שמות מאגרים רשומים
refresh_once->>GitMirrorService: list_mirror_names
GitMirrorService-->>refresh_once: שמות מראות מקומיות
refresh_once->>GitMirrorService: delete_mirror למראה ללא רשומה
Merge Risk: ⚪ Minimal · up to No identified issue prevents merging after normal checks. The new tests still need to run in CI. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Automatic cleanup reduces stale repository availability and includes safeguards against accidental deletion. Remaining uncertainty concerns concurrent filesystem changes and who can modify the repository-copy directory. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 30.77% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 39 functions across 5 files. (4 skipped: 4 unsupported.)
✨ Finishing Touches 💡 1📝 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 |
⏱️ Performance report(No performance test durations collected. Mark tests with |
📖 Documentation PreviewThe documentation has been built successfully!
To view locally:
|
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (10 files)
Previous Review Summary (commit fd37c3d)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit fd37c3d)Status: No Issues Found | Recommendation: Merge Files Reviewed (9 files)
Reviewed by nemotron-3-ultra-550b-a55b:free · Input: 423.6K · Output: 13.3K · Cached: 427.7K |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 9 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…, ושירות המראות מחליט מה מותר למחוק תיקוני ריוויו ל-#3520. - הגנת חפיפה במקום הגנת "ריק": לא מוחקים כלום כשאף שם ב-repo_metadata אינו שם של מראה שבדיסק. ההגנה הקודמת בדקה רק שיש שם כלשהו, ולכן רשומה אחת עם שם לא תקין הייתה מוחקת את כל המראות. אותה הגנה תופסת גם קריאה ריקה ומסד עם ריפואים אחרים; האזהרה נרשמת רק כשיש מראות שנחסכו. - repo_metadata נקרא מה-primary במפורש (ReadPreference.PRIMARY, כמו Repository.find_version_by_id): משני מפגר היה משמיט רשומה, והמראה שלה הייתה נמחקת. - GitMirrorService.list_managed_mirror_names: השירות מחליט מה נחשב מראה שמותר למחוק (שם תקין, לא קישור סמלי). repo_autosync כבר לא נוגע ב-_validate_repo_name וב-_get_repo_path, והתלות מוגדרת ב-Protocol (LocalMirrors). - מניית המראות ב-os.scandir במקום Path.glob, שבלע PermissionError והחזיר רשימה ריקה. תיקייה לא קריאה היא עכשיו כשל: בהתאמה נספר ב-errors, ובניקוי ה-credentials עולה כחריגה במקום שורת checked=0 failed=0. - תיעוד: האזהרה רק כשיש מראות בדיסק, ומחיקה שנכשלה משאירה את המראה עד המעבר הבא (mcp-server.rst, README, whats-new, docstrings). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016LWehkLW2ZDpXQuXuDqh5X
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Code Review ✅ Approved🔴 High risk · Autosync now deletes on-disk repository mirrors absent from Mongo metadata. Prunes orphaned MCP repository mirrors during autosync passes so the service's local disk stays consistent with OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
2 participants
✨ תיאור קצר
ריפו שהוסר בממשק נשאר לנצח בדיסק של שירות ה-MCP, וכלי הקריאה המשיכו להגיש אותו. כך קרה ל-
codekeeper-plugin: אין לו רשומה ב-repo_metadata, ובכל זאתcodekeeper_list_repo_treeמחזיר עליו עץ, מקומיט6ec18f2. עכשיו בסוף כל מעבר של ה-autosync, כל מראה בדיסק של השירות שאין לה רשומה ב-repo_metadataנמחקת. כך הדיסק משקף את Mongo, והמראה שלcodekeeper-pluginתימחק במעבר הראשון שאחרי הפריסה, בלי טיפול ידני.השורש: הסרה בוובאפ (
unmirror_repo) מוחקת את המראה שבדיסק של הוובאפ ואת הרשומות, ולדיסק של ה-MCP היא לא מגיעה. ב-refresh_onceלא היה שום שלב שמסיר מראה מקומית שאין לה רשומה. זה בדיוק הדפוסstate-record-without-state-change§3: הרשומה נמחקה, והמשאב נשאר.📦 שינויים עיקריים
פירוט:
mcp_server/repo_autosync.py— שלב התאמה חדש,_prune_orphans, שרץ אחרי המעבר על הרשומות ועל אותה קריאה שלrepo_metadata. הקריאה נעשית מה-primary במפורש (ReadPreference.PRIMARY). המחיקה עוברת ב-delete_mirror(_safe_rmtree), בליrmtreeחדש. לא נמחק:repo_metadataאינו שם של מראה שבדיסק: קריאה ריקה, שמות שאינם שמות של מראה, או מסד עם ריפואים אחרים. כשיש בדיסק מראות שנחסכו כך, נרשמת אזהרה._validate_repo_nameדוחה, או קישור סמלי. את ההחלטה הזו מקבל שירות המראות (list_managed_mirror_names), לא ה-autosync.is_refreshingמדווח עליו.refresh_onceבשירות המראות מוגדרת ב-Protocol (LocalMirrors), כך שה-autosync לא נוגע בפונקציות פרטיות שלו.prunedנוסף ל-stats, ושורתrepo autosync passנכתבת גם כשהדבר היחיד שקרה הוא מחיקה. לכל מראה שנמחקה יש שורת info עם השם. כשל שלdelete_mirror(בערך החזרה או בחריגה), או תיקיית מראות שאי אפשר לקרוא, נספרים ב-errorsונרשמים כאזהרה. המראה נשארת עד המעבר הבא, והמעבר ממשיך לשאר המראות.services/git_mirror_service.py—list_mirror_names()מחזירה את כל המראות בדיסק, ו-list_managed_mirror_names()רק את אלה שמותר למחוק. שתיהן נשענות על סריקה אחת ב-os.scandir, ההיפוך של_get_repo_path. הסיומת.gitעברה לקבוע אחד (MIRROR_DIR_SUFFIX). תיקייה שחסרה, או שאי אפשר לקרוא, זורקתOSErrorולא מחזירה רשימה ריקה.services/mirror_credentials.py— ה-sweep משתמש ב-list_mirror_names. עד עכשיו המנייה הייתה עותק בתוך ה-sweep, וזה איחוד לפי R6: כל תיקיית*.gitעדיין נבדקת, גם עם שם לא תקין. שינוי אחד: תיקיית מראות שאי אפשר לקרוא היא עכשיו כשל עם שם. במקום שורה שלchecked=0 failed=0, שנראית כמו "הכול נקי", ה-sweep זורק.docs/mcp-server.rst(עם עוגן חדש,mcp-repo-autosync), הסעיף על ה-sweep, שורתrepo_not_mirroredבטבלת התקלות,MCP_REPO_AUTOSYNCב-environment-variables.rst, ה-README שלmcp_server, ו-whats-new.🔎 ממצאי המחקר שלפני הקוד
codekeeper_list_reposמחזיר 4 ריפואים, ו-codekeeper_list_repo_treeעלcodekeeper-pluginמחזיר עץ עםref: "HEAD", כי בלי רשומה הענף הראשי נופל ל-HEAD.init_mirrorמשכפל ישר לתוך<name>.git. במקור של git v2.43.0 (builtin/clone.c),--mirrorמדליקoption_bare, תיקיית היעד היא ה-git dir עצמו, ו-remove_junkמוחק בכשל בדיוק אותה. זה נמדד גם עם strace עלgit clone --mirror: כל קובץ נעילה או קובץ זמני (config.lock,packed-refs.new) נוצר בתוך התיקייה, ושום רשומה לא נוצרת לידה.retry_cleanupהוא אותה תיקייה, ו-fetch_updatesרץ בתוכה. בשירות ה-MCPinit_mirrorנקרא רק מ-refresh_once, וזה נבדק ב-grep.codekeeper_docs_get_sectionקורא מהמראות האלה. ריפו תיעוד שיוסר מ-repo_metadataיחזיר מעכשיוrepo_not_mirrored, במקום להגיש עותק ישן שאיש לא מעדכן._safe_rmtreeעושה resolve לפני המחיקה, ולכן מחיקה שלalias.git → alpha.gitהייתה מוחקת אתalpha.gitומשאירה את הקישור.prunedרק אחרי ש-mirror_existsמראה שהיא איננה. ההצלחה ש-delete_mirrorמדווחת היא דיווח, לא מצב (K11).repo autosync passעברה מ-_loopאלrefresh_once. כך אפשר לבדוק בטסט שהיא נכתבת כש-prunedהוא הדבר היחיד שקרה.list_mirror_names/list_managed_mirror_names) — לא היה במפרט, ונובע מ-R6 ומהריוויו.🔁 סבב ריוויו (cubic) — מה תוקן
list_managed_mirror_namesבשירות, ו-Protocol (LocalMirrors) לתלות.globבולעPermissionError: המנייה עברה ל-os.scandir, ותיקייה שאי אפשר לקרוא היא עכשיו כשל ולא רשימה ריקה. זה חל גם על ה-sweep.🧪 בדיקות
tests/test_mcp_repo_autosync.py, מול מראות bare אמיתיות ב-tmp_pathו-GitMirrorServiceאמיתי. ההנחות נבדקות על הדיסק עצמו. המקרים:repo_metadataמבקשתReadPreference.PRIMARY.is_refreshingלא נמחק.delete_mirror, בערך החזרה או בחריגה, מגדיל אתerrors, והמעבר ממשיך.os.scandirהאמיתי של המנייה): שום דבר לא נמחק, ו-errorsגדל.tests/test_git_mirror_service.py: שתי הרשימות, ותיקייה חסרה או לא קריאה שזורקות.tests/test_git_mirror_credentials.py: ה-sweep על תיקייה לא קריאה זורק, ולא כותב שורתchecked=._Mirrorהושלמה בלי להרחיב שוםexcept. ה-except סביב המנייה תופס רקOSError, ולכן דמות חסרה נופלת בקול (T3).git clone --mirrorב-strace, והרצה אחת של הטסטים הקיימים, שהראתה את הדמות נופלת בקול כמו שציפיתי.🚀 אימות אחרי פריסה
repo autosync passעם'pruned': 1, ושורהpruned the local mirror of codekeeper-plugin.checked=4. בעלייה הראשונה הוא עדיין יראה 5, כי הוא רץ לפני המעבר הראשון.codekeeper_list_repo_treeעלcodekeeper-pluginמחזירrepo_not_mirrored.🧪 בדיקות נדרשות ב‑PR
📝 סוג שינוי
📚 עיון בתיעוד (CodeBot – Project Docs)
כן. עיינתי ב:
AI-MAP.mddocs/mcp-server.rst: "דפדפן הריפו — איך זה עובד" כולו, "הפעלה — צעד אחר צעד", וטבלת התקלות.docs/testing.rst,docs/doc-authoring.rst,docs/versioning-stable-anchors.rst,docs/style-glossary.rst.docs/environment-variables.rst:MCP_REPO_AUTOSYNCו-REPO_MIRROR_PATH.docs/whats-new.rst.docs/dev/sticky_notes_extending.rst: העיקרון "שאילתה שנכשלה אינה ריק".ב-amir-bug-patterns: K11, K13, K16, U1, U3, R6,
state-record-without-state-change,return-value-failure-unchecked,write-from-cached-read,race-toctou,external-input-isinstance,prose-restates-code-fact,line-number-coupling,secret-in-derived-text,silent-fallback-to-worse-path,side-effect-riding-on-log-line,TESTING-PATTERNS.md,claude-md-snippets/testing.md,widened-exception-scope, ו-BY-STACK/cron-jobs.mdדפוס 10.✅ צ'קליסט
MCP_REPO_AUTOSYNCהורחבdelete_mirror(_safe_rmtree, מוגבל ל-base_path)whats-new) עודכן🤖 Generated with Claude Code
https://claude.ai/code/session_016LWehkLW2ZDpXQuXuDqh5X
Generated by Claude Code
<source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg">Summary by Sourcery
Prune stale MCP repository mirrors during autosync so the service’s local disk stays consistent with repository metadata.
Bug Fixes:
Enhancements:
Documentation:
Tests: