diff --git a/CHANGELOG.md b/CHANGELOG.md index 5a64ac249..a1e23ead7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,18 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### Duplicating a Bot in the box keeps its instructions + +A coworker that runs on this deployment's own Bot has no endpoint — it has a prompt, which is the +whole of what makes it that coworker. Duplicate rebuilt every copy from the endpoint alone, found +none, and fell back to the managed Bot with the prompt dropped, so the copy carried the name, the +title, the role and the avatar and none of the instructions. Its entire instruction became the one +sentence of role description, which is the shape behind the compliance answer this repository +already has a note about. The two coworkers the default package ships are both of this kind, and one +of them is a careful do-not-fabricate instruction. A copy now keeps the prompt and stays a Bot in the +box, which also means it can still be granted the right to hand work on — written as a hosted +coworker it could never hold that grant, however the original was set up — and copying one no longer +needs a managed Bot to fall back to. ### Hiding a coworker no longer hides the grants pointing at it Hiding a coworker is a preference about your own roster — one row per person — and the grants saying diff --git a/server/src/agents/profile-store.ts b/server/src/agents/profile-store.ts index 315d6ee73..f96897043 100644 --- a/server/src/agents/profile-store.ts +++ b/server/src/agents/profile-store.ts @@ -183,6 +183,74 @@ function endpointOf(configuration: unknown): string | null { return typeof endpoint === "string" ? endpoint : null; } +/** + * The instruction a Bot in the box runs on, read back out of its stored configuration. + * + * The mirror of {@link endpointOf}, and needed for the same reason: a copy has to be made of what + * the original actually was, and for a `built_in` coworker the prompt IS the coworker. Trimmed and + * required to be non-empty, matching `registeredAgentFromRow`, which will not build a Bot from a + * blank one either. + */ +function systemPromptOf(configuration: unknown): string | null { + if (!configuration || typeof configuration !== "object") return null; + const prompt = (configuration as { systemPrompt?: unknown }).systemPrompt; + if (typeof prompt !== "string") return null; + const trimmed = prompt.trim(); + return trimmed.length > 0 ? trimmed : null; +} + +/** What a coworker is, and what it runs on: the two `agents` columns a copy has to reproduce. */ +export type AgentRun = { + type: "built_in" | "remote_ag_ui"; + configuration: Record; +}; + +/** + * What a duplicate runs on, decided from what the original ran on. + * + * WHY THIS IS NOT JUST THE ENDPOINT. Duplicate used to rebuild the copy from `source.endpoint` alone + * and write `type: "remote_ag_ui"` flat. #328 fixed the half of that a coworker with its own endpoint + * saw. The other half is a coworker that has no endpoint because it is not supposed to have one: a + * `built_in` Bot's configuration is `{ systemPrompt }`, so the endpoint read came back null, the copy + * fell through to the managed Bot, and the prompt was dropped on the floor. + * + * That copy is the failure this repository already has a paragraph about. It looks identical on every + * screen and its whole instruction becomes `standingRoleMessage` — see the note above that function + * in `copilot.ts`, which names the compliance Bot that answered a filing question with invented + * thresholds because one sentence of role description was all that reached it. The default tenant + * package ships two `built_in` coworkers, and one of them, `Knowledge`, is a careful + * do-not-fabricate instruction. Copy it and you get a coworker with the name, the title, the avatar, + * and none of that. + * + * The type is carried too, not only the configuration. A copy written as `remote_ag_ui` also cannot + * be granted handoff for the rest of its life: `agentRunsHere` and `botsReachableFrom` both key on + * `agents.type == "built_in"`, so the original may hand work on and its copy silently may not. + * + * `null` means there is nothing to run this copy on, which the caller turns into + * {@link ManagedAgentUnavailableError}. That can now only happen for a source that had neither an + * endpoint nor a prompt on a deployment with no managed Bot — never for a `built_in` source, which + * brings its own instruction and needs no managed Bot to fall back to. + * + * `auth` is deliberately not carried: it is a reference into the vault, and two coworkers sharing one + * credential would mean rotating either one's key silently changed the other's. + */ +export function runForDuplicate( + source: { type: "built_in" | "remote_ag_ui"; configuration: unknown }, + managed: Record | undefined, +): AgentRun | null { + const systemPrompt = systemPromptOf(source.configuration); + if (source.type === "built_in" && systemPrompt) { + return { type: "built_in", configuration: { systemPrompt } }; + } + + const endpoint = endpointOf(source.configuration); + if (endpoint) { + return { type: "remote_ag_ui", configuration: { endpoint } }; + } + + return managed ? { type: "remote_ag_ui", configuration: managed } : null; +} + async function findAccessibleProfile( executor: DatabaseExecutor, actor: AgentActor, @@ -437,20 +505,35 @@ export function createAgentProfileStore( const source = await findAccessibleProfile(transaction, actor, id); if (!source) throw new AgentNotFoundError(id); - // The endpoint alone: `auth` is a vault reference, and copying it shares one credential. - const configuration = source.endpoint - ? { endpoint: source.endpoint } - : managedConfiguration; - // After the source read, so a source with its own endpoint needs no managed Bot to fall back to. - if (!configuration) { + /* + * The stored row, because a profile does not carry what a copy has to reproduce. + * + * `AgentProfile` projects `endpoint` out of the configuration and nothing else, which is all + * an edit form needs and half of what this needs: a `built_in` coworker has no endpoint and + * a prompt instead. Read here rather than widened into the profile, so the DTO every surface + * gets does not start carrying a Bot's instructions. Inside the transaction, and after the + * access check, so this cannot read a row the caller may not see. + */ + const [stored] = await transaction + .select({ type: agents.type, configuration: agents.configuration }) + .from(agents) + .where(eq(agents.id, id)) + .limit(1); + if (!stored) throw new AgentNotFoundError(id); + + // `auth` is a vault reference and is deliberately not carried: see `runForDuplicate`. + const run = runForDuplicate(stored, managedConfiguration); + // After the source read, so a source that brings its own endpoint or its own prompt needs no + // managed Bot to fall back to. + if (!run) { throw new ManagedAgentUnavailableError(); } const duplicateId = newAgentId(); await transaction.insert(agents).values({ id: duplicateId, name: source.name, - type: "remote_ag_ui", - configuration, + type: run.type, + configuration: run.configuration, }); await transaction.insert(agentProfiles).values({ agentId: duplicateId, diff --git a/server/tests/duplicate-run.test.ts b/server/tests/duplicate-run.test.ts new file mode 100644 index 000000000..3d1d0c90b --- /dev/null +++ b/server/tests/duplicate-run.test.ts @@ -0,0 +1,125 @@ +import { describe, expect, test } from "bun:test"; +import { runForDuplicate } from "../src/agents/profile-store"; + +/** + * What a duplicated coworker runs on. + * + * A pure decision, tested without a database, because the failure it exists to stop is silent: the + * copy is created, appears on the roster, answers when asked, and is a different coworker from the + * one that was copied. + */ + +const managed = { endpoint: "http://managed.invalid/ag-ui" }; + +describe("what a copy runs on", () => { + /* + * The bug. A Bot in the box keeps its whole instruction in `configuration.systemPrompt` and has no + * endpoint at all, so an endpoint-only read came back empty, the copy fell through to the managed + * Bot, and the prompt went nowhere. The default tenant package ships two of these. + */ + test("keeps a Bot-in-the-box's prompt, and stays a Bot in the box", () => { + expect( + runForDuplicate( + { + type: "built_in", + configuration: { systemPrompt: "Never answer from memory." }, + }, + managed, + ), + ).toEqual({ + type: "built_in", + configuration: { systemPrompt: "Never answer from memory." }, + }); + }); + + test("needs no managed Bot to copy one that brought its own prompt", () => { + // The mirror of the endpoint case: a source that carries what it runs on does not fall back. + expect( + runForDuplicate( + { type: "built_in", configuration: { systemPrompt: "Be brief." } }, + undefined, + ), + ).toEqual({ + type: "built_in", + configuration: { systemPrompt: "Be brief." }, + }); + }); + + test("keeps the endpoint a hosted coworker was copied from", () => { + expect( + runForDuplicate( + { + type: "remote_ag_ui", + configuration: { endpoint: "https://theirs.invalid/ag-ui" }, + }, + managed, + ), + ).toEqual({ + type: "remote_ag_ui", + configuration: { endpoint: "https://theirs.invalid/ag-ui" }, + }); + }); + + test("never carries the key: two coworkers must not share one credential", () => { + expect( + runForDuplicate( + { + type: "remote_ag_ui", + configuration: { + endpoint: "https://theirs.invalid/ag-ui", + auth: { header: "Authorization", credentialId: "cred_1" }, + }, + }, + managed, + ), + ).toEqual({ + type: "remote_ag_ui", + configuration: { endpoint: "https://theirs.invalid/ag-ui" }, + }); + }); + + test("falls back to the managed Bot only when the source runs on nothing of its own", () => { + expect( + runForDuplicate({ type: "built_in", configuration: {} }, managed), + ).toEqual({ type: "remote_ag_ui", configuration: managed }); + }); + + test("has nowhere to put a copy of a coworker with nothing, and no managed Bot", () => { + // Null is what the caller turns into "give the coworker its own AG-UI endpoint". It must not be + // reachable for a source that had a prompt, which is what the second case above pins. + expect( + runForDuplicate({ type: "remote_ag_ui", configuration: {} }, undefined), + ).toBeNull(); + }); + + test("treats a blank prompt as no prompt, the way the runtime does", () => { + // `registeredAgentFromRow` refuses to build a Bot from a whitespace prompt, so copying one as + // `built_in` would produce a coworker that cannot be built at all. + expect( + runForDuplicate( + { type: "built_in", configuration: { systemPrompt: " " } }, + managed, + ), + ).toEqual({ type: "remote_ag_ui", configuration: managed }); + }); + + test("does not read a prompt off a coworker that runs somewhere else", () => { + // The type decides, not the presence of a key. A remote row carrying a stray `systemPrompt` is + // still a remote Bot, and copying it as built-in would move it into this process. + expect( + runForDuplicate( + { + type: "remote_ag_ui", + configuration: { + endpoint: "https://theirs.invalid/ag-ui", + systemPrompt: "ignored", + }, + }, + managed, + ), + ).toEqual({ + type: "remote_ag_ui", + configuration: { endpoint: "https://theirs.invalid/ag-ui" }, + }); + }); +});