Skip to content

The installable unit is a plugin, and a skill PR is a security review - #1

Merged
ms-shashank merged 1 commit into
mainfrom
restructure/plugin-layout
Aug 12, 2026
Merged

The installable unit is a plugin, and a skill PR is a security review#1
ms-shashank merged 1 commit into
mainfrom
restructure/plugin-layout

Conversation

@ms-shashank

@ms-shashank ms-shashank commented Aug 11, 2026

Copy link
Copy Markdown
Member

Task 1 of the skills distribution plan. This is the repository half, and it lands before the extension can install anything.

The install unit is a plugin

A plugin is what you install. A skill is one of the things a plugin contains.

I checked this against a real installation of the reference implementation rather than assuming it. A repository containing only skills still becomes marketplace, then plugin, then version. The unit is uniform even when the payload is a single skill.

skills/<id>/SKILL.md  ->  plugins/<name>/skills/<id>/SKILL.md

Adopting it now is not only future-proofing. Consent is a fingerprint over the whole plugin directory including files the loader does not read, so that teaching the loader to read more later cannot widen what was agreed without re-asking. A plugin directory makes that property hold for free when MCP servers and hooks arrive. Leaving the layout flat would mean a cache migration plus a forced re-consent for everyone who had ever installed anything, because the fingerprint boundary itself would move.

Nobody has installed anything yet, so today it costs two git mv calls.

What is published

Plugin Skills Why
code-review 1 already here
ui-review 1 already here
ml 7 the multi-skill case nothing had exercised

ml is deliberately one plugin carrying seven skills. The format has always allowed it and no plugin had ever done it.

Teach is held back, and that is the interesting part of this PR

Teach was going to ship here too. It is not, and the reason is worth writing down.

Teach's length rule lives in the final-override slot: text appended after every shared rule, which is the only position from which a skill can outrank one. An installed skill never receives that slot, because a downloaded file overruling a safety rule is exactly what the boundary exists to prevent.

So a published teach would carry that paragraph demoted into its body, where it sits before the brevity rule it argues with and loses on recency. That is precisely the failure the slot was invented to fix.

My first attempt shipped it anyway with a note in the file explaining the difference. That was wrong: the person affected is the user who installs teach and sees a picker entry and a consent screen, neither of which says this is a reduced copy. They would get a subtly more clipped teach, with no way to know why, and nobody handling the report would connect it to a prompt slot.

Since the ML family already covers everything the install path needs, teach adds visibility rather than coverage. Holding it back costs no test coverage and avoids a knowingly weaker copy of our most visible skill being the flagship of a public marketplace.

It comes back byte-identical once the cause is fixed. The real finding is that the exception is encoded as a privileged position instead of as a condition inside the rule, and that a comment claiming the rule could not be edited turned out to be false. Tracked internally.

CI, and why it is in this PR rather than the next one

A contributor whose first pull request is rejected by a check learns the rule in thirty seconds. One whose skill merges and then silently does nothing learns that we do not review, and that cannot be taken back. So the checks land in the same change as the invitation.

  • no oxcode:final-override block, which an installed skill never receives
  • no model or provider named anywhere
  • every plugin has a manifest, and it parses
  • every plugin on disk is listed, and every listing exists with a skills/ directory
  • every SKILL.md has frontmatter carrying name and description

CONTRIBUTING.md now opens by saying a pull request here is a security review, not a docs review, and lists what a reviewer checks.

Verification

All three content checks were mutation-tested by planting the thing they forbid and confirming they fire:

planted an override block        -> detected in plugins/teach/skills/teach/SKILL.md
added an unlisted plugin (ghost) -> detected as unlisted
added "model: kimi-k2.6"         -> detected

Each was then removed and the checks pass clean.

Every published SKILL.md was parsed through OxCode's own lenient profile, the same code path an installed skill takes. All parse, with no warnings and no finalOverride surviving. Commands and toolsets resolve as expected: /review and /ui-review on chat, the seven /ml-mode:* on full.

What this does not do

Nothing here installs anything. The extension cannot yet read a marketplace; that is C1, and it is the next PR on the extension repository.

@ms-shashank
ms-shashank force-pushed the restructure/plugin-layout branch from f7d5c9f to 6dd7c93 Compare August 11, 2026 21:58
THE LAYOUT. A plugin is the thing you install, and a skill is one of the things
a plugin contains. Verified against a real installation of the reference
implementation rather than assumed: a repository containing only skills still
becomes marketplace, then plugin, then version. The unit is uniform even when
the payload is one skill.

Adopting it now is not only future-proofing. Consent is a fingerprint over the
whole plugin directory INCLUDING files the loader does not read, so that
teaching the loader to read more later cannot widen what was agreed to without
re-asking. A plugin directory makes that hold for free when MCP servers and
hooks arrive. Leaving it flat would mean a cache migration and a forced
re-consent for everyone who ever installed anything, because the fingerprint
boundary itself moves.

  skills/<id>/SKILL.md  ->  plugins/<name>/skills/<id>/SKILL.md

WHY OUR OWN BUILT-INS ARE NOT PUBLISHED HERE, since teach and the ML family are
the obvious omissions and the reason is structural rather than editorial.

An installed skill claiming a bundled id is not applied at all. The registry
reserves bundled ids against origin `installed` and reports "was not applied:
that name belongs to a built-in skill". Rejected, not shadowed. Publishing the
ML family here would therefore have shipped a plugin that installs nothing and
emits seven notes, which is a worse first experience than shipping two plugins
that install cleanly.

That reservation is correct and deliberate: an installed file taking a bundled
id would replace the built-in AND strip the finalOverride slot it relies on.
Teach is the same boundary from the other side, needing that slot for its
length rule and so unable to be published faithfully at all.

The corollary is the useful half, and it is now written into the design spec:
dogfooding the install path always needs content that does not already ship.

CI, AND WHY IT COMES FIRST. A contributor whose first pull request is rejected
by a check learns the rule in thirty seconds. One whose skill merges and then
silently does nothing learns that we do not review, and that cannot be taken
back. So the checks land in the same change as the invitation: no override
block, no model or provider named, every plugin has a manifest that parses,
every plugin is listed and every listing exists, every SKILL.md has frontmatter
carrying name and description.
@ms-shashank
ms-shashank force-pushed the restructure/plugin-layout branch from 6dd7c93 to c088aaa Compare August 12, 2026 06:10
@ms-shashank
ms-shashank merged commit 738535f into main Aug 12, 2026
1 check passed
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.

1 participant