feat(sync): implement skills sync upload and apply commands with support for ignore patterns#144
Merged
Conversation
…ort for ignore patterns
Summary by CodeRabbit
WalkthroughThis pull request introduces a skills synchronization command ( Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CLI Client
participant Filesystem as Local Filesystem
participant SyncService as Sync Service
User->>CLI: oo skills sync upload
activate CLI
CLI->>Filesystem: Read canonical & host registry skills
Filesystem-->>CLI: Skill files/metadata
CLI->>CLI: Filter by ignore patterns\nDeduplicate records
CLI->>SyncService: PUT /sync (skill records)
activate SyncService
SyncService-->>CLI: 200 OK (success)
deactivate SyncService
CLI->>User: Print success message
deactivate CLI
sequenceDiagram
actor User
participant CLI as CLI Client
participant SyncService as Sync Service
participant Filesystem as Local Filesystem
User->>CLI: oo skills sync apply
activate CLI
CLI->>SyncService: GET /sync (request records)
activate SyncService
SyncService-->>CLI: 200 OK (skill records JSON)
deactivate SyncService
CLI->>CLI: Parse and filter records
CLI->>Filesystem: Install skills to local directories
Filesystem-->>CLI: Installation results
CLI->>User: Print success or no-results message
deactivate CLI
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/application/commands/skills/sync.ts (1)
150-161: ⚖️ Poor tradeoffConsider parallelizing independent skill installations.
The
for...ofloop installs records sequentially. If installations are independent,Promise.all()would improve performance. However, sequential execution may be intentional for rate limiting or consistent output ordering.🤖 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 `@src/application/commands/skills/sync.ts` around lines 150 - 161, The loop in sync.ts iterates records sequentially calling installRegistrySkills for each record, causing slow installs; replace the for...of with parallel execution by mapping records to promises that call installRegistrySkills(record, context) and await Promise.all(...) so independent installs run concurrently, or if rate-limiting/ordering is required use a bounded concurrency utility (e.g., p-map) to limit simultaneous calls while still improving throughput; ensure you reference the existing installRegistrySkills function, the records array, and the context parameter when implementing the change.
🤖 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 `@src/application/commands/skills/sync.ts`:
- Around line 171-173: The createSkillSyncRequestUrl function builds URLs with
the wrong host prefix; change the host from "cli-api" to "api" so it returns new
URL(`https://api.${endpoint}/v1/skills`) instead of using "cli-api" — update the
createSkillSyncRequestUrl function accordingly (tests referencing sync.test.ts
and index.test.ts expect the "api" prefix).
---
Nitpick comments:
In `@src/application/commands/skills/sync.ts`:
- Around line 150-161: The loop in sync.ts iterates records sequentially calling
installRegistrySkills for each record, causing slow installs; replace the
for...of with parallel execution by mapping records to promises that call
installRegistrySkills(record, context) and await Promise.all(...) so independent
installs run concurrently, or if rate-limiting/ordering is required use a
bounded concurrency utility (e.g., p-map) to limit simultaneous calls while
still improving throughput; ensure you reference the existing
installRegistrySkills function, the records array, and the context parameter
when implementing the change.
🪄 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
Run ID: 49e2d219-739b-45f4-8d08-08654cea46ea
📒 Files selected for processing (11)
docs/commands.mddocs/commands.zh-CN.mdsrc/application/commands/shared/keywords.tssrc/application/commands/shared/list-parsing.tssrc/application/commands/skills/index.test.tssrc/application/commands/skills/index.tssrc/application/commands/skills/registry-skill-install.tssrc/application/commands/skills/registry-skill-source.tssrc/application/commands/skills/sync.test.tssrc/application/commands/skills/sync.tssrc/i18n/catalog.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/application/commands/skills/index.test.ts (1)
1823-1910: ⚡ Quick winExtract repeated sync test setup into a local factory helper.
The request-capture + fetcher scaffolding is duplicated across these new sync tests. A small local helper would reduce drift and make future sync cases easier to add.
♻️ Example extraction
+function createJsonEchoFetcher( + requests: Array<{ + authorization: string | null; + method: string; + url: string; + body: unknown; + }>, +) { + return async (input: RequestInfo | URL, init?: RequestInit) => { + const request = toRequest(input, init); + const body = await request.json(); + requests.push({ + authorization: request.headers.get("Authorization"), + method: request.method, + url: request.url, + body, + }); + return new Response(JSON.stringify(body)); + }; +}As per coding guidelines,
**/*.test.{ts,tsx}: “Extract repeated setup (mocks, stubs, setup objects) into local factory functions in test files; avoid copy-pasting test setup.”Also applies to: 1957-2080
🤖 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 `@src/application/commands/skills/index.test.ts` around lines 1823 - 1910, Several sync upload tests repeat sandbox setup, directory/file creation, auth writing and a fetcher that captures requests; extract that repeated scaffolding into a local factory helper (e.g., makeSyncUploadTest or createSyncTestFixture) inside the test file and call it from the tests. The helper should perform createCliSandbox(), compute codexHomeDirectory, create skill directories (chatSkillDirectoryPath, captionSkillDirectoryPath), writeAuthFile, write managed metadata files via resolveManagedSkillMetadataFilePath and renderSkillMetadataJson, and return the sandbox, paths, and a requests array plus a fetcher function suitable for passing to sandbox.run; then replace the duplicated blocks in the tests (the setup around createCliSandbox, mkdir, Bun.write, requests array and the inline fetcher) with calls to that helper and use the returned fetcher when invoking sandbox.run.
🤖 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.
Nitpick comments:
In `@src/application/commands/skills/index.test.ts`:
- Around line 1823-1910: Several sync upload tests repeat sandbox setup,
directory/file creation, auth writing and a fetcher that captures requests;
extract that repeated scaffolding into a local factory helper (e.g.,
makeSyncUploadTest or createSyncTestFixture) inside the test file and call it
from the tests. The helper should perform createCliSandbox(), compute
codexHomeDirectory, create skill directories (chatSkillDirectoryPath,
captionSkillDirectoryPath), writeAuthFile, write managed metadata files via
resolveManagedSkillMetadataFilePath and renderSkillMetadataJson, and return the
sandbox, paths, and a requests array plus a fetcher function suitable for
passing to sandbox.run; then replace the duplicated blocks in the tests (the
setup around createCliSandbox, mkdir, Bun.write, requests array and the inline
fetcher) with calls to that helper and use the returned fetcher when invoking
sandbox.run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 25394225-0283-4f91-884b-15dd9809c54e
📒 Files selected for processing (2)
src/application/commands/skills/index.test.tssrc/application/commands/skills/sync.test.ts
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
1 participant
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.
Registry-Only Skills Sync
closes #138
Summary
oo skills sync uploadandoo skills sync apply.applyalso supports aliases:downloadandinstall.--source registry, default toregistry, and reject other sources.Key Changes
syncchild underoo skillswith:uploadapplyaliases:download,installuploadscans installed oo-managed registry skills only, thenPUT https://cli-api.<endpoint>/v1/skillswith:apply/download/installreadsGET https://cli-api.<endpoint>/v1/skillsand installs those registry skills into supported local agent directories.-i, --ignore <patterns...>toupload; repeated flags and comma-separated values are accepted, matched via the existingignorepackage againstpackageNameorskillName.User-Facing Behavior
oo skills sync uploadoo skills sync upload --source registryoo skills sync applyoo skills sync downloadoo skills sync installuploadoverwrites the remote registry-skill manifest, including uploading[]when none are installed.Tests
--ignore.apply,download, andinstallaliases all install exact versions from the remote manifest.--sourcedefault and invalid source errors.docs/commands.mdanddocs/commands.zh-CN.md.bun run lint:fix,bun run ts-check, andbun run test.Assumptions
/v1/skillsis served athttps://cli-api.<account.endpoint>/v1/skills.Authorizationheader.applyremains the canonical documented verb;downloadandinstallare aliases for discoverability.