feat(config): add disabled_skills denylist for shared skill directories - #1983
feat(config): add disabled_skills denylist for shared skill directories#1983zicochaos wants to merge 4 commits into
Conversation
🦋 Changeset detectedLatest commit: af63f9a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![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: df3ca14c9c
ℹ️ 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".
| } | ||
| if (this.skillCatalog.catalog.isSkillDisabled(input.name)) { | ||
| throw new Error2( | ||
| ErrorCodes.SKILL_DISABLED, |
There was a problem hiding this comment.
When a disabled skill is activated through POST /sessions/{id}/skills/{name}:activate, this new ErrorCodes.SKILL_DISABLED path reaches packages/kap-server/src/routes/skills.ts's sendMappedError, which only handles SKILL_NOT_FOUND, SKILL_NAME_EMPTY, and SKILL_TYPE_UNSUPPORTED; the error is then rethrown to the global handler as 50001. A direct REST call or stale slash-menu entry for a disabled skill should return a deterministic client error instead of an internal error, so add this code to the route/error-code mapping.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. Disabled activation now maps to the public non-activatable skill error (40912) instead of falling through to the internal error handler. The refreshed branch covers both the legacy service path and the kap-server route with regression tests.
| const disabledSkills = | ||
| this.config.get<DisabledSkillsConfig>(DISABLED_SKILLS_SECTION) ?? []; | ||
| const m = new InMemorySkillCatalog({ disabledSkills }); |
There was a problem hiding this comment.
This wires disabled_skills into the session-scoped catalog, but the no-session workspace listing still composes its own catalog in packages/kap-server/src/routes/skills.ts with new InMemorySkillCatalog() and returns catalog.listSkills(). apps/kimi-web uses GET /workspaces/{id}/skills to pre-warm the onboarding composer's slash menu before a session exists, so disabled skills still appear there until the first session is created; pass the same config into that workspace catalog as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. Session-less workspace skill listings now build the catalog with the configured disabled_skills denylist, matching the session-scoped catalog. The refreshed branch includes kap-server regression coverage for workspace listing and activation behavior.
d8e6864 to
d409665
Compare
|
Rebuilt the branch on the current @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d409665b40
ℹ️ 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".
| getSkillRoots(): readonly string[]; | ||
| getSkippedByPolicy(): readonly SkippedSkill[]; | ||
| getModelSkillListing(): string; | ||
| /** True when the skill name is listed in config `disabled_skills`. */ |
There was a problem hiding this comment.
In packages/agent-core-v2, the local guide only allows comments in the top-of-file module header, not beside interface members or functions. These newly added inline JSDoc comments in types.ts (and the matching option comment in registry.ts) put explanatory prose next to declarations, so they should be removed or folded into the module header to keep the v2 comment convention consistent.
AGENTS.md reference: packages/agent-core-v2/AGENTS.md:L11-L13
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. The inline JSDoc comments were removed to preserve the agent-core-v2 header-only comment convention. The branch was also rebuilt on the current upstream main and its generated config manifest is up to date.
d409665 to
660af40
Compare
|
Rebuilt this PR on the current @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 660af401e6
ℹ️ 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".
|
|
||
| if (event.type === 'configChanged') { | ||
| rawState.defaultModel = event.config.defaultModel ?? null; | ||
| void modelProvider.refreshLoadedSkills(); |
There was a problem hiding this comment.
When a live v2 session has cached skills and a config patch changes extra_skill_dirs or merge_all_available_skills, this refresh can read and permanently cache the old catalog. In SessionSkillCatalogService, the config-source listener starts reloadSource() without awaiting it, while GET /sessions/{id}/skills waits only for the initial catalog.ready; the config route publishes event.config.changed immediately after config.set(). A slow directory scan therefore lets this request finish before the new contribution is merged, and the later catalog onDidChange is not forwarded to the web, so no second refresh repairs the cache. Publish/handle a post-catalog-refresh signal or make the listing wait for outstanding source reloads.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. The session skill listing endpoint now waits for outstanding async catalog reloads (extra_skill_dirs, merge_all_available_skills) before returning, instead of only awaiting the initial catalog ready promise. Deferred tests cover both config-triggered reload paths, so the web refresh no longer caches a stale listing.
660af40 to
95b77e4
Compare
|
Addressed the latest review finding: session skill listings now drain in-flight catalog reloads before responding, which removes the stale web cache race after config changes. @codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…odel/agentfile (MoonshotAI#2232) Syncs 30 upstream commits. Upstream MoonshotAI#2232 ported custom agent files + secondary-model binding to the v1 engine, covering the fork's exact-alias subagent model selection (fb6d57c) and subagent model surfacing (353a1c5) — so the fork's v1-side implementation is retired in favor of upstream's: - v1 collision files restored to upstream (agent/agent-swarm tools, subagent-host, tool registry, flags, docs); fork's v1 subagent-model-directory.ts dropped as dead code - TUI subagent model display deduplicated onto upstream's stats placement (agent-group, tool-call); fork's per-row model chip removed - config schema/toml unions keep the fork's disabledSkills alongside upstream's extraAgentDirs - footer quota readout ported onto upstream's slot-based footer (status_line); managed-usage rows adapted to upstream's structured ManagedUsageRow shape (window/name, no label) - node-sdk getSessionWarnings passes pathClass for the fork's AGENTS.md include expansion - state manifest regenerated Fork-unique deltas kept: disabled_skills denylist (PR MoonshotAI#1983 upstream), session-only default model, managed plan quota display, AGENTS.md include expansion, v2-side subagent model selection, misc fixes. Review follow-ups (PR #12): - v1 subagent-host + node-sdk getSessionWarnings: thread agentsMdExpandIncludes through spawned-subagent and warning prompt contexts (with a subagent-host test covering include expansion) - TUI: keep the status-derived subagent model when later events carry no model (subagent-event-handler, tool-call); AgentSwarm header shows a shared model only when all reported members agree (setMemberModel) - kap-server subagentRosterTracker learns the model from agent.status.updated; kimi-web agentEventProjector projects it onto the task - lint: drop useless spread in skillCatalogService; FORK.md + config docs note the v1 exact-alias retirement (v2-only)
1 participant
Related Issue
Resolve #1982
Problem
See linked issue. Shared skill directories (for example
~/.agents/skills/) often contain skills that should stay on disk for other tools but must not appear in Kimi's model listing,Skilltool, or slash menu. Existing levers are incomplete:disableModelInvocation: trueonly blocks automatic model invocationSkill(...)can still leave the skill visible in listingsWhat changed
disabled_skillsinconfig.toml(case-insensitive name denylist, default[])skill.disabledand config REST/protocol fields for the new setting@moonshot-ai/kimi-code)Matching skills are fully hidden from Kimi surfaces while files remain on disk.
/reloador a new session applies config changes. This only covers the Skill surface; shell reimplementation still needs permission deny rules when required.Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.