Skip to content

Skills part 3: invoking a skill by name, from either composer - #92

Open
shanforge wants to merge 2 commits into
shan/issue-85-skill-executionfrom
shan/issue-86-skill-invocation
Open

Skills part 3: invoking a skill by name, from either composer#92
shanforge wants to merge 2 commits into
shan/issue-85-skill-executionfrom
shan/issue-86-skill-invocation

Conversation

@shanforge

Copy link
Copy Markdown
Collaborator

Part of #56. Closes #86 — the discoverability box of #64.

Stacked on #91 — review that first; this branch contains it.

One component, mounted twice

The skill list is an item in ComposerPlusMenu, which both surfaces already mount — so the
island gets skills because the menu has them, not because anything was written twice. That
is #61's rule, and the reason this box cost almost no island-specific code.

The menu shows Weekly Review /weekly-review, which is doing real work: what you read and
what you type are different strings, and the menu is where that gets said.

Typing the name

/weekly-review cover the last week runs that skill on "cover the last week". The rule is
pure and outside both composers, for the reason AskRouter is — a decision made inside a
View is one the other surface cannot reach — and AskSurface is deliberately not an input.

Four decisions, each with a case:

  • The marker has to lead, or what is 3/4 of this tries to run a skill called "4".
  • A bare / is someone still typing, not a failure. An error under the cursor before the
    word is finished is worse than saying nothing yet.
  • The name is one token. /Weekly Review reads "Weekly" as the name — a space is what
    separates a name from a question, and it has to be. This is exactly why the menu prints the
    invocation next to the title.
  • An unmatched name stops the send. Someone who typed /weekly-reveiw meant to run
    something; answering it as an ordinary message gives them a confident reply to a question
    they did not ask, with nothing saying the skill never ran. The message names what was
    tried, because "no such skill" leaves them wondering whether they mistyped it or never made
    it.

An armed skill is a mode, not an attachment

ComposerChipRow never hides a mode, and a skill belongs in that category: what the send is
about to do has to be visible before it happens. Both composers show the chip; both clear
it after a send.

What the review rounds found

The precedence rule was written twice. Both surfaces called the same pure resolve, then
each decided for itself whether a typed name beat an armed chip — the per-surface redraw the
rest of #61 exists to stop, in the two places most likely to drift.
SkillInvocation.turn is the whole decision now, and neither composer re-derives any of it.

A skill was silently dropped on any route but the agent loop. Arm a skill, arm Deep
Research, send: a skill layers onto the agent's system prompt and narrows the agent's tools,
and a research run has neither — so it was quietly ignored. The same failure this PR refuses
for an unknown name, and for the same reason. It is refused now, naming both the skill and
what it clashed with.

And the marker leaked into the research prompt. Both surfaces passed the raw text to Deep
Research, so /weekly-review look at Q3 researched a question with a stray token glued to
the front. Both now pass the message with the name read off.

The main window gained the island's error handling

The refusal needed somewhere to go, and coordinator.lastError(in:) is scoped to a
conversation a refused send does not have — the trap the island's localError exists for.
Rather than sending the main window down a different path (a toast, which the island cannot
host), it gets the same localError ?? lastError banner. Both surfaces refuse a send the
same way
, which is the ground rule rather than a nicety.

Verification

  • xcodebuild build — succeeds
  • ./scripts/test-no-llm.sh1871 tests in 169 suites pass
  • SwiftFormat 0.62.1 --lint — 0/570 files require formatting
  • SwiftLint 0.65.0 --strict — 0 violations in 728 files

Mutation-checked: answering an unmatched name as a plain message turns two cases red;
dropping a skill silently off the agent route turns three red.

Two size caps were hit and split rather than suppressed: sendMessage past the 60-line
function cap (the invocation reading became a helper), and CommandCenterChatView one line
past the 450-line type cap (hasStagedChips moved to +Composer).

What still needs a person

The click-through for the whole skills feature lands with #87, which adds the editor. What
is testable here:

  1. The + menu on both surfaces lists the three built-ins with their /names.
  2. Pick one — a chip appears saying which. Send: the answer follows the skill.
  3. Type /weekly-review something — same result, no chip needed.
  4. Type /nonsense something — the send is refused and the banner names nonsense.
    This is the case that used to answer a question you did not ask.
  5. Arm a skill and Deep Research together — refused, naming both.
  6. Attach four files with a skill armed — the skill chip is still visible; it is a mode,
    and modes are never the ones dropped.

