diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ab6e5e2..cbee9e84 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,31 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### Knowledge searches instead of guessing + +A package can say which of its skills each coworker gets, and the fintech example gives Knowledge the +four document skills it ships. + +Knowledge is one of three coworkers in the box, described as answering company questions and citing +sources. The skills that would let it do that were seeded attached to nobody, so every clone started +with them paired to no Bot: the per-run narrowing that skills exist for was switched off until +somebody opened the Skills page and made the pairing by hand, in each deployment, again after each +new connector. The pairing belongs with the package, which wrote both files and knows which coworker +it meant them for. + +THIS GRANTS NOTHING, which is what makes it safe to seed. A skill is an instruction; what a Bot may +call is its grants, and the offer each run is the intersection of the two. A skill naming a tool its +Bot does not hold loads nothing. Seeding an MCP grant would be the opposite, because those reach a +person's own account, so those stay an administrator's decision and are untouched here. + +A redeploy takes back only what the package gave. Grants it made carry `tenant-package`, and a grant +an administrator made through the Skills page keeps their name and survives, because a deploy quietly +undoing a deliberate decision is the kind of change nobody traces back to the deploy that caused it. +A coworker naming a skill its package does not ship is refused at load rather than dropped, the same +as a channel naming an agent that is not there: a typo that silently attaches nothing looks exactly +like working. + + ### A Bot's computer is no longer on the same network as the database Compose declared no networks, so every service shared one and reached the others by service name. diff --git a/examples/fintech/agents.yaml b/examples/fintech/agents.yaml index 4d58dfcd..292305ab 100644 --- a/examples/fintech/agents.yaml +++ b/examples/fintech/agents.yaml @@ -31,6 +31,21 @@ agents: used. If you have no tool for a source, or a tool tells you it is not connected or reports an error, say that plainly. Never answer from your own memory as though it came from a source, and never claim you lack access to something a tool has just returned. + # The document skills, which is what makes the prompt above reachable rather than aspirational. + # + # THIS GRANTS NOTHING. A skill is an instruction, and what a Bot may call is its grants: the + # offer each run is the intersection of the two, so until somebody connects Drive and grants + # these tools, naming them here loads nothing and this Bot correctly says it has no source. + # The moment they do, it searches instead of guessing, with no second step to remember. + # + # Stated here rather than left to the Skills page because the package wrote both files and knows + # which Bot it meant them for. Left to a screen, every clone starts with the skills attached to + # nobody, which switches off the per-run narrowing they exist for. + skills: + - find-a-document + - check-a-claim + - whats-changed + - who-owns-this # The Bot that ships in the box (agent-bot), addressed exactly the way a customer's own Bot would # be: an AG-UI endpoint in the registry. Names in dollar-brace form are read from the environment, # so the address belongs to the deployment. Replace it with your own service and nothing changes. diff --git a/server/src/tenant-package.ts b/server/src/tenant-package.ts index 94d48cb3..549b51a2 100644 --- a/server/src/tenant-package.ts +++ b/server/src/tenant-package.ts @@ -1,9 +1,9 @@ -import { DEPLOYMENT_ROUTES } from "./computer/deployment-routes"; import { createHash } from "node:crypto"; import { readFile } from "node:fs/promises"; import { join } from "node:path"; -import { desc, eq, inArray, isNull } from "drizzle-orm"; +import { and, desc, eq, inArray, isNull } from "drizzle-orm"; import { parse } from "yaml"; +import { DEPLOYMENT_ROUTES } from "./computer/deployment-routes"; import type { Database } from "./db/client"; import { agentProfiles, @@ -11,8 +11,9 @@ import { channelAgents, channels as channelTable, deploymentPackages, - skillTools, + pluginGrants, skills as skillTable, + skillTools, } from "./db/schema"; const approvedThemeVariables = new Set([ @@ -125,6 +126,14 @@ export type TenantSkill = { tools: string[]; }; +/** + * The mark a grant this package made carries, in `plugin_grants.granted_by`. + * + * Every other value there is the id of the person who pressed the button, so this cannot collide + * with one, and it is what lets a redeploy take back only what the package gave. + */ +const PACKAGE_GRANT = "tenant-package"; + type TenantAgent = { id: string; name: string; @@ -133,6 +142,19 @@ type TenantAgent = { avatarSeed?: string; type: "built_in" | "remote_ag_ui"; configuration: Record; + /** + * The package skills this coworker is given, by slug. + * + * SAFE TO SEED, unlike an MCP grant, and for a reason worth stating rather than assuming. A skill + * is an instruction and confers nothing: what a Bot may call is its grants, and the run-time offer + * is always the intersection of the two, so a skill naming a tool its Bot does not hold loads + * nothing. Seeding an MCP grant would be the opposite, since those reach a person's own account. + * + * Without this a fork boots with the skills its package ships attached to no Bot at all, and the + * per-run narrowing that skills exist for is switched off until somebody opens the Skills page and + * pairs them by hand. The pairing is the package's to state: it wrote both files. + */ + skills: string[]; }; type TenantChannel = { @@ -361,11 +383,35 @@ export function validateTenantPackage(files: PackageFiles): TenantPackage { : { endpoint: requiredString(agent.endpoint, "agent.endpoint"), }, + skills: + agent.skills === undefined || agent.skills === null + ? [] + : stringArray(agent.skills, "agent.skills"), }, ]; }, ); const agentIds = new Set(agents.map((agent) => agent.id)); + const packageSkills = parseTenantSkills(skillsYaml.skills); + const skillSlugs = new Set(packageSkills.map((skill) => skill.slug)); + for (const agent of agents) { + for (const slug of agent.skills) { + /* + * Checked against this package's own skills and nothing else, and refused rather than dropped. + * + * The two files ship together, so a slug matching none of them is a typo, and a typo that + * silently attaches no skill is the kind nobody finds: the Bot simply never narrows and the + * deployment looks like it is working. Deliberately not checked against skills already in the + * deployment, because those include any a person wrote, and a package must not be able to + * hand its Bots somebody else's instructions by naming their slug. + */ + if (!skillSlugs.has(slug)) { + throw new Error( + `agent "${agent.id}" names skill "${slug}", which this package does not ship`, + ); + } + } + } const channels = asList(channelsYaml.channels, "channels.yaml channels").map( (value) => { const channel = asRecord(value, "channel"); @@ -427,7 +473,7 @@ export function validateTenantPackage(files: PackageFiles): TenantPackage { defaultModel: requiredString(model.default_model, "model.default_model"), }, knowledgeSources: sources, - skills: parseTenantSkills(skillsYaml.skills), + skills: packageSkills, themeCss: files.themeCss, }; } @@ -688,6 +734,29 @@ export async function synchronizeTenantPackage( * of shipping it. An unknown ref sits inert until its connector exists, because the run-time * intersection only ever offers what the Bot was granted. */ + /* + * Which coworkers asked for each skill, so the loop below can pair them as it seeds. + * + * Written wholesale under one marker: every grant this package made last time is removed first, + * so a package that stops giving a Bot a skill takes it back. Only its own, though. A grant an + * administrator made through the Skills page carries their id and survives, because retracting + * somebody's deliberate decision is not something a redeploy should do quietly. + */ + const wantedBy = new Map(); + for (const agent of tenantPackage.agents) { + for (const slug of agent.skills) { + wantedBy.set(slug, [...(wantedBy.get(slug) ?? []), agent.id]); + } + } + await transaction + .delete(pluginGrants) + .where( + and( + eq(pluginGrants.kind, "skill"), + eq(pluginGrants.grantedBy, PACKAGE_GRANT), + ), + ); + for (const skill of tenantPackage.skills) { const [seeded] = await transaction .insert(skillTable) @@ -748,6 +817,34 @@ export async function synchronizeTenantPackage( })), ); } + + /* + * Paired only with the skill this package actually owns. + * + * Inside this branch on purpose: the seed above skips a slug a person had already taken, and + * granting there would hand the package's Bots an instruction somebody else wrote under a name + * the package expected to be its own. Skipped, it keeps the existing warning and grants + * nothing, which is the safe half of the same decision. + */ + const agentIds = wantedBy.get(skill.slug) ?? []; + if (agentIds.length > 0) { + await transaction + .insert(pluginGrants) + .values( + agentIds.map((agentId) => ({ + kind: "skill", + ref: seeded.id, + agentId, + grantedBy: PACKAGE_GRANT, + })), + ) + .onConflictDoUpdate({ + target: [pluginGrants.kind, pluginGrants.ref, pluginGrants.agentId], + // An administrator who granted this by hand keeps the credit; the package only ever + // adds what was missing, and the delete above already took back what it owned. + set: { updatedAt: new Date() }, + }); + } } return deploymentPackage; diff --git a/server/tests/tenant-package.test.ts b/server/tests/tenant-package.test.ts index 766fb9a9..07258532 100644 --- a/server/tests/tenant-package.test.ts +++ b/server/tests/tenant-package.test.ts @@ -1,14 +1,14 @@ import { afterAll, afterEach, describe, expect, test } from "bun:test"; import { randomUUID } from "node:crypto"; -import { eq } from "drizzle-orm"; +import { and, eq } from "drizzle-orm"; import { createDatabase } from "../src/db/client"; -import { TEST_POOL } from "./support/database"; import { agentProfiles, agents, deploymentPackages, - skillTools, + pluginGrants, skills as skillsTable, + skillTools, users, } from "../src/db/schema"; import { @@ -20,6 +20,7 @@ import { validateTenantPackage, validateThemeCss, } from "../src/tenant-package"; +import { TEST_POOL } from "./support/database"; const database = createDatabase( process.env.DATABASE_URL ?? @@ -64,6 +65,7 @@ function packageAgent( roleDescription: "Help with everyday work.", type: "built_in", configuration: { systemPrompt: "Be helpful." }, + skills: [], ...overrides, }; } @@ -305,7 +307,18 @@ describe("tenant YAML validation", () => { systemPrompt: "You are a helpful general assistant. Give clear, concise, and accurate answers.", }, + skills: [], }); + // The pairing the shipped package makes, which is the whole reason Knowledge narrows to document + // tools rather than being offered everything its grants hold. + expect( + tenantPackage.agents.find((agent) => agent.id === "knowledge")?.skills, + ).toEqual([ + "find-a-document", + "check-a-claim", + "whats-changed", + "who-owns-this", + ]); expect(tenantPackage.channels).toContainEqual({ id: "general-assistant", name: "General Assistant", @@ -1053,3 +1066,163 @@ describe("seeding the skills a package ships", () => { expect(row?.ownerUserId).toBe(owner); }); }); + +/** + * Pairing a package's coworkers with the skills it ships. + * + * A deployment that seeds skills attached to nobody has switched off the narrowing they exist for, + * so the package states the pairing. The cases below are the ones that decide whether seeding a + * grant is safe: it never confers a capability, it never reaches somebody else's skill, and a + * redeploy can take back its own without touching an administrator's. + */ +describe("pairing a package's coworkers with its skills", () => { + const createdSkillIds: string[] = []; + + afterEach(async () => { + for (const id of createdSkillIds.splice(0)) { + await database.delete(pluginGrants).where(eq(pluginGrants.ref, id)); + await database.delete(skillTools).where(eq(skillTools.skillId, id)); + await database.delete(skillsTable).where(eq(skillsTable.id, id)); + } + }); + + function packageGiving(slugs: string[]) { + const agent = packageAgent({ skills: slugs }); + const loaded = loadedPackage(agent); + const skills = slugs.map((slug) => ({ + slug, + title: "Find a document", + summary: "Search the sources and read what comes back.", + instructions: "Search first, then read the file you found.", + tools: ["google-drive/search_files"], + })); + for (const slug of slugs) createdSkillIds.push(slug); + createdAgentIds.push(agent.id); + return { ...loaded, skills }; + } + + const grantsFor = (agentId: string) => + database + .select() + .from(pluginGrants) + .where( + and(eq(pluginGrants.kind, "skill"), eq(pluginGrants.agentId, agentId)), + ); + + test("a coworker is granted the skills its package named", async () => { + const slug = `pkg-${randomUUID().slice(0, 8)}`; + const loaded = packageGiving([slug]); + const created = await synchronizeTenantPackage(database, loaded); + createdPackageIds.push(created.id); + + const granted = await grantsFor(loaded.agents[0]?.id as string); + expect(granted.map((row) => row.ref)).toEqual([slug]); + // The mark is what lets a later deploy take back its own and nobody else's. + expect(granted[0]?.grantedBy).toBe("tenant-package"); + }); + + test("a skill the package stopped naming is taken back", async () => { + const slug = `pkg-${randomUUID().slice(0, 8)}`; + const loaded = packageGiving([slug]); + createdPackageIds.push( + (await synchronizeTenantPackage(database, loaded)).id, + ); + + const agentId = loaded.agents[0]?.id as string; + expect(await grantsFor(agentId)).toHaveLength(1); + + // Same package, same skill still shipped, but the coworker no longer asks for it. + const withoutIt = { + ...loaded, + agents: [{ ...(loaded.agents[0] as never), skills: [] }], + } as typeof loaded; + createdPackageIds.push( + (await synchronizeTenantPackage(database, withoutIt)).id, + ); + + expect(await grantsFor(agentId)).toHaveLength(0); + }); + + test("a grant an administrator made by hand survives a redeploy", async () => { + const slug = `pkg-${randomUUID().slice(0, 8)}`; + const loaded = packageGiving([slug]); + createdPackageIds.push( + (await synchronizeTenantPackage(database, loaded)).id, + ); + + const agentId = loaded.agents[0]?.id as string; + // Somebody decides this Bot should keep it, through the Skills page. + await database + .update(pluginGrants) + .set({ grantedBy: "an-administrator" }) + .where( + and(eq(pluginGrants.kind, "skill"), eq(pluginGrants.agentId, agentId)), + ); + + const withoutIt = { + ...loaded, + agents: [{ ...(loaded.agents[0] as never), skills: [] }], + } as typeof loaded; + createdPackageIds.push( + (await synchronizeTenantPackage(database, withoutIt)).id, + ); + + /* + * Kept. The package retracts what it gave, not what somebody decided: a redeploy quietly undoing + * a deliberate grant is the kind of thing nobody connects to the deploy that caused it. + */ + const granted = await grantsFor(agentId); + expect(granted.map((row) => row.ref)).toEqual([slug]); + expect(granted[0]?.grantedBy).toBe("an-administrator"); + }); + + test("a slug a person already owns is never granted to the package's Bot", async () => { + /* + * The case that decides whether this is safe at all. The seed skips a slug somebody already + * took, so granting anyway would hand this Bot an instruction a stranger wrote, under a name the + * package believed was its own. + */ + const slug = `pkg-${randomUUID().slice(0, 8)}`; + const owner = await createUser(); + await database.insert(skillsTable).values({ + id: slug, + ownerUserId: owner, + slug, + title: "Mine", + summary: "Mine.", + instructions: "Do it my way.", + origin: "yours", + }); + + const loaded = packageGiving([slug]); + createdPackageIds.push( + (await synchronizeTenantPackage(database, loaded)).id, + ); + + expect(await grantsFor(loaded.agents[0]?.id as string)).toHaveLength(0); + }); + + test("a coworker naming a skill the package does not ship is refused at load", () => { + // A typo here attaches nothing and looks exactly like working, so it is refused rather than + // dropped, the same as a channel naming an agent that is not there. + expect(() => + validateTenantPackage({ + brand: "tenant: { id: fintech, product_name: Ledgerline }", + agents: + "agents: [{ id: knowledge, name: Knowledge, title: Company Knowledge, role_description: Answer., type: built-in, system_prompt: Answer., skills: [no-such-skill] }]", + channels: "channels: []", + model: + "model: { provider: openai, credential_secret_ref: openai-key, default_model: gpt-5.6-terra }", + knowledge: "sources: []", + skills: `skills: + - slug: find-a-document + title: Find a document + summary: Search. + instructions: Search.`, + themeCss: "", + }), + ).toThrow( + 'agent "knowledge" names skill "no-such-skill", which this package does not ship', + ); + }); +});