diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f7713ec..b78285b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,24 @@ refusing to boot over. The example package ships four: `/find-a-document`, `/whats-changed`, `/who-owns-this` and `/check-a-claim`. +### The tools a skill needs can be picked where the skill is written + +A package's skills arrive with their tools declared. A skill somebody writes here could not: the +`tools` field existed on the save endpoint and on no screen, so a skill written in the product declared +nothing, and the only way to change that was to call the API by hand. Writing or editing a skill now +lists the tools of every connected server, grouped by server, with the ones that change something +marked. + +Picking a tool here is not granting it. The offer is still intersected with what the Bot was granted, +so a skill naming a tool its Bot does not hold selects the skill and loads nothing — which is why +anybody may write a skill while connecting a server stays an administrator's decision. The screen says +so, next to the choice. + +A tool the skill names that no connected server offers is shown too, under its own heading, rather +than left out. A package ships skills declaring tools for connectors nobody has added yet, and a +skill outlives the server it was written against, so a screen that drew only what matched was +stating part of the declaration as though it were all of it. + ## 0.0.3 ### A Bot is offered the tools its message needs, not every tool it holds diff --git a/app/src/components/skills/edit-skill.tsx b/app/src/components/skills/edit-skill.tsx index f4b4cd04..6e2cdf44 100644 --- a/app/src/components/skills/edit-skill.tsx +++ b/app/src/components/skills/edit-skill.tsx @@ -62,6 +62,9 @@ export function EditSkill({ slug }: { slug: string }) { title: skill.title, summary: skill.summary ?? "", instructions: skill.instructions, + // Defaulted, so a skill written before this field existed opens with nothing ticked rather + // than with an undefined the form would refuse to submit. + tools: skill.tools ?? [], }} error={saveSkill.error} /* diff --git a/app/src/components/skills/skill-fields.tsx b/app/src/components/skills/skill-fields.tsx index f2c15597..8f6fd8ef 100644 --- a/app/src/components/skills/skill-fields.tsx +++ b/app/src/components/skills/skill-fields.tsx @@ -1,4 +1,5 @@ import { useForm } from "@tanstack/react-form"; +import { SkillTools } from "@/components/skills/skill-tools"; import { Button } from "@/components/ui/button"; import { Field, @@ -188,6 +189,14 @@ export function SkillFields({ ); }} + + {(field) => ( + + )} + {/* diff --git a/app/src/components/skills/skill-tools.tsx b/app/src/components/skills/skill-tools.tsx new file mode 100644 index 00000000..90633864 --- /dev/null +++ b/app/src/components/skills/skill-tools.tsx @@ -0,0 +1,173 @@ +import { useQuery } from "@tanstack/react-query"; +import { Button } from "@/components/ui/button"; +import { Field, FieldLabel } from "@/components/ui/field"; +import { pluginsPageQueryOptions } from "@/lib/plugins/queries"; +import { undeclaredElsewhere } from "@/lib/skills/form"; + +/** + * Which tools this skill says it needs. + * + * WHY IT MATTERS THAT THIS EXISTS AT ALL. Selection is over skills: before a run the deployment asks + * its own model which skills the message needs, and the Bot is built with those skills' tools plus + * every granted tool no skill claims. A deployment where no skill declares anything has nothing to + * select on, so every Bot is offered its whole catalogue — which is the case the narrowing was built + * to fix. Until this screen existed the only way to declare a tool was to call the API by hand. + * + * A DECLARATION IS NOT A GRANT, and this is the one thing the screen has to get across. Anybody + * signed in may write a skill; adding an MCP server is an administrator's decision. If naming a tool + * here made it callable, the first surface would be a way around the second. It does not: the offer + * is intersected with what the Bot was granted, so a skill naming a tool its Bot does not hold + * selects the skill and loads nothing. Said in the hint rather than left for somebody to discover, + * because the failure it prevents is somebody ticking boxes expecting access. + * + * PART OF THE FORM, unlike the Agents toggles below it. What a skill needs is the author's draft + * until they press save, so unticking one and closing the panel changes nothing. Granting is the + * opposite and saves on press, which is why the two look different on purpose. + */ +export function SkillTools({ + selected, + onChange, +}: { + selected: string[]; + onChange: (refs: string[]) => void; +}) { + const plugins = useQuery(pluginsPageQueryOptions()); + const held = new Set(selected); + + const toggle = (ref: string) => { + /* + * Rebuilt rather than mutated, and filtered rather than spliced, so the array handed to the form + * is a new one. React Form compares by reference; mutating in place leaves the field's value + * looking unchanged and the submit button disabled on a form the person has edited. + */ + onChange( + held.has(ref) + ? selected.filter((each) => each !== ref) + : [...selected, ref], + ); + }; + + /* + * Only servers that actually offer something. A server whose tools have never been refreshed has + * an empty list, and a heading with nothing under it reads as a tool that failed to draw. + */ + const servers = (plugins.data?.servers ?? []).filter( + (server) => server.tools.length > 0, + ); + + /* + * Declared, and offered by nothing this deployment has connected. + * + * Shown rather than dropped, because the alternative is a screen that states part of the + * declaration as though it were all of it. A package ships skills declaring tools for connectors + * nobody has added yet, and a person's own skill outlives the server it was written against, so + * this is the ordinary case rather than a corner of one. Left out, a skill needing two tools drew + * one and the second was invisible to the person governing it. + * + * Only computed once the list has actually loaded: while the query is pending every ref looks + * unmatched, and flashing the whole declared set into this group and out again would be worse + * than showing nothing for a moment. + */ + const elsewhere = plugins.data + ? undeclaredElsewhere( + selected, + servers.flatMap((server) => server.tools.map((tool) => tool.ref)), + ) + : []; + + return ( + + Tools it needs + + {plugins.isPending ? null : plugins.error ? ( +

