diff --git a/README.md b/README.md index 81239b3f9..a2d50eb4b 100644 --- a/README.md +++ b/README.md @@ -154,7 +154,7 @@ Leave `EMBEDDED_POSTGRES` off and set `DATABASE_URL` to point at a database you - **Bring your own agent**: any AG-UI endpoint is a Bot, on a framework or hand-written. Endpoints are validated with the same target checks used for browser navigation, and an auth header is stored write-only. - **Components instead of prose**: compiled React components live in `app/src/components/gallery/`, sandboxed ones are authored in `/admin/playground` and published with no deployment. Every call asks the server whether the component exists, is published, and is not withheld from that Bot. Data functions are granted per component. - **Governed MCP**: Google Drive and Notion ship in the catalogue, reached as the person asking. The catalogue carries only vendors this deployment stands behind, so adding one is a review of that vendor. Custom servers must pass URL checks; unknown tools and custom-server tools are treated as writes, and a catalogue tool the server advertises but does not name as a write classifies as a read. A Bot is told which connectors exist here and which it holds, so it says it has not been granted one rather than browsing to the vendor's website. -- **Skills are instructions, not capabilities**: personal skills attach only to Bots their author owns, deployment skills are admin-owned, and both are invoked with `/` in the composer. +- **Skills are instructions, not capabilities**: personal skills attach only to Bots their author owns, deployment skills are admin-owned, and both are invoked with `/` in the composer. A Bot granted the shipped `skill-creator` skill can write one with you in the conversation, and saves it only when you press the button on the card. - **Sign in with what your company already has**: Google, Microsoft or Okta from the environment, or a company's own SAML or OpenID Connect provider registered while the deployment runs and routed by email domain. Any one turns sign-in on; several may be configured at once. - **Decide who gets in**: `/admin/people` lists everybody who has signed in, promotes and demotes them, and removes access, which ends the session they are using and stops the next sign-in. Every change is on the audit trail. - **An audit trail you can read**: `/admin/audit` lists what was permitted, what was refused and what failed, and every refusal carries the rule that caused it. diff --git a/app/src/components/skills/proposed-skill.tsx b/app/src/components/skills/proposed-skill.tsx new file mode 100644 index 000000000..6c0fab6a6 --- /dev/null +++ b/app/src/components/skills/proposed-skill.tsx @@ -0,0 +1,252 @@ +import { Link } from "@tanstack/react-router"; +import { useEffect, useRef, useState } from "react"; +import { Badge, GalleryFrame } from "@/components/gallery/frame"; +import { Button } from "@/components/ui/button"; +import type { SkillFormValues } from "@/lib/skills/form"; +import { + checkProposal, + type ProposedSkill, + skillCardAnswer, + wasSaved, +} from "@/lib/skills/proposal"; + +/** + * The skill a Bot has written, put in front of the person before it is saved. + * + * WHY THERE IS A CARD AT ALL, when a skill adds no capability and anybody signed in may write one. + * Two reasons, and neither is about permission. The first is authorship: this is a named thing that + * will appear in everybody's `/` menu with somebody's name on it, and a person should read the + * instruction their Bot drafted before it starts running on their behalf. The second is the slug: + * saving is how an edit is spelled, so an unattended save can replace a skill somebody is using. + * + * So the whole tool is this card. There is no handler behind it — the run suspends here, and nothing + * is written until a button is pressed. + */ + +export type ProposedSkillCardProps = { + /** Partial while the model is still streaming the arguments. */ + args: Partial; + /** + * What this slug already names here, when it names something. + * + * Drives the wording rather than the outcome — "Replace" instead of "Create", and whose it is. + * The server decides whether the replacement is allowed and says so in its own words. + */ + replaces?: { title: string; ownership: string }; + /** Answering resumes the Bot. Absent while streaming, and once this card has been answered. */ + respond?: (result: unknown) => Promise; + /** The recorded answer, once there is one. Completed cards show it instead of controls. */ + result?: string; + save: (values: SkillFormValues) => Promise; +}; + +export function ProposedSkillCard({ + args, + replaces, + respond, + result, + save, +}: ProposedSkillCardProps) { + const [sending, setSending] = useState<"save" | "decline" | null>(null); + /** + * A refusal from the server, kept on the card rather than answered with. + * + * The run stays suspended, because the two things worth doing next both need it to be — pressing + * Create again after connecting the connector the refusal named, or declining. Answering the tool + * with the error would end the turn and leave the person re-typing their request. + */ + const [refusal, setRefusal] = useState(null); + + if (result !== undefined) { + return ( + Done} + title={titleFor(args.slug, replaces)} + > +

{result}

+ {args.slug && wasSaved(result) ? ( + + ) : null} + + ); + } + + if (!respond) { + return ( + +

Writing the skill…

+ + ); + } + + const checked = checkProposal(args); + if (!checked.ok) { + return ; + } + const values = checked.values; + + const create = async () => { + setSending("save"); + setRefusal(null); + try { + await save(values); + } catch (cause) { + // Left on the card with the buttons still live. See `refusal` above. + setSending(null); + setRefusal( + cause instanceof Error ? cause.message : "The skill was not saved.", + ); + return; + } + await answer(respond, skillCardAnswer.saved(values.slug)); + }; + + const decline = async () => { + setSending("decline"); + await answer(respond, skillCardAnswer.declined()); + }; + + return ( + Waiting on you} + caption={ + replaces + ? `This replaces ${replaces.title}, which is ${replaces.ownership}.` + : "Nothing is saved until you press the button." + } + title={titleFor(values.slug, replaces)} + > +
+
Command
+
/{values.slug}
+
Title
+
{values.title}
+ {values.summary ? ( + <> +
One-liner
+
{values.summary}
+ + ) : null} + {values.tools.length > 0 ? ( + <> +
Needs
+
+ {values.tools.join(", ")} +
+ + ) : null} +
+ +

Instructions

+ {/* Scrolled rather than clamped: this is the part worth reading before agreeing to it. */} +

+ {values.instructions} +

+ + {values.tools.length > 0 ? ( +

+ Naming a tool grants nothing. A Bot is offered these only if an + administrator has already granted them to it. +

+ ) : null} + + {refusal ? ( +

+ {refusal} +

+ ) : null} + +
+ + +
+ + ); +} + +function titleFor( + slug: string | undefined, + replaces: { title: string } | undefined, +): string { + const name = slug ? `/${slug}` : "a new skill"; + return replaces ? `Replace ${name}` : `Create ${name}`; +} + +/** Where the remaining step happens. A skill on no Bot is inert, and saying so beats implying done. */ +function PutItOnABot({ slug }: { slug: string }) { + return ( + + Put it on a Bot + + ); +} + +/** + * A proposal that cannot be saved as written. + * + * Answered rather than shown as a question, because there is nothing for the person to decide: the + * fields are wrong in a way the model can fix, and the problems name their field so it can. The card + * still appears, so the transcript does not have a silent gap where a skill was nearly written. + */ +function Unwritable({ + problems, + respond, +}: { + problems: string[]; + respond: (result: unknown) => Promise; +}) { + /** + * Answered once, guarded by a ref rather than by the dependency list. + * + * `problems` is rebuilt from the arguments on every render and the grant queries behind this card + * poll, so a value-equal array arrives as a new identity every few seconds. Left to the deps, this + * would answer the same tool call again on each of them. + */ + const answered = useRef(false); + useEffect(() => { + if (answered.current) return; + answered.current = true; + void answer(respond, skillCardAnswer.unwritable(problems)); + }, [problems, respond]); + + return ( + Not saved} + title="Skill" + > +

+ The Bot's draft was not a valid skill, so nothing was saved. It has been + told what to fix. +

+ + ); +} + +/** Resuming a run that is no longer there is not a failure worth reporting: nothing is left to answer. */ +function answer( + respond: (result: unknown) => Promise, + sentence: string, +): Promise { + return respond(sentence).catch(() => {}); +} diff --git a/app/src/lib/copilot/provider.tsx b/app/src/lib/copilot/provider.tsx index 46001b609..65830a03c 100644 --- a/app/src/lib/copilot/provider.tsx +++ b/app/src/lib/copilot/provider.tsx @@ -9,6 +9,7 @@ import { GalleryTools } from "./gallery-tools"; import { GENERATIVE_UI_DESIGN_SKILL } from "./generative-ui"; import { HandoffTool } from "./handoff-tool"; import { SandboxedTools } from "./sandboxed-tools"; +import { SkillTools } from "./skill-tools"; /** * The CopilotKit client, wrapped once for the whole authenticated app. @@ -62,6 +63,8 @@ export function CopilotProvider({ children }: { children: ReactNode }) { {/* Browser-authored components use the same component grants as the compiled gallery. */} + {/* Offered only on a Bot holding the skill-creator skill; see skill-tools.tsx. */} + {children} diff --git a/app/src/lib/copilot/skill-tools.tsx b/app/src/lib/copilot/skill-tools.tsx new file mode 100644 index 000000000..fee1e8c31 --- /dev/null +++ b/app/src/lib/copilot/skill-tools.tsx @@ -0,0 +1,175 @@ +import { useFrontendTool, useHumanInTheLoop } from "@copilotkit/react-core/v2"; +import { useMutation, useQuery } from "@tanstack/react-query"; +import { useMemo } from "react"; +import { z } from "zod"; +import { ProposedSkillCard } from "@/components/skills/proposed-skill"; +import { currentUserQueryOptions } from "@/lib/auth/queries"; +import { useDeclaredBotId } from "@/lib/copilot/active-bot"; +import { saveSkillMutationOptions } from "@/lib/plugins/mutations"; +import { + agentPluginsQueryOptions, + pluginsPageQueryOptions, +} from "@/lib/plugins/queries"; +import type { SkillFormValues } from "@/lib/skills/form"; +import { + describeSkill, + describeSkills, + describeToolRefs, + ownershipOf, + type ProposedSkill, + proposedSkillSchema, + SKILL_CREATOR_SLUG, +} from "@/lib/skills/proposal"; +import { queryClient } from "@/query-client"; + +/** + * Writing a skill from inside a conversation. + * + * The deployment ships a skill called `skill-creator` whose instruction is how to interview somebody + * about the skill they want. These four tools are what let that interview end in a saved skill rather + * than in a wall of text somebody has to retype into a form: three reads so the Bot can see what + * already exists and which tools it may name, and one write that suspends the run on a card. + * + * WHY THE BROWSER AND NOT THE SERVER. A skill is written as a person, not as a Bot: the slug goes + * under their name, and whether they may take it is a question about them. `POST /api/plugins/skills` + * already answers exactly that — their own skills, an administrator's for the deployment, a refusal + * naming the slug otherwise — and a tool that rides the signed-in session inherits every bit of it + * unchanged, including the audit row. A server-side tool would have to carry an actor into a run that + * does not always have one (a routine, a Slack thread, a schedule), and the first way that goes wrong + * is a skill written under the wrong name. Nothing is lost by it being here: authoring a skill is an + * interview, so there is nobody to interview where there is no browser. + * + * WHY THEY ARE GATED ON THE GRANT. Four more tools on every run is not free — a model picks the right + * tool reliably out of about ten, which is the whole reason per-run narrowing exists — and a Bot for + * looking up transactions has no business drafting skills. So the gate is the ordinary one: the Bot + * has to hold the `skill-creator` skill. Granting that skill is what turns a coworker into one you can + * write skills with, and it is granted the same way everything else is. + */ + +export function SkillTools() { + // Grants are only fetched once a surface has declared its Bot; the placeholder is not one the + // server knows, and asking would 404 on every poll. + const declared = useDeclaredBotId(); + // Empty rather than undefined: the factory's own `enabled: agentId.length > 0` is the guard for + // "no Bot declared yet", so the placeholder never reaches the server. + const { data: granted } = useQuery(agentPluginsQueryOptions(declared ?? "")); + const authoring = (granted?.skills ?? []).some( + (skill) => skill.slug === SKILL_CREATOR_SLUG, + ); + + /* + * The Plugins payload carries two of the three reads: every skill scoped to whoever is asking, and + * every connected server's tools. It is the same query the Skills page runs, so a person who has + * opened that page pays nothing for this — and it is not run at all on a Bot that cannot author + * skills, which is most of them. + */ + const { data: page } = useQuery({ + ...pluginsPageQueryOptions(), + enabled: authoring, + }); + const { data: me } = useQuery(currentUserQueryOptions()); + const saveSkill = useMutation(saveSkillMutationOptions(queryClient)); + + const skills = page?.skills ?? []; + + useFrontendTool({ + name: "list_skills", + description: + "The skills that already exist in this deployment, one line each, with whose each one is. Read this before writing a skill: a slug is an identity, so reusing one replaces that skill, and a slug that is not the person's own cannot be replaced at all.", + parameters: z.object({}), + available: authoring, + handler: async () => describeSkills(skills, me?.id), + }); + + useFrontendTool({ + name: "read_skill", + description: + "One existing skill in full, including its instructions. Use this when improving a skill rather than writing a new one — never rewrite an instruction you have not read.", + parameters: z.object({ + slug: z + .string() + .describe( + "The skill's `/` command, without the slash, as list_skills spells it.", + ), + }), + available: authoring, + handler: async ({ slug }) => { + const wanted = String(slug).replace(/^\//, ""); + const skill = skills.find((candidate) => candidate.slug === wanted); + if (!skill) { + return `There is no skill called ${wanted} that this person can see. Call list_skills for the ones there are.`; + } + return describeSkill(skill); + }, + }); + + useFrontendTool({ + name: "list_skill_tools", + description: + "The `serverId/toolName` refs a skill here can declare, grouped by connector. Declaring a tool grants nothing — the offer a run gets is always the intersection of what the skill names with what the Bot was granted — so a skill may name tools for a connector this Bot does not hold yet.", + parameters: z.object({}), + available: authoring, + handler: async () => describeToolRefs(page?.servers ?? []), + }); + + /** + * The save, as a card rather than a handler. + * + * Stabilised with `useMemo` for the reason the gallery's decisions are: the render component is + * identity-compared, and a fresh function every render remounts the card, which would reset the + * pressed-Saving state and any refusal the server has just given it. + */ + const Card = useMemo( + () => + function ProposedSkillRender(props: { + args: Partial; + respond?: (result: unknown) => Promise; + result?: string; + }) { + const slug = props.args.slug; + const existing = slug + ? skills.find((candidate) => candidate.slug === slug) + : undefined; + return ( + { + await saveSkill.mutateAsync({ + instructions: values.instructions, + slug: values.slug, + summary: values.summary, + title: values.title, + /* + * Sent on every save, including empty, because the server replaces the declared set + * rather than merging into it. See `SkillInput`. + */ + tools: values.tools, + }); + }} + /> + ); + }, + [me?.id, saveSkill, skills], + ); + + useHumanInTheLoop({ + name: "save_skill", + description: + "Put a finished skill in front of the person to save. They see the command, the title and the whole instruction, and nothing is written unless they press the button. Call this once, at the end, after they have agreed to what the skill says — not to check your draft.", + parameters: proposedSkillSchema, + available: authoring, + render: Card, + }); + + return null; +} diff --git a/app/src/lib/skills/proposal.ts b/app/src/lib/skills/proposal.ts new file mode 100644 index 000000000..4420ec701 --- /dev/null +++ b/app/src/lib/skills/proposal.ts @@ -0,0 +1,248 @@ +import { z } from "zod"; +import type { PluginServer, PluginSkill } from "@/lib/plugins/queries"; +import { type SkillFormValues, skillFormSchema } from "./form"; + +/** + * A skill a Bot proposes during a conversation, and the checks it goes through before anybody is + * asked to approve it. + * + * WHY A SEPARATE SHAPE FROM THE FORM'S. `form.ts` describes what a person typed into four fields; a + * model hands over the same four as arguments it may have got wrong in ways a form cannot — a slug + * with a capital letter in it, a title the length of a paragraph, `tools` arriving as a string. So + * the model-facing schema is permissive about types and the checking is done afterwards, against the + * same `skillFormSchema` the form uses. That way a Bot writing a skill and a person writing one are + * held to one rule, and there is no second parser to drift from the server's. + * + * Nothing here decides whether the skill may be saved. That is the server's, and it stays the + * server's: a skill is an instruction rather than a capability, so the question is only ever whose + * name the slug is under, which `POST /api/plugins/skills` answers with a sentence. + */ + +/** + * What the card answers its tool call with, and the one fact it reads back out. + * + * WHY THESE ARE HERE AND NOT ON THE CARD. A completed card has only the result string to go on — the + * SDK hands back what the tool answered, not what happened — so whether to offer the "put it on a + * Bot" link is decided by reading the sentence. Written on the card, the sentence and the reader of + * the sentence were two literals a reword could silently separate, and the failure is a link that + * quietly stops appearing. Paired here, and pinned by a test, they cannot drift. + * + * The sentences are addressed to the model, because that is who receives them. Each one says what to + * do next, since a tool result that only reports state leaves the Bot to guess whether the turn is + * finished. + */ +const SAVED_MARKER = "Saved."; + +export const skillCardAnswer = { + saved: (slug: string) => + `${SAVED_MARKER} /${slug} is now in the slash menu. It is on no Bot yet — putting it on one is done from the Skills page, so tell the person that is the remaining step.`, + declined: () => + "The person did not save this skill. Ask what to change rather than saving it again unchanged.", + /** A proposal the fields refuse, answered rather than shown as a question. See the card. */ + unwritable: (problems: readonly string[]) => + `Not saved, and the person was not asked, because the skill is not valid: ${problems.join(" ")} Fix those and propose it again.`, +}; + +/** Whether a completed card's recorded answer is one that wrote a skill. */ +export function wasSaved(result: string): boolean { + return result.startsWith(SAVED_MARKER); +} + +/** + * The skill whose grant turns a Bot into one you can write skills with. + * + * The app offers `save_skill` and the reads beside it only while the active Bot holds this slug, and + * the shipped package is what puts the slug in the deployment and on a Bot. So the two have to agree: + * rename it in one place and writing a skill in a conversation stops working with nothing on screen + * to say why. `skill-creator-slug.test.ts` is the guard, which is why this lives in a module with no + * React in it rather than beside the hooks that read it. + * + * Not an MCP ref, and deliberately not declared in `skills.yaml`'s `tools:`. A declaration there is + * intersected with the Bot's MCP grants, so naming the app's own tools would name nothing. + */ +export const SKILL_CREATOR_SLUG = "skill-creator"; + +/** + * The arguments the model is offered, described for the model rather than for us. + * + * Every `describe` here is read by the thing filling the field in, which is why they say what good + * looks like instead of restating the type. The slug rule is spelled out because a model that has + * seen `Find A Document` in a title will otherwise offer it as a command. + */ +export const proposedSkillSchema = z.object({ + slug: z + .string() + .describe( + "The `/` command, in lower-case letters, numbers and hyphens: `check-a-claim`, not `Check A Claim`. 2 to 40 characters. This is the skill's identity — reusing one that already exists replaces that skill.", + ), + title: z + .string() + .describe( + "What the skill is called in the menu, in a few words of sentence case: `Check a claim against a source`. Up to 120 characters.", + ), + summary: z + .string() + .optional() + .describe( + "One line under the title, saying what invoking it will do. Up to 200 characters. Omit only if the title already says everything.", + ), + instructions: z + .string() + .describe( + "The instruction the Bot follows when somebody invokes this skill, written as directions to the Bot in the imperative. Say what to do, in what order, what to do when a step turns up nothing, and what the answer should contain. Explain why a step matters rather than only naming it. Do not describe the skill in the third person, and do not address the person invoking it.", + ), + tools: z + .array(z.string()) + .optional() + .describe( + "The tools the skill needs, as `serverId/toolName` refs from list_skill_tools. This is a declaration, not a grant: naming a tool cannot make it callable, and a skill naming a tool its Bot does not hold simply loads nothing. Omit for a skill that is only prose.", + ), +}); + +export type ProposedSkill = z.infer; + +/** What checking a proposal answers with: the four fields to save, or the problems to fix. */ +export type CheckedProposal = + | { ok: true; values: SkillFormValues } + | { ok: false; problems: string[] }; + +/** + * Hold a proposal to the same rule the form is held to. + * + * Checked here rather than left to the server because the alternative is a person being shown a card + * for a skill that cannot be saved, pressing Create, and reading a validation message as though they + * had done something wrong. A problem the model can fix should reach the model, not the person. + * + * The problems name their field, because that is the only part a model needs in order to try again: + * "slug: Lower-case letters, numbers and hyphens, 2 to 40 characters." is a repair instruction, + * while "invalid input" is a guess. + */ +export function checkProposal(args: { + slug?: unknown; + title?: unknown; + summary?: unknown; + instructions?: unknown; + tools?: unknown; +}): CheckedProposal { + const parsed = skillFormSchema.safeParse({ + slug: typeof args.slug === "string" ? args.slug : "", + title: typeof args.title === "string" ? args.title : "", + // Optional on the server too, so an absent one is an empty one rather than a problem. + summary: typeof args.summary === "string" ? args.summary : "", + instructions: + typeof args.instructions === "string" ? args.instructions : "", + /* + * Anything that is not a list of strings is dropped rather than refused. A model that answers + * `tools: "google-drive/search_files"` meant the one tool, and a refusal here would send it back + * to redo the whole proposal over a comma. Declaring nothing is always a valid skill. + */ + tools: toolRefsIn(args.tools), + }); + if (parsed.success) return { ok: true, values: parsed.data }; + return { + ok: false, + problems: parsed.error.issues.map((issue) => { + const field = issue.path.join("."); + return field ? `${field}: ${issue.message}` : issue.message; + }), + }; +} + +/** Tool refs out of whatever the model sent, keeping the strings and a lone string on its own. */ +function toolRefsIn(value: unknown): string[] { + if (typeof value === "string") return value.trim() ? [value.trim()] : []; + if (!Array.isArray(value)) return []; + return value + .filter((ref): ref is string => typeof ref === "string") + .map((ref) => ref.trim()) + .filter((ref) => ref.length > 0); +} + +/** + * Whose a skill is, in the words the answer uses. + * + * Ownership rather than permission, and the distinction is the point. Who may replace a slug is + * decided by the server and nowhere else; what this says is the fact a model needs in order not to + * try — the same fact the Skills page groups its two lists by. A model told "someone else's" and + * told in its instructions that those cannot be replaced will pick another name, and if it tries + * anyway the server's own sentence comes back on the card. + */ +export function ownershipOf( + skill: Pick, + meId: string | undefined, +): "yours" | "the deployment's" | "someone else's" { + if (skill.ownerUserId === null) return "the deployment's"; + return skill.ownerUserId === meId ? "yours" : "someone else's"; +} + +/** + * The skills that already exist here, as the answer to `list_skills`. + * + * One line each, because the reason to ask is to pick a name and to find the skill being improved, + * and a paragraph per skill would push the useful part of a long list out of the run. The + * instructions are deliberately not included: forty skills' instructions is most of a context + * window, and a Bot improving one asks for that skill by slug. + */ +export function describeSkills( + skills: readonly PluginSkill[], + meId: string | undefined, +): string { + if (skills.length === 0) { + return "No skills exist here yet. Any slug is free."; + } + const lines = skills.map((skill) => { + const parts = [ + `/${skill.slug} — ${skill.title} (${ownershipOf(skill, meId)})`, + ]; + if (skill.summary) parts.push(skill.summary); + if (skill.tools.length > 0) parts.push(`needs ${skill.tools.join(", ")}`); + return `- ${parts.join(" · ")}`; + }); + return [ + `${skills.length} skill${skills.length === 1 ? "" : "s"} exist here. A slug that is not yours cannot be replaced — pick another name.`, + ...lines, + ].join("\n"); +} + +/** + * One skill's instructions in full, for improving it rather than listing it. + * + * Separate from {@link describeSkills} so the expensive field is fetched deliberately, one skill at + * a time, by a Bot that has been asked to change that skill. + */ +export function describeSkill(skill: PluginSkill): string { + return [ + `/${skill.slug} — ${skill.title}`, + skill.summary ? `Summary: ${skill.summary}` : "Summary: (none)", + skill.tools.length > 0 + ? `Declared tools: ${skill.tools.join(", ")}` + : "Declared tools: (none)", + "Instructions:", + skill.instructions, + ].join("\n"); +} + +/** + * The tool refs a skill here could name, as the answer to `list_skill_tools`. + * + * Every connected server's tools, not the active Bot's grants, and that is deliberate rather than + * loose. A declaration is not a grant: the offer a run gets is the intersection of what the skill + * names with what the Bot holds, so a skill may — and should — name tools for a connector this + * deployment has not granted to this Bot yet, which is what makes granting it later the only step. + * Narrowing this list to the current Bot's grants would quietly produce skills that stop working the + * moment they are put on a different Bot. + */ +export function describeToolRefs(servers: readonly PluginServer[]): string { + const connected = servers.filter((server) => server.tools.length > 0); + if (connected.length === 0) { + return "No connector here offers any tools yet, so a skill written now can only be prose. That is ordinary — most skills are."; + } + const blocks = connected.map((server) => { + const tools = server.tools.map( + (tool) => + ` - ${tool.ref}${tool.effect === "write" ? " (writes)" : ""} — ${tool.description || tool.name}`, + ); + return [`${server.title} (${server.id}):`, ...tools].join("\n"); + }); + return ["Refs are `serverId/toolName`.", ...blocks].join("\n"); +} diff --git a/app/tests/proposed-skill-card.test.tsx b/app/tests/proposed-skill-card.test.tsx new file mode 100644 index 000000000..6136defb0 --- /dev/null +++ b/app/tests/proposed-skill-card.test.tsx @@ -0,0 +1,186 @@ +import { afterAll, afterEach, beforeAll, expect, test } from "bun:test"; +import { GlobalRegistrator } from "@happy-dom/global-registrator"; +import { cleanup, render, waitFor } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { ProposedSkillCard } from "@/components/skills/proposed-skill"; +import { skillCardAnswer } from "@/lib/skills/proposal"; + +/** + * The card a Bot's draft skill stops on, and the answers it can give. + * + * The card IS the tool: `save_skill` has no handler, so the run suspends here and every way out is a + * render away from a `respond` call. That makes the states worth pinning individually — a card that + * answers twice resumes a turn that has already moved on, and one that answers a server refusal ends + * the turn at the moment the person could have fixed it and pressed again. + * + * Nothing is stubbed. Saving is a prop, so the card is exercised with a plain function and the + * transport never enters this file. + * + * Queries come off `render()`'s return rather than `screen`, and the window is installed and removed + * around this file rather than at module scope, so the suite can be walked in any order without this + * file deciding whether a later one has a DOM. + */ + +beforeAll(() => GlobalRegistrator.register()); +afterEach(cleanup); +afterAll(() => GlobalRegistrator.unregister()); + +/** A draft that passes the fields, so each test varies only the thing it is about. */ +const DRAFT = { + slug: "weekly-summary", + title: "Weekly summary", + summary: "Say what moved this week.", + instructions: "List what moved this week, newest first. Name each source.", + tools: ["google-drive/search_files"], +}; + +test("a draft still streaming shows no controls to press", () => { + // `respond` is absent until the arguments are complete. A button drawn before then would answer + // the tool call with a skill the model had not finished writing. + const { getByText, queryByRole } = render( + {}} />, + ); + + expect(getByText("Writing the skill…")).toBeTruthy(); + expect(queryByRole("button")).toBeNull(); +}); + +test("a complete draft shows the whole instruction before anything is written", () => { + const { getByText, getByRole } = render( + {}} + save={async () => {}} + />, + ); + + // The instruction in full, because it is the part somebody is actually agreeing to. + expect( + getByText("List what moved this week, newest first. Name each source."), + ).toBeTruthy(); + expect(getByText("/weekly-summary")).toBeTruthy(); + expect(getByText("google-drive/search_files")).toBeTruthy(); + expect( + getByText("Nothing is saved until you press the button."), + ).toBeTruthy(); + expect(getByRole("button", { name: "Create it" })).toBeTruthy(); +}); + +test("pressing create saves the checked fields, then resumes the run", async () => { + const saved: unknown[] = []; + const answers: unknown[] = []; + const { getByRole } = render( + { + answers.push(result); + }} + save={async (values) => { + saved.push(values); + }} + />, + ); + + await userEvent.click(getByRole("button", { name: "Create it" })); + + await waitFor(() => expect(saved).toHaveLength(1)); + expect(saved[0]).toEqual(DRAFT); + // Saved first, answered second: a run resumed before the write lands would be told about a skill + // that does not exist yet. + await waitFor(() => expect(answers).toHaveLength(1)); + expect(answers[0]).toBe(skillCardAnswer.saved("weekly-summary")); +}); + +test("a slug that already exists is offered as a replacement, not a create", () => { + const { getByRole, getByText } = render( + {}} + save={async () => {}} + />, + ); + + // Saving is how an edit is spelled here, so the wording has to say which one this press is. + expect(getByRole("button", { name: "Replace it" })).toBeTruthy(); + expect(getByText(/This replaces Weekly summary/)).toBeTruthy(); +}); + +test("a refusal from the server stays on the card and leaves the run suspended", async () => { + const answers: unknown[] = []; + const { getByRole, getByText } = render( + { + answers.push(result); + }} + save={async () => { + throw new Error("weekly-summary is somebody else's skill."); + }} + />, + ); + + await userEvent.click(getByRole("button", { name: "Create it" })); + + // The server's own sentence, because paraphrasing it throws away the only part worth reading. + await waitFor(() => + expect(getByText("weekly-summary is somebody else's skill.")).toBeTruthy(), + ); + // Not answered: the two useful next moves — press again, or decline — both need the run still there. + expect(answers).toHaveLength(0); + expect(getByRole("button", { name: "Create it" })).toBeTruthy(); +}); + +test("declining answers the run and tells the Bot to ask rather than retry", async () => { + const answers: unknown[] = []; + const saved: unknown[] = []; + const { getByRole } = render( + { + answers.push(result); + }} + save={async () => { + saved.push(1); + }} + />, + ); + + await userEvent.click(getByRole("button", { name: "Don't save" })); + + await waitFor(() => expect(answers).toHaveLength(1)); + expect(answers[0]).toBe(skillCardAnswer.declined()); + expect(saved).toHaveLength(0); +}); + +test("a draft the fields refuse answers once with the problems, and never asks the person", async () => { + const answers: unknown[] = []; + const saved: unknown[] = []; + const card = ( + { + answers.push(result); + }} + save={async () => { + saved.push(1); + }} + /> + ); + const { rerender, queryByRole } = render(card); + + await waitFor(() => expect(answers).toHaveLength(1)); + expect(String(answers[0])).toContain("slug:"); + expect(saved).toHaveLength(0); + // Nothing to decide, so nothing is offered. + expect(queryByRole("button")).toBeNull(); + + /* + * THE REGRESSION THIS HOLDS DOWN. The problems are rebuilt from the arguments on every render and + * the grant queries behind this card poll, so a value-equal array arrives as a new identity every + * few seconds. Left to the effect's dependency list, this answered the same tool call again on each + * of them, resuming a turn that had already moved on. + */ + for (let index = 0; index < 3; index++) rerender(card); + expect(answers).toHaveLength(1); +}); diff --git a/app/tests/skill-creator-slug.test.ts b/app/tests/skill-creator-slug.test.ts new file mode 100644 index 000000000..a67460f54 --- /dev/null +++ b/app/tests/skill-creator-slug.test.ts @@ -0,0 +1,50 @@ +import { describe, expect, test } from "bun:test"; +import { readFileSync } from "node:fs"; +import { parse } from "yaml"; +import { SKILL_CREATOR_SLUG } from "@/lib/skills/proposal"; + +/** + * The one coupling in this feature that can break silently. + * + * The app offers the four authoring tools only to a Bot holding `SKILL_CREATOR_SLUG`, and the shipped + * package is what puts a skill under that slug in the deployment and grants it to a Bot. Neither half + * fails loudly without the other: rename the slug in the YAML and the tools are simply never offered, + * grant it to nobody and there is no Bot to write a skill with. Both look like a working deployment + * where writing a skill in a conversation does nothing, which is the kind of break nobody finds. + * + * Read from the file rather than from a fixture, because the file is what ships. + */ + +const packageDir = new URL("../../examples/fintech", import.meta.url).pathname; + +const shipped = parse(readFileSync(`${packageDir}/skills.yaml`, "utf8")) as { + skills: { slug: string; title: string; instructions: string }[]; +}; + +const roster = parse(readFileSync(`${packageDir}/agents.yaml`, "utf8")) as { + agents: { id: string; skills?: string[] }[]; +}; + +describe("the shipped package and the app agree on the authoring skill", () => { + test("the package ships a skill under the slug the app gates on", () => { + const slugs = shipped.skills.map((skill) => skill.slug); + expect(slugs).toContain(SKILL_CREATOR_SLUG); + }); + + test("at least one shipped coworker is granted it", () => { + // A skill on no Bot is inert: it is in everybody's `/` menu and no run can act on it. + const holders = roster.agents.filter((agent) => + (agent.skills ?? []).includes(SKILL_CREATOR_SLUG), + ); + expect(holders.length).toBeGreaterThan(0); + }); + + test("its instruction names the tool that actually saves the skill", () => { + // The instruction is the feature. One that never reaches `save_skill` produces a Bot that + // interviews somebody thoroughly and then asks them to retype it into the form. + const creator = shipped.skills.find( + (skill) => skill.slug === SKILL_CREATOR_SLUG, + ); + expect(creator?.instructions).toContain("save_skill"); + }); +}); diff --git a/app/tests/skill-proposal.test.ts b/app/tests/skill-proposal.test.ts new file mode 100644 index 000000000..01b94e60b --- /dev/null +++ b/app/tests/skill-proposal.test.ts @@ -0,0 +1,285 @@ +import { describe, expect, test } from "bun:test"; +import type { PluginServer, PluginSkill } from "@/lib/plugins/queries"; +import { + checkProposal, + describeSkill, + describeSkills, + describeToolRefs, + ownershipOf, + skillCardAnswer, + wasSaved, +} from "@/lib/skills/proposal"; + +/** + * A skill a Bot wrote, on its way to the card somebody presses. + * + * The checking is here rather than left to the server because the alternative is a person being shown + * a card for a skill that cannot be saved. So these tests are about one thing: a problem a model can + * fix reaches the model, and everything else reaches the person unchanged. + */ + +const skill = (over: Partial = {}): PluginSkill => ({ + id: "skill_1", + slug: "check-a-claim", + ownerUserId: null, + title: "Check a claim against a source", + summary: "Take a statement and check it against the documents.", + instructions: "You are checking a claim, not answering a question.", + origin: "package", + installedBy: null, + grantedTo: [], + tools: [], + ...over, +}); + +describe("checking what a Bot proposed", () => { + test("a complete proposal passes and keeps every field", () => { + const checked = checkProposal({ + slug: "weekly-summary", + title: "Weekly summary", + summary: "Summarise the week.", + instructions: "List what moved this week, newest first.", + tools: ["google-drive/search_files"], + }); + expect(checked.ok).toBeTrue(); + if (!checked.ok) return; + expect(checked.values).toEqual({ + slug: "weekly-summary", + title: "Weekly summary", + summary: "Summarise the week.", + instructions: "List what moved this week, newest first.", + tools: ["google-drive/search_files"], + }); + }); + + test("a slug a model wrote as a title is refused, naming the field", () => { + const checked = checkProposal({ + slug: "Weekly Summary", + title: "Weekly summary", + instructions: "List what moved.", + }); + expect(checked.ok).toBeFalse(); + if (checked.ok) return; + // The field is the repair instruction: a model told only "invalid" would guess at which of four. + expect(checked.problems.join(" ")).toContain("slug:"); + }); + + test("missing instructions are a problem rather than an empty skill", () => { + const checked = checkProposal({ slug: "standup", title: "Standup" }); + expect(checked.ok).toBeFalse(); + if (checked.ok) return; + expect(checked.problems.join(" ")).toContain("instructions:"); + }); + + test("an absent summary is an empty one, not a problem", () => { + const checked = checkProposal({ + slug: "standup", + title: "Standup", + instructions: "Summarise yesterday.", + }); + expect(checked.ok).toBeTrue(); + if (!checked.ok) return; + expect(checked.values.summary).toBe(""); + }); + + test("declaring no tools is ordinary, and arrives as an empty array", () => { + const checked = checkProposal({ + slug: "standup", + title: "Standup", + instructions: "Summarise yesterday.", + }); + expect(checked.ok).toBeTrue(); + if (!checked.ok) return; + // Sent rather than omitted: the server replaces the declared set when the field is present. + expect(checked.values.tools).toEqual([]); + }); + + test("one tool sent as a bare string is read as that one tool", () => { + // A model that answers with the string meant the single tool. Refusing would send it back to + // redo a whole proposal over a comma, and declaring nothing is always valid anyway. + const checked = checkProposal({ + slug: "standup", + title: "Standup", + instructions: "Summarise yesterday.", + tools: "notion/notion-search", + }); + expect(checked.ok).toBeTrue(); + if (!checked.ok) return; + expect(checked.values.tools).toEqual(["notion/notion-search"]); + }); + + test("junk among the tools is dropped rather than failing the proposal", () => { + const checked = checkProposal({ + slug: "standup", + title: "Standup", + instructions: "Summarise yesterday.", + tools: ["notion/notion-search", 7, null, " ", "notion/notion-fetch"], + }); + expect(checked.ok).toBeTrue(); + if (!checked.ok) return; + expect(checked.values.tools).toEqual([ + "notion/notion-search", + "notion/notion-fetch", + ]); + }); +}); + +describe("whose a skill is", () => { + test("a deployment skill belongs to nobody in particular", () => { + expect(ownershipOf(skill({ ownerUserId: null }), "user_1")).toBe( + "the deployment's", + ); + }); + + test("a person's own skill is theirs", () => { + expect(ownershipOf(skill({ ownerUserId: "user_1" }), "user_1")).toBe( + "yours", + ); + }); + + test("somebody else's is named as such, including when nobody is signed in", () => { + expect(ownershipOf(skill({ ownerUserId: "user_2" }), "user_1")).toBe( + "someone else's", + ); + expect(ownershipOf(skill({ ownerUserId: "user_2" }), undefined)).toBe( + "someone else's", + ); + }); +}); + +describe("what already exists here", () => { + test("an empty deployment says every slug is free", () => { + expect(describeSkills([], "user_1")).toContain("Any slug is free"); + }); + + test("each skill is one line, with whose it is and what it needs", () => { + const answer = describeSkills( + [ + skill({ slug: "mine", ownerUserId: "user_1", title: "Mine" }), + skill({ + slug: "theirs", + ownerUserId: "user_2", + title: "Theirs", + tools: ["notion/notion-search"], + }), + ], + "user_1", + ); + expect(answer).toContain("/mine — Mine (yours)"); + expect(answer).toContain("/theirs — Theirs (someone else's)"); + expect(answer).toContain("needs notion/notion-search"); + // The count and the rule that stops a model taking a name it cannot have. + expect(answer).toContain("2 skills exist here"); + expect(answer).toContain("cannot be replaced"); + }); + + test("the listing carries no instructions, because forty of them is a context window", () => { + const answer = describeSkills( + [skill({ instructions: "SECRET-LONG-INSTRUCTION" })], + "user_1", + ); + expect(answer).not.toContain("SECRET-LONG-INSTRUCTION"); + }); + + test("reading one skill does carry its instructions, which is the point of it", () => { + const answer = describeSkill( + skill({ instructions: "Quote the sentence you rely on." }), + ); + expect(answer).toContain("Quote the sentence you rely on."); + expect(answer).toContain("Declared tools: (none)"); + }); +}); + +describe("the tool refs a skill may declare", () => { + const server = (over: Partial = {}): PluginServer => + ({ + id: "notion", + title: "Notion", + tools: [ + { + serverId: "notion", + name: "notion-search", + description: "Search Notion.", + inputSchema: {}, + ref: "notion/notion-search", + effect: "read", + grantedTo: [], + }, + ], + ...over, + }) as PluginServer; + + test("a deployment with no connector says a skill can only be prose", () => { + expect(describeToolRefs([])).toContain("only be prose"); + }); + + test("refs are grouped by connector and a write says so", () => { + const answer = describeToolRefs([ + server({ + tools: [ + { + serverId: "notion", + name: "notion-create", + description: "Create a page.", + inputSchema: {}, + ref: "notion/notion-create", + effect: "write", + grantedTo: [], + }, + ], + }), + ]); + expect(answer).toContain("Notion (notion):"); + expect(answer).toContain("notion/notion-create (writes)"); + }); + + test("a connector advertising nothing is left out rather than shown empty", () => { + const answer = describeToolRefs([ + server(), + server({ id: "drive", title: "Drive", tools: [] }), + ]); + expect(answer).toContain("Notion (notion):"); + expect(answer).not.toContain("Drive (drive):"); + }); + + test("every connector is offered, not only the ones this Bot holds", () => { + // A declaration is not a grant. Narrowing this to the current Bot's grants would quietly produce + // skills that stop working the moment they are put on a different Bot. + const answer = describeToolRefs([server({ tools: server().tools })]); + expect(answer).toContain("notion/notion-search"); + }); +}); + +describe("what the card answers its tool call with", () => { + test("a saved answer is recognised as one, by the card that wrote it", () => { + // The pairing is the point: a completed card decides whether to offer the "put it on a Bot" link + // by reading its own recorded answer, so a reword must not be able to separate the two. + expect(wasSaved(skillCardAnswer.saved("weekly-summary"))).toBeTrue(); + expect(skillCardAnswer.saved("weekly-summary")).toContain( + "/weekly-summary", + ); + }); + + test("declining and refusing are not saves", () => { + expect(wasSaved(skillCardAnswer.declined())).toBeFalse(); + expect(wasSaved(skillCardAnswer.unwritable(["slug: no."]))).toBeFalse(); + }); + + test("a saved answer names the step that is left, because the skill is on no Bot", () => { + expect(skillCardAnswer.saved("standup")).toContain("Skills page"); + }); + + test("an invalid proposal is answered with the problems, so the model can retry", () => { + const answer = skillCardAnswer.unwritable([ + "slug: Lower-case letters.", + "instructions: Instructions are required.", + ]); + expect(answer).toContain("slug: Lower-case letters."); + expect(answer).toContain("instructions: Instructions are required."); + expect(answer).toContain("propose it again"); + }); + + test("declining tells the Bot to ask rather than to try again unchanged", () => { + expect(skillCardAnswer.declined()).toContain("Ask what to change"); + }); +}); diff --git a/docs/architecture.md b/docs/architecture.md index 46b4e2def..8edff87d2 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -249,6 +249,16 @@ A catalogue entry says whose credential a Bot reaches it with, which is a differ Every MCP call checks the grant first, then evaluates the same action policy engine with MCP context, then audits the result. +### Writing a skill in a conversation + +A skill can be written from the composer as well as from `/skills`. The deployment ships a skill called `skill-creator` whose instruction is how to interview somebody about the skill they want; a Bot holding it is also offered four tools the app registers — `list_skills`, `read_skill`, `list_skill_tools`, and `save_skill`, which suspends the run on a card showing the command, the title and the whole instruction. Nothing is written until the person presses the button. + +The grant is the gate. Those four tools are offered only while the Bot holds `skill-creator`, because four extra tools on every run costs the narrowing above what it exists to buy, and a Bot for looking up transactions has no business drafting skills. + +They run in the browser as the signed-in person, through the same `POST /api/plugins/skills` the Skills page uses, so the ownership rules and the audit row are the endpoint's rather than a second copy of them: a person's own slug, an administrator's for the deployment, and a refusal naming the slug otherwise. Written server-side, the tool would have to carry an actor into runs that do not have one — a routine, a Slack thread, a schedule — and the first way that goes wrong is a skill written under the wrong name. Nothing is lost by the restriction, because authoring is an interview and there is nobody to interview where there is no browser. + +A saved skill is on no Bot yet. Granting it is the remaining step, and it stays on the Skills page, where a skill somebody wrote can go only on Bots they own. + ### Which tools a run is offered A model picks the right tool reliably out of about ten, and unreliably out of thirty. A deployment that connects two vendors passes that point on its first afternoon, so a Bot holding more than a handful of tools is offered, per run, only the tools of the skills that match the message. diff --git a/docs/configuration.md b/docs/configuration.md index 738d1a1d8..6b58365ec 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -498,6 +498,8 @@ Each skill becomes a deployment skill on boot: everybody sees it in the `/` menu Refs are `serverId/toolName`, the same form a grant is written in. A package may name tools for a connector nobody has added — the ref sits inert until that connector exists, because what a Bot is offered is always intersected with what it was granted. **Naming a tool here grants nothing.** +One slug is load-bearing. A Bot granted `skill-creator` is offered the four tools that let a conversation end in a saved skill, so a package shipping that skill should also grant it to a Bot in `agents.yaml` — shipping it and granting it to nobody boots a deployment where writing a skill in the composer quietly does nothing. It declares no `tools`, and should not: those four are the app's own rather than a connector's, so they are not `serverId/toolName` refs. See [architecture.md](architecture.md#writing-a-skill-in-a-conversation). + Slugs are lowercase letters, digits and hyphens. If a package ships a slug somebody in the deployment already wrote a skill under, theirs keeps the name, the package loses that skill, and startup continues. Omit the file entirely for a package with no skills. diff --git a/examples/fintech/agents.yaml b/examples/fintech/agents.yaml index 292305ab7..2427e4257 100644 --- a/examples/fintech/agents.yaml +++ b/examples/fintech/agents.yaml @@ -8,6 +8,15 @@ agents: avatar_seed: general-assistant type: built-in system_prompt: You are a helpful general assistant. Give clear, concise, and accurate answers. + # Authoring lives on the everyday Bot, and on this one only. + # + # Holding `skill-creator` is what makes the app offer this Bot the four authoring tools, so this + # line is the difference between a deployment where people can write skills in a conversation and + # one where they fill in the form on /skills. It is on the general assistant because that is the + # Bot somebody already has open when they notice they keep asking for the same thing; putting it + # on Knowledge as well would spend four of its tool slots on a job nobody would ask it to do. + skills: + - skill-creator - id: knowledge name: Knowledge title: Company Knowledge diff --git a/examples/fintech/skills.yaml b/examples/fintech/skills.yaml index 24a1cca4c..d5687285a 100644 --- a/examples/fintech/skills.yaml +++ b/examples/fintech/skills.yaml @@ -78,3 +78,103 @@ skills: tools: - google-drive/search_files - google-drive/read_file_content + + # The skill that writes skills. Its instruction is the whole feature: `save_skill` and the three + # reads beside it are offered to a Bot only while it holds this slug, so granting this is what turns + # a coworker into one you can author with. See app/src/lib/copilot/skill-tools.tsx. + # + # Declares no tools, and should not. The authoring tools are the app's own rather than a connector's, + # so they are not `serverId/toolName` refs and a declaration here would name nothing. Selection only + # ever narrows MCP tools; these are offered whenever the grant is there. + - slug: skill-creator + title: Write a skill + summary: Interview me about a skill I want, write it, and save it as a new `/` command. + instructions: |- + You are writing a skill: a named instruction anybody in this deployment can invoke with `/`. + Interview first, write second, and save only what the person has agreed to. + + A skill here is four things and nothing else. A command, which is its slug and its identity. A + title, for the menu. A one-line summary. And the instruction a Bot follows when somebody invokes + it. It may also declare the tools it needs, which is a declaration rather than a grant: naming a + tool cannot make it callable, and a skill naming a tool its Bot does not hold simply loads + nothing. There are no files, no scripts, no attachments and no example folders. Anything that + cannot be said in those fields does not belong in a skill, and saying so early is kinder than + discovering it at the end. + + Work in this order. + + 1. Find out what the skill is for, in the person's own words. What should the Bot do that it does + not reliably do now? When would they reach for this rather than just asking? What should come + back — a list, a verdict, a drafted message, a number? If they have asked for the same thing + three times in different words, that repetition is the skill; say so. + + 2. Interview until you could do the task yourself. Ask about the order of steps, what counts as + done, what the answer must contain, and above all what should happen when a step turns up + nothing — that is the case a weak skill leaves out and the case that actually occurs. Ask + about the shape of the output if the output has a shape. Do not ask more than three questions + in one message, and do not ask what you can infer. + + 3. Look before you name. Call `list_skills`. A slug is an identity, so reusing one replaces that + skill: if what they want already exists, improve that one instead of writing a near-duplicate, + and if the name they want belongs to somebody else, pick another and tell them why. A slug is + lower-case letters, numbers and hyphens, and reads as a command — `check-a-claim`, not + `Check_A_Claim`. + + 4. Write the instruction. Address the Bot, in the imperative, as directions for the run. Never + describe the skill in the third person and never address the person invoking it. + + 5. Declare the tools it needs, if it needs any. Call `list_skill_tools` for the refs; never + invent one. Declare tools for a connector this Bot does not hold if the skill genuinely needs + them — that is what makes the skill work the moment somebody grants them — but say plainly + which of them are not connected yet, because until they are, the skill will run without them. + Most skills are prose and need none. + + 6. Rehearse it before you save it. Take one realistic request the person would actually type, + and walk through what the instruction as written would have you do, step by step, out loud. + This is the only test available before saving, and it is worth doing: it is where you find the + step that assumes a tool you did not declare, the ambiguity that could go two ways, and the + missing answer for the case where the search returns nothing. Fix what the rehearsal exposes + and rehearse again if you changed anything that matters. + + 7. Save it with `save_skill`, once, at the end. That puts the whole thing in front of the person + to read and press a button on; nothing is written until they do. Do not call it to check a + draft — show them the draft in the conversation instead. If they decline, ask what to change. + + 8. Say what is left. A saved skill is on no Bot yet, so nobody can invoke it. Tell them the + remaining step is to put it on a Bot from the Skills page, and that a skill they wrote can go + only on Bots they own. + + Improving a skill that already exists is the same job with a different first move. Call + `read_skill` and read the instruction in full before you touch it — never rewrite an instruction + you have not read. Ask what went wrong in the run that brought them here, change the part that + caused it, and leave the rest alone. Saving under the same slug is how an edit is spelled. + + What makes the instruction good, and why each of these matters: + + - Say why, not only what. A Bot that knows an instruction exists to stop it answering from + memory will apply it to a case the instruction never named. A bare rule breaks on the first + case just outside it. + - Say what to do when there is nothing. "Say you found nothing and say what you searched for" is + a complete instruction; leaving the case out is what produces a confident invented answer. + - Keep it lean. Every sentence that is not pulling weight makes the ones that are harder to + follow. If you cannot say what a sentence changes about the run, cut it. + - Generalise past the example. The person will describe one case; write for the shape of it, or + you have written a skill that works once. + - Name what the answer contains. A skill that does the work and then answers in a different + shape every time has not finished the job. + - Do not restate what the Bot already is. Its role, its manner and its standing instructions are + already in the run. A skill that opens by saying it is a helpful assistant has spent its first + sentence on nothing. + + The summary is not decoration. It is what somebody reads in the `/` menu, and it is what this + deployment's own model reads when it decides which skills a message is about — a Bot holding many + tools is offered, per run, only the tools of the skills that match. So write the summary as the + thing a person would be trying to do, in their words, not as a label for a feature. + + Two things never to write, whatever you are asked. A skill that tells a Bot to work around a + refusal, retry a blocked action by another route, or present a refused action as done: the + gateway refuses, the refusal is recorded, and an instruction to hide that is an instruction to + lie to the person relying on it. And a skill that tells a Bot to withhold what it did, from whom + it did it for. A skill adds no capability, which is exactly why anybody may write one; that + holds only while a skill cannot be used to launder one. If you are asked for either, say which + part you will not write and offer the version you will. diff --git a/server/tests/tenant-package.test.ts b/server/tests/tenant-package.test.ts index 072585328..123de9c93 100644 --- a/server/tests/tenant-package.test.ts +++ b/server/tests/tenant-package.test.ts @@ -307,7 +307,13 @@ describe("tenant YAML validation", () => { systemPrompt: "You are a helpful general assistant. Give clear, concise, and accurate answers.", }, - skills: [], + /* + * Authoring, and only on this Bot. Holding `skill-creator` is what makes the app offer the four + * tools that turn an interview into a saved skill, so this pairing is the feature rather than a + * detail of the example: a package that shipped the skill and granted it to nobody would boot a + * deployment where writing a skill in a conversation quietly does not work. + */ + skills: ["skill-creator"], }); // The pairing the shipped package makes, which is the whole reason Knowledge narrows to document // tools rather than being offered everything its grants hold. @@ -319,6 +325,19 @@ describe("tenant YAML validation", () => { "whats-changed", "who-owns-this", ]); + /* + * The skill that writes skills, shipped with no declared tools on purpose. Its tools are the app's + * own rather than a connector's, so they are not `serverId/toolName` refs; a declaration here would + * name nothing, and selection only ever narrows MCP tools. + */ + const creator = tenantPackage.skills.find( + (skill) => skill.slug === "skill-creator", + ); + expect(creator?.title).toBe("Write a skill"); + expect(creator?.tools).toEqual([]); + // The instruction is the whole feature, so pin the two ends of the loop it prescribes. + expect(creator?.instructions).toContain("Interview first"); + expect(creator?.instructions).toContain("save_skill"); expect(tenantPackage.channels).toContainEqual({ id: "general-assistant", name: "General Assistant",