feat(editor): add GitHub-based skill sync window with safe incremental mirroring#845
Open
BaronCyrus wants to merge 6 commits intoCoplayDev:betafrom
Open
feat(editor): add GitHub-based skill sync window with safe incremental mirroring#845BaronCyrus wants to merge 6 commits intoCoplayDev:betafrom
BaronCyrus wants to merge 6 commits intoCoplayDev:betafrom
Conversation
Add an EditorWindow (McpForUnitySkillInstaller) that syncs the unity-mcp skill from a GitHub repository without cloning. The tool reads the repo tree via the GitHub API, computes git blob SHA-1s to build an incremental sync plan (added/updated/deleted), downloads raw files for changed items, validates hashes, and writes a last-synced commit to EditorPrefs. UI supports branch and CLI (codex/claude) selection, configurable install path, logging, and safety checks (aborts on truncated trees). Also add the corresponding .meta file.
Contributor
Reviewer's GuideAdds a new Unity EditorWindow that incrementally syncs the Sequence diagram for GitHub-based incremental skill syncsequenceDiagram
actor Developer
participant UnityEditor as UnityEditor
participant Installer as McpForUnitySkillInstaller
participant GitHubAPI as GitHub_API
participant RawContent as Raw_GitHub_Content
participant FS as FileSystem
participant Prefs as EditorPrefs
Developer->>UnityEditor: Open Window/MCP For Unity/Install(Sync) MCP Skill
UnityEditor->>Installer: Create window instance
Installer->>Prefs: Load RepoUrl, Branch, Cli, InstallDir
Developer->>Installer: Click Sync Latest
Installer->>Installer: ExecuteWithGuard(action)
Installer->>Installer: RunSyncLatest()
Installer->>Installer: TryParseGitHubRepository(_repoUrl)
Installer->>GitHubAPI: GET /repos/{owner}/{repo}/git/trees/{branch}?recursive=1
GitHubAPI-->>Installer: GitHubTreeResponse
Installer->>Installer: Check truncated flag
Installer->>Installer: Build RemoteSnapshot(commitSha, files)
Installer->>FS: ListFiles(installPath)
FS-->>Installer: local files map
Installer->>Installer: BuildPlan(remoteFiles, localFiles)
Installer->>RawContent: DownloadBytes() for Added/Updated files
RawContent-->>Installer: file bytes
Installer->>FS: WriteAllBytes(targetFile)
loop For each Deleted file
Installer->>FS: Delete(targetFile)
end
Installer->>FS: RemoveEmptyDirectories(installRoot)
loop Validate each remote file
Installer->>FS: ReadAllBytes(localFile)
FS-->>Installer: bytes
Installer->>Installer: ComputeGitBlobSha1(bytes)
end
Installer->>Prefs: SetString(LastSyncedCommitKeyScope, commitSha)
Installer-->>Developer: Display log summary in window
Class diagram for McpForUnitySkillInstaller and related typesclassDiagram
class McpForUnitySkillInstaller {
-string RepoUrlKey
-string BranchKey
-string CliKey
-string InstallDirKey
-string LastSyncedCommitKey
-string FixedSkillSubdir
-string CodexCli
-string ClaudeCli
-string[] BranchOptions
-string[] CliOptions
-string _repoUrl
-string _targetBranch
-string _cliType
-string _installDir
-Vector2 _scroll
-bool _isRunning
-ConcurrentQueue~Action~ _mainThreadActions
-ConcurrentQueue~string~ _pendingLogs
-StringBuilder _logBuilder
+OpenWindow()
+OnEnable()
+OnDisable()
+OnGUI()
-OnEditorUpdate()
-RunSyncLatest()
-ExecuteWithGuard(action)
-GetLastSyncedCommitKey() string
-FetchRemoteSnapshot(repoInfo, branch, subdir) RemoteSnapshot
-ApplyPlan(repoInfo, commitSha, remoteSubdir, targetRoot, plan)
-ValidateFileHashes(installRoot, remoteFiles)
-GetInstallPath() string
-TryApplyCliDefaultInstallPath(previousCli, currentCli)
-GetDefaultInstallDir(userHome, cliType) string
-IsClaudeCli(cliType) bool
-PathsEqual(left, right) bool
-ExpandPath(path) string
-EnqueueMainThreadAction(action)
-ExecuteMainThreadActions()
-AppendLine(line)
-AppendLineImmediate(line)
-FlushPendingLogs() bool
-SanitizeLogLine(line) string
-LogPlanDetails(plan)
-AppendSummary(plan, commitChanged)
-ShortCommit(commit) string
-ShortHash(hash) string
-CreateGitHubClient() HttpClient
-BuildTreeApiUrl(repoInfo, branch) string
-BuildRawFileUrl(repoInfo, commitSha, remoteFilePath) string
-DownloadString(client, url) string
-DownloadBytes(client, url) byte[]
-NormalizeRemotePath(path) string
-CombineRemotePath(left, right) string
-BuildPlan(remoteFiles, localFiles) SyncPlan
-ComputeGitBlobSha1(filePath) string
-ComputeGitBlobSha1(bytes) string
-ListFiles(root) Dictionary~string,string~
-RemoveEmptyDirectories(root)
-TryParseGitHubRepository(url) bool
-TryParseOwnerAndRepo(path) bool
}
class GitHubRepoInfo {
+string Owner
+string Repo
+GitHubRepoInfo(owner, repo)
}
class RemoteSnapshot {
+string CommitSha
+string SubdirPath
+Dictionary~string,string~ Files
+RemoteSnapshot(commitSha, subdirPath, files)
}
class GitHubTreeResponse {
+string sha
+GitHubTreeEntry[] tree
+bool truncated
}
class GitHubTreeEntry {
+string path
+string type
+string sha
}
class SyncPlan {
+List~string~ Added
+List~string~ Updated
+List~string~ Deleted
}
McpForUnitySkillInstaller *-- GitHubRepoInfo
McpForUnitySkillInstaller *-- RemoteSnapshot
McpForUnitySkillInstaller *-- GitHubTreeResponse
GitHubTreeResponse *-- GitHubTreeEntry
McpForUnitySkillInstaller *-- SyncPlan
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Unity Editor skill installer and a Git-backed SkillSyncService; exposes configurator APIs for client skill support and wires an "Install Skills" UI button that syncs a GitHub subtree into a local install path with plan computation, file apply, and verification. Changes
Sequence DiagramsequenceDiagram
participant Editor as Unity Editor
participant UI as Installer UI
participant GitHub as GitHub API
participant FS as Local FileSystem
participant Prefs as EditorPrefs
Editor->>UI: OpenWindow()
UI->>Prefs: Load saved config (repo, branch, cli, dir)
UI->>GitHub: Request git/trees?recursive=1 (repo, branch)
GitHub-->>UI: Return tree entries + SHAs
UI->>FS: Read local install dir and marker files
UI->>UI: Compute SyncPlan (Added/Updated/Deleted)
UI->>Editor: Display plan / start sync
UI->>GitHub: Fetch raw blobs for Added/Updated files
GitHub-->>UI: Return file contents
UI->>FS: Write/update files, delete removed files, cleanup dirs
FS-->>UI: Confirm writes/deletes
UI->>UI: Verify Git blob SHA1s match remote SHAs
UI->>Prefs: Persist last-run metadata & settings
UI-->>Editor: Log results and return SyncResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The window class is quite large and mixes UI, GitHub API, planning, and filesystem concerns; consider extracting the GitHub client/snapshot logic and the sync planner/applicator into separate classes to simplify the EditorWindow and make behavior easier to reason about.
- Each sync run creates and disposes multiple HttpClient instances and uses
GetAwaiter().GetResult(); consider reusing a single HttpClient instance and/or switching to async I/O to avoid connection churn and potential thread pool blocking.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The window class is quite large and mixes UI, GitHub API, planning, and filesystem concerns; consider extracting the GitHub client/snapshot logic and the sync planner/applicator into separate classes to simplify the EditorWindow and make behavior easier to reason about.
- Each sync run creates and disposes multiple HttpClient instances and uses `GetAwaiter().GetResult()`; consider reusing a single HttpClient instance and/or switching to async I/O to avoid connection churn and potential thread pool blocking.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49901a3b9a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@MCPForUnity/Editor/Setup/McpForUnitySkillInstaller.cs`:
- Line 102: The installer currently accepts any path via the _installDir field
and later deletes files in that directory; add safeguards in
McpForUnitySkillInstaller around path selection and deletion: validate
_installDir to ensure it is within the Unity project (e.g., under
Application.dataPath) or contains a marker file (like ".mcp_managed") before
performing any destructive sync in the method that performs deletions (the
sync/install routine that iterates files at lines ~435-475), present a blocking
EditorUtility.DisplayDialog confirmation requiring the user to type the
directory name for destructive actions, and refuse to run the deletion code if
the ownership check or validation fails (log an error and abort) so only
managed, explicitly confirmed folders are modified; also apply the same
validation to any other UI setters that assign _installDir (the TextField and
any code paths around lines ~163-175).
- Around line 303-323: The code currently accepts remotePath from
NormalizeRemotePath and builds relativePath which is later combined with install
root; add strict path traversal containment checks before adding to remoteFiles
and before any Path.Combine I/O: ensure relativePath is not rooted
(Path.IsPathRooted), split the path on Path.DirectorySeparatorChar and
Path.AltDirectorySeparatorChar and reject if any segment equals ".." or empty
segments that imply traversal, and after resolving the final path use
Path.GetFullPath(combinedPath) and verify it starts with the normalized install
root full path (installRootFullPath + Path.DirectorySeparatorChar) to guarantee
containment; if any check fails, skip the entry and log or ignore it.
- Around line 331-338: The code is using treeResponse.sha (the tree object SHA)
as the commit SHA; instead, retrieve the branch head commit SHA before
requesting the tree and propagate that commit SHA into RemoteSnapshot and
BuildRawFileUrl. Concretely: in McpForUnitySkillInstaller where you call the
Tree API, first fetch the branch reference (the branch head commit SHA from the
branch ref/commit object) and store it (e.g., branchCommitSha), then replace
uses of treeResponse.sha with that branchCommitSha when constructing the
RemoteSnapshot and when calling BuildRawFileUrl so raw.githubusercontent.com
receives a valid commit-ish identifier.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
MCPForUnity/Editor/Setup/McpForUnitySkillInstaller.csMCPForUnity/Editor/Setup/McpForUnitySkillInstaller.cs.meta
- Resolve branch head commit SHA via GitHub branches API and use it for snapshot/raw download refs - Reject unsafe remote relative paths and enforce install-root containment for all file IO - Handle case-only renames on case-insensitive filesystems during sync planning - Add managed install-root marker guard to avoid destructive deletes in unmanaged directories
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@MCPForUnity/Editor/Setup/McpForUnitySkillInstaller.cs`:
- Around line 72-104: OnGUI/RunSyncLatest currently capture mutable fields
(_repoUrl, _targetBranch, _installDir) into a background job while the UI
remains editable (_isRunning gating is incomplete); fix by snapshotting those
values into local readonly variables (e.g., var repoUrl = _repoUrl; var
targetBranch = _targetBranch; var installDir = _installDir;) immediately before
queuing the background action in RunSyncLatest and use those locals inside the
queued lambda instead of the fields; ensure the check/setting of _isRunning
still happens on the main thread and that TryApplyCliDefaultInstallPath and
other UI state changes are not invoked from the background task.
- Around line 157-162: Add explicit validation of the install path returned by
GetInstallPath before any filesystem calls: check that installPath is not
null/empty and is a valid path (e.g., not whitespace and does not contain
invalid path chars) and if invalid throw or log a clear error and return early
instead of calling Directory.CreateDirectory(installPath); update both places
where this pattern appears (the block using GetInstallPath around the
CreateDirectory call and the similar code at the other occurrence) to fail fast
with a descriptive message referencing GetInstallPath/installPath.
- Snapshot repo/branch/installDir before starting background sync task - Disable config inputs while sync is running to avoid mid-run mutations - Validate install directory explicitly with clear errors before filesystem operations - Scope last-synced-commit key derivation to captured repo/branch values
Author
|
Thanks everyone for the reviews. All actionable issues from this round have been fixed and pushed to
Current status:
Note:
|
1. Incorporate the Config into a simple one-click in Client Configuration. 2. Simplify the logs to only 1 entry, convenient for LLM checking.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
MCPForUnity/Editor/Setup/SkillSyncService.cs (1)
280-289: Consider noting HttpClient reuse as a follow-up.
HttpClientis designed to be reused rather than created per-request. Creating multiple instances can lead to socket exhaustion under heavy use. Per PR comments, this is acknowledged for a follow-up PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Setup/SkillSyncService.cs` around lines 280 - 289, CreateGitHubClient currently instantiates a new HttpClient per call which can cause socket exhaustion; change it to return a single reused instance by introducing a static readonly HttpClient (e.g., _githubClient) initialized once with the same Timeout, UserAgent and Accept headers and have CreateGitHubClient return that instance; update any callers to continue using CreateGitHubClient and add a comment/TODO indicating this reuse is intentional and was moved here as the follow-up noted in the PR.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@MCPForUnity/Editor/Setup/SkillSyncService.cs`:
- Around line 41-62: The log callback passed into RunSync is invoked from a
background Task.Run thread which can call Unity APIs unsafely; wrap or replace
the passed-in log delegate so all log invocations are marshalled to the main
thread via EditorApplication.delayCall before calling the original callback.
Specifically, before calling RunSync in the Task.Run block, create a thread-safe
wrapper (e.g., mainThreadLog) that captures the original log and invokes
EditorApplication.delayCall += () => originalLog(message) for each call, then
pass that wrapper into RunSync; keep using EditorApplication.delayCall for the
final onComplete and lastSyncedCommit update as already done.
- Around line 436-465: ApplyPlan can incorrectly delete files on
case-insensitive filesystems when a rename only changes case because the code
writes Added entries then deletes Deleted entries; detect case-only renames by
comparing entries from plan.Added and plan.Deleted using
StringComparison.OrdinalIgnoreCase but not exact equality, resolve their target
paths via ResolvePathUnderRoot, and perform the deletion of the old-case file(s)
before writing the new file(s). Update ApplyPlan to (1) build a set of
case-only-deletes (using ResolvePathUnderRoot to get exact file paths) and
delete those files up front, (2) then proceed with downloading/writing for
plan.Added/Updated using DownloadBytes/CombineRemotePath/BuildRawFileUrl as
before, and (3) keep the remaining Deleted processing and call
RemoveEmptyDirectories at the end.
---
Nitpick comments:
In `@MCPForUnity/Editor/Setup/SkillSyncService.cs`:
- Around line 280-289: CreateGitHubClient currently instantiates a new
HttpClient per call which can cause socket exhaustion; change it to return a
single reused instance by introducing a static readonly HttpClient (e.g.,
_githubClient) initialized once with the same Timeout, UserAgent and Accept
headers and have CreateGitHubClient return that instance; update any callers to
continue using CreateGitHubClient and add a comment/TODO indicating this reuse
is intentional and was moved here as the follow-up noted in the PR.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
MCPForUnity/Editor/Clients/Configurators/ClaudeCodeConfigurator.csMCPForUnity/Editor/Clients/Configurators/ClaudeDesktopConfigurator.csMCPForUnity/Editor/Clients/Configurators/CodexConfigurator.csMCPForUnity/Editor/Clients/IMcpClientConfigurator.csMCPForUnity/Editor/Clients/McpClientConfiguratorBase.csMCPForUnity/Editor/Setup/McpForUnitySkillInstaller.csMCPForUnity/Editor/Setup/SkillSyncService.csMCPForUnity/Editor/Setup/SkillSyncService.cs.metaMCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.csMCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.uxml
🚧 Files skipped from review as they are similar to previous changes (1)
- MCPForUnity/Editor/Setup/McpForUnitySkillInstaller.cs
Comment on lines
+41
to
+62
| Task.Run(() => | ||
| { | ||
| try | ||
| { | ||
| var result = RunSync(repoUrl, installDir, branch, lastSyncedCommit, log); | ||
| EditorApplication.delayCall += () => | ||
| { | ||
| if (result.Success && !string.IsNullOrEmpty(result.CommitSha)) | ||
| { | ||
| EditorPrefs.SetString(lastSyncedCommitKey, result.CommitSha); | ||
| } | ||
| onComplete?.Invoke(result); | ||
| }; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| EditorApplication.delayCall += () => | ||
| { | ||
| onComplete?.Invoke(new SyncResult { Success = false, Error = ex.Message }); | ||
| }; | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
Log callback may be invoked from background thread.
The log callback is passed to RunSync which executes on Task.Run. If the caller's log implementation interacts with Unity APIs (e.g., updating UI elements, calling Debug.Log), this will cause threading issues since Unity APIs must be called from the main thread.
Consider marshalling log invocations back to the main thread, or document that the callback must be thread-safe.
🛠️ Proposed fix: Marshal log calls to main thread
- public static void SyncAsync(string repoUrl, string installDir, string branch, Action<string> log, Action<SyncResult> onComplete)
+ public static void SyncAsync(string repoUrl, string installDir, string branch, Action<string> log, Action<SyncResult> onComplete)
{
var lastSyncedCommitKey = GetLastSyncedCommitKey(repoUrl, branch);
var lastSyncedCommit = EditorPrefs.GetString(lastSyncedCommitKey, string.Empty);
+ // Thread-safe log wrapper that marshals to main thread
+ Action<string> threadSafeLog = message =>
+ {
+ if (log != null)
+ {
+ EditorApplication.delayCall += () => log(message);
+ }
+ };
+
Task.Run(() =>
{
try
{
- var result = RunSync(repoUrl, installDir, branch, lastSyncedCommit, log);
+ var result = RunSync(repoUrl, installDir, branch, lastSyncedCommit, threadSafeLog);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Task.Run(() => | |
| { | |
| try | |
| { | |
| var result = RunSync(repoUrl, installDir, branch, lastSyncedCommit, log); | |
| EditorApplication.delayCall += () => | |
| { | |
| if (result.Success && !string.IsNullOrEmpty(result.CommitSha)) | |
| { | |
| EditorPrefs.SetString(lastSyncedCommitKey, result.CommitSha); | |
| } | |
| onComplete?.Invoke(result); | |
| }; | |
| } | |
| catch (Exception ex) | |
| { | |
| EditorApplication.delayCall += () => | |
| { | |
| onComplete?.Invoke(new SyncResult { Success = false, Error = ex.Message }); | |
| }; | |
| } | |
| }); | |
| public static void SyncAsync(string repoUrl, string installDir, string branch, Action<string> log, Action<SyncResult> onComplete) | |
| { | |
| var lastSyncedCommitKey = GetLastSyncedCommitKey(repoUrl, branch); | |
| var lastSyncedCommit = EditorPrefs.GetString(lastSyncedCommitKey, string.Empty); | |
| // Thread-safe log wrapper that marshals to main thread | |
| Action<string> threadSafeLog = message => | |
| { | |
| if (log != null) | |
| { | |
| EditorApplication.delayCall += () => log(message); | |
| } | |
| }; | |
| Task.Run(() => | |
| { | |
| try | |
| { | |
| var result = RunSync(repoUrl, installDir, branch, lastSyncedCommit, threadSafeLog); | |
| EditorApplication.delayCall += () => | |
| { | |
| if (result.Success && !string.IsNullOrEmpty(result.CommitSha)) | |
| { | |
| EditorPrefs.SetString(lastSyncedCommitKey, result.CommitSha); | |
| } | |
| onComplete?.Invoke(result); | |
| }; | |
| } | |
| catch (Exception ex) | |
| { | |
| EditorApplication.delayCall += () => | |
| { | |
| onComplete?.Invoke(new SyncResult { Success = false, Error = ex.Message }); | |
| }; | |
| } | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Setup/SkillSyncService.cs` around lines 41 - 62, The log
callback passed into RunSync is invoked from a background Task.Run thread which
can call Unity APIs unsafely; wrap or replace the passed-in log delegate so all
log invocations are marshalled to the main thread via
EditorApplication.delayCall before calling the original callback. Specifically,
before calling RunSync in the Task.Run block, create a thread-safe wrapper
(e.g., mainThreadLog) that captures the original log and invokes
EditorApplication.delayCall += () => originalLog(message) for each call, then
pass that wrapper into RunSync; keep using EditorApplication.delayCall for the
final onComplete and lastSyncedCommit update as already done.
Comment on lines
+436
to
+465
| private static void ApplyPlan(GitHubRepoInfo repoInfo, string commitSha, string remoteSubdir, string targetRoot, SyncPlan plan, StringComparison pathComparison, Action<string> log) | ||
| { | ||
| using var client = CreateGitHubClient(); | ||
| foreach (var relativePath in plan.Added.Concat(plan.Updated)) | ||
| { | ||
| var remoteFilePath = CombineRemotePath(remoteSubdir, relativePath); | ||
| var downloadUrl = BuildRawFileUrl(repoInfo, commitSha, remoteFilePath); | ||
| var targetFile = ResolvePathUnderRoot(targetRoot, relativePath, pathComparison); | ||
| var targetDirectory = Path.GetDirectoryName(targetFile); | ||
| if (!string.IsNullOrEmpty(targetDirectory)) | ||
| { | ||
| Directory.CreateDirectory(targetDirectory); | ||
| } | ||
|
|
||
| log?.Invoke($"Download: {relativePath}"); | ||
| var bytes = DownloadBytes(client, downloadUrl); | ||
| File.WriteAllBytes(targetFile, bytes); | ||
| } | ||
|
|
||
| foreach (var relativePath in plan.Deleted) | ||
| { | ||
| var targetFile = ResolvePathUnderRoot(targetRoot, relativePath, pathComparison); | ||
| if (File.Exists(targetFile)) | ||
| { | ||
| File.Delete(targetFile); | ||
| } | ||
| } | ||
|
|
||
| RemoveEmptyDirectories(targetRoot); | ||
| } |
Contributor
There was a problem hiding this comment.
Case-only rename may delete the file on case-insensitive filesystems.
When a file is renamed with only case changes (e.g., File.txt → file.txt), the plan will have the new name in Added and the old name in Deleted. The current operation order (add/update first, then delete) will:
- Write
file.txt - Delete
File.txt— which on case-insensitive FS deletes the file just written
This results in data loss.
🐛 Proposed fix: Handle case-only renames explicitly
private static void ApplyPlan(GitHubRepoInfo repoInfo, string commitSha, string remoteSubdir, string targetRoot, SyncPlan plan, StringComparison pathComparison, Action<string> log)
{
using var client = CreateGitHubClient();
+
+ // On case-insensitive FS, detect case-only renames to avoid delete clobbering add
+ var caseOnlyRenames = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
+ if (pathComparison == StringComparison.OrdinalIgnoreCase)
+ {
+ var addedLower = new HashSet<string>(plan.Added, StringComparer.OrdinalIgnoreCase);
+ foreach (var deleted in plan.Deleted)
+ {
+ if (addedLower.Contains(deleted))
+ {
+ caseOnlyRenames.Add(deleted);
+ }
+ }
+ }
+
foreach (var relativePath in plan.Added.Concat(plan.Updated))
{
var remoteFilePath = CombineRemotePath(remoteSubdir, relativePath);
var downloadUrl = BuildRawFileUrl(repoInfo, commitSha, remoteFilePath);
var targetFile = ResolvePathUnderRoot(targetRoot, relativePath, pathComparison);
var targetDirectory = Path.GetDirectoryName(targetFile);
if (!string.IsNullOrEmpty(targetDirectory))
{
Directory.CreateDirectory(targetDirectory);
}
log?.Invoke($"Download: {relativePath}");
var bytes = DownloadBytes(client, downloadUrl);
File.WriteAllBytes(targetFile, bytes);
}
foreach (var relativePath in plan.Deleted)
{
+ // Skip deletion if this is a case-only rename (file already overwritten)
+ if (caseOnlyRenames.Contains(relativePath))
+ {
+ continue;
+ }
+
var targetFile = ResolvePathUnderRoot(targetRoot, relativePath, pathComparison);
if (File.Exists(targetFile))
{
File.Delete(targetFile);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Setup/SkillSyncService.cs` around lines 436 - 465,
ApplyPlan can incorrectly delete files on case-insensitive filesystems when a
rename only changes case because the code writes Added entries then deletes
Deleted entries; detect case-only renames by comparing entries from plan.Added
and plan.Deleted using StringComparison.OrdinalIgnoreCase but not exact
equality, resolve their target paths via ResolvePathUnderRoot, and perform the
deletion of the old-case file(s) before writing the new file(s). Update
ApplyPlan to (1) build a set of case-only-deletes (using ResolvePathUnderRoot to
get exact file paths) and delete those files up front, (2) then proceed with
downloading/writing for plan.Added/Updated using
DownloadBytes/CombineRemotePath/BuildRawFileUrl as before, and (3) keep the
remaining Deleted processing and call RemoveEmptyDirectories at the end.
Collaborator
|
Hi, thanks for this PR! I was like "Oh I missed this spot" when I saw this PR, since I am constantly writing and updating the Skills but forgot how the community is having trouble using it directly. Thanks for reminding me. |
Author
that's nice thanks for the change |
Author
which beta version will release this feature ? |
Collaborator
|
Once we merge the PR, it should be in the next version (V9.4.8-beta.12 or 13). Will merge it later today! |
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.
Description
This PR adds a new Unity
EditorWindow(McpForUnitySkillInstaller) that syncs theunity-mcpskill directly from a GitHub repository without cloning.The installer:
Added/Updated/Deleted) by comparing remote blob SHAs with local git blob SHA-1 values.raw.githubusercontent.com.EditorPrefs.codex/claude), configurable install path, and detailed log output.A corresponding
.metafile was also added for the new script asset.Type of Change
Changes Made
McpForUnitySkillInstallerEditorWindow implementation..metaforMcpForUnitySkillInstaller.cs.recursive=1) for remote snapshot creation.EditorPrefspersistence for last synced commit (scoped keying).Testing/Screenshots/Recordings
Manual validation performed:
record.2026-02-28.16.02.05.mov
Related Issues
Relates to review feedback about handling truncated GitHub tree responses safely.
Additional Notes
Summary by Sourcery
Add a Unity Editor window to sync the unity-mcp skill from a GitHub repo via incremental, hash-validated mirroring without cloning.
New Features:
Bug Fixes:
Enhancements:
Summary by CodeRabbit