Skip to content

fix(cli): restore external skill loading - #12226

Closed
shssoichiro wants to merge 1 commit into
Kilo-Org:mainfrom
shssoichiro:fix/issue-12222
Closed

fix(cli): restore external skill loading#12226
shssoichiro wants to merge 1 commit into
Kilo-Org:mainfrom
shssoichiro:fix/issue-12222

Conversation

@shssoichiro

Copy link
Copy Markdown
Contributor

Issue

Fixes #12222

Context

Skills external to the canonical ~/.kilo/skills directory currently fail to load. This breaks valid uses cases such as plugin skills.

Implementation

Uses two targeted changes while preserving the security protections from #12168:

  1. Record paths added by plugin config hooks as trusted runtime origins. Executing plugins already have full process access, so this grants no additional capability. Keep this logic in a Kilo-owned helper called from plugin/index.ts.
  2. Allow project-configured external SKILL.md sources to be read without trusting their contents. This restores documented absolute skills.paths support while retaining:
  • {env:...} rejection for project-selected skills.
  • {file:...} confinement to the project root.
  • Descriptor-pinned reads and realpath/symlink protections.
  • Trusted substitutions for global and plugin-provided skills.

Screenshots / Video

Superpowers skills now loaded successfully:

image

How to Test

  • Have a plugin installed that loads skills, like Superpowers
  • Verify Kilo loads its skills without erroring

Checklist

  • Issue linked above, or exception explained
  • Tests/verification described
  • Screenshots/video included for visual changes, or marked N/A
  • Changeset considered for user-facing changes
  • I personally reviewed the diff and can explain the changes, including any AI-assisted work.

Get in Touch

ExpedientFalcon on Discord

@kilo-code-bot

kilo-code-bot Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (5 files)
  • .changeset/restore-external-skills.md
  • packages/opencode/src/kilocode/plugin/config.ts
  • packages/opencode/src/plugin/index.ts
  • packages/opencode/src/skill/index.ts
  • packages/opencode/test/kilocode/external-skill-paths.test.ts

Notes: Incremental review at head fcc6def. The only change since the previous review at 0e1ba88 is the revert of the unrelated packages/opencode/package.json dependency reorder — the file now matches main exactly and is no longer part of this PR, resolving the previous suggestion. The remaining five files are byte-identical to the previously reviewed revision, where the trust model was verified sound: KilocodePluginConfig.apply snapshots skills.paths before each config hook and only marks newly introduced paths as trusted (!origins?.[item] guard prevents elevating project-declared untrusted paths), with trust recording in finally and rejection propagation covered by tests. sourceRoot: dir in discoverSkills scopes only the SKILL.md read to the configured external directory — {env:} stays rejected and {file:} stays confined to the project root for untrusted sources, with realpath/symlink and descriptor-pinned read protections intact. Tests exercise the real implementation end to end without mocks. No memory-leak concerns: no new subscriptions, timers, handles, or module-level retained state — only per-call locals.

Previous Review Summaries (3 snapshots, latest commit 0e1ba88)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 0e1ba88)

Status: 1 Issue Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 0
SUGGESTION 1
Issue Details (click to expand)

SUGGESTION

File Line Issue
packages/opencode/package.json 97 Unrelated dependency reorder on a shared upstream file — no deps added or removed; confirm intentional or drop
Files Reviewed (6 files)
  • .changeset/restore-external-skills.md
  • packages/opencode/package.json - 1 issue
  • packages/opencode/src/kilocode/plugin/config.ts
  • packages/opencode/src/plugin/index.ts
  • packages/opencode/src/skill/index.ts
  • packages/opencode/test/kilocode/external-skill-paths.test.ts

Notes: Full review at head 0e1ba88 (the branch was rebased since the previous review, so the incremental range covered 1500+ unrelated files and was unusable). The trust model is sound: KilocodePluginConfig.apply snapshots skills.paths before each config hook and only marks newly introduced paths as trusted, so a plugin cannot elevate a project-declared untrusted path (!origins?.[item] guard), and trust recording in finally plus rejection propagation is explicitly tested. The sourceRoot: dir addition in discoverSkills scopes only the SKILL.md read to the configured external directory — {env:} stays rejected and {file:} stays confined to the project root for untrusted sources, with realpath/symlink and fd-pinning protections intact via ConfigVariableGuard. Tests exercise the real implementation end to end with no mocks, covering the plugin-vs-project trust boundary and secret-leak prevention. No memory-leak concerns: no new subscriptions, timers, handles, or retained state beyond per-call locals. The design question from the PR discussion (breadth of trusting plugin-declared paths) remains a maintainer policy call rather than a code defect — plugins already execute with full process access.

Fix these issues in Kilo Cloud

