Repository navigation
fix(server): await plugin activation before listing skills - #51687
Closed
ANSHSINGH050404 wants to merge 1 commit into
Closed
ANSHSINGH050404 wants to merge 1 commit into
ANSHSINGH050404 wants to merge 1 commit into
Conversation
A cold Location activates its plugins asynchronously, so the first skill.list on a fresh server could answer from the empty registry and report no skills at all. Session entry points already wait on Plugin.awaitActivation for exactly this reason; the HTTP read did not.
github-actions
Bot
added
the
needs:compliance
Contributor
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
Contributor
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
Contributor
|
The following comment was made by an LLM, it may be inaccurate: |
Contributor
|
This pull request has been automatically closed because it was not updated to meet our contributing guidelines within the 2-hour window. Feel free to open a new pull request that follows our guidelines. |
github-actions
Bot
removed
the
needs:compliance
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
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.
Summary
GET /api/skillcould answer from an empty skill registry on a cold server. Skills are only registered by plugins (ConfigSkillPluginand plugin-provided skills), and a cold Location activates its plugins asynchronously:PluginSupervisorforks the initial activation, so a location layer finishes building while the registry is still empty. The handler readSkill.list()immediately, so the first SDKskills.list()returned[]and a later call returned the real list.Plugin.awaitActivationexists precisely for this, andpackages/core/src/plugin/service.tsdocuments the failure mode it prevents: "A cold Location activates its plugins asynchronously, so without the wait an early request observes an empty registry." Session entry points (session/skill.ts,session/prompt.ts,session/command.ts,session/shell.ts) and theintegration.list/rpc.callhandlers already wait on it. The skill read did not, so it now does too.Empty is a valid answer for "this project has no skills", which leaves a client with no way to tell "not loaded yet" from "nothing registered" and no readiness signal to wait on. The test pins both halves of the contract: the read does not answer while activation is in flight, and the answer it eventually gives already carries the plugin-registered skill.
Test
packages/server/test/skill.test.tsregisters a host plugin gated on aDeferredand holds the release until the first/api/skillrequest is in flight. Without the fix, that request completes from the empty registry and the assertion that it stayed pending fails; with the fix, it returns the skill the plugin registered on release.bun testinpackages/server: 62 pass, 3 skip, 0 fail.Note
agent.listhas the same missing wait and is tracked separately in #51282, so it is left alone here. This also makesskill.listblock for the duration of activation, the same tradeoffintegration.listalready makes; catalog reads that must stay nonblocking (provider.list,model.list, covered in #36117) are unaffected.Closes #51680