feat(app-server): add a skills/changed v2 notification#13414
Conversation
e9d0354 to
450501d
Compare
450501d to
5e1e83c
Compare
|
@codex review |
| "properties": { | ||
| "method": { | ||
| "enum": [ | ||
| "skills/changed" |
There was a problem hiding this comment.
why changed vs updated?
There was a problem hiding this comment.
I had updated in mind as well but codex pushed me away with this justification:
skills/changedis a better fit for what this notification actually means.updated implies the server is telling the client “here is the new authoritative state” or at least “the resource has been refreshed.” That’s not what we have here. This notification is just an invalidation signal coming from the watcher: something on disk changed, and the client should re-run skills/list if it cares.
changed is weaker and more accurate:
- it doesn’t imply the server is pushing the updated skill list
- it matches the underlying watcher concept (
SkillsChanged)- it leaves room for changes that are additive, deletions, edits, enable/disable effects, or anything else that should cause a refresh
There was a problem hiding this comment.
if you feel strongly happy to update! i'm ok either way I think
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
This adds a first-class app-server v2
skills/changednotification for the existing skills live-reload signal.Before this change, clients only had the legacy raw
codex/event/skills_update_availableevent. With this PR, v2 clients can listen for a typed JSON-RPC notification instead of depending on the legacycodex/event/*stream, which we want to remove soon.