Previous review (commit 1419893)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (5 files)
  • .changeset/restore-external-skills.md
  • packages/opencode/src/kilocode/plugin/config.ts
  • packages/opencode/src/plugin/index.ts
  • packages/opencode/src/skill/index.ts
  • packages/opencode/test/kilocode/external-skill-paths.test.ts

Notes: Re-verified against the current head commit. The diff content is unchanged from the previously reviewed state (same sourceRoot scoping in discoverSkills, same trust-recording logic in KilocodePluginConfig.apply, same test coverage) — no new commits altered the code itself. The recent PR discussion raises a legitimate design question (whether trusting arbitrary plugin-declared skills.paths, including user-configured external directories, is too broad given upstream's planned generic skill-directory support), but that is a product/security policy decision for the maintainers, not a defect in the code as written. No new inline issues to report.

Previous review (commit e605771)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (5 files)
  • .changeset/restore-external-skills.md
  • packages/opencode/src/kilocode/plugin/config.ts
  • packages/opencode/src/plugin/index.ts
  • packages/opencode/src/skill/index.ts
  • packages/opencode/test/kilocode/external-skill-paths.test.ts

Notes: Traced the trust/scope logic end-to-end (KilocodePluginConfig.applyplugin/index.ts config-hook wrapping → discoverSkills/scan/add in skill/index.tsConfigMarkdown.parse/KilocodeMarkdown.read/ConfigVariable.substitute). The new sourceRoot correctly scopes SKILL.md reads to the configured external directory while leaving {file:} substitutions confined to the project root, matching the PR's stated security invariants. Plugin-added skill paths are only marked trusted when newly introduced by that specific hook call (snapshot-before-call), so a plugin cannot retroactively elevate trust on a path a project config already declared untrusted. The added test exercises the real implementation (no mocks) and covers both the hook-rejection/trust-recording interaction and the plugin-vs-project trust boundary end to end. Fork-hygiene is respected: new logic lives in src/kilocode/plugin/config.ts, and the one-line change to shared plugin/index.ts and the small addition inside an existing kilocode_change block in skill/index.ts are both properly marked.


Reviewed by kimi-k3 · Input: 61.1K · Output: 4.8K · Cached: 269.9K

Review guidance: REVIEW.md from base branch main

@shssoichiro

Copy link
Copy Markdown
Contributor Author

test (linux) CI job appears to be failing on all new PRs for an unknown reason. Failure is not related to this change.

@marius-kilocode

marius-kilocode commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

Do you have an example where the skill would then sit on disk?

  • We should be careful with allowing to load any skill
  • Upstream will soon allow generic skill directories to be loaded (.agents etc)

I would therefore like to understand if this change represents a security risk or conflicts a future upstream merge.

@shssoichiro

shssoichiro commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

For plugin skills, kilo automatically pulls the plugin into its cache folder, so for superpowers added via kilo config's plugin key, the skills would sit at a path like /home/soichiro/.cache/kilo/packages/superpowers@git+https:/github.com/obra/superpowers.git/node_modules/superpowers/skills/writing-skills/SKILL.md.

@ivanbaldo

Copy link
Copy Markdown

On my laptop I have ~/.agents/ with a skills symlink pointing to a Git project somewhere else on my home directory.
The skills worked before but now don't.
Does this PR also solve this use case?
Thanks!!!

@marius-kilocode

Copy link
Copy Markdown
Collaborator

Going forward we want to support ~/.agents/. However, since this will come upstream any implementation in that direction represents a duplicate. I am quite hesitate to say that we should support skills from arbitrary directories like proposed here. Shouldn't superpowers allow configuring the skill in the harness directory?

@shssoichiro

shssoichiro commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

Based on my research, you can generally relocate the entire installed plugin/package using features provided by the harness—for example, OpenCode can load Superpowers from a local package path—but you cannot independently configure the location of Superpowers’ bundled skills/ directory without modifying the integration or using a filesystem workaround such as a symlink. In other words, this would add additional installation steps that are non-standard. Users following the default installation path of adding Superpowers, or other plugins, to the config file's plugin list, would encounter a broken installation.

@shssoichiro

shssoichiro commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

Perhaps a middle ground would be an explicit allow on the ~/.cache/kilo/packages directory? Since this is the directory that Kilo installs plugins to automatically. Although it does not resolve the issue if users are adding untrusted plugins to their installation. I suppose it depends on which cases the security hardening is trying to solve.

Comment thread packages/opencode/package.json Outdated
@marius-kilocode

Copy link
Copy Markdown
Collaborator

Quick update:

#12846 should allow configuring explicit symlinks again. We removed this due to a few reported security risks. This should now keep it secure but allow this workflow. @shssoichiro I assume that would also workaround introducing this feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Regression 7.4.7: external skills via skills.paths blocked - "blocked file reference outside project config scope"

3 participants