Bug: ctx.skills.register() does not default source, so plugin-registered skills fail to load

[AutumunC927]

Bug: ctx.skills.register() does not default source, so any plugin-registered skill fails to load

Summary

A plugin that registers a skill with ctx.skills.register({ name, description, content, invocation }) gets a skill that appears in the skill list but cannot be loaded. Loading it fails with:

Error: loaded skill "<name>" source must be a string

There is no error at registration time, so the failure is only discovered when the skill is actually used — which makes this hard to attribute.

Root cause

Two internal inconsistencies in the skills registry:

1. register() defaults invocation and provider, but not source.

// packages/.../dsh-skill — register()
register(skill) {
  validateRuntimeSkill(skill);            // does NOT check `source`
  const definition = {
    ...skill,
    invocation: skill.invocation ?? { modelInvocable: true, userInvocable: true },
    provider: skill.provider ?? RUNTIME_PROVIDER   // "runtime"
    // ← no `source` default here
  };
  ...
}

2. runtimeCandidate() passes source straight through.

function runtimeCandidate(skill) {
  return {
    name: skill.name,
    description: skill.description,
    invocation: skill.invocation,
    source: skill.source,        // undefined when the caller omitted it
    provider: skill.provider,    // "runtime" (defaulted above)
    rank: RUNTIME_RANK,
    locator: skill,
    ...
  };
}

3. validateDefinition() then requires it to be a string.

function validateDefinition(skill) {
  ...
  if (typeof source !== "string") throw new TypeError(`loaded skill "${name}" source must be a string`);
  if (typeof provider !== "string") throw new TypeError(`loaded skill "${name}" provider must be a string`);
  ...
}

So provider is always fine (it has a default) while source is always undefined (it does not). The registration path validates a different shape than the read-back path, so an incomplete definition is accepted at registration and rejected on use.

Impact

Any plugin using ctx.skills.register() is affected unless it happens to pass source explicitly. The skill is listed (so it looks installed) but is unusable, and the error message points at the skill rather than at the registry, which makes it look like the plugin is at fault.

Reproduction

Register a skill exactly as documented, without source:

root.inject(['skills'], ctx => {
  return ctx.skills.register({
    name: 'my-plugin-skill',
    description: 'A skill registered by a plugin.',
    content: '# Hello',
    invocation: { modelInvocable: true, userInvocable: true },
  })
})

Then load it (skill(name: "my-plugin-skill")). Result:

Error: loaded skill "my-plugin-skill" source must be a string

Adding source: 'runtime' makes it load correctly.

Suggested fix

Default source in register() alongside the existing provider default, e.g.:

const definition = {
  ...skill,
  invocation: skill.invocation ?? { modelInvocable: true, userInvocable: true },
  provider: skill.provider ?? RUNTIME_PROVIDER,
  source: skill.source ?? RUNTIME_PROVIDER,   // ← same value; "runtime"
};

Given RUNTIME_PROVIDER === "runtime" is already the reserved name for this registration kind, reusing it for source keeps the two fields consistent.

Alternatively, if source is genuinely meant to be caller-supplied, validateRuntimeSkill() should require it at registration so the mistake fails immediately and locally instead of at load time. Either way, the two validation paths should agree on which fields are mandatory.

Note

This is easy to work around from the plugin side (pass source: 'runtime' explicitly), and that is what I have done. But the defaulting asymmetry between provider and source looks unintentional, and the delayed/indirect error makes it a costly trap for plugin authors.

[argszero]

Good catch, and you have the right three call sites. Two refinements that decide what the fix should be.

1. The asymmetry is real and asymmetric in both directions of the type.

• register() (packages/skill/skill/src/index.ts:439-451) validates with validateRuntimeSkill (:741-746), which checks name, description and invocation — not source — and then fills defaults for invocation and provider only.
• runtimeCandidate() (:691-700) passes source: skill.source straight through, and validateDefinition() (:748, :763) requires it to be a string.

But note where the type stands today: SkillRegistration = Omit<SkillDefinition, 'invocation' | 'provider'> & {...} and SkillSummary.source: SkillSource is required, so a typed caller cannot omit source — the compile error is the first line of defence and it already fires. What reaches the runtime is therefore an untyped/JS caller, or a caller whose source is not a string at runtime (a value coming from JSON, any, or an explicit undefined). Worth stating in the thread so the blast radius is not overstated.

The README sentence for embedded skills ("the registry fills in a default invocation policy and the runtime provider label", packages/skill/skill/README.md:52) names exactly two defaults and not a third, which is why I read this as a less-validated write path rather than a documented default that regressed.

2. That makes "default it" and "reject it" two different fixes, and I'd let the maintainers choose — but the fail-fast one is the more faithful reading.

• Defaulting (source: skill.source ?? RUNTIME_PROVIDER) makes the documented shape complete, and SkillSource does include 'runtime', so the value is legal.
• Rejecting at registration — extending validateRuntimeSkill to validate the same shape validateDefinition will later demand — turns a "listed but unusable, error names the skill and not the registry" failure into a registration-time error that names the registry. That also covers the neighbouring case that a default cannot: a source present but not a string.

Either way the invariant that is missing today is the one worth writing down: the write path must validate the shape the read path requires — validateRuntimeSkill should be a subset-of-or-equal-to validateDefinition, not an independently drifting laxer check. That framing also makes the regression test obvious (register → list() → get(), which is the path you walked).

Two smaller observations from reading the tree: the same incomplete definition is also what list() projects, so a source-less runtime skill shows up in summaries with source: undefined at runtime despite the required field, i.e. the inconsistency is visible in the catalog before anyone tries to load it; and since runtime is a reserved provider name (:406-407), defaulting source to 'runtime' stays consistent with the existing reservation rather than introducing a new spelling.

I have no stake in which of the two fixes lands — but if it is useful, I can hold a plugin-side shim in the meantime (a registered-source default applied on the registry instance, so untyped callers on already-released versions get a loadable skill plus a warning that names the omission). Say the word and I'll publish it; otherwise I'll leave this as a pure upstream fix.

[AutumunC927]

请发表，如果日后有更好用的方案辛苦更新。