🧭 fix: Restore Empty Skill Allowlist Catalog#13526
Merged
Merged
Conversation
Owner
Author
|
@codex review |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes agent skill allowlist semantics so a persisted empty allowlist (skills: []) is treated as “no allowlist / full accessible catalog” rather than “no skills”, preventing agents from being misinterpreted as skills-disabled after list hydration.
Changes:
- Updated backend skill scoping semantics so
skills: []returns the full accessible skill catalog. - Adjusted VIEW-mode agent list sanitization to preserve
skills_enabled: trueeven when the persisted allowlist is empty. - Added/updated regression tests covering backend list hydration behavior and the client
$skill picker behavior for empty allowlists.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/api/src/agents/skills.ts | Changes scoping semantics so empty allowlists fall back to full accessible catalog. |
| packages/api/src/agents/tests/skills.test.ts | Updates unit test expectations to lock in the new empty-allowlist semantics. |
| api/server/controllers/agents/v1.js | Ensures VIEW list responses keep skills_enabled: true when allowlist is empty (and omits raw skills). |
| api/server/controllers/agents/v1.spec.js | Adds regression test for VIEW list hydration with empty allowlist. |
| client/src/components/Chat/Input/tests/SkillsCommand.spec.tsx | Adjusts test fixture so persisted agents with skills: [] + skills_enabled: true show full catalog. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ 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". |
Contributor
GitNexus: 🚀 deployedThe |
7 tasks
danny-avila
added a commit
that referenced
this pull request
Jun 12, 2026
Codex round 2: an automated prune that empties an enabled allowlist would silently widen the agent to the full accessible catalog (empty + enabled = full per the #13526 semantics). Hygiene must only ever narrow. - deleteSkill/deleteUserSkills: agents whose entire allowlist is being deleted get skills disabled instead of an emptied-but-enabled list; ids are lowercased before the $pull so an uppercase-but-valid id cannot leave the dangling entry behind - createAgent/updateAgent/revertAgentVersion: pruning a non-empty allowlist to zero survivors disables skills; an explicit user-sent skills: [] keeps the full-catalog semantics - builder: a per-id skill lookup only renders the removable "Unavailable skill" chip on a confirmed 404/403 — transient and server errors keep the chip hidden rather than inviting removal
danny-avila
added a commit
that referenced
this pull request
Jun 13, 2026
* 🧹 fix: Prune Dangling Skill IDs from Agent Allowlists Deleted skills left their ids behind in every agent's `skills` allowlist: nothing removed them on skill deletion, the builder rendered no chip for unresolvable ids (so users could neither see nor remove them), and at runtime the non-empty allowlist intersected with accessible skills to an empty set — silently disabling the entire skills catalog for the agent even though the panel looked like "no skills selected." - deleteSkill / deleteUserSkills now $pull deleted ids from all agent allowlists (no versioning, timestamps untouched) - createAgent / updateAgent prune allowlist ids whose skill doc no longer exists (existence-only check, never ACL), so poisoned agents self-heal on the next save — including duplicates and sync paths - the builder renders unresolvable allowlist entries as removable "Unavailable skill" chips once the catalog query resolves * 🪞 fix: Keep Skill Queries and Authoring Labels Truthful After Chat Edits Skills authored mid-chat via create_file/edit_file never reached the Skills panel or builder without a manual refresh, and a create_file that overwrote an existing file still announced "Created" in the tool card. - invalidate all skill query caches (refetchType: 'all', since the skill hooks opt out of refetchOnMount) when a completed create_file/edit_file call targets a skills/ path - label create_file completions from the host-authored output summary: overwrites now read "Updated <file>" with the edit icon * ♻️ refactor: Inject Skill Authoring Callback Instead of Query Client useStepHandler took useQueryClient directly, forcing a QueryClientProvider wrapper onto all 54 renderHook calls in its spec. Its only consumer, useEventHandlers, already holds the query client and does this exact invalidation pattern for project/MCP keys — so pass an optional onSkillAuthoringComplete callback instead. Detection stays in the completion handler; the side effect lives with the client. Spec diff collapses to pure additions. * 🩹 fix: Resolve Codex Review Findings on Allowlist Pruning - normalize allowlist candidates to lowercase in filterExistingSkillIds: isValidObjectIdString accepts uppercase hex, but _id.toString() is lowercase, so a casing mismatch silently emptied a valid allowlist (widening scope to the full catalog) - prune agent allowlists immediately after the Skill row deletion in deleteSkill: a SkillFile cleanup failure previously skipped the prune forever, since retries exit early on deletedCount === 0 - filter version-snapshot skills through filterExistingSkillIds in revertAgentVersion so reverting to a pre-delete version cannot resurrect dangling ids - resolve allowlist ids missing from the builder's first catalog page individually via getSkill before labeling them unavailable — a cache miss on a >100-skill catalog no longer invites removing a valid skill * 🚪 fix: Fail Closed When Pruning Empties a Skill Allowlist Codex round 2: an automated prune that empties an enabled allowlist would silently widen the agent to the full accessible catalog (empty + enabled = full per the #13526 semantics). Hygiene must only ever narrow. - deleteSkill/deleteUserSkills: agents whose entire allowlist is being deleted get skills disabled instead of an emptied-but-enabled list; ids are lowercased before the $pull so an uppercase-but-valid id cannot leave the dangling entry behind - createAgent/updateAgent/revertAgentVersion: pruning a non-empty allowlist to zero survivors disables skills; an explicit user-sent skills: [] keeps the full-catalog semantics - builder: a per-id skill lookup only renders the removable "Unavailable skill" chip on a confirmed 404/403 — transient and server errors keep the chip hidden rather than inviting removal
fuuuzzy
pushed a commit
to fuuuzzy/LibreChat
that referenced
this pull request
Jun 18, 2026
fuuuzzy
pushed a commit
to fuuuzzy/LibreChat
that referenced
this pull request
Jun 18, 2026
) * 🧹 fix: Prune Dangling Skill IDs from Agent Allowlists Deleted skills left their ids behind in every agent's `skills` allowlist: nothing removed them on skill deletion, the builder rendered no chip for unresolvable ids (so users could neither see nor remove them), and at runtime the non-empty allowlist intersected with accessible skills to an empty set — silently disabling the entire skills catalog for the agent even though the panel looked like "no skills selected." - deleteSkill / deleteUserSkills now $pull deleted ids from all agent allowlists (no versioning, timestamps untouched) - createAgent / updateAgent prune allowlist ids whose skill doc no longer exists (existence-only check, never ACL), so poisoned agents self-heal on the next save — including duplicates and sync paths - the builder renders unresolvable allowlist entries as removable "Unavailable skill" chips once the catalog query resolves * 🪞 fix: Keep Skill Queries and Authoring Labels Truthful After Chat Edits Skills authored mid-chat via create_file/edit_file never reached the Skills panel or builder without a manual refresh, and a create_file that overwrote an existing file still announced "Created" in the tool card. - invalidate all skill query caches (refetchType: 'all', since the skill hooks opt out of refetchOnMount) when a completed create_file/edit_file call targets a skills/ path - label create_file completions from the host-authored output summary: overwrites now read "Updated <file>" with the edit icon * ♻️ refactor: Inject Skill Authoring Callback Instead of Query Client useStepHandler took useQueryClient directly, forcing a QueryClientProvider wrapper onto all 54 renderHook calls in its spec. Its only consumer, useEventHandlers, already holds the query client and does this exact invalidation pattern for project/MCP keys — so pass an optional onSkillAuthoringComplete callback instead. Detection stays in the completion handler; the side effect lives with the client. Spec diff collapses to pure additions. * 🩹 fix: Resolve Codex Review Findings on Allowlist Pruning - normalize allowlist candidates to lowercase in filterExistingSkillIds: isValidObjectIdString accepts uppercase hex, but _id.toString() is lowercase, so a casing mismatch silently emptied a valid allowlist (widening scope to the full catalog) - prune agent allowlists immediately after the Skill row deletion in deleteSkill: a SkillFile cleanup failure previously skipped the prune forever, since retries exit early on deletedCount === 0 - filter version-snapshot skills through filterExistingSkillIds in revertAgentVersion so reverting to a pre-delete version cannot resurrect dangling ids - resolve allowlist ids missing from the builder's first catalog page individually via getSkill before labeling them unavailable — a cache miss on a >100-skill catalog no longer invites removing a valid skill * 🚪 fix: Fail Closed When Pruning Empties a Skill Allowlist Codex round 2: an automated prune that empties an enabled allowlist would silently widen the agent to the full accessible catalog (empty + enabled = full per the danny-avila#13526 semantics). Hygiene must only ever narrow. - deleteSkill/deleteUserSkills: agents whose entire allowlist is being deleted get skills disabled instead of an emptied-but-enabled list; ids are lowercased before the $pull so an uppercase-but-valid id cannot leave the dangling entry behind - createAgent/updateAgent/revertAgentVersion: pruning a non-empty allowlist to zero survivors disables skills; an explicit user-sent skills: [] keeps the full-catalog semantics - builder: a per-id skill lookup only renders the removable "Unavailable skill" chip on a confirmed 404/403 — transient and server errors keep the chip hidden rather than inviting removal
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
I fixed the agent skill allowlist behavior so removing all selected skills keeps skills enabled and restores the full accessible skill catalog.
skills_enabled: truein the VIEW agent list response when an enabled agent has an empty skill allowlist.skills: []means no allowlist instead of no skills.$skill picker behavior.Root cause: the saved empty allowlist could be flattened during agent list hydration, which made the client treat the persisted agent as skills-disabled even though the builder semantics expect an empty allowlist to mean full catalog.
Change Type
Testing
git diff --check.node --check api/server/controllers/agents/v1.js.node --check api/server/controllers/agents/v1.spec.js.cd packages/api && npx jest src/agents/__tests__/skills.test.ts --runInBandcd api && npx jest server/controllers/agents/v1.spec.js --runInBandcd client && npx jest src/components/Chat/Input/__tests__/SkillsCommand.spec.tsx --runInBandjest-junit,@babel/preset-env,jest-environment-jsdom).Test Configuration:
Checklist