Fix community skill catalog installs#19
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
📝 WalkthroughWalkthroughA new ChangesProvider Skill Installation: Registry Capability, UCSD Fallback, and Skill Adoption
Browser Test Provider Instance Updates to Opencode
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Suggested labels
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
size:L
vouch:trusted
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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: 131150a284
ℹ️ 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".
| ...installed, | ||
| providers: mergeInstalledProviderSkill({ | ||
| providers: refreshedProviders, | ||
| providers, |
There was a problem hiding this comment.
Using the pre-install providers snapshot here means the RPC response is patched for this caller, but ProviderRegistry itself is never updated. In the normal install flow, a reconnect or any later RPC that reads providerRegistry.getProviders (for example remove/disable, which validate the skill against the current provider inventory) will still see the old snapshot and can report the newly installed skill as missing until some unrelated refresh happens.
Useful? React with 👍 / 👎.
|
@codex review |
![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)
apps/server/src/provider/installProviderSkill.test.ts (1)
145-185: ⚡ Quick winCover UCSD config registration in the adoption test.
This test uses the non-UCSD
installTestSkillRoot, so it would still pass if the adoption path stopped callingregisterUcsdOpenCodeSkillPath. Please add a UCSD-shaped config/root assertion here, or add a sibling test that adopts an existing folder and verifiesopencode.jsongains the adopted skill directory inskills.paths.🤖 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 `@apps/server/src/provider/installProviderSkill.test.ts` around lines 145 - 185, The test for the adoption of an existing catalog skill folder uses the non-UCSD installTestSkillRoot helper, which means it would not catch if the adoption flow stopped calling registerUcsdOpenCodeSkillPath. Add assertions to verify UCSD-specific behavior by checking that the opencode.json configuration file is properly updated with the adopted skill directory path in its skills.paths array, or alternatively create a sibling test that specifically validates this UCSD config registration behavior during skill adoption.
🤖 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 `@apps/server/src/provider/installProviderSkill.ts`:
- Around line 852-867: The issue is that when OPENCODE_CONFIG is explicitly set
but resolveUcsdConfigSkillsDirectory returns a falsy skillsDirectory, the code
silently skips it and falls through to use the HOME candidate, allowing skills
to be installed under the wrong OpenCode profile. Fix this by throwing a
targeted error when configuredPath is truthy but skillsDirectory resolution
fails, instead of continuing. Only use the HOME candidate path when
OPENCODE_CONFIG is absent (configuredPath is falsy or not set).
---
Nitpick comments:
In `@apps/server/src/provider/installProviderSkill.test.ts`:
- Around line 145-185: The test for the adoption of an existing catalog skill
folder uses the non-UCSD installTestSkillRoot helper, which means it would not
catch if the adoption flow stopped calling registerUcsdOpenCodeSkillPath. Add
assertions to verify UCSD-specific behavior by checking that the opencode.json
configuration file is properly updated with the adopted skill directory path in
its skills.paths array, or alternatively create a sibling test that specifically
validates this UCSD config registration behavior during skill adoption.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b3145e97-f5ed-4940-816a-e944cb3c7e4c
📒 Files selected for processing (3)
apps/server/src/provider/installProviderSkill.test.tsapps/server/src/provider/installProviderSkill.tsapps/server/src/ws.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44aa15ce9f
ℹ️ 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".
| skill.path === input.skillPath || skill.name === input.skillName | ||
| ? { ...skill, enabled: true, scope: skill.scope ?? "user" } |
There was a problem hiding this comment.
When the provider snapshot already contains a skill with the same name but a different path (for example a stale cached/plugin entry while this install writes the UCSD skills folder), this branch matches it but only flips enabled, leaving the old path in the RPC response and provider cache. Downstream operations validate and remove skills by skill.path, so the UI will continue to manage the stale location instead of the newly installed input.skillPath; update the matched entry's name/path from the install result or only match by path here.
Useful? React with 👍 / 👎.
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
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 `@apps/server/src/provider/Layers/ProviderRegistry.ts`:
- Around line 455-459: In the skill remapping logic within ProviderRegistry.ts
around lines 457-458, the matched skill is being updated with enabled and scope
properties, but the canonical identity fields are not synchronized with the
input. When a skill matches on one field (e.g., same name but new path), the
updated object should also include input.skillName and input.skillPath in the
spread operation to ensure the cached snapshot reflects the current canonical
identity rather than retaining stale path or name values. Update the object
construction in the ternary operator to include name: input.skillName and path:
input.skillPath along with the existing enabled and scope updates.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a9311a69-6991-4e9f-b359-1acb0fae8c5c
📒 Files selected for processing (9)
apps/server/src/provider/Layers/ProviderRegistry.test.tsapps/server/src/provider/Layers/ProviderRegistry.tsapps/server/src/provider/Services/ProviderRegistry.tsapps/server/src/provider/installProviderSkill.test.tsapps/server/src/provider/installProviderSkill.tsapps/server/src/provider/providerMaintenanceRunner.test.tsapps/server/src/provider/testUtils/providerRegistryMock.tsapps/server/src/server.test.tsapps/server/src/ws.ts
✅ Files skipped from review due to trivial changes (1)
- apps/server/src/server.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/server/src/ws.ts
- apps/server/src/provider/installProviderSkill.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17bb90d629
ℹ️ 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".
| return Effect.gen(function* () { | ||
| const fs = yield* FileSystem.FileSystem; | ||
| const pathService = yield* Path.Path; | ||
| const environment = input.environment ?? process.env; |
There was a problem hiding this comment.
When an OpenCode provider reports no skills, this path falls back to process.env unless the caller passes environment, but the websocket install RPC only passes the provider snapshot/request and OpenCode instances can have per-instance env such as OPENCODE_CONFIG or HOME merged when probing. For an instance whose config lives outside the server process env, the first skill install is written to the process user's UCSD config (or rejected by an unrelated process OPENCODE_CONFIG) rather than the selected provider's config, so that provider never sees the installed skill. Plumb the effective instance environment into this fallback or avoid process-wide defaults for instance-scoped installs.
Useful? React with 👍 / 👎.
|
@codex review @coderabbitai review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8261ae6f66
ℹ️ 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".
| return Effect.gen(function* () { | ||
| const fs = yield* FileSystem.FileSystem; | ||
| const pathService = yield* Path.Path; | ||
| const environment = input.environment ?? process.env; |
There was a problem hiding this comment.
When the selected OpenCode instance has no reported skills and is configured via providerInstances.*.environment (for example its own OPENCODE_CONFIG/HOME for a custom or secondary instance), the install RPC does not pass that effective instance environment into this helper, so this fallback reads the server process environment instead. In that scenario the skill is installed and registered under the default process ~/.agents/ucsd config while the target OpenCode process continues using its separate config and never sees the skill, even though the RPC returns success for that instance. The fallback should use the target provider instance's effective environment or fail rather than defaulting to process.env.
Useful? React with 👍 / 👎.
| if (existingFrontmatter.name !== skillName) { | ||
| return yield* installError( | ||
| `Skill '${skillName}' cannot be installed because ${skillDirectory} already contains skill '${existingFrontmatter.name}'.`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
In the existing-folder path, this name check is the only compatibility check before returning success without copying any files from the requested bundle. If a prior failed/stale install left SKILL.md with the same frontmatter name but missing or outdated companion files, reinstalling a catalog or linked skill reports success and registers the path while leaving the requested bundle absent or incomplete. Please either verify the existing folder matches the bundle or merge/replace the bundle files before returning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/components/ChatView.browser.tsx (1)
7428-7435: ⚡ Quick winPrefer immutable fixture updates over in-place mutation.
The current implementation uses a type assertion to strip readonly modifiers and mutate the provider object directly. This pattern can lead to subtle bugs if the fixture object is reused across tests or if mutation order matters.
♻️ Suggested immutable update pattern
- const mutableProvider = provider as { - driver: ServerConfig["providers"][number]["driver"]; - instanceId: ServerConfig["providers"][number]["instanceId"]; - skills: ServerConfig["providers"][number]["skills"]; - }; - mutableProvider.driver = ProviderDriverKind.make("opencode"); - mutableProvider.instanceId = ProviderInstanceId.make("opencode"); - mutableProvider.skills = [ + nextFixture.serverConfig.providers[0] = { + ...provider, + driver: ProviderDriverKind.make("opencode"), + instanceId: ProviderInstanceId.make("opencode"), + skills: [ - { - name: "agent-browser", - displayName: "Agent Browser", - description: "Open pages, click around, and inspect web apps.", - path: "/Users/test/.agents/skills/agent-browser/SKILL.md", - enabled: true, - }, - ]; + { + name: "agent-browser", + displayName: "Agent Browser", + description: "Open pages, click around, and inspect web apps.", + path: "/Users/test/.agents/skills/agent-browser/SKILL.md", + enabled: true, + }, + ], + };🤖 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 `@apps/web/src/components/ChatView.browser.tsx` around lines 7428 - 7435, The code uses a type assertion to strip readonly modifiers and then mutates the provider object directly, which can cause bugs if the fixture is reused. Instead of casting the provider and mutating its properties (driver, instanceId, and skills), create a new object using an immutable update pattern such as spreading the original provider and overriding only the properties that need to change. This way, the original provider remains unchanged and you avoid the unsafe type assertion that removes readonly protections.
🤖 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 `@apps/web/src/components/ChatView.browser.tsx`:
- Around line 7428-7435: The code uses a type assertion to strip readonly
modifiers and then mutates the provider object directly, which can cause bugs if
the fixture is reused. Instead of casting the provider and mutating its
properties (driver, instanceId, and skills), create a new object using an
immutable update pattern such as spreading the original provider and overriding
only the properties that need to change. This way, the original provider remains
unchanged and you avoid the unsafe type assertion that removes readonly
protections.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: de590610-1d96-474f-b71f-739fa38faf35
📒 Files selected for processing (5)
apps/server/src/provider/Layers/ProviderRegistry.test.tsapps/server/src/provider/Layers/ProviderRegistry.tsapps/server/src/provider/installProviderSkill.test.tsapps/server/src/provider/installProviderSkill.tsapps/web/src/components/ChatView.browser.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/server/src/provider/Layers/ProviderRegistry.test.ts
- apps/server/src/provider/Layers/ProviderRegistry.ts
- apps/server/src/provider/installProviderSkill.test.ts
- apps/server/src/provider/installProviderSkill.ts
|
@codex review @coderabbitai review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a74fa4452
ℹ️ 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".
| return Effect.gen(function* () { | ||
| const fs = yield* FileSystem.FileSystem; | ||
| const pathService = yield* Path.Path; | ||
| const environment = input.environment ?? process.env; |
There was a problem hiding this comment.
When the target OpenCode instance has OPENCODE_CONFIG or HOME set only via its provider-instance environment and reports no skills yet, this fallback reads the server process environment instead because the install RPC calls installProviderSkill without an environment. That means the first catalog install for that instance can fail with “no UCSD OpenCode config” or install into the server user’s default ~/.agents/ucsd tree rather than the config OpenCodeDriver actually uses for the instance.
Useful? React with 👍 / 👎.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
Validation
./node_modules/.bin/vp test run src/provider/installProviderSkill.test.ts src/provider/removeProviderSkill.test.ts./node_modules/.bin/vp run typecheck./node_modules/.bin/vp checkSummary by CodeRabbit
Bug Fixes
SKILL.mdonly when the frontmatter name matches; otherwise installation fails with a more specific error.Improvements
Tests