fix(cli): preserve trust for plugin-provided skills - #12760
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| // project-root sandbox to its external package directory. | ||
| const origins = { ...cfg.skill_path_origins } | ||
| for (const path of paths) { | ||
| if (origins[path]) continue |
There was a problem hiding this comment.
WARNING: Trust here is inferred from a missing origin, which fails open
The heuristic "no recorded origin => added by a plugin => trusted" holds for today's code (every config source flows through merge() in config/config.ts, which records skill_path_origins), but it is a fail-open default in the trust map added by the markdown-exfiltration fix (#12168). Any future code path that appends to cfg.skills.paths without recording provenance — a server route, an injected config, an upstream merge — silently becomes trusted: true, which re-enables {env:}/{file:} substitution and shell execution for that directory.
A precise alternative that keeps the same behavior without the assumption: snapshot the paths before the config-hook loop in plugin/index.ts and pass the delta, e.g. const before = new Set(cfg.skills?.paths ?? []) before the loop and PluginSkillOrigins.mark(cfg, before) after, marking only paths not in before. That way only paths actually added by a hook get plugin trust.
Related nit: a plugin that contributes a relative path still ends up untrusted, because skill/index.ts:274 requires path.isAbsolute(expanded). Worth a comment if that is intentional.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| init: (dir) => | ||
| Effect.promise(() => | ||
| Bun.write( | ||
| path.join(path.dirname(dir), "plugin-skills", "example", "SKILL.md"), |
There was a problem hiding this comment.
SUGGESTION: Fixture writes to a fixed path in the system temp root and never cleans it up
dir is os.tmpdir()/opencode-test-<random> (see test/fixture/fixture.ts:153), so path.dirname(dir) is the system temp root and this creates ${TMPDIR}/plugin-skills/example/SKILL.md. The scoped finalizer only removes dir, so the directory leaks and is shared by every run and every concurrent shard. A leftover copy from a previous run also means the test can pass even if this write silently fails.
A unique sibling directory keeps the "outside the project root" property without the shared name, e.g. path.join(`${dir}-plugin-skills`, "example", "SKILL.md") here plus the matching `${input.directory}-plugin-skills` in test/kilocode/fixtures/plugin-skill.ts:8.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| const list = yield* Skill.Service.use((service) => service.all()) | ||
|
|
||
| expect(list.find((item) => item.name === "plugin-skill")).toMatchObject({ | ||
| description: "Registered by a plugin config hook.", |
There was a problem hiding this comment.
SUGGESTION: Assert the trust flag, not just discovery
The fix is specifically about provenance/trust, but the assertion only checks that the skill was discovered. Adding trusted: true to the matched object pins the actual behavior being fixed, so a future regression that discovers the skill as untrusted (which changes substitution and shell-execution behavior) still fails the test.
| description: "Registered by a plugin config hook.", | |
| description: "Registered by a plugin config hook.", | |
| trusted: true, |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (5 files)
Incremental review of Fix these issues in Kilo Cloud Previous Review Summaries (3 snapshots, latest commit c2b791b)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit c2b791b)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (3 files)
Incremental review of Fix these issues in Kilo Cloud Previous review (commit 1c2de7d)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (5 files)
Resolved since the last review: the trust map is now derived from a pre-hook snapshot of Fix these issues in Kilo Cloud Previous review (commit 3613fbf)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (5 files)
Notes: fork hygiene looks good — the shared upstream file only gains a two-line annotated hook and the logic lives under Reviewed by claude-opus-5 · Input: 40 · Output: 9.5K · Cached: 1.4M Review guidance: REVIEW.md from base branch |
3613fbf to
7bac10c
Compare
| @@ -0,0 +1,11 @@ | |||
| import path from "node:path" | |||
There was a problem hiding this comment.
SUGGESTION: Unused import
path is no longer used now that the directory is built with a template string, so this import is dead.
| import path from "node:path" | |
| export default async (input: { directory: string }) => ({ |
(Applying the suggestion above also drops the following blank line's purpose — if you prefer, just delete line 1 and the blank line manually.)
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
To use Kilo from GitHub you first need to link your GitHub account to Kilo. Link your Kilo account to continue. After linking, mention me again in this issue or pull request.
| init: (dir) => | ||
| Effect.promise(async () => { | ||
| await Bun.write( | ||
| path.join(`${dir}-plugin-skills`, "example", "SKILL.md"), |
There was a problem hiding this comment.
SUGGESTION: The sibling skills directory is never removed
Using ${dir}-plugin-skills fixes the shared-name problem from the earlier revision, but the scoped finalizer in tmpdirScoped (test/fixture/fixture.ts:157) only cleans dir itself, so each run leaves an opencode-test-<random>-plugin-skills tree behind in the system temp dir. init runs inside the tmpdir scope, so a yield* Effect.addFinalizer(() => Effect.promise(() => fs.rm(${dir}-plugin-skills, { recursive: true, force: true }))) inside the init effect (or wrapping the promise body in Effect.gen) would clean it up with the rest of the fixture.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
To use Kilo from GitHub you first need to link your GitHub account to Kilo. Link your Kilo account to continue. After linking, mention me again in this issue or pull request.
| } | ||
|
|
||
| // Notify plugins of current config | ||
| const configuredSkillPaths = new Set(cfg.skills?.paths ?? []) // kilocode_change |
There was a problem hiding this comment.
SUGGESTION: Prefer a single-word name here
AGENTS.md asks for single-word locals unless a compound is genuinely needed; mark() already calls this parameter before, so the compound adds nothing and makes the shared upstream line longer than it needs to be.
| const configuredSkillPaths = new Set(cfg.skills?.paths ?? []) // kilocode_change | |
| const before = new Set(cfg.skills?.paths ?? []) // kilocode_change |
The call on the line below would become PluginSkillOrigins.mark(cfg, before).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
To use Kilo from GitHub you first need to link your GitHub account to Kilo. Link your Kilo account to continue. After linking, mention me again in this issue or pull request.
|
To stay organized pull requests are automatically closed after 30 days of inactivity. If the pull request is still relevant please reopen it or create a fresh new one. |
1 participant
Issue
Fixes #12468
What changed
Plugin
confighooks can add skill directories after the normal config loader has recorded skill-path provenance. Those paths were subsequently treated as untrusted project content, so plugin skills outside the workspace could fail the markdown scope check and disappear with only a generic parse error.The plugin initialization step now records provenance for skill paths added by config hooks and marks them trusted. A regression test loads a skill from a plugin-provided directory outside the project root.
Validation
npx --yes bun@1.3.14 test test/skill/skill.test.ts test/kilocode/plugin-skill-path.test.tsnpx --yes bun@1.3.14 run typecheckgit diff --check