feat(sandbox): sync skill trees asynchronously#585
Conversation
![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: 10a07943d6
ℹ️ 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".
| throw new HttpException(`containerPath is required for SandboxCopyTreeCommand`, 400) | ||
| } | ||
|
|
||
| const versionCacheKey = this.getVersionCacheKey(backend.id) |
There was a problem hiding this comment.
SandboxCopyTreeHandler keys sync state only by backend.id (getVersionCacheKey(backend.id)), but this change now permits distinct sandbox instances for the same provider/workingDirectory when workspaceBinding differs (SandboxAcquireBackendHandler.getInstanceKey). Providers can reuse the same backend id for those instances (e.g., ids derived from workingDirectory), so a successful sync for binding A can make binding B incorrectly return skipped for the same containerPath/version, leaving the second workspace unsynced. Include workspace identity (such as volumeRoot/bindSource) in the cache key to avoid cross-binding cache collisions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR introduces an asynchronous “skill tree” sync pipeline for sandboxes by adding a directory-tree copy command/handler (with version-based skipping and timing logs) and updating the skills middleware to schedule syncs in the background while ensuring read_skill_file waits for the relevant in-flight sync. It also threads workspaceBinding through the acquired sandbox context to enable direct copies into locally mapped workspaces, falling back to uploadFiles for other providers.
Changes:
- Added
SandboxCopyTreeCommand+SandboxCopyTreeHandlerto copy a directory tree into a sandbox (direct local copy when workspace is mapped; otherwise upload) with version-based skip caching and timing metrics. - Switched
SkillsMiddlewareskill package syncing from awaited/serial to async scheduling with in-flight reuse, and added waiting logic inread_skill_file. - Extended sandbox acquisition to return
workspaceBindingin the sandbox configurable and to differentiate backend instances by workspace binding.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/server-ai/src/skill-package/plugins/skills-middleware/index.ts | Schedules skill syncs asynchronously, reuses in-flight sync promises, and waits on sync before read_skill_file. |
| packages/server-ai/src/skill-package/plugins/skills-middleware/index.spec.ts | Updates mocks/assertions and adds tests for non-blocking scheduling, in-flight reuse, and read-time waiting. |
| packages/server-ai/src/sandbox/commands/sandbox.copy-tree.command.ts | Introduces the CQRS command contract for directory-tree copying with mode/status metadata. |
| packages/server-ai/src/sandbox/commands/index.ts | Exports the new sandbox.copy-tree command. |
| packages/server-ai/src/sandbox/commands/handlers/sandbox.copy-tree.handler.ts | Implements tree copy with local-mapped fast path, upload fallback, version skip caching, and timing logs. |
| packages/server-ai/src/sandbox/commands/handlers/sandbox.copy-tree.handler.spec.ts | Adds coverage for local-mapped copying, upload fallback, out-of-scope targets, empty trees, version skipping, and invalid paths. |
| packages/server-ai/src/sandbox/commands/handlers/index.ts | Registers the new SandboxCopyTreeHandler. |
| packages/server-ai/src/sandbox/commands/handlers/acquire-backend.handler.ts | Threads workspaceBinding through sandbox configurable and includes it in backend instance caching. |
| packages/server-ai/src/sandbox/commands/handlers/acquire-backend.handler.spec.ts | Updates existing expectations and adds workspace-binding-specific backend reuse tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| workingDirectory?: string | null, | ||
| workspaceBinding?: SandboxProviderCreateOptions['workspaceBinding'] | ||
| ) { | ||
| const workspaceIdentity = workspaceBinding?.volumeRoot ?? workspaceBinding?.bindSource ?? '' |
| const walk = async (currentDir: string) => { | ||
| const entries = await fs.promises.readdir(currentDir, { withFileTypes: true }) | ||
| for (const entry of entries) { | ||
| const fullPath = path.join(currentDir, entry.name) | ||
| const stats = await fs.promises.stat(fullPath) | ||
| if (stats.isDirectory()) { | ||
| await walk(fullPath) | ||
| continue | ||
| } | ||
| if (!stats.isFile()) { | ||
| continue | ||
| } | ||
|
|
||
| const relativePath = path.relative(baseDir, fullPath) | ||
| const destinationPath = path.posix.join(containerPath, this.toPosixPath(relativePath)) | ||
| const content = await fs.promises.readFile(fullPath) | ||
| totalBytes += content.byteLength | ||
| files.push([destinationPath, new Uint8Array(content)]) | ||
| } |
| private async copyTreeToLocalPath(localPath: string, targetPath: string, overwrite: boolean): Promise<void> { | ||
| await fs.promises.mkdir(path.dirname(targetPath), { recursive: true }) | ||
| if (overwrite) { | ||
| await fs.promises.rm(targetPath, { recursive: true, force: true }) | ||
| } | ||
| await fs.promises.cp(localPath, targetPath, { | ||
| recursive: true, | ||
| force: true, | ||
| dereference: true | ||
| }) |
10a0794 to
20e137e
Compare
Summary
Tests