Skip to content

fix(composer): write a skill named like a slash command as /skill:<name> so choosing it invokes the skill - #630

Merged
mrgoonie merged 4 commits into
mainfrom
fix/623-skill-command-collision
Oct 8, 2026
Merged

mrgoonie merged 4 commits into
mainfrom
fix/623-skill-command-collision

Conversation

@mrgoonie

@mrgoonie mrgoonie commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

What was wrong

A skill can share its name with a built-in slash command, for example a skill called new. Choosing that skill in the / picker wrote /new … into the draft. The node reads slash commands from the text before it looks at references, so the message ran /new and opened a fresh conversation instead of invoking the skill.

Fix

  • referenceToken (contracts) writes a skill as /skill:<name> when /<name> would parse as a command. Skills that don't collide keep /<name>. The rule comes from parseSlashCommand, so it follows SLASH_COMMANDS and stays compatible with feat(widgets): let the person choose a project folder for widget dev sessions #612 (/develop).
  • The picker row for such a skill shows the token it will write. A query of skill:… after / lists only skills.
  • A typed /new with no chosen skill still runs the command.
  • Words take the same path. /skill:<name> is also the form pi expands by itself, so typing it invokes the same skill.
  • When the host already briefed the chosen skill (checked revision, no file location), the turn's words start with a space. This stops pi from expanding the leading /skill: a second time. A /skill: the person typed without choosing the skill is not briefed, so pi still expands it.
  • Docs (EN/VI): DESIGN.md composer rule and docs/open-interfaces.md picker paragraph. Official API docs: docs(api): write a skill that shares a command's name as /skill:<name> clarkcant-web#136.

Tests

  • Unit tests:
    • packages/conversation-client/test/composer-trigger.spec.ts: choosing new writes /skill:new, which is not a command and carries the skill. A typed /new carries no skill.
    • apps/runtime/test/composer-references.spec.ts: the token and the skill: query. Sending /skill:new … with the reference returns a model turn, with no appIntent and no new conversation, and the skill is briefed. A typed /new still answers nav.home.
  • Playwright (apps/web/e2e/composer-references.spec.ts): the fixture node gets a skill named new. The test chooses that skill's row (not the command's) and sends. It checks:
    • the request text is /skill:new … and carries the skill reference;
    • the reply is briefed with <skill name="new">;
    • there is no nav.home and no "new conversation" answer.
  • Fails on main. With only the source changes reverted:
    • 2 client and 2 runtime unit tests fail.
    • The e2e fails on the row label. A behaviour-only copy of the e2e also fails: the send ran /new, and no assistant turn appeared.
  • pnpm verify passed: 570 files and 7698 tests, plus typecheck, lint and invariants. The full composer-references e2e passed (10/10).

Changes after review

  • Stale revision. The leading space is now decided from the message's own skill references (referencedSkillIds + wordsBesideReferences), wired through a skillsFor lookup beside briefFor. It no longer searches the brief text. Before this, a skill whose chosen revision changed before the turn ran was left out of the brief, so the words had no space and pi inserted the current file.
  • Helper tests. apps/runtime/test/composer-references.spec.ts covers the normal case (referenced new), a hand-typed /skill:new with no reference, another skill referenced, a typed /new, a mid-text token, and the stale-revision case (the brief says the skill was not inserted, and the words still start with a space).
  • pi's real rule. packages/pi-adapter/test/skill-command-boundary.spec.ts adds a test on the real SDK, with and without the context guard: /skill:deploy … is expanded, while /skill:deploy … is sent as typed.
  • Nits. The brief line names the skill by its draft token (/skill:new). The client no longer reads the /skill of /skill:new as a skill called skill. The DESIGN lines are rewrapped. Old stored messages that show a /skill:<name> chip are left as they are: they show what was sent, and changing them would need a stored-message migration for a display-only nit.
  • Verified on 72f0cb3: focused vitest 113/113 (composer-references, composer-trigger, skill-command-boundary, slash-commands, context-wiring, model-generation); pnpm typecheck, eslint on the changed files and pnpm invariants clean; Playwright composer-references + slash-commands 14/14.

Round 2

  • Colon regression. The first version of the colon rule let a colon end a token only before a space or the end of the text. That dropped the chosen reference in @main.ts:42, @proj:C:\x, @team:10:30 and /review:xem. Now only the /skill token before :<name> is held back. Every other token ends at a colon as it did before this PR.
  • Regression tests in packages/conversation-client/test/composer-trigger.spec.ts: @main.ts:42, @proj:C:\x, @team:10:30 and /review:xem keep their reference. The existing test still shows that /skill:new is not read as a skill called skill. The new test fails on 72f0cb3 and passes now.
  • Merged main (including feat(widgets): let the person choose a project folder for widget dev sessions #612 /develop) with no conflicts.
  • Verified on 0144040: focused vitest 117/117; pnpm typecheck, eslint on the changed files and pnpm invariants (15/15) clean; Playwright composer-references + slash-commands 15/15, including the /develop test from feat(widgets): let the person choose a project folder for widget dev sessions #612.

Fixes #623

…me> so choosing it invokes the skill

A skill called new was written into the draft as /new, so the message ran the /new command instead of invoking the skill. The token for a skill whose /<name> parses as a command is now /skill:<name>, the form pi itself reads; a typed /new keeps running the command. The picker row shows that token, /skill: lists skills only, and a turn whose skill the host already briefed does not hand pi the leading /skill: to expand a second time.
…ences

A /skill:<name> chosen from the picker is briefed by the node, so pi must not
expand it again. The guard now reads the message's own skill references
instead of searching the brief, so a revision gone stale before the turn no
longer lets pi insert the current file. The brief names the skill by its
draft token, the client no longer reads the /skill of /skill:new as a skill
called skill, and a real-SDK test pins that a leading space stops expansion.
Only /skill before :<name> is held back from ending a token, so /skill:new
is still not a skill called skill, while @main.ts:42, @Proj:C:\x,
@team:10:30 and /review:xem keep their chosen reference again.
@mrgoonie

mrgoonie commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Review attestation: ready to merge at 014404083c38be2f7152c4bfbea182a2e13f2bd7, reviewed by agent:code-reviewer.

A push to this PR makes this attestation stale; the new head needs its own review.

@mrgoonie
mrgoonie enabled auto-merge (squash) October 8, 2026 01:43
@mrgoonie
mrgoonie merged commit c30a436 into main Oct 8, 2026
22 of 23 checks passed
@mrgoonie
mrgoonie deleted the fix/623-skill-command-collision branch October 8, 2026 01:51
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.

fix(composer): choosing a skill named like a slash command runs the command

1 participant