feat: skill activation routing — prefer a matching skill over model defaults (closes #271)#298
Conversation
…efaults (closes #271) An installed skill whose description matched a request was ignored: the model answered from its own defaults ("I need a timezone…") until the user said "use the skill". The catalog is inert without a directive to consult it first. Two-sided fix: 1. Runtime routing directive (forge-cli/runtime/runner.go, buildSkillCatalog): the `## Available Skills` preamble now tells the model to check for a matching skill BEFORE answering from its own defaults and, on a match, read_skill + follow it — with a guard to fall back to defaults only when NO skill matches (no over-routing). The mechanical read_skill how-to stays. 2. Trigger-rich descriptions (forge-ui/skill_builder_context.go): the Skill Builder now writes a `description` that states WHEN the skill fires (the phrases/intents a user says), because the agent routes by matching the request against the catalog description before loading the skill. Added a "Trigger-rich description" section (weak vs strong example) and strengthened the worked german-brisbane-time example's description. Tests: TestSkillCatalog_HasRoutingDirective (routing directive + no-over-route guard) and TestSkillBuilderPrompt_TriggerRichDescription. Docs: skill-md-format gains a "Skill activation / routing" section; skill-builder-llm notes trigger- rich descriptions.
initializ-mk
left a comment
initializ-mk
left a comment
There was a problem hiding this comment.
Review: skill activation routing
The two-sided approach is right, and the directive's wording is well-constructed — "FIRST check", "instead of answering directly", and the guard sentence each map to a specific #271 failure mode, with all three plus the retained read_skill how-to pinned in the test. The deterministic keyword-routing escalation is sensibly deferred with a measure-first rationale.
Verified: #297 merged so the stacking is clean; the parser has no description length cap, so longer trigger-rich descriptions won't break validation; the docs cross-link anchor (#skill-activation--routing) slugifies correctly; Lint + doc-link already pass.
No blocking findings — two minors inline, one follow-up ask here:
Minor (follow-up): the bundled skills still have "weak"-style descriptions
The routing directive now governs every installed skill, but the embedded skills (tavily-research, k8s-incident-triage, web-browse, …) predate the guidance — their catalog descriptions are exactly the capability-style this PR's own prompt warns "never routes." #271's fix is only as good as the descriptions in the catalog, so the shipped skills remain as prone to the original failure as before. Please file (or ride along) a sweep of forge-skills/local/embedded/*/SKILL.md descriptions using the new trigger-rich guidance — that's what closes the loop for the out-of-box experience.
Verdict
Merge-ready pending the in-flight checks. The one real ask is the embedded-skill description sweep above — everything else is observation-grade.
|
|
||
| ### Skill activation / routing | ||
|
|
||
| Installed skills are advertised to the agent in an `## Available Skills` catalog (built by the runtime) as `- name: description`. The agent is directed to **check the catalog for a matching skill before answering from its own defaults** and, on a match, `read_skill` to load and follow it (issue #271). Two things make this reliable: |
There was a problem hiding this comment.
Minor (design observation worth one sentence here): description now serves two audiences. It's (a) the internal routing trigger in this catalog AND (b) the public capability summary projected into the A2A Agent Card skills[] (FWS-1). Trigger-rich descriptions — When the user asks the time ("what time is it", "current time", "clock"), … — will appear verbatim in the public card, reading like routing rules rather than capability summaries to external A2A callers. Defensible trade-off (trigger phrasing is arguably more informative for callers too), but currently an undocumented consequence: one sentence acknowledging the card projection makes it a deliberate choice, and a separate triggers: field remains the escape hatch if the two audiences ever diverge.
| // matches the request is ignored (the model answers from its defaults and | ||
| // the user has to say "use the skill"). The final sentence guards against | ||
| // over-routing: fall back to defaults only when nothing matches. | ||
| b.WriteString("Before answering any request from your own knowledge or default behavior, FIRST check whether it matches one of the skill descriptions below. If a skill matches, call `read_skill` to load it and follow its instructions instead of answering directly — skills exist precisely to override your defaults for these cases, and a loaded skill's instructions are authoritative over your general behavior. Only answer from your own defaults when NO skill matches the request.\n\n") |
There was a problem hiding this comment.
Nit (no change needed, noting for future reviewers): "a loaded skill's instructions are authoritative over your general behavior" deliberately escalates skill-content authority in the system prompt. It's consistent with the trust model — skills are operator-installed, guardrails still gate all five gates, and the pre-existing preamble already said "follow them" — but prompt-injection-via-skill-content audits will trip on this line later, so it's worth being on record that the escalation is intentional and bounded.
…tional skill-authority escalation (#298 review)
|
Thanks — and for verifying the stacking / no-length-cap / anchor details. Follow-up filed: #299 — sweep every Both inline minors handled here (neither changes behavior):
|
1 participant
Closes #271.
Problem
With a correct, installed skill, the agent still answered from its own defaults instead of using the skill:
The forced call proves the skill works — activation/dispatch is the failure. The
## Available Skillscatalog preamble only explained how toread_skill, never told the model to prefer a matching skill over its own default answer.Fix (two-sided)
1. Runtime routing directive (
runner.go,buildSkillCatalog). The catalog preamble now leads with:The last sentence is the no-over-routing guard (unrelated requests still answer normally). The mechanical
read_skillhow-to is retained.2. Trigger-rich descriptions (Skill Builder prompt). The agent routes by matching a request against each skill's catalog
descriptionbefore loading it, so a vague description never routes. The builder now writes descriptions that state when the skill fires:description: German time skilldescription: When the user asks the time ("what time is it", "current time", "clock"), reply in German with Brisbane time.Added a "Trigger-rich description" section + strengthened the worked example.
Acceptance criteria
The open question (deterministic keyword→auto-
read_skilllayer) is deliberately deferred per the issue: prompt directive first, measure, escalate only if prompt-only routing proves unreliable.Tests
TestSkillCatalog_HasRoutingDirective(forge-cli) andTestSkillBuilderPrompt_TriggerRichDescription(forge-ui) pin both sides. Build / vet / test / lint clean across both modules. (forge-ui tests run in CI via the coverage added in #276.)Relates to #270 (merged) — trigger-rich descriptions build on the same Skill Builder prompt.