🤖 Generated with Claude Code

https://claude.ai/code/session_01M3Wpnj9ZmWPKYdFPB1AVY3

shanforge and others added 2 commits September 4, 2026 13:19
Part of #56. Closes #86 — the discoverability box of #64.

## One component, mounted twice

The skill list is an item in `ComposerPlusMenu`, which both surfaces already mount — so the
island gets skills because the menu has them, not because anything was written twice. That
is #61's rule, and the reason this box cost almost no island-specific code.

The menu shows `Weekly Review  /weekly-review`, which is doing real work: what you read and
what you type are different strings, and the menu is where that is said.

## Typing the name

`/weekly-review cover the last week` runs that skill on "cover the last week". The rule is
pure and outside both composers, for the reason `AskRouter` is — a decision made inside a
`View` is one the other surface cannot reach — and `AskSurface` is deliberately not an
input.

Four decisions in it, each with a case:

- **The marker has to lead.** Otherwise `what is 3/4 of this` tries to run a skill called
  "4".
- **A bare `/` is someone still typing**, not a failed invocation. Putting an error under
  the cursor before the word is finished is worse than saying nothing yet.
- **The name is one token.** `/Weekly Review` reads "Weekly" as the name — the space is what
  separates a name from a question, and it has to be.
- **An unmatched name stops the send.** Someone who typed `/weekly-reveiw` meant to run
  something; answering it as an ordinary message gives them a confident reply to a question
  they did not ask, with nothing anywhere saying the skill never ran. The message names what
  was tried, because "no such skill" leaves them checking whether they mistyped it or never
  made it.

## An armed skill is a mode, not an attachment

`ComposerChipRow` never hides a mode, and a skill belongs in that category: what the send is
about to **do** has to be visible before it happens. Both composers show the chip, and both
clear it after a send.

A typed name beats an armed chip — it is the more specific instruction, and the one under
the cursor as Return is pressed.

## The main window gained the island's error handling

The refusal needed somewhere to go, and `coordinator.lastError(in:)` is scoped to a
conversation a refused send does not have — the same trap the island's `localError` exists
for. Rather than sending the main window down a different path (a toast, which the island
cannot host), it gets the same `localError ?? lastError` banner the island already has. Both
surfaces refuse a send the same way.

## Verification

- `xcodebuild build` — succeeds
- `./scripts/test-no-llm.sh` — 1862 tests in 168 suites pass
- SwiftFormat 0.62.1 `--lint` — 0/570 files require formatting
- SwiftLint 0.65.0 `--strict` — 0 violations in 728 files

Mutation-checked: making an unmatched name fall back to an ordinary message turns two cases
red, including the one that exists for exactly that.

`sendMessage` went past the 60-line function cap on the way, so the invocation reading is
its own helper — the project's rule is to split, not to suppress.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M3Wpnj9ZmWPKYdFPB1AVY3
…ped in silence

**The precedence rule was written twice (major).** Both surfaces called the same pure
`resolve`, but each then decided for itself whether a typed name beat an armed chip. That is
the per-surface redraw the rest of #61 exists to stop — two copies of a rule that can drift,
in the two places most likely to drift.

`SkillInvocation.turn` is now the whole decision: read the name, choose between typed and
armed, and settle whether a skill applies to this route. Both composers call it and neither
re-derives any part of it.

**A skill was silently dropped on any route but the agent loop (major).** Arm a skill, arm
Deep Research, send: the skill layers onto the agent's system prompt and narrows the agent's
tools, and a research run has neither — so it was quietly ignored. That is the same failure
as answering an unknown name as an ordinary message, which this PR refuses precisely because
the user asked for one thing and got another with nothing saying so. It is refused now, and
the message names both the skill and what it clashed with.

**And the marker leaked into the research prompt.** Both surfaces passed the raw text to
Deep Research, so `/weekly-review look at Q3` researched a question with a stray token glued
to the front of it. Both now pass the message with the name read off.

Mutation-checked: dropping the skill silently instead of refusing turns three cases red.

`CommandCenterChatView` went one line past the 450-line type-body cap on the way, so
`hasStagedChips` moved to `+Composer` — where everything it asks about is already drawn.

Verified: build succeeds, 1871 tests in 169 suites pass, SwiftFormat --lint clean over 570
files, SwiftLint --strict 0 violations in 728 files.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M3Wpnj9ZmWPKYdFPB1AVY3
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