+ Could not load the tools this deployment has. +

+ ) : servers.length === 0 ? ( +

+ No connected server offers a tool yet. A skill can still be written — + most are instructions rather than tool use. +

+ ) : ( +
+ {servers.map((server) => ( +
+

{server.title}

+
+ {server.tools.map((tool) => { + const on = held.has(tool.ref); + return ( + + ); + })} +
+
+ ))} +
+ )} + + {elsewhere.length > 0 ? ( +
+

Not connected here

+
+ {elsewhere.map((ref) => ( + + ))} +
+

+ This skill names these, and no server connected here offers them — + because the connector has not been added, or was removed. They cost + nothing and load nothing until it exists. Click one to stop naming + it. +

+
+ ) : null} + +

+ Picking this skill is what loads these tools for a turn. It does not + grant them — a Bot still only calls what it was granted, so naming a + tool here gives nobody access to it. +

+ + ); +} diff --git a/app/src/lib/plugins/mutations.ts b/app/src/lib/plugins/mutations.ts index e2e2228c..7ead8df7 100644 --- a/app/src/lib/plugins/mutations.ts +++ b/app/src/lib/plugins/mutations.ts @@ -17,6 +17,14 @@ export type SkillInput = { summary?: string; instructions: string; global?: boolean; + /** + * The tools this skill says it needs, as `/` refs. + * + * Sent on every save, including empty, because the server replaces the set rather than merging + * into it: omitting the field to mean "leave them alone" and sending `[]` to mean "clear them" + * would be the same request from a form that just had its last one unticked. + */ + tools?: string[]; }; /** A curated server from the catalogue, which supplies the URL. */ diff --git a/app/src/lib/plugins/queries.ts b/app/src/lib/plugins/queries.ts index 9c7241f5..27accab6 100644 --- a/app/src/lib/plugins/queries.ts +++ b/app/src/lib/plugins/queries.ts @@ -41,6 +41,15 @@ export type PluginSkill = { origin: string; installedBy: string | null; grantedTo: string[]; + /** + * The tools this skill says it needs, as `/` refs. + * + * A declaration, not a grant, and the difference is the whole reason anybody may write a skill: + * naming a tool here cannot make it callable. Selection intersects this with what the Bot was + * granted, so a skill naming a tool its Bot does not hold selects the skill and loads nothing. + * `grantedTo` on the tool side is what decides. + */ + tools: string[]; }; export type CatalogueItem = { diff --git a/app/src/lib/skills/form.ts b/app/src/lib/skills/form.ts index 596243b1..74baed8d 100644 --- a/app/src/lib/skills/form.ts +++ b/app/src/lib/skills/form.ts @@ -34,6 +34,17 @@ export const skillFormSchema = z.object({ .string() .trim() .min(1, "Instructions are required — this is what the Bot follows."), + /** + * The tools this skill says it needs, as `/` refs. + * + * No minimum, and no validation of the refs themselves. A skill needing no tool is ordinary — most + * are prose — and the server already refuses a ref it has never seen with a sentence naming it, + * which is a better answer than anything this file could reconstruct about which tools exist. + * + * Part of the form rather than saved on press, unlike granting. What a skill needs is the author's + * draft until they save it, so unticking one and closing the panel has to change nothing. + */ + tools: z.array(z.string()), }); export type SkillFormValues = z.infer; @@ -43,4 +54,27 @@ export const emptySkillForm: SkillFormValues = { title: "", summary: "", instructions: "", + tools: [], }; + +/** + * The declared refs no connected server offers. + * + * WHY THIS EXISTS. The picker draws the tools of the servers this deployment has connected, and a + * skill's declared set is not confined to those: a package ships skills declaring tools for + * connectors nobody has added yet, and a person's own skill outlives the server it was written + * against. Rendering only what matched meant the screen showed a subset of the declaration and + * presented it as the whole thing — a skill needing two tools drew one, and nothing said the other + * was there. Editing it silently kept a tool the author had just been shown they did not have. + * + * A wrong number on a screen somebody governs with is worse than no number, so the leftovers are + * named rather than dropped. They are not an error: a ref for a connector that does not exist here + * loads nothing, because the offer is intersected with the Bot's grants. + */ +export function undeclaredElsewhere( + selected: readonly string[], + offered: readonly string[], +): string[] { + const known = new Set(offered); + return selected.filter((ref) => !known.has(ref)); +} diff --git a/app/tests/skill-form.test.ts b/app/tests/skill-form.test.ts new file mode 100644 index 00000000..0d35729a --- /dev/null +++ b/app/tests/skill-form.test.ts @@ -0,0 +1,115 @@ +import { describe, expect, test } from "bun:test"; +import { + emptySkillForm, + skillFormSchema, + undeclaredElsewhere, +} from "@/lib/skills/form"; + +/** + * The declared tools, as the form carries them. + * + * A skill needing no tool is the ordinary case, and a skill that had its last one unticked has to + * submit as an empty array rather than as an absent field — the server replaces the set when the + * field is present and leaves it alone when it is not, so those two are different requests. + */ + +const valid = { + slug: "standup", + title: "Standup", + summary: "", + instructions: "Summarise yesterday.", +}; + +describe("declaring the tools a skill needs", () => { + test("a new skill starts with none declared", () => { + expect(emptySkillForm.tools).toEqual([]); + // And the empty form is a shape the schema accepts, apart from the fields a person has to fill. + expect( + skillFormSchema.safeParse({ ...emptySkillForm, ...valid }).success, + ).toBeTrue(); + }); + + test("no tools is valid, and stays an array", () => { + const parsed = skillFormSchema.safeParse({ ...valid, tools: [] }); + expect(parsed.success).toBeTrue(); + // Present rather than stripped: this is what clears the set on a skill that used to declare one. + expect(parsed.success && parsed.data.tools).toEqual([]); + }); + + test("refs are carried through as written", () => { + // `/`, the same key a grant is written against. The form does not reformat + // them, because the server compares them to grant refs character for character. + const tools = ["acme-docs/find_document", "acme-chat/search_messages"]; + const parsed = skillFormSchema.safeParse({ ...valid, tools }); + expect(parsed.success && parsed.data.tools).toEqual(tools); + }); + + test("the field is required, so a save always says what the set is now", () => { + // Omitting it would submit a save the server reads as "leave the tools alone", which is not what + // a form with nothing ticked means. + expect(skillFormSchema.safeParse(valid).success).toBeFalse(); + }); + + test("a ref this deployment has never seen is left to the server", () => { + /* + * Not validated here on purpose. The server refuses an unknown ref with a sentence naming it, and + * it is the only side that knows which tools exist — a list reconstructed in the browser would go + * stale the moment a server's tools were refreshed. + */ + expect( + skillFormSchema.safeParse({ ...valid, tools: ["nope/not_a_tool"] }) + .success, + ).toBeTrue(); + }); +}); + +/** + * Declared refs the picker cannot draw as a tool. + * + * The picker lists the tools of the servers this deployment has connected, and a declaration is not + * confined to those: a package ships skills naming tools for connectors nobody has added yet, and a + * person's own skill outlives the server it was written against. Anything left over has to be shown, + * or the screen states part of the declaration as though it were the whole of it. + */ +describe("declared tools no connected server offers", () => { + const offered = [ + "google-drive/search_files", + "google-drive/read_file_content", + ]; + + test("a ref for a connector nobody has added is surfaced", () => { + expect( + undeclaredElsewhere( + ["google-drive/search_files", "jira/search_issues"], + offered, + ), + ).toEqual(["jira/search_issues"]); + }); + + test("a fully matched declaration leaves nothing over", () => { + expect(undeclaredElsewhere(offered, offered)).toEqual([]); + }); + + test("declaring nothing leaves nothing over", () => { + expect(undeclaredElsewhere([], offered)).toEqual([]); + }); + + test("with no server connected, every declared ref is left over", () => { + // The case a fresh clone is in: the package ships skills declaring Drive tools and Drive has not + // been connected. Every one of them has to be visible, or the skill reads as declaring nothing. + expect( + undeclaredElsewhere( + ["google-drive/search_files", "google-drive/read_file_content"], + [], + ), + ).toEqual(["google-drive/search_files", "google-drive/read_file_content"]); + }); + + test("order is the declaration's, so the list does not reshuffle as servers connect", () => { + expect(undeclaredElsewhere(["z/one", "a/two", "m/three"], [])).toEqual([ + "z/one", + "a/two", + "m/three", + ]); + }); +});