feat(ai-sessions): turn skills on by default, and group them by folder (#11058)

* feat(ai-sessions): turn skills on by default, and group them by folder

A skill is instructions the workspace wrote for the assistant to use, so what
carrying one costs is context rather than access. Selecting each one before it
applied made publishing a skill a two-step affair, and left most of them unused.

Skills now default to on. No storage is rewritten to get there: the preference
keeps its key and holds a decision per path, so the older array of enabled paths
still reads as "these were on" and only the paths nobody decided about move. MCP
servers stay opt-in through the same factory — their tools reach an external
system, which is a different question from context.

The Skills settings list groups into a tree once skills span more than one
folder, with a switch per folder acting on everything beneath it, and the list
answers the keyboard: Up/Down walk it, Left/Right fold, Space flips the switch
under the highlight, Enter opens the skill.

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

* fix: give a modal the option to stand only as tall as the window

`AIPromptsModal` asks for 1000px of height, which is taller than a laptop
window: the dialog then scrolled inside the overlay while its list scrolled
inside the dialog — two scrollbars, one of them moving the modal itself. The
cap `Modal2` appeared to have, `max-h-screen-80`, is defined nowhere in the
tailwind config, so it never applied to anything.

`fixedHeight="viewport"` is a new value that stands as tall as the window
allows. Deliberately a definite height rather than a max-height: bodies here
size against the box with `h-full` / `grow min-h-0` and scroll inside it, and a
max-height leaves them nothing to resolve against — they grow past the surface
instead. Every existing size keeps the height it has today, so no other modal
moves. The two classes that resolved to nothing are removed.

The prompts modal and the assistant settings modal take the new value; both
already scroll inside themselves.

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

* fix: pin read_skill's gate in its test, and say who a delete affects

The refusal `read_skill` gives for a path that is not a skill changed shape —
it checks the workspace listing now, not just the off-switch — and its test was
still asserting the old wording against an unmocked listing.

The delete confirmation said everyone "who selected it" loses the skill, which
stopped being true when skills started defaulting to on.

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

* fix: keep the keyboard walk when the list scrolls under the pointer

The mouse takes the skills list back on a real movement over it, not on
`mouseenter`. The browser fires that one whenever rows arrive under a
stationary pointer — every scroll the keyboard itself causes, and every folder
collapse — so walking Down past the bottom of the list handed control back to a
mouse nobody had touched, and the next press restarted at the top.

Also from the review round: the "+" menu sorted skills on-first, a key that is
constant now that they start on, and pushed the one row it did move — a skill
just turned off there — out of the shortcut that turns it back on. It orders by
path. The remaining "selection" wording follows the vocabulary the rest of this
change moved to.

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

* fix: carry the keyboard walk on from the row the mouse left it on

Handing the list to the mouse dropped the highlight, so the next arrow press
started again at the top. It moves to the row under the pointer instead —
invisible while the mouse leads, since drawing and acting both wait on the
keyboard being in charge, and exactly where someone would expect the walk to
carry on from.

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

* fix: cap the AI prompts modal from its own call site

Reverts `Modal2` and the assistant settings modal to what they were. The
prompts modal asks for `xxl`, 1000px, which is taller than a laptop window, so
the dialog scrolled inside the overlay while its list scrolled inside the
dialog. It now passes `max-h-[80vh]` through the `css.popup` the component
already forwards.

The height stays definite underneath, which is what lets the list bound its own
scroller, and nothing outside this one modal changes.

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

* refactor: drive the skills list highlight with useListHighlight

The Tools section next door already had this: `useListHighlight` owns the
highlighted index, wrapping, `scrollIntoView`, and the rule that a scroll under
a resting pointer must not hand the list back to the mouse — the bug this
section rediscovered the hard way. Reusing it drops the parallel implementation.

What stays local is what is actually a tree: Left and Right fold a folder or
step into it, Space flips the switch under the highlight, and Enter opens the
lit skill. `restingIndex` is what keeps the highlight on a folder through a
fold, where a search would instead send it back to its top hit.

The keys are answered at the window rather than on the list: leaving the editor
parks focus elsewhere, and a container-scoped handler goes silent when it does.
`move` is now returned by the composable, for the step into a folder's children.

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

* fix: stop the fold's sticky row resetting the keyboard walk

`stickyKey` is read through `restingIndex`, which `useListHighlight` calls
inside the effect that reacts to the row count. As `$state` it was also a
dependency of that effect, so clearing it on the next arrow re-ran the effect
and wrote the highlight back to nothing: after collapsing a folder, one Down
lit nothing and the one after it started again at the top.

It is a plain variable now, read when the effect runs and invalidating nothing.

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

* fix: keep one lit row, and keep the fold's sticky row to its fold

Three from the review of the `useListHighlight` swap:

The sticky row a fold takes is now given up as soon as that fold has rendered.
Held until the next arrow, it pulled the highlight back to that folder on any
later change — another fold, a save, a delete, a workspace switch.

Space and Enter on a focused control bring the highlight to that control's row
before the control answers them. A switch keeps focus after a plain click, and
the row drawn as highlighted was then a different one from the row that flipped.

Up and Down carry on from a row reached with Tab. `useListHighlight` cannot see
that by itself: `ListRow` puts the row's id on its outer div while focus sits on
the button inside it.

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

* fix: land the highlight on a named row rather than stepping to it

`move` counts steps from wherever the highlight is, and from nothing lit it can
only reach an end of the list — so the three places that meant "put it on this
row" (a row reached with Tab, the row of a focused control, a folder's parent)
sent it to the first row whenever nothing was lit yet. `useListHighlight` grows
a `moveTo` for naming the row outright, and those three use it.

The handler's own doc still said the keys are answered on the list; they went
back to the window when the editor's page transition proved able to take focus
away from it.

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

* fix: fold from the header click the way every other fold does

The header's own click wrote `collapsed` directly instead of going through
`fold`, so the row count changed with no row named to keep: the highlight reset,
and since the highlight is the header's only hover feedback, it went flat under
a pointer that had not moved and stayed flat.

Also from the round: a duplicated `svelte-ignore`, the missing one on the header
wrapper that takes `onmouseenter`, and a trailing comma prettier wanted gone.

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

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Guilhem
2026-09-11 10:27:37 +00:00
committed by GitHub
co-authored by Claude Opus 5
parent 172d6c275b
commit d8b9174235
14 changed files with 762 additions and 187 deletions
@@ -35,9 +35,9 @@ export function useListHighlight(opts: {
untrack(() => (index = resting))
})
function move(delta: number) {
function land(next: number) {
const count = opts.count()
if (count === 0) return
if (count === 0 || next < 0 || next >= count) return
pointerOwns = false
// Rows are tabbable, so focus can sit on one. Enter then activates whatever is
// focused, which has to stay the highlighted row — so any row counts, not just
@@ -47,16 +47,29 @@ export function useListHighlight(opts: {
const focusedId = document.activeElement?.id
const rowWasFocused =
!!focusedId && Array.from({ length: count }, (_, i) => opts.rowId(i)).includes(focusedId)
index = index < 0 ? (delta > 0 ? 0 : count - 1) : (index + delta + count) % count
index = next
const row = document.getElementById(opts.rowId(index))
row?.scrollIntoView({ block: 'nearest' })
if (rowWasFocused) row?.focus()
}
function move(delta: number) {
const count = opts.count()
if (count === 0) return
land(index < 0 ? (delta > 0 ? 0 : count - 1) : (index + delta + count) % count)
}
return {
get index() {
return index
},
/** Step the highlight, for a list whose own keys move it beyond Up and Down —
* a tree stepping into the children a folder just revealed. */
move,
/** Put the highlight on a row named outright, rather than a step from wherever
* it is — the row a caller's own key landed on, or the one that has focus. A
* step cannot say this: from nothing lit it can only reach an end of the list. */
moveTo: land,
/** Wire to each row's `onMouseEnter`. */
hovered(i: number) {
if (pointerOwns) index = i
@@ -580,11 +580,11 @@ describe('AIChatManager global skills', () => {
mocks.tryGetCurrentModel.mockReturnValue(model)
})
// Only selected skills reach the prompt, and the selection is keyed by
// workspace and account (see skills/enabledSkills.ts).
function selectSkills(workspace: string, ...paths: string[]) {
// Every readable skill reaches the prompt; only the paths someone decided about
// are stored, keyed by workspace and account (see skills/enabledSkills.ts).
function turnOffSkills(workspace: string, ...paths: string[]) {
const stored = JSON.parse(localStorage.getItem('wm_skills_enabled') ?? '{}')
stored[`${workspace}:${TEST_EMAIL}`] = paths
stored[`${workspace}:${TEST_EMAIL}`] = Object.fromEntries(paths.map((p) => [p, false]))
localStorage.setItem('wm_skills_enabled', JSON.stringify(stored))
}
@@ -594,8 +594,6 @@ describe('AIChatManager global skills', () => {
resolveParentSkills = resolve
})
mocks.workspace = 'parent'
selectSkills('parent', 'f/skills/parent-skill')
selectSkills('child', 'f/skills/child-skill')
mocks.listResource.mockImplementation(({ workspace }: { workspace: string }) => {
if (workspace === 'parent') {
return parentSkills
@@ -639,12 +637,12 @@ describe('AIChatManager global skills', () => {
expect(manager.systemMessage.content).not.toContain('parent-skill')
})
it('leaves a readable but unselected skill out of the prompt', async () => {
it('leaves a skill turned off out of the prompt', async () => {
mocks.listResource.mockResolvedValue([
{ path: 'f/skills/selected', description: 'the one turned on' },
{ path: 'f/skills/unselected', description: 'readable but never turned on' }
{ path: 'f/skills/selected', description: 'left on, like every skill starts' },
{ path: 'f/skills/unselected', description: 'the one turned off' }
])
selectSkills('test_workspace', 'f/skills/selected')
turnOffSkills('test_workspace', 'f/skills/unselected')
const manager = new AIChatManager()
manager.isSessionChat = true
@@ -659,7 +657,6 @@ describe('AIChatManager global skills', () => {
mocks.listResource.mockResolvedValue([
{ path: 'u/admin/review-code', description: 'review code for bugs' }
])
selectSkills('test_workspace', 'u/admin/review-code')
mocks.runChatLoop.mockImplementation(async (config: any) => {
const userMessage = config.messages[config.messages.length - 1]
expect(userMessage.content).toContain('Use the skill at "u/admin/review-code". find bugs')
@@ -686,7 +683,6 @@ describe('AIChatManager global skills', () => {
{ path: 'u/admin/deploy', description: 'personal deploy steps' },
{ path: 'f/team/deploy', description: 'the team deploy steps' }
])
selectSkills('test_workspace', 'u/admin/deploy', 'f/team/deploy')
mocks.runChatLoop.mockImplementation(async (config: any) => {
// Picking either one would silently apply instructions the user did not
// choose, so the text is left alone for the model to ask about.
@@ -482,7 +482,7 @@ switch that decides whether this chat carries its tools.
{#snippet subtitle()}{server.description}{/snippet}
{#snippet trailing()}
<Toggle
size="sm"
size="xs"
disabled={forkPending}
checked={server.enabled}
on:change={async (e) => await toggle(server.path, e.detail)}
@@ -24,19 +24,25 @@ and the actions that create, edit, import and delete them.
import { userStore } from '$lib/stores'
import { sendUserToast } from '$lib/toast'
import { logFeatureUsage } from '$lib/utils/featureUsage'
import { untrack } from 'svelte'
import { tick, untrack } from 'svelte'
import {
ArrowLeft,
BookOpen,
ChevronDown,
ChevronRight,
Eye,
Folder,
FolderTree,
FolderUp,
Pencil,
Plus,
RotateCcw,
Trash2
Trash2,
User
} from 'lucide-svelte'
import { useListHighlight } from '$lib/components/common/listRow/listHighlight.svelte'
import { getAiChatManager } from './aiChatManagerContext'
import { isSkillEnabled, setSkillEnabled } from './skills/enabledSkills'
import { forgetSkill, isSkillEnabled, setSkillEnabled } from './skills/enabledSkills'
import {
ambiguousSkillNames,
deleteSkillResource,
@@ -52,6 +58,17 @@ and the actions that create, edit, import and delete them.
parseSkillMd,
type SkillUpload
} from './skills/skillMd'
import {
buildSkillTree,
countSkills,
folderKey,
nodeSkills,
skillFolderPaths,
skillKey,
visibleEntries,
type SkillTreeEntry,
type SkillTreeNode
} from './skills/skillTree'
let {
ws,
@@ -162,6 +179,53 @@ What the assistant should do when this skill applies.
let overwriteChoices: Record<string, boolean> = $state({})
let ambiguous = $derived(ambiguousSkillNames(skills))
// Skills in one folder are a list; spread over several, the folder is what tells
// two of them apart, so they are grouped under it instead.
let grouped = $derived(skillFolderPaths(skills).size > 1)
let tree = $derived(buildSkillTree(skills))
/** Folders the user closed, keyed by full prefix. Everything opens expanded: the
* point of this list is seeing what the assistant carries. */
let collapsed = $state<Record<string, boolean>>({})
/** The list top to bottom as it stands, for the keyboard to walk. Empty whenever
* the rows are not the thing on screen — a reload after a save or a delete shows
* the loading line, and a failed one shows the error, both over the rows this
* would otherwise still walk. Flat mode has no folders, so the same keys drive
* both layouts. */
let entries: SkillTreeEntry<Row>[] = $derived(
loading || loadError !== undefined
? []
: grouped
? visibleEntries(tree, (path) => collapsed[path] === true)
: skills.map((skill) => ({ kind: 'skill', key: skillKey(skill.path), skill }))
)
/** Where each entry sits in the walk, so a row rendered deep in the tree can say
* whether it is the highlighted one. */
let entryIndexByKey = $derived(new Map(entries.map((e, i) => [e.key, i])))
/** The row the highlight goes back to when the list changes shape under it.
* Folding is the case: it changes the row count without reshuffling what the rows
* mean, and the folder just folded is where someone still is — unlike a search,
* which reranks everything and belongs back at its top hit.
*
* Deliberately not `$state`. `useListHighlight` reads this through `restingIndex`
* inside the effect that reacts to the row count, so a reactive write here would
* re-run that effect and reset the highlight — clearing it on the next arrow would
* undo the very move that cleared it. */
let stickyKey: string | undefined = undefined
const highlight = useListHighlight({
count: () => entries.length,
rowId: (index) => entryDomId(entries[index]?.key ?? ''),
// Otherwise nothing is lit until a key or the pointer picks a row: this list has
// no search ranking one to the top.
restingIndex: () => (stickyKey === undefined ? -1 : (entryIndexByKey.get(stickyKey) ?? -1)),
onActivate: (index) => {
const entry = entries[index]
if (entry === undefined) return
if (entry.kind === 'skill') openSkill(entry.skill)
else fold(entry.key, entry.node.path, !collapsed[entry.node.path])
}
// No `activateEnterFrom`: Enter is answered by `onListKeydown`, which does not
// depend on where focus happens to be.
})
let parsed = $derived(parseSkillMd(content))
// The Path field is what names the skill, and Path validates it. The frontmatter
// `name` only seeds that field and is never persisted, so validating it here
@@ -219,21 +283,154 @@ What the assistant should do when this skill applies.
if (!active && !contentChanged && !pathChanged) editorOpen = false
})
/** The list takes focus when this section is the one on screen.
*
* Without it the keyboard here is unreachable by the way people arrive: the click
* on "Skills" in the settings sidebar leaves that button focused, and the sidebar
* answers Up/Down itself (`arrowTabNav`), so the arrows would walk the sections
* rather than the skills. Taking focus is also what makes the keys unambiguous —
* a control keeps Space and Enter, and none holds them once the list has focus. */
let listEl: HTMLDivElement | undefined = $state(undefined)
$effect(() => {
if (!active || editorOpen) return
const el = listEl
untrack(() => {
if (el && !el.contains(document.activeElement)) el.focus({ preventScroll: true })
})
})
/** Escape leaves the editor rather than the whole modal: `blocksClose` stops the
* modal's own handler, so this is the only thing left to answer the key. */
function onKeydown(event: KeyboardEvent) {
// Every section stays mounted while the modal is open, and `stopPropagation`
// does nothing between listeners on `window`: without this, a key aimed at the
// section on screen is answered by the four behind it too.
if (!active || event.key !== 'Escape' || !editorOpen) return
if (!active) return
// A dialog of ours is up and owns the keyboard.
if (toDelete !== undefined || pendingImport !== undefined) return
event.preventDefault()
event.stopPropagation()
closeEditor()
if (editorOpen) {
if (event.key !== 'Escape') return
event.preventDefault()
event.stopPropagation()
closeEditor()
return
}
onListKeydown(event)
}
/** DOM id of a row, so the highlight can bring itself into view. */
function entryDomId(key: string): string {
return `wm-skill-entry-${key}`
}
/** Fold or unfold, keeping the highlight on the folder rather than losing it to
* the row count changing underneath. */
function fold(key: string, path: string, shut: boolean) {
stickyKey = key
collapsed[path] = shut
// Held only across the re-render this fold causes. Left set, it would pull the
// highlight back to this folder on the next change of any kind — another fold, a
// save, a delete, a workspace switch.
tick().then(() => (stickyKey = undefined))
}
/** The tree keys this list adds to `useListHighlight`: Left and Right fold a folder
* or step into it, Space flips the switch under the highlight. Up, Down and Enter
* are the composable's, and so is everything about the mouse — a scroll under a
* resting pointer does not move the highlight, only a real movement does.
*
* Answered at the `window`, not on the list: focus moves around this modal — the
* page transition out of the editor parks it elsewhere — and a handler bound to the
* list would go silent whenever it did. Keys left unanswered keep their meaning:
* Left and Right with nothing lit still step between this list and the editor,
* which is `PagedContent` reading the same event. */
function onListKeydown(event: KeyboardEvent) {
if (event.metaKey || event.ctrlKey || event.altKey || event.defaultPrevented) return
const target = event.target as HTMLElement | null
// A focused control answers its own activation keys — a row's switch, a folder
// header, the buttons above the list. `Toggle` hides a real checkbox behind its
// label (frontend/AGENTS.md), and that checkbox holds focus after a plain click.
const control = target?.closest?.('button, a, input, select, textarea, [role="button"]') as
| HTMLElement
| null
| undefined
if ((event.key === ' ' || event.key === 'Enter') && control) {
// The control answers the key, and the highlight follows it there: a switch
// holds focus after a plain click, and leaving another row lit would draw one
// row while flipping another.
const row = control.closest('[id^="wm-skill-entry-"]')
const index = row ? (entryIndexByKey.get(row.id.replace('wm-skill-entry-', '')) ?? -1) : -1
if (index >= 0) highlight.moveTo(index)
return
}
if (event.key === 'ArrowDown' || event.key === 'ArrowUp') {
// A row reached with Tab is where the walk carries on from. `useListHighlight`
// cannot see that itself: `ListRow` puts the row's id on its outer div while
// focus sits on the button inside it.
const focusedRow = target?.closest?.('[id^="wm-skill-entry-"]')
const focusedIndex = focusedRow
? (entryIndexByKey.get(focusedRow.id.replace('wm-skill-entry-', '')) ?? -1)
: -1
if (focusedIndex >= 0) highlight.moveTo(focusedIndex)
// Taking the highlight takes the keyboard with it: a control left focused
// would keep Space and act on its own row while another one is lit.
if (control) {
control.blur()
listEl?.focus({ preventScroll: true })
}
highlight.onKeydown(event)
return
}
const current = entries[highlight.index]
if (current === undefined) {
highlight.onKeydown(event)
return
}
const answer = () => {
event.preventDefault()
event.stopPropagation()
}
if (event.key === ' ') {
answer()
if (current.kind === 'skill') void toggle(current.skill.path, !current.skill.enabled)
else void toggleFolder(current.node, !folderEnabled(current.node))
} else if (event.key === 'Enter') {
answer()
if (current.kind === 'skill') openSkill(current.skill)
else fold(current.key, current.node.path, !collapsed[current.node.path])
} else if (event.key === 'ArrowRight') {
answer()
if (current.kind === 'skill') {
// Forward from a lit skill is that skill. Left unanswered the key reaches
// `PagedContent`, which steps to the editor page — showing whichever skill
// it was last parked on, not this one.
openSkill(current.skill)
} else if (collapsed[current.node.path]) {
fold(current.key, current.node.path, false)
} else {
// Into what opening it revealed: the next row down is its first child.
highlight.move(1)
}
} else if (event.key === 'ArrowLeft') {
if (current.kind === 'folder' && !collapsed[current.node.path]) {
answer()
fold(current.key, current.node.path, true)
} else if (current.parentKey !== undefined) {
answer()
const parent = entries.findIndex((e) => e.key === current.parentKey)
if (parent >= 0) highlight.moveTo(parent)
}
} else {
highlight.onKeydown(event)
}
}
function closeEditor() {
editorOpen = false
// The list is what answers the keys, so it takes focus back as the editor gives
// way. Left to the effect above, the page transition lands focus somewhere else
// afterwards and the arrows do nothing until something is clicked.
tick().then(() => listEl?.focus({ preventScroll: true }))
}
/** Left and Right step between the two pages, which is `PagedContent` answering the
@@ -254,6 +451,8 @@ What the assistant should do when this skill applies.
untrack(() => {
loadSeq++
skills = []
// Keyed by path, and another workspace's folders are not these.
collapsed = {}
listNotice = undefined
toDelete = undefined
pendingImport = undefined
@@ -289,7 +488,7 @@ What the assistant should do when this skill applies.
// A notice, not `loadError`: that one replaces the list, and a truncated
// read still has skills worth showing.
listNotice = truncated
? `Showing the first ${found.length} skills; this workspace has more. Delete unused ones so the rest can be selected.`
? `Showing the first ${found.length} skills; this workspace has more. Delete unused ones so the rest are listed here.`
: undefined
// Seeded rather than filled by the bindings: an unset entry would hand
// DropdownV2 an `undefined` open state instead of a closed one.
@@ -307,19 +506,41 @@ What the assistant should do when this skill applies.
async function toggle(p: string, enabled: boolean) {
if (blockedByPendingFork()) return
if (!setSkillEnabled(ws, p, enabled)) {
sendUserToast('Could not save the selection for this account.', true)
sendUserToast('Could not save this choice for this account.', true)
return
}
const skill = skills.find((s) => s.path === p)
if (skill) skill.enabled = enabled
// Whether people select skills at all. Never the skill itself: a path is
// Whether people turn skills off at all. Never the skill itself: a path is
// workspace-authored text.
logFeatureUsage('ai_session', 'skill_toggle', { key: enabled ? 'on' : 'off', workspace: ws })
// The prompt lists exactly the enabled skills, so it has to be rebuilt
// The prompt lists exactly the skills that are on, so it has to be rebuilt
// before the next message rather than on the next mode change.
await aiChatManager.refreshGlobalSkills(ws)
}
/** A folder is on only when everything under it is, so its switch always offers
* the completing action: a folder with one skill off turns fully on first. */
function folderEnabled(node: SkillTreeNode<Row>): boolean {
return nodeSkills(node).every((s) => s.enabled)
}
async function toggleFolder(node: SkillTreeNode<Row>, enabled: boolean) {
if (blockedByPendingFork()) return
for (const skill of nodeSkills(node)) {
if (skill.enabled === enabled) continue
if (!setSkillEnabled(ws, skill.path, enabled)) {
sendUserToast('Could not save this choice for this account.', true)
return
}
skill.enabled = enabled
}
// Once for the folder, not once per skill: what this counts is the action a
// person took. Never the folder itself, which is workspace-authored text.
logFeatureUsage('ai_session', 'skill_toggle', { key: enabled ? 'on' : 'off', workspace: ws })
await aiChatManager.refreshGlobalSkills(ws)
}
/** Personal folder the folder import writes into. The username is not always a
* legal path segment — a superadmin who is not a member of the workspace gets
* their email back from `whoami` — and `resource.path` is CHECK-constrained, so
@@ -425,11 +646,13 @@ What the assistant should do when this skill applies.
validated.skill.description,
validated.skill.instructions
)
// A move leaves the old path selected but gone; carry the choice over
// so an edit that renames does not silently switch the skill off.
if (editing.path !== path && isSkillEnabled(target, editing.path)) {
setSkillEnabled(target, editing.path, false)
setSkillEnabled(target, path, true)
// A move leaves the old path's state behind on a path that no longer
// exists; carry it over so an edit that renames does not switch the
// skill back on, and does not inherit whatever the new path held.
if (editing.path !== path) {
const wasEnabled = isSkillEnabled(target, editing.path)
forgetSkill(target, editing.path)
setSkillEnabled(target, path, wasEnabled)
}
} else {
await saveSkillResource(
@@ -438,8 +661,9 @@ What the assistant should do when this skill applies.
validated.skill.description,
validated.skill.instructions
)
// Authoring a skill is the act of choosing it.
setSkillEnabled(target, path, true)
// A skill written here now is not the one someone turned off at this path
// earlier, so it starts from the default rather than inheriting that.
forgetSkill(target, path)
}
editorOpen = false
await refresh(target)
@@ -456,9 +680,9 @@ What the assistant should do when this skill applies.
const target = ws
try {
await deleteSkillResource(target, skill.path)
// A later skill at this path is a different one; it must be turned on
// deliberately rather than inherit this one's selection.
setSkillEnabled(target, skill.path, false)
// A later skill at this path is a different one, and must not inherit a
// decision aimed at this one.
forgetSkill(target, skill.path)
sendUserToast(`Deleted ${skill.path}`)
await refresh(target)
} catch (e) {
@@ -589,7 +813,7 @@ What the assistant should do when this skill applies.
await saveSkillResource(target, dest, skill.description, skill.instructions, {
overwrite
})
setSkillEnabled(target, dest, true)
forgetSkill(target, dest)
written++
} catch (e) {
failed.push(`${skill.name} (${e.body ?? e.message})`)
@@ -626,10 +850,22 @@ What the assistant should do when this skill applies.
/>
{#snippet listPage()}
<div class="grow min-h-0 overflow-y-auto pr-2">
<!-- Stable gutter: collapsing a folder can take the list under the scrollable
height, and without the reserved space every row would jump sideways as the
scrollbar came and went. Focusable so the keys have somewhere to belong — see
the effect that focuses it. -->
<!-- svelte-ignore a11y_no_static_element_interactions -->
<!-- svelte-ignore a11y_no_noninteractive_tabindex -->
<div
bind:this={listEl}
tabindex="-1"
class="grow min-h-0 overflow-y-auto pr-2 outline-none"
style="scrollbar-gutter: stable;"
onpointermove={highlight.pointerMoved}
>
<Section
label="Skills"
description="Reusable instruction sets the assistant loads when they apply. Turning one on is personal to you and to this workspace."
description="Reusable instruction sets the assistant loads when they apply. Every skill in the workspace is on; turning one off is personal to you and to this workspace."
>
{#snippet action()}
<div class="flex items-center gap-2 shrink-0">
@@ -668,8 +904,8 @@ What the assistant should do when this skill applies.
{#if forkPending}
<Alert type="info" title="This session has no workspace yet" size="xs" class="mb-4">
Skills are read-only until the first message creates this session's fork. Editing or
selecting one now would apply to the parent workspace and stop applying once the fork is
Skills are read-only until the first message creates this session's fork. Editing one or
turning it off now would apply to the parent workspace and stop applying once the fork is
created.
</Alert>
{/if}
@@ -698,63 +934,143 @@ What the assistant should do when this skill applies.
/>
{:else}
<div class="flex flex-col gap-0.5">
{#each skills as skill (skill.path)}
{#snippet icon()}
<BookOpen size={16} class="text-tertiary" />
{/snippet}
{#snippet title()}
<span class="truncate leading-5">
{ambiguous.has(skill.name) ? skill.path : skill.name}
</span>
{/snippet}
{#snippet subtitle()}{skill.description}{/snippet}
{#snippet trailing()}
<Toggle
size="sm"
disabled={forkPending}
checked={skill.enabled}
on:change={async (e) => await toggle(skill.path, e.detail)}
/>
<DropdownV2
size="sm"
bind:open={rowMenuOpen[skill.path]}
items={[
{
// One entry for both halves of the detail: it opens on the rendered
// skill, and editing is the switch in its header.
displayName: 'Manage skill',
icon: skill.canWrite ? Pencil : Eye,
action: () => openSkill(skill)
},
{
displayName: 'Delete',
icon: Trash2,
type: 'delete',
disabled: !skill.canWrite || forkPending,
action: () => (toDelete = skill)
}
]}
/>
{/snippet}
<ListRow
{icon}
{title}
{trailing}
subtitle={skill.description ? subtitle : undefined}
onClick={() => openSkill(skill)}
/>
{/each}
{#if grouped}
{#each tree as node (node.path)}
{@render folder(node, true)}
{/each}
{:else}
{#each skills as skill (skill.path)}
{@render row(skill)}
{/each}
{/if}
</div>
{/if}
</Section>
</div>
{/snippet}
<!-- One skill. A snippet rather than markup inside the list, because the tree
renders the same row at every depth. -->
{#snippet row(skill: Row)}
{#snippet icon()}
<BookOpen size={16} class="text-tertiary" />
{/snippet}
{#snippet title()}
<span class="truncate leading-5">
<!-- Grouped, the folder above the row is what tells two skills of the same
name apart; flat, the path has to do it. -->
{!grouped && ambiguous.has(skill.name) ? skill.path : skill.name}
</span>
{/snippet}
{#snippet subtitle()}{skill.description}{/snippet}
{#snippet trailing()}
<Toggle
size="xs"
disabled={forkPending}
checked={skill.enabled}
on:change={async (e) => await toggle(skill.path, e.detail)}
/>
<DropdownV2
size="sm"
bind:open={rowMenuOpen[skill.path]}
items={[
{
// One entry for both halves of the detail: it opens on the rendered
// skill, and editing is the switch in its header.
displayName: 'Manage skill',
icon: skill.canWrite ? Pencil : Eye,
action: () => openSkill(skill)
},
{
displayName: 'Delete',
icon: Trash2,
type: 'delete',
disabled: !skill.canWrite || forkPending,
action: () => (toDelete = skill)
}
]}
/>
{/snippet}
<ListRow
{icon}
{title}
{trailing}
id={entryDomId(skillKey(skill.path))}
highlighted={entryIndexByKey.get(skillKey(skill.path)) === highlight.index}
onMouseEnter={() => highlight.hovered(entryIndexByKey.get(skillKey(skill.path)) ?? -1)}
subtitle={skill.description ? subtitle : undefined}
onClick={() => openSkill(skill)}
/>
{/snippet}
<!-- One folder and everything under it. Recursive: a path may name folders below
the `u/x` or `f/x` root it starts from. -->
{#snippet folder(node: SkillTreeNode<Row>, root: boolean)}
<div>
<!-- The switch sits beside the header button rather than inside it: a switch
nested in a button would fold the folder on every flip. `pr-14` clears the
row menu column, so a folder's switch stands in the same column as the
switches of the rows under it. -->
<!-- svelte-ignore a11y_no_static_element_interactions -->
<div
id={entryDomId(folderKey(node.path))}
class="w-full flex items-center gap-2 pr-14 rounded-md {entryIndexByKey.get(
folderKey(node.path)
) === highlight.index
? 'bg-surface-hover'
: ''}"
onmouseenter={() => highlight.hovered(entryIndexByKey.get(folderKey(node.path)) ?? -1)}
>
<button
type="button"
class="grow min-w-0 flex items-center gap-2 px-2 py-2 text-left"
aria-expanded={!collapsed[node.path]}
onclick={() => fold(folderKey(node.path), node.path, !collapsed[node.path])}
>
{#if collapsed[node.path]}
<ChevronRight size={14} class="text-tertiary shrink-0" />
{:else}
<ChevronDown size={14} class="text-tertiary shrink-0" />
{/if}
{#if root && node.path.startsWith('u/')}
<User size={14} class="text-secondary shrink-0" />
{:else if root}
<Folder size={14} class="text-secondary shrink-0" />
{:else}
<FolderTree size={14} class="text-secondary shrink-0" />
{/if}
<!-- Explicitly normal: the header is a button, and buttons carry a heavier
weight of their own that the label would otherwise inherit. -->
<span class="text-xs font-normal text-emphasis truncate">{node.label}</span>
<span class="text-2xs text-secondary">{countSkills(node)}</span>
</button>
<Toggle
size="xs"
disabled={forkPending}
checked={folderEnabled(node)}
on:change={async (e) => await toggleFolder(node, e.detail)}
/>
</div>
{#if !collapsed[node.path]}
<div class="pl-4 flex flex-col gap-0.5">
{#each node.children as child (child.path)}
{@render folder(child, false)}
{/each}
{#each node.skills as skill (skill.path)}
{@render row(skill)}
{/each}
</div>
{/if}
</div>
{/snippet}
{#snippet editorPage()}
<!-- The editor takes the panel over rather than opening on top of it: a form
stacked on the settings modal leaves two surfaces arguing over which one a
click or an Escape belongs to. -->
<div class="grow min-h-0 overflow-y-auto pr-2">
<!-- Same reserved gutter as the list, so the slide between the two pages does not
shift what is under the cursor. -->
<div class="grow min-h-0 overflow-y-auto pr-2" style="scrollbar-gutter: stable;">
<!-- Sticky so the way back is always one click away, however far the page scrolls. -->
<div class="flex sticky top-0 z-10 bg-surface pb-1">
<Button
@@ -886,7 +1202,7 @@ What the assistant should do when this skill applies.
>
<span class="text-xs text-primary">
This deletes the resource at <span class="font-semibold">{toDelete?.path}</span>, so everyone
who selected it loses the skill.
who can read it loses the skill.
</span>
</ConfirmationModal>
@@ -2,36 +2,61 @@ import { get } from 'svelte/store'
import { userStore } from '$lib/stores'
/**
* A set of workspace-object paths the chat may act through, remembered per
* workspace and per account.
* Which workspace-object paths the chat may act through, remembered per workspace
* and per account.
*
* Being able to read a resource is not the same as wanting the chat to use it: a
* resource in a shared folder is readable by a whole team, and each enabled entry
* costs something on every turn — an MCP server puts its tool descriptions in the
* model's context and reaches an external system, a skill puts its description
* there. So an entry is off until it is turned on.
* Only the paths someone actually decided about are stored; everything else is the
* kind's default. That is what lets a default flip — skills went from off-until-on
* to on-until-off — without rewriting anyone's storage: the stored entries keep
* meaning what the person chose, and only the paths they never touched move.
*
* Stored per browser, like the chat's other per-user preferences, but keyed by
* email as well as workspace: browser storage outlives a logout, and inheriting
* the previous account's selection would hand the next person capabilities they
* never turned on. Workspace ids cannot contain `:`, so the composite key is
* unambiguous.
* email as well as workspace: browser storage outlives a logout, and one person's
* decisions must not be read as the next person's — whichever way they went.
* Workspace ids cannot contain `:`, so the composite key is unambiguous.
*/
export type EnabledPathsPreference = {
enabledPaths: (workspace: string) => string[]
export type PathsPreference = {
/** Paths stored as on, which is the enabled set only for a kind that defaults to
* off. For one that defaults to on this is whatever happens to be written down —
* nothing for a decision made since the default flipped, and the whole selection
* for a browser still carrying storage from before it. Neither is the answer to
* "what is enabled": ask `isEnabled` per path for that. */
explicitlyEnabledPaths: (workspace: string) => string[]
isEnabled: (workspace: string, path: string) => boolean
/** Returns false when there is no account to record the preference against, so
* a caller that just created the object can say it did not stay on. */
setEnabled: (workspace: string, path: string, enabled: boolean) => boolean
/** Drop any decision about `path`, leaving it at the default. What a caller wants
* when the thing at that path is gone — a deleted or renamed skill — rather than
* writing the default as a decision, which reads as one and would mean the
* opposite the day the default moves. */
forget: (workspace: string, path: string) => void
}
export function createEnabledPathsPreference(storageKey: string): EnabledPathsPreference {
/** One decision per path. The older shape was an array of the paths that were on,
* which says exactly that and so needs no conversion to be read. */
type Choices = Record<string, boolean>
function toChoices(raw: unknown): Choices {
if (Array.isArray(raw)) return Object.fromEntries(raw.map((path) => [String(path), true]))
if (raw && typeof raw === 'object') {
return Object.fromEntries(
Object.entries(raw as Record<string, unknown>).map(([path, on]) => [path, on === true])
)
}
return {}
}
export function createPathsPreference(
storageKey: string,
defaultEnabled: boolean
): PathsPreference {
function scope(workspace: string): string | undefined {
const email = get(userStore)?.email
return email ? `${workspace}:${email}` : undefined
}
function read(): Record<string, string[]> {
function read(): Record<string, unknown> {
if (typeof localStorage === 'undefined') return {}
try {
return JSON.parse(localStorage.getItem(storageKey) ?? '{}')
@@ -40,35 +65,42 @@ export function createEnabledPathsPreference(storageKey: string): EnabledPathsPr
}
}
function write(all: Record<string, string[]>) {
function choices(workspace: string): Choices {
const key = scope(workspace)
return key ? toChoices(read()[key]) : {}
}
/** `decision` of undefined drops the entry. */
function write(workspace: string, path: string, decision: boolean | undefined): boolean {
const key = scope(workspace)
if (!key) return false
const all = read()
const current = toChoices(all[key])
if (decision === undefined) {
delete current[path]
} else {
current[path] = decision
}
all[key] = current
try {
localStorage.setItem(storageKey, JSON.stringify(all))
} catch (e) {
console.error(`Failed to persist ${storageKey}`, e)
}
}
function enabledPaths(workspace: string): string[] {
const key = scope(workspace)
return key ? (read()[key] ?? []) : []
return true
}
return {
enabledPaths,
isEnabled: (workspace, path) => enabledPaths(workspace).includes(path),
setEnabled: (workspace, path, enabled) => {
const key = scope(workspace)
if (!key) return false
const all = read()
const current = new Set(all[key] ?? [])
if (enabled) {
current.add(path)
} else {
current.delete(path)
}
all[key] = [...current]
write(all)
return true
}
explicitlyEnabledPaths: (workspace) =>
Object.entries(choices(workspace))
.filter(([, on]) => on)
.map(([path]) => path),
isEnabled: (workspace, path) => choices(workspace)[path] ?? defaultEnabled,
// A path put back to the default is dropped rather than stored as one: the
// entries are the decisions, so a re-enabled skill leaves nothing behind and a
// later skill at that path starts from the default like any other.
setEnabled: (workspace, path, enabled) =>
write(workspace, path, enabled === defaultEnabled ? undefined : enabled),
forget: (workspace, path) => void write(workspace, path, undefined)
}
}
@@ -220,6 +220,9 @@ vi.mock('$lib/gen', async () => {
createResource: vi.fn(async () => 'created'),
updateResource: vi.fn(async () => 'updated'),
deleteResource: vi.fn(async () => 'deleted'),
// A workspace with no skills, which is what makes `read_skill` refuse a path
// the model composed: the gate is membership in the listing.
listResource: vi.fn(async () => []),
getResourceValue: vi.fn(async () => ({ content: 'skill body' }))
}),
VariableService: wrapService(actual.VariableService, {
@@ -6195,13 +6198,16 @@ describe('session-only preview tools gating', () => {
})
describe('read_skill', () => {
it('refuses a path the user has not selected, without reading it', async () => {
// Every path is enabled by default now, so the listing is what keeps the tool to
// skills: without it the model could name any resource holding a string `content`
// and have it read back.
it('refuses a path that is not a skill in the workspace, without reading it', async () => {
localStorage.clear()
userStore.set({ username: 'bob', email: 'bob@windmill.dev', workspace_id: WORKSPACE } as any)
const res = await callGlobalTool('read_skill', { path: 'u/someone/private-notes' })
expect(res).toContain('not one of the skills selected')
expect(res).toContain('not one of the skills available')
expect(vi.mocked(ResourceService.getResourceValue)).not.toHaveBeenCalled()
})
})
@@ -94,7 +94,7 @@ import {
import { searchNpmPackagesTool } from '../script/core'
import type { McpServer } from './mcpTools'
import { logFeatureUsage } from '$lib/utils/featureUsage'
import { enabledSkillPaths } from '../skills/enabledSkills'
import { isSkillEnabled } from '../skills/enabledSkills'
import {
listSkillResources,
readSkillBody,
@@ -1399,7 +1399,7 @@ Data Tables:
? `
Skills:
- Skills are reusable instruction sets the user selected for this chat, each covering a specific kind of task. The available skills are listed below by resource path and description.
- Skills are reusable instruction sets available in this workspace, each covering a specific kind of task. The available skills are listed below by resource path and description.
- When a user's request matches a skill's description, call read_skill with its exact path to load the full instructions BEFORE acting, then follow them.
${skills.map((s) => `- ${s.path}: ${s.description}`).join('\n')}`
: ''
@@ -2366,23 +2366,17 @@ export type ChatCommandItem = {
}
/**
* The skills this user turned on in this workspace, for the global system prompt.
* A readable `ai_skill` resource is only a candidate enabling one is a personal
* choice, since each enabled skill spends context on every turn.
* The skills in play in this workspace, for the global system prompt: every
* readable `ai_skill` resource except the ones this user turned off.
*/
export async function loadWorkspaceSkills(workspace: string): Promise<AiSkillListItem[]> {
if (!workspace) return []
try {
const enabled = new Set(enabledSkillPaths(workspace))
if (enabled.size === 0) return []
// Filtered against what is actually readable now, so a skill that was
// deleted or whose folder access was revoked drops out instead of being
// advertised to the model as something read_skill can load.
// A truncated listing still carries most of the workspace, and the drawer is
// where that is surfaced; dropping everything here would silently empty the
// Skills section instead.
return (await listSkillResources(workspace)).skills
.filter((s) => enabled.has(s.path))
.filter((s) => isSkillEnabled(workspace, s.path))
.map(({ path, name, description }) => ({
path,
name,
@@ -2408,24 +2402,28 @@ export const readSkillTool: Tool<{}> = {
def: createToolDef(
readSkillSchema,
'read_skill',
'Load the full instructions for a selected AI skill by resource path. Skills are listed in the system prompt under "Skills"; call this before acting on a task a skill covers, then follow its instructions.'
'Load the full instructions for an AI skill by resource path. Skills are listed in the system prompt under "Skills"; call this before acting on a task a skill covers, then follow its instructions.'
),
planModeSafe: true,
fn: async ({ args, workspace, toolId, toolCallbacks }) => {
const parsed = readSkillSchema.parse(args)
const name = skillNameFromPath(parsed.path)
// The prompt lists only selected skills, but the tool takes a path the model
// composed, so the selection is enforced here too rather than assumed. Without
// it the tool reads any resource holding a string `content` — the user's own
// access, but not what "load a selected skill" says it does.
if (!enabledSkillPaths(workspace).includes(parsed.path)) {
toolCallbacks.setToolStatus(toolId, { content: `Skill "${name}" is not selected` })
return `"${parsed.path}" is not one of the skills selected for this chat. Only the paths listed under "Skills" in the system prompt can be read.`
}
toolCallbacks.setToolStatus(toolId, { content: `Reading skill "${name}"...` })
try {
// Bounded here rather than in the reader: any `ai_skill` resource can be
// selected, including ones written through git sync or the resource editor
// The prompt lists the skills in play, but the tool takes a path the model
// composed, so what may be read is checked here rather than assumed. Against
// the listing, not just the off-switch: any other path is now enabled too,
// and without this the tool reads any resource holding a string `content` —
// the user's own access, but not what "load a skill" says it does.
const isSkill = (await listSkillResources(workspace)).skills.some(
(s) => s.path === parsed.path
)
if (!isSkill || !isSkillEnabled(workspace, parsed.path)) {
toolCallbacks.setToolStatus(toolId, { content: `Skill "${name}" is not available` })
return `"${parsed.path}" is not one of the skills available to this chat. Only the paths listed under "Skills" in the system prompt can be read.`
}
// Bounded here rather than in the reader: any `ai_skill` resource is in play,
// including ones written through git sync or the resource editor
// that never passed the authoring form's limits, and an unbounded body
// would exhaust the context on one tool call. The editor reads the same
// resource untruncated, so opening a long skill cannot rewrite it short.
@@ -2434,7 +2432,7 @@ export const readSkillTool: Tool<{}> = {
MAX_SKILL_INSTRUCTIONS_LENGTH
)
toolCallbacks.setToolStatus(toolId, { content: `Read skill "${name}"` })
// Whether a selected skill is actually reached for. No key: the path is
// Whether a skill is actually reached for. No key: the path is
// workspace-authored text.
logFeatureUsage('ai_session', 'skill_read', { workspace })
return `Skill: ${parsed.path}\n\nInstructions:\n${instructions}`
@@ -1,10 +1,11 @@
import { createEnabledPathsPreference } from '../enabledPathsPreference'
import { createPathsPreference } from '../enabledPathsPreference'
/** Which `ai_skill` resources the chat may follow, per workspace and per account.
* Every enabled skill spends context on every turn, so selecting one is a personal
* choice rather than a consequence of being able to read it. */
const preference = createEnabledPathsPreference('wm_skills_enabled')
* Every skill readable in the workspace is on unless someone turned it off: a skill
* is instructions the workspace wrote for the assistant to use, so what carrying one
* costs is context, not access. */
const preference = createPathsPreference('wm_skills_enabled', true)
export const enabledSkillPaths = preference.enabledPaths
export const isSkillEnabled = preference.isEnabled
export const setSkillEnabled = preference.setEnabled
export const forgetSkill = preference.forget
@@ -38,14 +38,14 @@ export function ambiguousSkillNames(skills: readonly { name: string }[]): Set<st
const SKILLS_PAGE_SIZE = 100
/** Pages to walk before giving up. Ordinary resources and repeated imports can
* make any number of skills, and a single page would drop the rest including a
* selected one, which would then vanish from the prompt with nothing to explain
* skill in play, which would then vanish from the prompt with nothing to explain
* it. The bound is a guard against a paging bug looping forever, not a product
* cap, so reaching it is reported rather than passed off as the whole set. */
const MAX_SKILLS_PAGES = 100
/** The rows read, and whether the walk stopped at the bound rather than the end.
* Reported rather than thrown: a truncated read is still most of the skills, and
* dropping them all would take every selected skill out of the prompt at once. */
* dropping them all would take every skill out of the prompt at once. */
export type SkillListing = { skills: SkillResource[]; truncated: boolean }
/** Every skill resource readable in the workspace.
@@ -54,10 +54,7 @@ export type SkillListing = { skills: SkillResource[]; truncated: boolean }
* the account the workspace is being browsed as. Ownership is mostly implicit in
* the path (`u/<me>/…`, a folder the user owns), which is why this goes through
* the shared `canWrite` rather than reading `extra_perms` alone. */
export async function listSkillResources(
workspace: string,
user?: UserExt
): Promise<SkillListing> {
export async function listSkillResources(workspace: string, user?: UserExt): Promise<SkillListing> {
if (!workspace) return { skills: [], truncated: false }
const rows: SkillResource[] = []
for (let page = 1; page <= MAX_SKILLS_PAGES; page++) {
@@ -0,0 +1,119 @@
/** Grouping for the Skills list: skills sorted into the folders that hold them.
*
* Only worth showing when the skills are spread over more than one folder see
* `skillFolderPaths` since a tree over a single folder is a list with a header
* on top of it. */
/** A folder in the skills tree. `path` is the whole prefix the node covers, so it
* is unique across the tree and can key collapsed state. */
export type SkillTreeNode<T> = {
path: string
/** Header text: the whole scope (`u/admin`, `f/skills`) at the root, one
* segment (`deploy`) deeper, where the ancestors are already on screen. */
label: string
/** A `u/<user>`, `f/<folder>` or `g/<group>` root rather than a subfolder. */
scope: boolean
children: SkillTreeNode<T>[]
skills: T[]
}
type Skill = { path: string; name: string }
/** The folders holding these skills — one entry per distinct parent path. */
export function skillFolderPaths(skills: readonly Skill[]): Set<string> {
return new Set(skills.map((s) => s.path.split('/').slice(0, -1).join('/')))
}
/** Every skill under `node`, its subfolders included. */
export function countSkills<T>(node: SkillTreeNode<T>): number {
return node.skills.length + node.children.reduce((n, c) => n + countSkills(c), 0)
}
/** The skills a folder switch acts on: everything under it, however deep. */
export function nodeSkills<T>(node: SkillTreeNode<T>): T[] {
return [...node.skills, ...node.children.flatMap((c) => nodeSkills(c))]
}
/** One line of the rendered list, folders included. */
export type SkillTreeEntry<T> = { key: string; parentKey?: string } & (
| { kind: 'folder'; node: SkillTreeNode<T> }
| { kind: 'skill'; skill: T }
)
/** A folder and a skill can hold the same path `f/skills/deploy` names both when
* `f/skills/deploy/rollback` exists beside it so the kind is part of the key. */
export function folderKey(path: string): string {
return `folder:${path}`
}
export function skillKey(path: string): string {
return `skill:${path}`
}
/** The list as it stands on screen, top to bottom, with collapsed folders holding
* their contents back. This is what the keyboard walks; the markup renders the same
* forest recursively under the same `isCollapsed`, so the two agree line for line. */
export function visibleEntries<T extends Skill>(
forest: readonly SkillTreeNode<T>[],
isCollapsed: (path: string) => boolean
): SkillTreeEntry<T>[] {
const out: SkillTreeEntry<T>[] = []
const walk = (node: SkillTreeNode<T>, parentKey?: string) => {
const key = folderKey(node.path)
out.push({ kind: 'folder', key, parentKey, node })
if (isCollapsed(node.path)) return
for (const child of node.children) walk(child, key)
for (const skill of node.skills) {
out.push({ kind: 'skill', key: skillKey(skill.path), parentKey: key, skill })
}
}
for (const root of forest) walk(root)
return out
}
/** Sort a skill's path into the forest, creating the folders it names.
*
* Paths are `[ufg]/<owner>/<name>` and may nest further (`f/skills/deploy/rollback`
* passes the resource path CHECK), so the first two segments are one node and each
* segment after that is a node of its own. */
export function buildSkillTree<T extends Skill>(skills: readonly T[]): SkillTreeNode<T>[] {
const roots = new Map<string, SkillTreeNode<T>>()
for (const skill of skills) {
const parts = skill.path.split('/')
// Two segments cannot happen through the API, but a hand-written path in a
// test or a future path shape should land somewhere rather than vanish.
const rootPath = parts.slice(0, 2).join('/')
let node: SkillTreeNode<T> = roots.get(rootPath) ?? {
path: rootPath,
label: rootPath,
scope: true,
children: [],
skills: []
}
roots.set(rootPath, node)
for (const segment of parts.slice(2, -1)) {
const path = `${node.path}/${segment}`
let child = node.children.find((c) => c.path === path)
if (child === undefined) {
child = { path, label: segment, scope: false, children: [], skills: [] }
node.children.push(child)
}
node = child
}
node.skills.push(skill)
}
const sort = (node: SkillTreeNode<T>) => {
node.children.sort((a, b) => a.label.localeCompare(b.label))
node.skills.sort((a, b) => a.name.localeCompare(b.name))
node.children.forEach(sort)
}
const forest = [...roots.values()]
forest.forEach(sort)
// Personal scopes before shared folders, alphabetical within each: `u/<me>` is
// where this modal's New skill and folder import write, so it is the half of the
// list someone is most often here to change.
return forest.sort((a, b) => {
const scopeRank = (p: string) => (p.startsWith('u/') ? 0 : 1)
return scopeRank(a.path) - scopeRank(b.path) || a.path.localeCompare(b.path)
})
}
@@ -9,8 +9,10 @@ vi.mock('$lib/stores', () => ({
userStore: { subscribe: (run: (v: unknown) => void) => (run({ ...session }), () => {}) }
}))
import { enabledSkillPaths, isSkillEnabled, setSkillEnabled } from './enabledSkills'
import { isMcpEnabled } from '$lib/components/mcp/enabledServers'
import { isSkillEnabled, setSkillEnabled } from './enabledSkills'
import { ambiguousSkillNames, truncateChars, truncateForPrompt } from './skillResources'
import { buildSkillTree, countSkills, skillFolderPaths, visibleEntries } from './skillTree'
describe('enabledSkills', () => {
beforeEach(() => {
@@ -18,22 +20,103 @@ describe('enabledSkills', () => {
session.email = 'first@windmill.dev'
})
it('keeps the selection separate per workspace', () => {
setSkillEnabled('ws_a', 'u/me/deploy', true)
expect(isSkillEnabled('ws_a', 'u/me/deploy')).toBe(true)
expect(isSkillEnabled('ws_b', 'u/me/deploy')).toBe(false)
it('keeps a skill turned off separate per workspace', () => {
setSkillEnabled('ws_a', 'u/me/deploy', false)
expect(isSkillEnabled('ws_a', 'u/me/deploy')).toBe(false)
expect(isSkillEnabled('ws_b', 'u/me/deploy')).toBe(true)
})
it('does not hand the next account the previous ones selection', () => {
setSkillEnabled('ws_a', 'u/me/deploy', true)
it('does not hand the next account the previous ones choice', () => {
setSkillEnabled('ws_a', 'u/me/deploy', false)
session.email = 'second@windmill.dev'
expect(enabledSkillPaths('ws_a')).toEqual([])
expect(isSkillEnabled('ws_a', 'u/me/deploy')).toBe(true)
})
it('reports failure when there is no account to record the choice against', () => {
session.email = undefined
expect(setSkillEnabled('ws_a', 'u/me/deploy', true)).toBe(false)
expect(enabledSkillPaths('ws_a')).toEqual([])
expect(setSkillEnabled('ws_a', 'u/me/deploy', false)).toBe(false)
})
})
// Storage written before skills defaulted to on holds an array of the paths that
// were on. Nothing rewrites it, so reading it wrong is what would silently move
// somebody's choices — in either direction, for either default.
describe('choices stored under the older shape', () => {
const scope = 'ws_a:first@windmill.dev'
beforeEach(() => {
localStorage.clear()
session.email = 'first@windmill.dev'
})
it('keeps a skill that was turned on, and defaults the rest to on', () => {
localStorage.setItem('wm_skills_enabled', JSON.stringify({ [scope]: ['u/me/deploy'] }))
expect(isSkillEnabled('ws_a', 'u/me/deploy')).toBe(true)
expect(isSkillEnabled('ws_a', 'u/me/never-picked')).toBe(true)
})
it('leaves an MCP server that was never turned on off', () => {
localStorage.setItem('wm_mcp_enabled', JSON.stringify({ [scope]: ['u/me/github'] }))
expect(isMcpEnabled('ws_a', 'u/me/github')).toBe(true)
expect(isMcpEnabled('ws_a', 'u/me/other')).toBe(false)
})
it('records an off decision beside the entries already there', () => {
localStorage.setItem('wm_skills_enabled', JSON.stringify({ [scope]: ['u/me/deploy'] }))
setSkillEnabled('ws_a', 'u/me/review', false)
expect(isSkillEnabled('ws_a', 'u/me/review')).toBe(false)
expect(isSkillEnabled('ws_a', 'u/me/deploy')).toBe(true)
})
it('drops a path put back to the default instead of storing it', () => {
setSkillEnabled('ws_a', 'u/me/review', false)
setSkillEnabled('ws_a', 'u/me/review', true)
expect(JSON.parse(localStorage.getItem('wm_skills_enabled') ?? '{}')[scope]).toEqual({})
})
})
describe('skill tree', () => {
const skills = [
{ path: 'f/skills/deploy/rollback', name: 'rollback' },
{ path: 'f/skills/onboarding', name: 'onboarding' },
{ path: 'u/admin/release-notes', name: 'release-notes' }
]
it('nests each path segment under its owner, own folder first', () => {
const tree = buildSkillTree(skills)
expect(tree.map((n) => n.path)).toEqual(['u/admin', 'f/skills'])
const shared = tree[1]
expect(shared.skills.map((s) => s.name)).toEqual(['onboarding'])
// A skill two levels down gets its folder, rather than being flattened into
// the owner's own rows where the path it came from is lost.
expect(shared.children.map((c) => c.path)).toEqual(['f/skills/deploy'])
expect(shared.children[0].skills.map((s) => s.name)).toEqual(['rollback'])
expect(countSkills(shared)).toBe(2)
})
// The keyboard walks this list while the markup renders the forest recursively. If
// the two disagree, Down lands on a row that is not the one lit.
it('lists what is on screen, and holds back what a collapsed folder hides', () => {
const tree = buildSkillTree(skills)
expect(visibleEntries(tree, () => false).map((e) => e.key)).toEqual([
'folder:u/admin',
'skill:u/admin/release-notes',
'folder:f/skills',
'folder:f/skills/deploy',
'skill:f/skills/deploy/rollback',
'skill:f/skills/onboarding'
])
expect(visibleEntries(tree, (path) => path === 'f/skills').map((e) => e.key)).toEqual([
'folder:u/admin',
'skill:u/admin/release-notes',
'folder:f/skills'
])
})
// What the list uses to choose between the tree and a flat list.
it('counts the folders holding the skills, not the skills', () => {
expect(skillFolderPaths(skills).size).toBe(3)
expect(skillFolderPaths([skills[1], { path: 'f/skills/deploy', name: 'deploy' }]).size).toBe(1)
})
})
@@ -55,7 +138,9 @@ describe('prompt truncation', () => {
const body = '漢'.repeat(100) // 300 bytes
expect(truncateForPrompt(body, 3000)).toBe(body)
const cut = truncateForPrompt(body, 30)
expect(new TextEncoder().encode(cut.replace('… [truncated]', '')).byteLength).toBeLessThanOrEqual(30)
expect(
new TextEncoder().encode(cut.replace('… [truncated]', '')).byteLength
).toBeLessThanOrEqual(30)
expect(cut).toContain('[truncated]')
// A byte-aligned cut must not leave a broken code point behind.
expect(cut).not.toContain('\ufffd')
@@ -65,26 +65,26 @@ export class SkillsMenu {
async #toggle(ws: string, path: string, enabled: boolean) {
// A session whose fork is still staged has no workspace of its own yet, so `ws`
// is the PARENT: the selection would be stored under it and quietly stop
// is the PARENT: the choice would be stored under it and quietly stop
// applying the moment the first send commits the fork.
const pendingForkOf = this.#manager.sessionContextResolver?.()?.pendingForkOf
if (pendingForkOf !== undefined) {
sendUserToast(
`This session has not created its workspace yet, so the selection would be stored under "${pendingForkOf}". Send a message first.`,
`This session has not created its workspace yet, so the choice would be stored under "${pendingForkOf}". Send a message first.`,
true
)
return
}
if (!setSkillEnabled(ws, path, enabled)) {
sendUserToast('Could not save the selection for this account.', true)
sendUserToast('Could not save this choice for this account.', true)
return
}
const row = this.#row(path)
if (row) row.enabled = enabled
// Whether people select skills at all. Never the skill itself: a path is
// Whether people turn skills off at all. Never the skill itself: a path is
// workspace-authored text.
logFeatureUsage('ai_session', 'skill_toggle', { key: enabled ? 'on' : 'off', workspace: ws })
// The prompt lists exactly the enabled skills, so it has to be rebuilt
// The prompt lists exactly the skills that are on, so it has to be rebuilt
// before the next message rather than on the next mode change.
await this.#manager.refreshGlobalSkills(ws)
}
@@ -103,10 +103,10 @@ export class SkillsMenu {
void this.#load(ws)
}
const ambiguous = ambiguousSkillNames(this.#rows)
// Enabled first: those are the ones a quick visit is most likely about.
const ordered = [...this.#rows].sort(
(a, b) => Number(b.enabled) - Number(a.enabled) || a.path.localeCompare(b.path)
)
// By path. Ordering the ones that are on first would put every row in the same
// bucket now that skills start that way, and drop the one row it did move — a
// skill just turned off here — out of the shortcut that turns it back on.
const ordered = [...this.#rows].sort((a, b) => a.path.localeCompare(b.path))
const shown = ordered.slice(0, MAX_MENU_SKILLS)
const manage = () => {
closeMenu?.()
@@ -1,11 +1,12 @@
import { createEnabledPathsPreference } from '$lib/components/copilot/chat/enabledPathsPreference'
import { createPathsPreference } from '$lib/components/copilot/chat/enabledPathsPreference'
/** Which MCP servers the chat may act through, per workspace and per account. A
* server's tools both reach an external system and put their descriptions in the
* model's context, so one is off until it is turned on; connecting one through the
* chat turns it on for the person who connected it. */
const preference = createEnabledPathsPreference('wm_mcp_enabled')
const preference = createPathsPreference('wm_mcp_enabled', false)
export const enabledMcpPaths = preference.enabledPaths
/** Servers are off by default, so what is stored as on is the whole enabled set. */
export const enabledMcpPaths = preference.explicitlyEnabledPaths
export const isMcpEnabled = preference.isEnabled
export const setMcpEnabled = preference.setEnabled
@@ -99,7 +99,18 @@
}
</script>
<Modal2 bind:isOpen={open} {title} fixedWidth="md" {fixedHeight} {target}>
<!-- `xxl` is 1000px, taller than a laptop window, and the dialog would then scroll
inside the overlay while this list scrolls inside the dialog. Capped here rather
than in `Modal2`: the height stays definite, which is what lets the list below
bound its own scroller, and no other modal is touched. -->
<Modal2
bind:isOpen={open}
{title}
fixedWidth="md"
{fixedHeight}
{target}
css={{ popup: { class: 'max-h-[80vh]' } }}
>
<div class="flex flex-col gap-6 h-full px-1 w-full">
<div class="grow min-h-0 overflow-y-auto" style="scrollbar-gutter: stable;">
{#if readOnly}