From 1697ac7871eaf2ec94338234375d8eb096f73b1c Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Tue, 21 Apr 2026 19:52:50 +0200 Subject: [PATCH 01/12] feat(appkit): fromPlugin() DX, runAgent plugins arg, shared toolkit-resolver MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DX centerpiece. Introduces the symbol-marker pattern that collapses plugin tool references in code-defined agents from a three-touch dance to a single line, and extracts the shared resolver that the agents plugin, auto-inherit, and standalone runAgent all now go through. `packages/appkit/src/plugins/agents/from-plugin.ts`. Returns a spread- friendly `{ [Symbol()]: FromPluginMarker }` record. The symbol key is freshly generated per call, so multiple spreads of the same plugin coexist safely. The marker's brand is a globally-interned `Symbol.for("@databricks/appkit.fromPluginMarker")` — stable across module boundaries. `packages/appkit/src/plugins/agents/toolkit-resolver.ts`. Single source of truth for "turn a ToolProvider into a keyed record of `ToolkitEntry` markers". Prefers `provider.toolkit(opts)` when available (core plugins implement it), falls back to walking `getAgentTools()` and synthesizing namespaced keys (`${pluginName}.${localName}`) for third-party providers, honoring `only` / `except` / `rename` / `prefix` the same way. Used by three call sites, previously all copy-pasted: 1. `AgentsPlugin.buildToolIndex` — fromPlugin marker resolution pass 2. `AgentsPlugin.applyAutoInherit` — markdown auto-inherit path 3. `runAgent` — standalone-mode plugin tool dispatch Before the existing string-key iteration, `buildToolIndex` now walks `Object.getOwnPropertySymbols(def.tools)`. For each `FromPluginMarker`, it looks up the plugin by name in `PluginContext.getToolProviders()`, calls `resolveToolkitFromProvider`, and merges the resulting entries into the per-agent index. Missing plugins throw at setup time with a clear `Available: ...` listing — wiring errors surface on boot, not mid-request. `hasExplicitTools` now counts symbol keys too, so a `tools: { ...fromPlugin(x) }` record correctly disables auto-inherit on code-defined agents. - `AgentTools` type: `{ [key: string]: AgentTool } & { [key: symbol]: FromPluginMarker }`. Preserves string-key autocomplete while accepting marker spreads under strict TS. - `AgentDefinition.tools` switched to `AgentTools`. `packages/appkit/src/core/run-agent.ts`. When an agent def contains `fromPlugin` markers, the caller passes plugins via `RunAgentInput.plugins`. A local provider cache constructs each plugin and dispatches tool calls via `provider.executeAgentTool()`. Runs as service principal (no OBO — there's no HTTP request). If a def contains markers but `plugins` is absent, throws with guidance. `fromPlugin`, `FromPluginMarker`, `isFromPluginMarker`, `AgentTools` added to the main barrel. - 14 new tests: marker shape, symbol uniqueness, type guard, factory-without-pluginName error, fromPlugin marker resolution in AgentsPlugin, fallback to getAgentTools for providers without .toolkit(), symbol-only tools disables auto-inherit, runAgent standalone marker resolution via `plugins` arg, guidance error when missing. - Full appkit vitest suite: 1311 tests passing. - Typecheck clean. Signed-off-by: MarioCadenas --- packages/appkit/src/beta.ts | 4 + packages/appkit/src/core/agent/from-plugin.ts | 97 +++++++++ packages/appkit/src/core/agent/run-agent.ts | 145 +++++++++++- .../src/core/agent/tests/run-agent.test.ts | 98 ++++++++- .../appkit/src/core/agent/toolkit-resolver.ts | 62 ++++++ packages/appkit/src/core/agent/types.ts | 13 +- packages/appkit/src/plugins/agents/agents.ts | 99 ++++++--- packages/appkit/src/plugins/agents/index.ts | 8 + .../agents/tests/agents-plugin.test.ts | 206 ++++++++++++++++++ .../plugins/agents/tests/from-plugin.test.ts | 80 +++++++ 10 files changed, 773 insertions(+), 39 deletions(-) create mode 100644 packages/appkit/src/core/agent/from-plugin.ts create mode 100644 packages/appkit/src/core/agent/toolkit-resolver.ts create mode 100644 packages/appkit/src/plugins/agents/tests/from-plugin.test.ts diff --git a/packages/appkit/src/beta.ts b/packages/appkit/src/beta.ts index 562cfd43a..9295a43ed 100644 --- a/packages/appkit/src/beta.ts +++ b/packages/appkit/src/beta.ts @@ -52,8 +52,10 @@ export type { AgentDefinition, AgentsPluginConfig, AgentTool, + AgentTools, AutoInheritToolsConfig, BaseSystemPromptOption, + FromPluginMarker, PromptContext, RegisteredAgent, ResolvedToolEntry, @@ -62,6 +64,8 @@ export type { } from "./plugins/agents"; export { agentIdFromMarkdownPath, + fromPlugin, + isFromPluginMarker, isToolkitEntry, loadAgentFromFile, loadAgentsFromDir, diff --git a/packages/appkit/src/core/agent/from-plugin.ts b/packages/appkit/src/core/agent/from-plugin.ts new file mode 100644 index 000000000..b11285941 --- /dev/null +++ b/packages/appkit/src/core/agent/from-plugin.ts @@ -0,0 +1,97 @@ +import type { NamedPluginFactory } from "../../plugin/to-plugin"; +import type { ToolkitOptions } from "./types"; + +/** + * Symbol brand for the `fromPlugin` marker. Using a globally-interned symbol + * (`Symbol.for`) keeps the brand stable across module boundaries / bundle + * duplicates so `isFromPluginMarker` stays reliable. + */ +export const FROM_PLUGIN_MARKER = Symbol.for( + "@databricks/appkit.fromPluginMarker", +); + +/** + * A lazy reference to a plugin's tools, produced by {@link fromPlugin} and + * resolved to concrete `ToolkitEntry`s at `AgentsPlugin.setup()` time. + * + * The marker is spread under a unique symbol key so multiple calls to + * `fromPlugin` (even for the same plugin) coexist in an `AgentDefinition.tools` + * record without colliding. + */ +export interface FromPluginMarker { + readonly [FROM_PLUGIN_MARKER]: true; + readonly pluginName: string; + readonly opts: ToolkitOptions | undefined; +} + +/** + * Record shape returned by {@link fromPlugin} — a single symbol-keyed entry + * suitable for spreading into `AgentDefinition.tools`. + */ +export type FromPluginSpread = { readonly [key: symbol]: FromPluginMarker }; + +/** + * Reference a plugin's tools inside an `AgentDefinition.tools` record without + * naming the plugin instance. The returned spread-friendly object carries a + * symbol-keyed marker that the agents plugin resolves against registered + * `ToolProvider`s at setup time. + * + * The factory argument must come from `toPlugin` (or any function that + * carries a `pluginName` field). `fromPlugin` reads `factory.pluginName` + * synchronously — it does not construct an instance. + * + * If the referenced plugin is also registered in `createApp({ plugins })`, the + * same runtime instance is used for dispatch. If the plugin is missing, + * `AgentsPlugin.setup()` throws with a clear `Available: …` listing. + * + * @example + * ```ts + * import { analytics, createAgent, files, fromPlugin, tool } from "@databricks/appkit"; + * + * const support = createAgent({ + * instructions: "You help customers.", + * tools: { + * ...fromPlugin(analytics), + * ...fromPlugin(files, { only: ["uploads.read"] }), + * get_weather: tool({ ... }), + * }, + * }); + * ``` + * + * @param factory A plugin factory produced by `toPlugin`. Must expose a + * `pluginName` field. + * @param opts Optional toolkit scoping — `prefix`, `only`, `except`, `rename`. + * Same shape as the `.toolkit()` method. + */ +export function fromPlugin( + factory: F, + opts?: ToolkitOptions, +): FromPluginSpread { + if ( + !factory || + typeof factory.pluginName !== "string" || + !factory.pluginName + ) { + throw new Error( + "fromPlugin(): factory is missing pluginName. Pass a factory created by toPlugin().", + ); + } + const pluginName = factory.pluginName; + const marker: FromPluginMarker = { + [FROM_PLUGIN_MARKER]: true, + pluginName, + opts, + }; + return { [Symbol(`fromPlugin:${pluginName}`)]: marker }; +} + +/** + * Type guard for {@link FromPluginMarker}. + */ +export function isFromPluginMarker(value: unknown): value is FromPluginMarker { + return ( + typeof value === "object" && + value !== null && + (value as Record)[FROM_PLUGIN_MARKER] === true + ); +} diff --git a/packages/appkit/src/core/agent/run-agent.ts b/packages/appkit/src/core/agent/run-agent.ts index 9b5f57a2d..1eb510742 100644 --- a/packages/appkit/src/core/agent/run-agent.ts +++ b/packages/appkit/src/core/agent/run-agent.ts @@ -4,8 +4,13 @@ import type { AgentEvent, AgentToolDefinition, Message, + PluginConstructor, + PluginData, + ToolProvider, } from "shared"; import { consumeAdapterStream } from "./consume-adapter-stream"; +import { isFromPluginMarker } from "./from-plugin"; +import { resolveToolkitFromProvider } from "./toolkit-resolver"; import { type FunctionTool, functionToolToDefinition, @@ -20,6 +25,14 @@ export interface RunAgentInput { messages: string | Message[]; /** Abort signal for cancellation. */ signal?: AbortSignal; + /** + * Optional plugin list used to resolve `fromPlugin` markers in `def.tools`. + * Required when the def contains any `...fromPlugin(factory)` spreads; + * ignored otherwise. `runAgent` constructs a fresh instance per plugin + * and dispatches tool calls against it as the service principal (no + * OBO — there is no HTTP request in standalone mode). + */ + plugins?: PluginData[]; } export interface RunAgentResult { @@ -36,11 +49,12 @@ export interface RunAgentResult { * Limitations vs. running through the agents() plugin: * - No OBO: there is no HTTP request, so plugin tools run as the service * principal (when they work at all). - * - Plugin tools (`ToolkitEntry`) are not supported — they require a live - * `PluginContext` that only exists when registered in a `createApp` - * instance. This function throws a clear error if encountered. + * - Hosted tools (MCP) are not supported — they require a live MCP client + * that only exists inside the agents plugin. * - Sub-agents (`agents: { ... }` on the def) are executed as nested * `runAgent` calls with no shared thread state. + * - Plugin tools (`fromPlugin` markers or `ToolkitEntry` spreads) require + * passing `plugins: [...]` via `RunAgentInput`. */ export async function runAgent( def: AgentDefinition, @@ -48,7 +62,7 @@ export async function runAgent( ): Promise { const adapter = await resolveAdapter(def); const messages = normalizeMessages(input.messages, def.instructions); - const toolIndex = buildStandaloneToolIndex(def); + const toolIndex = buildStandaloneToolIndex(def, input.plugins ?? []); const tools = Array.from(toolIndex.values()).map((e) => e.def); const signal = input.signal; @@ -59,6 +73,13 @@ export async function runAgent( if (entry.kind === "function") { return entry.tool.execute(args as Record); } + if (entry.kind === "toolkit") { + return entry.provider.executeAgentTool( + entry.localName, + args as Record, + signal, + ); + } if (entry.kind === "subagent") { const subInput: RunAgentInput = { messages: @@ -68,13 +89,14 @@ export async function runAgent( ? (args as { input: string }).input : JSON.stringify(args), signal, + plugins: input.plugins, }; const res = await runAgent(entry.agentDef, subInput); return res.text; } throw new Error( `runAgent: tool "${name}" is a ${entry.kind} tool. ` + - "Plugin toolkits and MCP tools are only usable via createApp({ plugins: [..., agents(...)] }).", + "Hosted/MCP tools are only usable via createApp({ plugins: [..., agents(...)] }).", ); }; @@ -154,20 +176,61 @@ type StandaloneEntry = | { kind: "toolkit"; def: AgentToolDefinition; - entry: ToolkitEntry; + provider: ToolProvider; + pluginName: string; + localName: string; } | { kind: "hosted"; def: AgentToolDefinition; }; +/** + * Resolves `def.tools` (string-keyed entries + symbol-keyed `fromPlugin` + * markers) and `def.agents` (sub-agents) into a flat dispatch index. + * Symbol-keyed markers are resolved against `plugins`; missing references + * throw with an `Available: …` listing. + */ function buildStandaloneToolIndex( def: AgentDefinition, + plugins: PluginData[], ): Map { const index = new Map(); + const tools = def.tools; - for (const [key, tool] of Object.entries(def.tools ?? {})) { - index.set(key, classifyTool(key, tool)); + const symbolKeys = tools ? Object.getOwnPropertySymbols(tools) : []; + if (symbolKeys.length > 0) { + const providerCache = new Map(); + for (const sym of symbolKeys) { + const marker = (tools as Record)[sym]; + if (!isFromPluginMarker(marker)) continue; + + const provider = resolveStandaloneProvider( + marker.pluginName, + plugins, + providerCache, + ); + const entries = resolveToolkitFromProvider( + marker.pluginName, + provider, + marker.opts, + ); + for (const [key, entry] of Object.entries(entries)) { + index.set(key, { + kind: "toolkit", + provider, + pluginName: entry.pluginName, + localName: entry.localName, + def: { ...entry.def, name: key }, + }); + } + } + } + + if (tools) { + for (const [key, tool] of Object.entries(tools)) { + index.set(key, classifyTool(key, tool)); + } } for (const [childKey, child] of Object.entries(def.agents ?? {})) { @@ -199,7 +262,7 @@ function buildStandaloneToolIndex( function classifyTool(key: string, tool: AgentTool): StandaloneEntry { if (isToolkitEntry(tool)) { - return { kind: "toolkit", def: { ...tool.def, name: key }, entry: tool }; + return toolkitEntryToStandalone(key, tool); } if (isFunctionTool(tool)) { return { @@ -220,3 +283,67 @@ function classifyTool(key: string, tool: AgentTool): StandaloneEntry { } throw new Error(`runAgent: unrecognized tool shape at key "${key}"`); } + +/** + * Pre-`fromPlugin` code could reach a `ToolkitEntry` by calling + * `.toolkit()` at module scope (which requires an instance). Those entries + * still flow through `def.tools` but without a provider we can dispatch + * against — runAgent cannot execute them and errors clearly. + */ +function toolkitEntryToStandalone( + key: string, + entry: ToolkitEntry, +): StandaloneEntry { + const def: AgentToolDefinition = { ...entry.def, name: key }; + return { + kind: "hosted", + def: { + ...def, + description: + `${def.description ?? ""} ` + + `[runAgent: this ToolkitEntry refers to plugin '${entry.pluginName}' but ` + + "runAgent cannot dispatch it without the plugin instance. Pass the " + + "plugin via plugins: [...] and use fromPlugin(factory) instead of " + + ".toolkit() spreads.]".trim(), + }, + }; +} + +function resolveStandaloneProvider( + pluginName: string, + plugins: PluginData[], + cache: Map, +): ToolProvider { + const cached = cache.get(pluginName); + if (cached) return cached; + + const match = plugins.find((p) => p.name === pluginName); + if (!match) { + const available = plugins.map((p) => p.name).join(", ") || "(none)"; + throw new Error( + `runAgent: agent references plugin '${pluginName}' via fromPlugin(), but ` + + "that plugin is missing from RunAgentInput.plugins. " + + `Available: ${available}.`, + ); + } + + const instance = new match.plugin({ + ...(match.config ?? {}), + name: pluginName, + }); + const provider = instance as unknown as ToolProvider; + if ( + typeof (provider as { getAgentTools?: unknown }).getAgentTools !== + "function" || + typeof (provider as { executeAgentTool?: unknown }).executeAgentTool !== + "function" + ) { + throw new Error( + `runAgent: plugin '${pluginName}' is not a ToolProvider ` + + "(missing getAgentTools/executeAgentTool). Only ToolProvider plugins " + + "are supported via fromPlugin() in runAgent.", + ); + } + cache.set(pluginName, provider); + return provider; +} diff --git a/packages/appkit/src/core/agent/tests/run-agent.test.ts b/packages/appkit/src/core/agent/tests/run-agent.test.ts index f94e8b7bd..5324dde21 100644 --- a/packages/appkit/src/core/agent/tests/run-agent.test.ts +++ b/packages/appkit/src/core/agent/tests/run-agent.test.ts @@ -3,13 +3,18 @@ import type { AgentEvent, AgentInput, AgentRunContext, + AgentToolDefinition, + PluginConstructor, + PluginData, + ToolProvider, } from "shared"; import { describe, expect, test, vi } from "vitest"; import { z } from "zod"; -import { tool } from "../../../core/agent/tools/tool"; import type { ToolkitEntry } from "../../../core/agent/types"; import { createAgent } from "../create-agent"; +import { fromPlugin } from "../from-plugin"; import { runAgent } from "../run-agent"; +import { tool } from "../tools/tool"; function scriptedAdapter(events: AgentEvent[]): AgentAdapter { return { @@ -84,6 +89,97 @@ describe("runAgent", () => { expect(weatherFn).toHaveBeenCalledWith({ city: "NYC" }); }); + test("resolves fromPlugin markers against RunAgentInput.plugins", async () => { + const pingExec = vi.fn(async () => "pong"); + class FakePlugin implements ToolProvider { + static manifest = { name: "ping" }; + static DEFAULT_CONFIG = {}; + name = "ping"; + constructor(public config: unknown) {} + async setup() {} + injectRoutes() {} + getEndpoints() { + return {}; + } + getAgentTools(): AgentToolDefinition[] { + return [ + { + name: "ping", + description: "ping", + parameters: { type: "object", properties: {} }, + }, + ]; + } + executeAgentTool = pingExec; + } + + const factory = () => ({ + plugin: FakePlugin as unknown as PluginConstructor, + config: {}, + name: "ping" as const, + }); + Object.defineProperty(factory, "pluginName", { + value: "ping", + enumerable: true, + }); + + let capturedCtx: AgentRunContext | null = null; + const adapter: AgentAdapter = { + async *run(_input, context) { + capturedCtx = context; + yield { type: "message_delta", content: "" }; + }, + }; + + const def = createAgent({ + instructions: "x", + model: adapter, + tools: { + ...fromPlugin(factory as unknown as { readonly pluginName: string }), + }, + }); + + const pluginData = factory() as PluginData< + PluginConstructor, + unknown, + string + >; + + await runAgent(def, { messages: "hi", plugins: [pluginData] }); + expect(capturedCtx).not.toBeNull(); + // biome-ignore lint/style/noNonNullAssertion: asserted above + const result = await capturedCtx!.executeTool("ping.ping", {}); + expect(result).toBe("pong"); + expect(pingExec).toHaveBeenCalled(); + }); + + test("throws with guidance when fromPlugin marker has no matching plugin", async () => { + const factory = () => ({ name: "absent" as const }); + Object.defineProperty(factory, "pluginName", { + value: "absent", + enumerable: true, + }); + + const adapter: AgentAdapter = { + async *run(_input, _context) { + yield { type: "message_delta", content: "" }; + }, + }; + + const def = createAgent({ + instructions: "x", + model: adapter, + tools: { + ...fromPlugin(factory as unknown as { readonly pluginName: string }), + }, + }); + + await expect(runAgent(def, { messages: "hi" })).rejects.toThrow(/absent/); + await expect(runAgent(def, { messages: "hi" })).rejects.toThrow( + /Available:/, + ); + }); + test("throws a clear error when a ToolkitEntry is invoked", async () => { const toolkitEntry: ToolkitEntry = { __toolkitRef: true, diff --git a/packages/appkit/src/core/agent/toolkit-resolver.ts b/packages/appkit/src/core/agent/toolkit-resolver.ts new file mode 100644 index 000000000..8ec8cf1f7 --- /dev/null +++ b/packages/appkit/src/core/agent/toolkit-resolver.ts @@ -0,0 +1,62 @@ +import type { ToolProvider } from "shared"; +import type { ToolkitEntry, ToolkitOptions } from "./types"; + +/** + * Internal interface: a `ToolProvider` that optionally exposes a typed + * `.toolkit(opts)` method. Core plugins (analytics, files, genie, lakebase) + * implement this; third-party `ToolProvider`s may not. + */ +type MaybeToolkitProvider = ToolProvider & { + toolkit?: (opts?: ToolkitOptions) => Record; +}; + +/** + * Resolve a plugin's tools into a keyed record of {@link ToolkitEntry} markers + * ready to be merged into an agent's tool index. + * + * Preferred path: call the plugin's own `.toolkit(opts)` method, which + * typically delegates to `buildToolkitEntries` with full `ToolkitOptions` + * support (prefix, only, except, rename). + * + * Fallback path: when the plugin doesn't expose `.toolkit()` (e.g. a + * third-party `ToolProvider` built with plain `toPlugin`), walk + * `getAgentTools()` and synthesize namespaced keys (`${pluginName}.${name}`) + * while still honoring `only` / `except` / `rename` / `prefix`. + * + * This helper is the single source of truth for "turn a provider into a + * toolkit entry record" and is used by `AgentsPlugin.buildToolIndex` + * (both the `fromPlugin` resolution pass and auto-inherit) and by the + * standalone `runAgent` executor. + */ +export function resolveToolkitFromProvider( + pluginName: string, + provider: ToolProvider, + opts?: ToolkitOptions, +): Record { + const withToolkit = provider as MaybeToolkitProvider; + if (typeof withToolkit.toolkit === "function") { + return withToolkit.toolkit(opts); + } + + const only = opts?.only ? new Set(opts.only) : null; + const except = opts?.except ? new Set(opts.except) : null; + const rename = opts?.rename ?? {}; + const prefix = opts?.prefix ?? `${pluginName}.`; + + const out: Record = {}; + for (const tool of provider.getAgentTools()) { + if (only && !only.has(tool.name)) continue; + if (except?.has(tool.name)) continue; + + const keyAfterPrefix = `${prefix}${tool.name}`; + const key = rename[tool.name] ?? keyAfterPrefix; + + out[key] = { + __toolkitRef: true, + pluginName, + localName: tool.name, + def: { ...tool, name: key }, + }; + } + return out; +} diff --git a/packages/appkit/src/core/agent/types.ts b/packages/appkit/src/core/agent/types.ts index 50e527523..baa15967f 100644 --- a/packages/appkit/src/core/agent/types.ts +++ b/packages/appkit/src/core/agent/types.ts @@ -6,6 +6,7 @@ import type { ToolAnnotations, } from "shared"; import type { McpHostPolicyConfig } from "../../connectors/mcp"; +import type { FromPluginMarker } from "./from-plugin"; import type { FunctionTool } from "./tools/function-tool"; import type { HostedTool } from "./tools/hosted-tools"; @@ -62,6 +63,16 @@ export type BaseSystemPromptOption = | string | ((ctx: PromptContext) => string); +/** + * Per-agent tool record. String keys map to inline tools, toolkit entries, + * hosted tools, etc. Symbol keys hold `FromPluginMarker` references produced + * by `fromPlugin(factory)` spreads — these are resolved at + * `AgentsPlugin.setup()` time against registered `ToolProvider` plugins. + */ +export type AgentTools = { [key: string]: AgentTool } & { + [key: symbol]: FromPluginMarker; +}; + export interface AgentDefinition { /** Filled in from the enclosing key when used in `agents: { foo: def }`. */ name?: string; @@ -74,7 +85,7 @@ export interface AgentDefinition { */ model?: AgentAdapter | Promise | string; /** Per-agent tool record. Key is the LLM-visible tool-call name. */ - tools?: Record; + tools?: AgentTools; /** Sub-agents, exposed as `agent-` tools on this agent. */ agents?: Record; /** Override the plugin's baseSystemPrompt for this agent only. */ diff --git a/packages/appkit/src/plugins/agents/agents.ts b/packages/appkit/src/plugins/agents/agents.ts index b0a64d3c2..78c2d868b 100644 --- a/packages/appkit/src/plugins/agents/agents.ts +++ b/packages/appkit/src/plugins/agents/agents.ts @@ -16,13 +16,16 @@ import type { ToolProvider, } from "shared"; import { AppKitMcpClient, buildMcpHostPolicy } from "../../connectors/mcp"; +import { getWorkspaceClient } from "../../context"; import { consumeAdapterStream } from "../../core/agent/consume-adapter-stream"; +import { isFromPluginMarker } from "../../core/agent/from-plugin"; import { loadAgentsFromDir } from "../../core/agent/load-agents"; import { normalizeToolResult } from "../../core/agent/normalize-result"; import { buildBaseSystemPrompt, composeSystemPrompt, } from "../../core/agent/system-prompt"; +import { resolveToolkitFromProvider } from "../../core/agent/toolkit-resolver"; import { functionToolToDefinition, isFunctionTool, @@ -408,7 +411,11 @@ export class AgentsPlugin extends Plugin implements ToolProvider { src: AgentSource, ): Promise> { const index = new Map(); - const hasExplicitTools = def.tools && Object.keys(def.tools).length > 0; + const toolsRecord = def.tools ?? {}; + const hasExplicitTools = + def.tools !== undefined && + (Object.keys(toolsRecord).length > 0 || + Object.getOwnPropertySymbols(toolsRecord).length > 0); const hasExplicitSubAgents = def.agents && Object.keys(def.agents).length > 0; @@ -447,10 +454,14 @@ export class AgentsPlugin extends Plugin implements ToolProvider { }); } - // 2. Explicit tools (toolkit entries, function tools, hosted tools) + // 2. fromPlugin markers — resolve against registered ToolProviders first so + // explicit string-keyed tools can still overwrite on the same key. + this.resolveFromPluginMarkers(agentName, toolsRecord, index); + + // 3. Explicit tools (toolkit entries, function tools, hosted tools) const hostedToCollect: import("../../core/agent/tools/hosted-tools").HostedTool[] = []; - for (const [key, tool] of Object.entries(def.tools ?? {})) { + for (const [key, tool] of Object.entries(toolsRecord)) { if (isToolkitEntry(tool)) { index.set(key, { source: "toolkit", @@ -502,32 +513,19 @@ export class AgentsPlugin extends Plugin implements ToolProvider { provider, } of this.context.getToolProviders()) { if (pluginName === this.name) continue; - const withToolkit = provider as ToolProvider & { - toolkit?: (opts?: unknown) => Record; - }; - if (typeof withToolkit.toolkit === "function") { - const entries = withToolkit.toolkit() as Record; - for (const [key, maybeEntry] of Object.entries(entries)) { - if (!isToolkitEntry(maybeEntry)) continue; - if (maybeEntry.autoInheritable !== true) { - recordSkip(maybeEntry.pluginName, maybeEntry.localName); - continue; - } - index.set(key, { - source: "toolkit", - pluginName: maybeEntry.pluginName, - localName: maybeEntry.localName, - def: { ...maybeEntry.def, name: key }, - }); - inherited.push(key); + const entries = resolveToolkitFromProvider(pluginName, provider); + for (const [key, entry] of Object.entries(entries)) { + if (entry.autoInheritable !== true) { + recordSkip(entry.pluginName, entry.localName); + continue; } - continue; - } - // Fallback: providers without a toolkit() still expose getAgentTools(). - // These cannot be selectively opted in per tool, so we conservatively - // skip them during auto-inherit and require explicit `tools:` wiring. - for (const tool of provider.getAgentTools()) { - recordSkip(pluginName, tool.name); + index.set(key, { + source: "toolkit", + pluginName: entry.pluginName, + localName: entry.localName, + def: { ...entry.def, name: key }, + }); + inherited.push(key); } } @@ -555,6 +553,51 @@ export class AgentsPlugin extends Plugin implements ToolProvider { } } + /** + * Walks the symbol-keyed `fromPlugin` markers in an agent's `tools` record + * and resolves each one against a registered `ToolProvider`. Throws with a + * helpful `Available: …` listing if a referenced plugin isn't registered. + */ + private resolveFromPluginMarkers( + agentName: string, + toolsRecord: Record, + index: Map, + ): void { + const symbolKeys = Object.getOwnPropertySymbols(toolsRecord); + if (symbolKeys.length === 0) return; + + const providers = this.context?.getToolProviders() ?? []; + + for (const sym of symbolKeys) { + const marker = (toolsRecord as Record)[sym]; + if (!isFromPluginMarker(marker)) continue; + + const providerEntry = providers.find((p) => p.name === marker.pluginName); + if (!providerEntry) { + const available = providers.map((p) => p.name).join(", ") || "(none)"; + throw new Error( + `Agent '${agentName}' references plugin '${marker.pluginName}' via ` + + `fromPlugin(), but that plugin is not registered in createApp. ` + + `Available: ${available}.`, + ); + } + + const entries = resolveToolkitFromProvider( + marker.pluginName, + providerEntry.provider, + marker.opts, + ); + for (const [key, entry] of Object.entries(entries)) { + index.set(key, { + source: "toolkit", + pluginName: entry.pluginName, + localName: entry.localName, + def: { ...entry.def, name: key }, + }); + } + } + } + private async connectHostedTools( hostedTools: import("../../core/agent/tools/hosted-tools").HostedTool[], index: Map, diff --git a/packages/appkit/src/plugins/agents/index.ts b/packages/appkit/src/plugins/agents/index.ts index 0c8964109..404da2432 100644 --- a/packages/appkit/src/plugins/agents/index.ts +++ b/packages/appkit/src/plugins/agents/index.ts @@ -1,4 +1,11 @@ export { buildToolkitEntries } from "../../core/agent/build-toolkit"; +export { + FROM_PLUGIN_MARKER, + type FromPluginMarker, + type FromPluginSpread, + fromPlugin, + isFromPluginMarker, +} from "../../core/agent/from-plugin"; export { agentIdFromMarkdownPath, type LoadContext, @@ -11,6 +18,7 @@ export { type AgentDefinition, type AgentsPluginConfig, type AgentTool, + type AgentTools, type AutoInheritToolsConfig, type BaseSystemPromptOption, isToolkitEntry, diff --git a/packages/appkit/src/plugins/agents/tests/agents-plugin.test.ts b/packages/appkit/src/plugins/agents/tests/agents-plugin.test.ts index c42cc97f8..42cb6b127 100644 --- a/packages/appkit/src/plugins/agents/tests/agents-plugin.test.ts +++ b/packages/appkit/src/plugins/agents/tests/agents-plugin.test.ts @@ -12,10 +12,12 @@ import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; import { z } from "zod"; import { CacheManager } from "../../../cache"; import { buildToolkitEntries } from "../../../core/agent/build-toolkit"; +import { fromPlugin } from "../../../core/agent/from-plugin"; import { defineTool, type ToolRegistry, } from "../../../core/agent/tools/define-tool"; +import { tool } from "../../../core/agent/tools/tool"; import type { AgentsPluginConfig, ToolkitEntry, @@ -24,6 +26,12 @@ import { isToolkitEntry } from "../../../core/agent/types"; // Import the class directly so we can construct it without a createApp import { AgentsPlugin } from "../agents"; +function namedFactory(name: string) { + const f = () => ({ name }); + Object.defineProperty(f, "pluginName", { value: name, enumerable: true }); + return f as typeof f & { readonly pluginName: string }; +} + interface FakeContext { providers: Array<{ name: string; provider: ToolProvider }>; getToolProviders(): Array<{ name: string; provider: ToolProvider }>; @@ -370,4 +378,202 @@ describe("AgentsPlugin", () => { expect(isToolkitEntry({ foo: 1 })).toBe(false); expect(isToolkitEntry(null)).toBe(false); }); + + describe("fromPlugin markers", () => { + test("spreading fromPlugin registers all tools from the referenced plugin", async () => { + const registry: ToolRegistry = { + query: defineTool({ + description: "q", + schema: z.object({ sql: z.string() }), + handler: () => "ok", + }), + }; + const ctx = fakeContext([ + { + name: "analytics", + provider: makeToolProvider("analytics", registry), + }, + ]); + + const plugin = instantiate( + { + dir: false, + agents: { + support: { + instructions: "...", + model: stubAdapter(), + tools: { ...fromPlugin(namedFactory("analytics")) }, + }, + }, + }, + ctx, + ); + await plugin.setup(); + + const api = plugin.exports() as { + get: (name: string) => { toolIndex: Map } | null; + }; + const agent = api.get("support"); + expect(agent?.toolIndex.has("analytics.query")).toBe(true); + }); + + test("mixed inline + fromPlugin tools coexist", async () => { + const registry: ToolRegistry = { + query: defineTool({ + description: "q", + schema: z.object({ sql: z.string() }), + handler: () => "ok", + }), + }; + const ctx = fakeContext([ + { + name: "analytics", + provider: makeToolProvider("analytics", registry), + }, + ]); + + const plugin = instantiate( + { + dir: false, + agents: { + support: { + instructions: "...", + model: stubAdapter(), + tools: { + ...fromPlugin(namedFactory("analytics")), + get_weather: tool({ + name: "get_weather", + description: "Weather", + schema: z.object({ city: z.string() }), + execute: async ({ city }) => `Sunny in ${city}`, + }), + }, + }, + }, + }, + ctx, + ); + await plugin.setup(); + + const api = plugin.exports() as { + get: (name: string) => { toolIndex: Map } | null; + }; + const agent = api.get("support"); + expect(agent?.toolIndex.has("analytics.query")).toBe(true); + expect(agent?.toolIndex.has("get_weather")).toBe(true); + }); + + test("missing plugin throws at setup with Available: listing", async () => { + const ctx = fakeContext([ + { + name: "files", + provider: makeToolProvider("files", {}), + }, + ]); + + const plugin = instantiate( + { + dir: false, + agents: { + support: { + instructions: "...", + model: stubAdapter(), + tools: { ...fromPlugin(namedFactory("analytics")) }, + }, + }, + }, + ctx, + ); + await expect(plugin.setup()).rejects.toThrow(/analytics/); + await expect(plugin.setup()).rejects.toThrow(/Available:/); + await expect(plugin.setup()).rejects.toThrow(/files/); + }); + + test("symbol-only tools record disables auto-inherit", async () => { + const analyticsReg: ToolRegistry = { + query: defineTool({ + description: "q", + schema: z.object({ sql: z.string() }), + handler: () => "ok", + }), + }; + const filesReg: ToolRegistry = { + list: defineTool({ + description: "l", + schema: z.object({}), + handler: () => [], + }), + }; + const ctx = fakeContext([ + { + name: "analytics", + provider: makeToolProvider("analytics", analyticsReg), + }, + { + name: "files", + provider: makeToolProvider("files", filesReg), + }, + ]); + + const plugin = instantiate( + { + dir: false, + autoInheritTools: { code: true }, + agents: { + support: { + instructions: "...", + model: stubAdapter(), + tools: { ...fromPlugin(namedFactory("analytics")) }, + }, + }, + }, + ctx, + ); + await plugin.setup(); + + const api = plugin.exports() as { + get: (name: string) => { toolIndex: Map } | null; + }; + const agent = api.get("support"); + const toolNames = Array.from(agent?.toolIndex.keys() ?? []); + expect(toolNames.some((n) => n.startsWith("analytics."))).toBe(true); + expect(toolNames.some((n) => n.startsWith("files."))).toBe(false); + }); + + test("falls back to getAgentTools() for providers without toolkit()", async () => { + // Provider lacks .toolkit() — only getAgentTools/executeAgentTool. + const bareProvider: ToolProvider = { + getAgentTools: () => [ + { + name: "ping", + description: "ping", + parameters: { type: "object", properties: {} }, + }, + ], + executeAgentTool: vi.fn(async () => "pong"), + }; + const ctx = fakeContext([{ name: "bare", provider: bareProvider }]); + + const plugin = instantiate( + { + dir: false, + agents: { + support: { + instructions: "...", + model: stubAdapter(), + tools: { ...fromPlugin(namedFactory("bare")) }, + }, + }, + }, + ctx, + ); + await plugin.setup(); + + const api = plugin.exports() as { + get: (name: string) => { toolIndex: Map } | null; + }; + const agent = api.get("support"); + expect(agent?.toolIndex.has("bare.ping")).toBe(true); + }); + }); }); diff --git a/packages/appkit/src/plugins/agents/tests/from-plugin.test.ts b/packages/appkit/src/plugins/agents/tests/from-plugin.test.ts new file mode 100644 index 000000000..eb31d0f7d --- /dev/null +++ b/packages/appkit/src/plugins/agents/tests/from-plugin.test.ts @@ -0,0 +1,80 @@ +import { describe, expect, test } from "vitest"; +import { + FROM_PLUGIN_MARKER, + fromPlugin, + isFromPluginMarker, +} from "../../../core/agent/from-plugin"; + +function fakeFactory(name: string) { + const f = () => ({ name }); + Object.defineProperty(f, "pluginName", { value: name, enumerable: true }); + return f as typeof f & { readonly pluginName: string }; +} + +describe("fromPlugin", () => { + test("returns a spread-friendly object with a single symbol-keyed marker", () => { + const spread = fromPlugin(fakeFactory("analytics")); + + expect(Object.keys(spread)).toHaveLength(0); + const syms = Object.getOwnPropertySymbols(spread); + expect(syms).toHaveLength(1); + + const marker = (spread as Record)[syms[0]!]; + expect(isFromPluginMarker(marker)).toBe(true); + expect((marker as { pluginName: string }).pluginName).toBe("analytics"); + }); + + test("multiple calls produce distinct symbol keys (spreads coexist)", () => { + const spread = { + ...fromPlugin(fakeFactory("analytics")), + ...fromPlugin(fakeFactory("analytics")), + ...fromPlugin(fakeFactory("files")), + }; + + const syms = Object.getOwnPropertySymbols(spread); + expect(syms).toHaveLength(3); + }); + + test("passes opts through to the marker", () => { + const spread = fromPlugin(fakeFactory("analytics"), { + only: ["query"], + prefix: "q_", + }); + const sym = Object.getOwnPropertySymbols(spread)[0]!; + const marker = (spread as Record)[sym] as { + opts: { only: string[]; prefix: string }; + }; + expect(marker.opts.only).toEqual(["query"]); + expect(marker.opts.prefix).toBe("q_"); + }); + + test("throws when factory has no pluginName", () => { + const missing = () => ({ name: "nope" }); + expect(() => + fromPlugin(missing as unknown as { readonly pluginName: string }), + ).toThrow(/missing pluginName/); + }); + + test("FROM_PLUGIN_MARKER is a globally-interned symbol", () => { + expect(FROM_PLUGIN_MARKER).toBe( + Symbol.for("@databricks/appkit.fromPluginMarker"), + ); + }); +}); + +describe("isFromPluginMarker", () => { + test("returns true for real markers", () => { + const spread = fromPlugin(fakeFactory("analytics")); + const sym = Object.getOwnPropertySymbols(spread)[0]!; + expect(isFromPluginMarker((spread as Record)[sym])).toBe( + true, + ); + }); + + test("returns false for objects without the brand", () => { + expect(isFromPluginMarker({ pluginName: "x" })).toBe(false); + expect(isFromPluginMarker(null)).toBe(false); + expect(isFromPluginMarker(undefined)).toBe(false); + expect(isFromPluginMarker("string")).toBe(false); + }); +}); From 86e949fa6278f3e37c39c06cb3dd1fa6ba9cb47a Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Mon, 4 May 2026 14:59:08 +0200 Subject: [PATCH 02/12] fix(agents): update agents.ts imports to core/agent/ paths after Option A rewrite normalize-result, consume-adapter-stream, tool-dispatch were extracted to core/agent/ but agents.ts still imported them from plugins/agents/. Update the import paths to match the final file locations. --- packages/appkit/src/plugins/agents/agents.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/appkit/src/plugins/agents/agents.ts b/packages/appkit/src/plugins/agents/agents.ts index 78c2d868b..11deef3a9 100644 --- a/packages/appkit/src/plugins/agents/agents.ts +++ b/packages/appkit/src/plugins/agents/agents.ts @@ -25,6 +25,7 @@ import { buildBaseSystemPrompt, composeSystemPrompt, } from "../../core/agent/system-prompt"; +import { dispatchToolCall } from "../../core/agent/tool-dispatch"; import { resolveToolkitFromProvider } from "../../core/agent/toolkit-resolver"; import { functionToolToDefinition, From b9791e73df8448b6cb4e344c8b3ddfbc518a04fd Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Wed, 29 Apr 2026 18:19:25 +0200 Subject: [PATCH 03/12] refactor(appkit): move agent runtime to core/agent/ Flips the layering: agent types, helpers, and the standalone runner now live in core/agent/ instead of plugins/agents/. The HTTP-facing agents() plugin still owns its routes/streaming/threads but no longer re-exports framework primitives that peer plugins depend on. Moved (with git mv to preserve history): - plugins/agents/{types,from-plugin,build-toolkit,toolkit-resolver, consume-adapter-stream,normalize-result,tool-dispatch,system-prompt, load-agents}.ts -> core/agent/ - plugins/agents/tools/{tool,define-tool,function-tool,hosted-tools, sql-policy,json-schema,index}.ts -> core/agent/tools/ - core/{run-agent,create-agent-def}.ts -> core/agent/{run-agent,create-agent}.ts - 14 corresponding test files -> core/agent/tests/ Stayed in plugins/agents/ (HTTP/route concerns): - agents.ts, event-channel.ts, event-translator.ts, tool-approval-gate.ts, thread-store.ts, schemas.ts, defaults.ts, manifest.json, index.ts Updated imports across analytics, files, genie, lakebase to source from core/agent/ directly. plugins/agents/index.ts stays as a back-compat barrel that re-exports the moved primitives, so the public package surface (@databricks/appkit) is byte-identical. Verified: tsc --noEmit clean, 1581/1581 appkit tests pass. --- knip.json | 10 +-- packages/appkit/src/connectors/mcp/types.ts | 2 +- packages/appkit/src/core/agent/index.ts | 63 +++++++++++++++++++ .../src/core/agent/tests/create-agent.test.ts | 4 +- .../agent}/tests/from-plugin.test.ts | 0 .../agent}/tests/mcp-server-helper.test.ts | 0 .../src/core/agent/tests/run-agent.test.ts | 2 +- packages/appkit/src/plugins/agents/agents.ts | 1 - packages/appkit/src/plugins/agents/index.ts | 3 + 9 files changed, 75 insertions(+), 10 deletions(-) create mode 100644 packages/appkit/src/core/agent/index.ts rename packages/appkit/src/{plugins/agents => core/agent}/tests/from-plugin.test.ts (100%) rename packages/appkit/src/{plugins/agents => core/agent}/tests/mcp-server-helper.test.ts (100%) diff --git a/knip.json b/knip.json index 9692fa3fb..bf22afd6d 100644 --- a/knip.json +++ b/knip.json @@ -19,15 +19,15 @@ "packages/appkit/src/plugin/index.ts", "packages/appkit/src/plugin/to-plugin.ts", "packages/appkit/src/plugins/agents/**", - "template/**", - "tools/**", - "docs/**", - ".github/scripts/**", + "packages/appkit/src/core/agent/index.ts", "packages/appkit/src/core/agent/tools/index.ts", "packages/appkit/src/core/agent/from-plugin.ts", "packages/appkit/src/core/agent/load-agents.ts", "packages/appkit/src/connectors/mcp/index.ts", - "packages/appkit/src/plugin/to-plugin.ts" + "template/**", + "tools/**", + "docs/**", + ".github/scripts/**" ], "ignoreDependencies": ["json-schema-to-typescript"], "ignoreBinaries": ["tarball"] diff --git a/packages/appkit/src/connectors/mcp/types.ts b/packages/appkit/src/connectors/mcp/types.ts index d74f0a46c..aeb61788e 100644 --- a/packages/appkit/src/connectors/mcp/types.ts +++ b/packages/appkit/src/connectors/mcp/types.ts @@ -1,7 +1,7 @@ /** * Input shape consumed by {@link AppKitMcpClient.connect}. Produced by the * agents plugin from user-facing `HostedTool` declarations (see - * `plugins/agents/tools/hosted-tools.ts`) and accepted directly by the + * `core/agent/tools/hosted-tools.ts`) and accepted directly by the * connector to keep its surface free of agent-layer concepts. */ export interface McpEndpointConfig { diff --git a/packages/appkit/src/core/agent/index.ts b/packages/appkit/src/core/agent/index.ts new file mode 100644 index 000000000..96fb3eb8c --- /dev/null +++ b/packages/appkit/src/core/agent/index.ts @@ -0,0 +1,63 @@ +/** + * Agent runtime primitives. All framework-level agent types, tool helpers, + * and the standalone runner live here. The HTTP-facing `agents()` plugin in + * `plugins/agents/` consumes these but does not own them — peer plugins + * (analytics, files, genie, lakebase) can depend on this module without + * reaching across the sibling boundary. + */ +export { buildToolkitEntries } from "./build-toolkit"; +export { consumeAdapterStream } from "./consume-adapter-stream"; +export { createAgent } from "./create-agent"; +export { + FROM_PLUGIN_MARKER, + type FromPluginMarker, + type FromPluginSpread, + fromPlugin, + isFromPluginMarker, +} from "./from-plugin"; +export { + agentIdFromMarkdownPath, + type LoadContext, + type LoadResult, + loadAgentFromFile, + loadAgentsFromDir, + parseFrontmatter, +} from "./load-agents"; +export { normalizeToolResult } from "./normalize-result"; +export { + type RunAgentInput, + type RunAgentResult, + runAgent, +} from "./run-agent"; +export { buildBaseSystemPrompt, composeSystemPrompt } from "./system-prompt"; +export { resolveToolkitFromProvider } from "./toolkit-resolver"; +export { + defineTool, + executeFromRegistry, + type FunctionTool, + functionToolToDefinition, + type HostedTool, + isFunctionTool, + isHostedTool, + mcpServer, + resolveHostedTools, + type ToolConfig, + type ToolEntry, + type ToolRegistry, + tool, + toolsFromRegistry, +} from "./tools"; +export { + type AgentDefinition, + type AgentsPluginConfig, + type AgentTool, + type AgentTools, + type AutoInheritToolsConfig, + type BaseSystemPromptOption, + isToolkitEntry, + type PromptContext, + type RegisteredAgent, + type ResolvedToolEntry, + type ToolkitEntry, + type ToolkitOptions, +} from "./types"; diff --git a/packages/appkit/src/core/agent/tests/create-agent.test.ts b/packages/appkit/src/core/agent/tests/create-agent.test.ts index a095439e5..df920369e 100644 --- a/packages/appkit/src/core/agent/tests/create-agent.test.ts +++ b/packages/appkit/src/core/agent/tests/create-agent.test.ts @@ -1,8 +1,8 @@ import { describe, expect, test } from "vitest"; import { z } from "zod"; -import { tool } from "../../../core/agent/tools/tool"; -import type { AgentDefinition } from "../../../core/agent/types"; import { createAgent } from "../create-agent"; +import { tool } from "../tools/tool"; +import type { AgentDefinition } from "../types"; describe("createAgent", () => { test("returns the definition unchanged for a simple agent", () => { diff --git a/packages/appkit/src/plugins/agents/tests/from-plugin.test.ts b/packages/appkit/src/core/agent/tests/from-plugin.test.ts similarity index 100% rename from packages/appkit/src/plugins/agents/tests/from-plugin.test.ts rename to packages/appkit/src/core/agent/tests/from-plugin.test.ts diff --git a/packages/appkit/src/plugins/agents/tests/mcp-server-helper.test.ts b/packages/appkit/src/core/agent/tests/mcp-server-helper.test.ts similarity index 100% rename from packages/appkit/src/plugins/agents/tests/mcp-server-helper.test.ts rename to packages/appkit/src/core/agent/tests/mcp-server-helper.test.ts diff --git a/packages/appkit/src/core/agent/tests/run-agent.test.ts b/packages/appkit/src/core/agent/tests/run-agent.test.ts index 5324dde21..9ead2c177 100644 --- a/packages/appkit/src/core/agent/tests/run-agent.test.ts +++ b/packages/appkit/src/core/agent/tests/run-agent.test.ts @@ -10,11 +10,11 @@ import type { } from "shared"; import { describe, expect, test, vi } from "vitest"; import { z } from "zod"; -import type { ToolkitEntry } from "../../../core/agent/types"; import { createAgent } from "../create-agent"; import { fromPlugin } from "../from-plugin"; import { runAgent } from "../run-agent"; import { tool } from "../tools/tool"; +import type { ToolkitEntry } from "../types"; function scriptedAdapter(events: AgentEvent[]): AgentAdapter { return { diff --git a/packages/appkit/src/plugins/agents/agents.ts b/packages/appkit/src/plugins/agents/agents.ts index 11deef3a9..78c2d868b 100644 --- a/packages/appkit/src/plugins/agents/agents.ts +++ b/packages/appkit/src/plugins/agents/agents.ts @@ -25,7 +25,6 @@ import { buildBaseSystemPrompt, composeSystemPrompt, } from "../../core/agent/system-prompt"; -import { dispatchToolCall } from "../../core/agent/tool-dispatch"; import { resolveToolkitFromProvider } from "../../core/agent/toolkit-resolver"; import { functionToolToDefinition, diff --git a/packages/appkit/src/plugins/agents/index.ts b/packages/appkit/src/plugins/agents/index.ts index 404da2432..f630cc681 100644 --- a/packages/appkit/src/plugins/agents/index.ts +++ b/packages/appkit/src/plugins/agents/index.ts @@ -1,3 +1,6 @@ +// Re-exports of agent primitives that now live in core/agent/. Kept here so +// the public package barrel (`@databricks/appkit`) and any callers that +// already imported via `./plugins/agents` continue to resolve unchanged. export { buildToolkitEntries } from "../../core/agent/build-toolkit"; export { FROM_PLUGIN_MARKER, From 58debd89b426adc8abddc0190155caab2578087d Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Wed, 29 Apr 2026 18:41:56 +0200 Subject: [PATCH 04/12] refactor(appkit): split agents.ts helpers into separate modules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extracts `composePromptForAgent` + `normalizeAutoInherit` into plugins/agents/prompt.ts and `printRegistry` into plugins/agents/registry-printer.ts. These were free-function helpers at the bottom of agents.ts with no dependency on plugin state — pure candidates for extraction. Also opens the door for the bigger split (route handlers and `_streamAgent`/`runSubAgent` extracted into routes/*.ts and tool-execution.ts) by relaxing the access modifier on plugin members those modules will need (`agents`, `activeStreams`, `mcpClient`, `threadStore`, `approvalGate`, `resolvedApprovalPolicy`, `resolvedLimits`, `countUserStreams`). All marked `@internal` to keep the public surface unchanged. Note: the full split into `routes/` and `tool-execution.ts` proposed in plans/agent-architecture-followup.md is deferred. Route handlers and `_streamAgent`/`runSubAgent` remain as methods on AgentsPlugin because they have heavy plugin-state coupling and cross-call patterns (`runSubAgent` recurses, `_handleChat` calls `_streamAgent`, etc.) that don't translate cleanly to free functions without a larger refactor. Tracked as a follow-up. agents.ts: 1262 -> 1212 lines (-50). The plan's aspirational target of <=280 isn't met because the per-route extraction pass is deferred, but the helper extraction + access-modifier relaxation lays the groundwork. Verified: tsc --noEmit clean, 1589/1589 appkit tests pass. --- packages/appkit/src/plugins/agents/prompt.ts | 57 +++++++++++++++++++ .../src/plugins/agents/registry-printer.ts | 25 ++++++++ 2 files changed, 82 insertions(+) create mode 100644 packages/appkit/src/plugins/agents/prompt.ts create mode 100644 packages/appkit/src/plugins/agents/registry-printer.ts diff --git a/packages/appkit/src/plugins/agents/prompt.ts b/packages/appkit/src/plugins/agents/prompt.ts new file mode 100644 index 000000000..d38c6e645 --- /dev/null +++ b/packages/appkit/src/plugins/agents/prompt.ts @@ -0,0 +1,57 @@ +import { + buildBaseSystemPrompt, + composeSystemPrompt, +} from "../../core/agent/system-prompt"; +import type { + AgentsPluginConfig, + BaseSystemPromptOption, + PromptContext, + RegisteredAgent, +} from "../../core/agent/types"; + +/** + * Resolves the per-agent and plugin-level base prompt options into the + * final system prompt sent to the adapter. Per-agent setting wins over + * plugin-level; `false` opts out entirely; functions receive the same + * `PromptContext` that the default builder uses. + */ +export function composePromptForAgent( + registered: RegisteredAgent, + pluginLevel: BaseSystemPromptOption | undefined, + ctx: PromptContext, +): string { + const perAgent = registered.baseSystemPrompt; + const resolved = perAgent !== undefined ? perAgent : pluginLevel; + + let base = ""; + if (resolved === false) { + base = ""; + } else if (typeof resolved === "string") { + base = resolved; + } else if (typeof resolved === "function") { + base = resolved(ctx); + } else { + base = buildBaseSystemPrompt(ctx); + } + + return composeSystemPrompt(base, registered.instructions); +} + +/** + * Resolves the plugin-level `autoInheritTools` config into a per-origin + * decision. Default is opt-out for both origins. A markdown agent or + * code-defined agent with no declared `tools:` gets an empty tool index + * unless the developer explicitly flips `autoInheritTools` on. Even then, + * only tools whose plugin author marked `autoInheritable: true` are + * spread — see `applyAutoInherit` for the filter. + */ +export function normalizeAutoInherit( + value: AgentsPluginConfig["autoInheritTools"], +): { + file: boolean; + code: boolean; +} { + if (value === undefined) return { file: false, code: false }; + if (typeof value === "boolean") return { file: value, code: value }; + return { file: value.file ?? false, code: value.code ?? false }; +} diff --git a/packages/appkit/src/plugins/agents/registry-printer.ts b/packages/appkit/src/plugins/agents/registry-printer.ts new file mode 100644 index 000000000..9231ee077 --- /dev/null +++ b/packages/appkit/src/plugins/agents/registry-printer.ts @@ -0,0 +1,25 @@ +import pc from "picocolors"; +import type { RegisteredAgent } from "../../core/agent/types"; + +/** + * Pretty-prints the registered agent set during plugin setup. Decorative — + * no behaviour change if it's skipped (e.g., from tests). + */ +export function printRegistry( + agents: Map, + defaultAgentName: string | null, +): void { + if (agents.size === 0) return; + console.log(""); + console.log(` ${pc.bold("Agents")} ${pc.dim(`(${agents.size})`)}`); + console.log(` ${pc.dim("─".repeat(60))}`); + for (const [name, reg] of agents) { + const tools = reg.toolIndex.size; + const marker = name === defaultAgentName ? pc.green("●") : " "; + console.log( + ` ${marker} ${pc.bold(name.padEnd(24))} ${pc.dim(`${tools} tools`)}`, + ); + } + console.log(` ${pc.dim("─".repeat(60))}`); + console.log(""); +} From 60d7b83334c1e32e730bd030a599a231e313cd98 Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Mon, 4 May 2026 19:50:25 +0200 Subject: [PATCH 05/12] feat(appkit): unify on DATABRICKS_SERVING_ENDPOINT_NAME (SDK + template manifest) Signed-off-by: MarioCadenas --- packages/appkit/src/plugins/agents/agents.ts | 2 +- packages/appkit/src/plugins/agents/manifest.json | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/appkit/src/plugins/agents/agents.ts b/packages/appkit/src/plugins/agents/agents.ts index 78c2d868b..714b1643c 100644 --- a/packages/appkit/src/plugins/agents/agents.ts +++ b/packages/appkit/src/plugins/agents/agents.ts @@ -388,7 +388,7 @@ export class AgentsPlugin extends Plugin implements ToolProvider { ); } catch (err) { throw new Error( - `Agent '${name}' has no model configured and no DATABRICKS_AGENT_ENDPOINT default available`, + `Agent '${name}' has no model configured and no DATABRICKS_SERVING_ENDPOINT_NAME default available`, { cause: err instanceof Error ? err : undefined }, ); } diff --git a/packages/appkit/src/plugins/agents/manifest.json b/packages/appkit/src/plugins/agents/manifest.json index a33f75ce4..922a10fee 100644 --- a/packages/appkit/src/plugins/agents/manifest.json +++ b/packages/appkit/src/plugins/agents/manifest.json @@ -12,12 +12,12 @@ "type": "serving_endpoint", "alias": "Model Serving (agents)", "resourceKey": "agents-serving-endpoint", - "description": "Databricks Model Serving endpoint for agents using workspace-hosted models (`DatabricksAdapter.fromModelServing`). Wire the same endpoint name AppKit reads from `DATABRICKS_AGENT_ENDPOINT` when no per-agent model is configured. Omit when agents use only external adapters.", + "description": "Databricks Model Serving endpoint for agents using workspace-hosted models (`DatabricksAdapter.fromModelServing`). Wire the same endpoint name AppKit reads from `DATABRICKS_SERVING_ENDPOINT_NAME` when no per-agent model is configured. The same env var the `serving` plugin reads — one value covers both. Omit when agents use only external adapters.", "permission": "CAN_QUERY", "fields": { "name": { - "env": "DATABRICKS_AGENT_ENDPOINT", - "description": "Endpoint name passed to Model Serving when agents default to `DatabricksAdapter.fromModelServing()`" + "env": "DATABRICKS_SERVING_ENDPOINT_NAME", + "description": "Endpoint name passed to Model Serving when agents default to `DatabricksAdapter.fromModelServing()`. Shared with the `serving` plugin." } } } From ba8228090db30bcd9221ddd76ae397bf5f0bab54 Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Thu, 7 May 2026 16:42:55 +0200 Subject: [PATCH 06/12] chore(appkit): drop duplicate prompt/registry-printer modules after rebase Signed-off-by: MarioCadenas --- packages/appkit/src/plugins/agents/prompt.ts | 57 ------------------- .../src/plugins/agents/registry-printer.ts | 25 -------- 2 files changed, 82 deletions(-) delete mode 100644 packages/appkit/src/plugins/agents/prompt.ts delete mode 100644 packages/appkit/src/plugins/agents/registry-printer.ts diff --git a/packages/appkit/src/plugins/agents/prompt.ts b/packages/appkit/src/plugins/agents/prompt.ts deleted file mode 100644 index d38c6e645..000000000 --- a/packages/appkit/src/plugins/agents/prompt.ts +++ /dev/null @@ -1,57 +0,0 @@ -import { - buildBaseSystemPrompt, - composeSystemPrompt, -} from "../../core/agent/system-prompt"; -import type { - AgentsPluginConfig, - BaseSystemPromptOption, - PromptContext, - RegisteredAgent, -} from "../../core/agent/types"; - -/** - * Resolves the per-agent and plugin-level base prompt options into the - * final system prompt sent to the adapter. Per-agent setting wins over - * plugin-level; `false` opts out entirely; functions receive the same - * `PromptContext` that the default builder uses. - */ -export function composePromptForAgent( - registered: RegisteredAgent, - pluginLevel: BaseSystemPromptOption | undefined, - ctx: PromptContext, -): string { - const perAgent = registered.baseSystemPrompt; - const resolved = perAgent !== undefined ? perAgent : pluginLevel; - - let base = ""; - if (resolved === false) { - base = ""; - } else if (typeof resolved === "string") { - base = resolved; - } else if (typeof resolved === "function") { - base = resolved(ctx); - } else { - base = buildBaseSystemPrompt(ctx); - } - - return composeSystemPrompt(base, registered.instructions); -} - -/** - * Resolves the plugin-level `autoInheritTools` config into a per-origin - * decision. Default is opt-out for both origins. A markdown agent or - * code-defined agent with no declared `tools:` gets an empty tool index - * unless the developer explicitly flips `autoInheritTools` on. Even then, - * only tools whose plugin author marked `autoInheritable: true` are - * spread — see `applyAutoInherit` for the filter. - */ -export function normalizeAutoInherit( - value: AgentsPluginConfig["autoInheritTools"], -): { - file: boolean; - code: boolean; -} { - if (value === undefined) return { file: false, code: false }; - if (typeof value === "boolean") return { file: value, code: value }; - return { file: value.file ?? false, code: value.code ?? false }; -} diff --git a/packages/appkit/src/plugins/agents/registry-printer.ts b/packages/appkit/src/plugins/agents/registry-printer.ts deleted file mode 100644 index 9231ee077..000000000 --- a/packages/appkit/src/plugins/agents/registry-printer.ts +++ /dev/null @@ -1,25 +0,0 @@ -import pc from "picocolors"; -import type { RegisteredAgent } from "../../core/agent/types"; - -/** - * Pretty-prints the registered agent set during plugin setup. Decorative — - * no behaviour change if it's skipped (e.g., from tests). - */ -export function printRegistry( - agents: Map, - defaultAgentName: string | null, -): void { - if (agents.size === 0) return; - console.log(""); - console.log(` ${pc.bold("Agents")} ${pc.dim(`(${agents.size})`)}`); - console.log(` ${pc.dim("─".repeat(60))}`); - for (const [name, reg] of agents) { - const tools = reg.toolIndex.size; - const marker = name === defaultAgentName ? pc.green("●") : " "; - console.log( - ` ${marker} ${pc.bold(name.padEnd(24))} ${pc.dim(`${tools} tools`)}`, - ); - } - console.log(` ${pc.dim("─".repeat(60))}`); - console.log(""); -} From a0269243d953e6407d347403e40687b263aa6049 Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Fri, 8 May 2026 10:40:48 +0200 Subject: [PATCH 07/12] fix(appkit): apply review feedback to runAgent + agents plugin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #305 agentic-review fixes (P1 + cheap P2): - _handleChat / _handleInvocations now wrap threadStore calls in try/catch and surface failures as a 500. Without this an unreachable store hung the SSE client until the upstream proxy timeout because the async Express handler propagated the rejection without a response. - runAgent rejects hosted/MCP tools at index-build time with a clear pointer to createApp({ plugins: [..., agents()] }). Previously the adapter saw the tool list and the failure surfaced mid-conversation. - Extract applyToolkitOptions: single source of truth for the prefix/only/except/rename filtering shared by build-toolkit (registry path) and toolkit-resolver (getAgentTools fallback). Bug fixes here apply to both paths instead of drifting between them. - resolveStandaloneProvider now uses an isStandaloneToolProvider type guard instead of `instance as unknown as ToolProvider`. Distinct from core/plugin-context.isToolProvider which also requires asUser (request-scoped, only meaningful inside createApp). Tests added: - threadStore failure paths on /chat (get/create/addMessage rejections) and /invocations (create + addMessage mid-loop) — assert 500 responses with the canonical error body. - runAgent rejects hosted tools at standalone resolution. - runAgent surfaces a clear error when fromPlugin references a plugin lacking ToolProvider methods. - runAgent recursively executes sub-agents declared on def.agents. #4 (countUserStreams O(n)) deferred — n bounded by 5 × users in the default config; not a hot path. #11 (printRegistry console.log) and Signed-off-by: MarioCadenas #9-#15 advisory items left as-is per the PR-comment thread. --- .../appkit/src/core/agent/build-toolkit.ts | 13 +- packages/appkit/src/core/agent/run-agent.ts | 46 +++-- .../src/core/agent/tests/run-agent.test.ts | 110 +++++++++++ .../appkit/src/core/agent/toolkit-options.ts | 32 +++ .../appkit/src/core/agent/toolkit-resolver.ts | 13 +- packages/appkit/src/plugins/agents/agents.ts | 85 +++++--- .../agents/tests/route-handler-errors.test.ts | 187 ++++++++++++++++++ 7 files changed, 417 insertions(+), 69 deletions(-) create mode 100644 packages/appkit/src/core/agent/toolkit-options.ts create mode 100644 packages/appkit/src/plugins/agents/tests/route-handler-errors.test.ts diff --git a/packages/appkit/src/core/agent/build-toolkit.ts b/packages/appkit/src/core/agent/build-toolkit.ts index 0140425d2..6e5a8ad19 100644 --- a/packages/appkit/src/core/agent/build-toolkit.ts +++ b/packages/appkit/src/core/agent/build-toolkit.ts @@ -1,4 +1,5 @@ import type { AgentToolDefinition } from "shared"; +import { applyToolkitOptions } from "./toolkit-options"; import type { ToolRegistry } from "./tools/define-tool"; import { toToolJSONSchema } from "./tools/json-schema"; import type { ToolkitEntry, ToolkitOptions } from "./types"; @@ -22,19 +23,11 @@ export function buildToolkitEntries( registry: ToolRegistry, opts: ToolkitOptions = {}, ): Record { - const prefix = opts.prefix ?? `${pluginName}.`; - const only = opts.only ? new Set(opts.only) : null; - const except = opts.except ? new Set(opts.except) : null; - const rename = opts.rename ?? {}; - const out: Record = {}; for (const [localName, entry] of Object.entries(registry)) { - if (only && !only.has(localName)) continue; - if (except?.has(localName)) continue; - - const keyAfterPrefix = `${prefix}${localName}`; - const key = rename[localName] ?? keyAfterPrefix; + const key = applyToolkitOptions(localName, pluginName, opts); + if (key === null) continue; const parameters = toToolJSONSchema( entry.schema, diff --git a/packages/appkit/src/core/agent/run-agent.ts b/packages/appkit/src/core/agent/run-agent.ts index 1eb510742..9118c025f 100644 --- a/packages/appkit/src/core/agent/run-agent.ts +++ b/packages/appkit/src/core/agent/run-agent.ts @@ -272,14 +272,14 @@ function classifyTool(key: string, tool: AgentTool): StandaloneEntry { }; } if (isHostedTool(tool)) { - return { - kind: "hosted", - def: { - name: key, - description: `Hosted tool: ${tool.type}`, - parameters: { type: "object", properties: {} }, - }, - }; + // Hosted tools (e.g. MCP `mcpServer(...)`) need a live MCP client that + // only exists inside the agents plugin's lifecycle. In standalone + // `runAgent` they would have errored at dispatch time with a confusing + // mid-conversation failure; reject them up front so misconfiguration + // surfaces before the adapter sees the tool list. + throw new Error( + `runAgent: tool "${key}" is a hosted tool (type="${tool.type}") which is only supported via createApp({ plugins: [..., agents(...)] }). Standalone runAgent has no MCP client.`, + ); } throw new Error(`runAgent: unrecognized tool shape at key "${key}"`); } @@ -309,6 +309,24 @@ function toolkitEntryToStandalone( }; } +/** + * Lightweight `ToolProvider` shape check used by standalone `runAgent`. + * + * Distinct from `core/plugin-context.isToolProvider` which also requires + * `asUser` (request-scoped, only meaningful when running inside `createApp` + * with a live HTTP context). Standalone plugins are constructed without a + * `WorkspaceClient` and have no request to scope to, so checking only the + * two `ToolProvider` methods is the right narrowing here. + */ +function isStandaloneToolProvider(value: unknown): value is ToolProvider { + if (typeof value !== "object" || value === null) return false; + const obj = value as Record; + return ( + typeof obj.getAgentTools === "function" && + typeof obj.executeAgentTool === "function" + ); +} + function resolveStandaloneProvider( pluginName: string, plugins: PluginData[], @@ -331,19 +349,13 @@ function resolveStandaloneProvider( ...(match.config ?? {}), name: pluginName, }); - const provider = instance as unknown as ToolProvider; - if ( - typeof (provider as { getAgentTools?: unknown }).getAgentTools !== - "function" || - typeof (provider as { executeAgentTool?: unknown }).executeAgentTool !== - "function" - ) { + if (!isStandaloneToolProvider(instance)) { throw new Error( `runAgent: plugin '${pluginName}' is not a ToolProvider ` + "(missing getAgentTools/executeAgentTool). Only ToolProvider plugins " + "are supported via fromPlugin() in runAgent.", ); } - cache.set(pluginName, provider); - return provider; + cache.set(pluginName, instance); + return instance; } diff --git a/packages/appkit/src/core/agent/tests/run-agent.test.ts b/packages/appkit/src/core/agent/tests/run-agent.test.ts index 9ead2c177..7a87a4cec 100644 --- a/packages/appkit/src/core/agent/tests/run-agent.test.ts +++ b/packages/appkit/src/core/agent/tests/run-agent.test.ts @@ -13,6 +13,7 @@ import { z } from "zod"; import { createAgent } from "../create-agent"; import { fromPlugin } from "../from-plugin"; import { runAgent } from "../run-agent"; +import { mcpServer } from "../tools/hosted-tools"; import { tool } from "../tools/tool"; import type { ToolkitEntry } from "../types"; @@ -213,4 +214,113 @@ describe("runAgent", () => { capturedCtx!.executeTool("analytics.query", {}), ).rejects.toThrow(/only usable via createApp/); }); + + test("rejects fromPlugin against a plugin lacking ToolProvider methods", async () => { + // Pre-condition for #305 review finding #7: a plain plugin (no + // getAgentTools / executeAgentTool) referenced via fromPlugin must + // surface a clear error at standalone resolution, not at dispatch. + class NotAToolProvider { + static manifest = { name: "noop" }; + static DEFAULT_CONFIG = {}; + name = "noop"; + constructor(public config: unknown) {} + async setup() {} + injectRoutes() {} + getEndpoints() { + return {}; + } + } + + const factory = () => ({ + plugin: NotAToolProvider as unknown as PluginConstructor, + config: {}, + name: "noop" as const, + }); + Object.defineProperty(factory, "pluginName", { + value: "noop", + enumerable: true, + }); + + const adapter: AgentAdapter = { + async *run(_input, context) { + // Force resolution by attempting a dispatch. + await context.executeTool("noop.anything", {}).catch(() => undefined); + yield { type: "message_delta", content: "" }; + }, + }; + + const def = createAgent({ + instructions: "x", + model: adapter, + tools: { + ...fromPlugin(factory as unknown as { readonly pluginName: string }), + }, + }); + + const pluginData = factory() as PluginData< + PluginConstructor, + unknown, + string + >; + + await expect( + runAgent(def, { messages: "hi", plugins: [pluginData] }), + ).rejects.toThrow(/not a ToolProvider/); + }); + + test("rejects hosted/MCP tools at index-build time, not at dispatch", async () => { + // Pre-condition for #305 review finding #3: a hosted tool slipped into + // an agent def used with standalone runAgent must surface a clear error + // before the adapter sees the tool list — not later when the model + // emits a function_call mid-conversation. + const def = createAgent({ + instructions: "x", + // biome-ignore lint/suspicious/noExplicitAny: stub adapter — we never reach it + model: { async *run() {} } as any, + tools: { + analytics: mcpServer("analytics-mcp", "https://example.com/mcp"), + }, + }); + + await expect(runAgent(def, { messages: "hi" })).rejects.toThrow( + /hosted tool .* only supported via createApp/, + ); + }); + + test("recursively executes sub-agents declared on def.agents", async () => { + // Pre-condition for #305 review finding #8: parent's `agent-` tool + // call should kick a nested runAgent that returns the child's text + // output as the tool result. + const childAdapter: AgentAdapter = { + async *run(_input, _context) { + yield { type: "message_delta", content: "child says hi" }; + }, + }; + + let capturedCtx: AgentRunContext | null = null; + const parentAdapter: AgentAdapter = { + async *run(_input, context) { + capturedCtx = context; + yield { type: "message_delta", content: "" }; + }, + }; + + const parent = createAgent({ + instructions: "parent", + model: parentAdapter, + agents: { + helper: createAgent({ + instructions: "child", + model: childAdapter, + }), + }, + }); + + await runAgent(parent, { messages: "go" }); + expect(capturedCtx).not.toBeNull(); + const result = + await // biome-ignore lint/style/noNonNullAssertion: asserted above + capturedCtx!.executeTool("agent-helper", { input: "say hi" }); + expect(result).toBe("child says hi"); + }); }); diff --git a/packages/appkit/src/core/agent/toolkit-options.ts b/packages/appkit/src/core/agent/toolkit-options.ts new file mode 100644 index 000000000..c8b1dedd1 --- /dev/null +++ b/packages/appkit/src/core/agent/toolkit-options.ts @@ -0,0 +1,32 @@ +import type { ToolkitOptions } from "./types"; + +/** + * Filter / prefix / rename a tool's local name into its toolkit key, or + * skip it entirely. Encapsulates the four-knob `ToolkitOptions` contract: + * + * - `only` — allowlist of local names; everything else is dropped. + * - `except` — denylist of local names. + * - `prefix` — string prepended to the local name; defaults to + * `${pluginName}.`. Pass `""` to drop the prefix entirely. + * - `rename` — per-tool exact remapping; wins over `prefix`. + * + * Returns the final key, or `null` when the tool is filtered out. + * + * Single source of truth so {@link buildToolkitEntries} (registry path) + * and {@link resolveToolkitFromProvider} (`getAgentTools()` fallback) stay + * in lockstep — bug fixes here apply to both. + */ +export function applyToolkitOptions( + localName: string, + pluginName: string, + opts: ToolkitOptions = {}, +): string | null { + if (opts.only && !opts.only.includes(localName)) return null; + if (opts.except?.includes(localName)) return null; + + const rename = opts.rename ?? {}; + if (Object.hasOwn(rename, localName)) return rename[localName]; + + const prefix = opts.prefix ?? `${pluginName}.`; + return `${prefix}${localName}`; +} diff --git a/packages/appkit/src/core/agent/toolkit-resolver.ts b/packages/appkit/src/core/agent/toolkit-resolver.ts index 8ec8cf1f7..d49921269 100644 --- a/packages/appkit/src/core/agent/toolkit-resolver.ts +++ b/packages/appkit/src/core/agent/toolkit-resolver.ts @@ -1,4 +1,5 @@ import type { ToolProvider } from "shared"; +import { applyToolkitOptions } from "./toolkit-options"; import type { ToolkitEntry, ToolkitOptions } from "./types"; /** @@ -38,18 +39,10 @@ export function resolveToolkitFromProvider( return withToolkit.toolkit(opts); } - const only = opts?.only ? new Set(opts.only) : null; - const except = opts?.except ? new Set(opts.except) : null; - const rename = opts?.rename ?? {}; - const prefix = opts?.prefix ?? `${pluginName}.`; - const out: Record = {}; for (const tool of provider.getAgentTools()) { - if (only && !only.has(tool.name)) continue; - if (except?.has(tool.name)) continue; - - const keyAfterPrefix = `${prefix}${tool.name}`; - const key = rename[tool.name] ?? keyAfterPrefix; + const key = applyToolkitOptions(tool.name, pluginName, opts); + if (key === null) continue; out[key] = { __toolkitRef: true, diff --git a/packages/appkit/src/plugins/agents/agents.ts b/packages/appkit/src/plugins/agents/agents.ts index 714b1643c..c80e6149c 100644 --- a/packages/appkit/src/plugins/agents/agents.ts +++ b/packages/appkit/src/plugins/agents/agents.ts @@ -763,22 +763,34 @@ export class AgentsPlugin extends Plugin implements ToolProvider { return; } - let thread = threadId ? await this.threadStore.get(threadId, userId) : null; - if (threadId && !thread) { - res.status(404).json({ error: `Thread ${threadId} not found` }); + // ThreadStore can throw on backing-storage failures (DB unreachable, + // permission errors, transient I/O). Without a try/catch the + // `async` Express handler bubbles the rejection without a response and + // the client connection hangs until the proxy times out. Surface the + // failure as a 500 so the SSE client falls back instead of waiting. + let thread: Thread; + try { + const existing = threadId + ? await this.threadStore.get(threadId, userId) + : null; + if (threadId && !existing) { + res.status(404).json({ error: `Thread ${threadId} not found` }); + return; + } + thread = existing ?? (await this.threadStore.create(userId)); + + const userMessage: Message = { + id: randomUUID(), + role: "user", + content: message, + createdAt: new Date(), + }; + await this.threadStore.addMessage(thread.id, userId, userMessage); + } catch (err) { + logger.error("threadStore failed in /chat: %O", err); + res.status(500).json({ error: "Thread operation failed" }); return; } - if (!thread) { - thread = await this.threadStore.create(userId); - } - - const userMessage: Message = { - id: randomUUID(), - role: "user", - content: message, - createdAt: new Date(), - }; - await this.threadStore.addMessage(thread.id, userId, userMessage); return this._streamAgent(req, res, registered, thread, userId); } @@ -813,30 +825,39 @@ export class AgentsPlugin extends Plugin implements ToolProvider { return; } - const thread = await this.threadStore.create(userId); + // Same rationale as `_handleChat`: surface threadStore failures as a + // 500 instead of letting the async handler hang the client connection. + let thread: Thread; + try { + thread = await this.threadStore.create(userId); - if (typeof input === "string") { - await this.threadStore.addMessage(thread.id, userId, { - id: randomUUID(), - role: "user", - content: input, - createdAt: new Date(), - }); - } else { - for (const item of input) { - const role = (item.role ?? "user") as Message["role"]; - const content = - typeof item.content === "string" - ? item.content - : JSON.stringify(item.content ?? ""); - if (!content) continue; + if (typeof input === "string") { await this.threadStore.addMessage(thread.id, userId, { id: randomUUID(), - role, - content, + role: "user", + content: input, createdAt: new Date(), }); + } else { + for (const item of input) { + const role = (item.role ?? "user") as Message["role"]; + const content = + typeof item.content === "string" + ? item.content + : JSON.stringify(item.content ?? ""); + if (!content) continue; + await this.threadStore.addMessage(thread.id, userId, { + id: randomUUID(), + role, + content, + createdAt: new Date(), + }); + } } + } catch (err) { + logger.error("threadStore failed in /invocations: %O", err); + res.status(500).json({ error: "Thread operation failed" }); + return; } return this._streamAgent(req, res, registered, thread, userId); diff --git a/packages/appkit/src/plugins/agents/tests/route-handler-errors.test.ts b/packages/appkit/src/plugins/agents/tests/route-handler-errors.test.ts new file mode 100644 index 000000000..e654f5cad --- /dev/null +++ b/packages/appkit/src/plugins/agents/tests/route-handler-errors.test.ts @@ -0,0 +1,187 @@ +import type express from "express"; +import { beforeEach, describe, expect, test, vi } from "vitest"; +import { CacheManager } from "../../../cache"; +import { AgentsPlugin } from "../agents"; + +/** + * Surface-level guarantees on the agents plugin's HTTP route handlers when + * downstream dependencies fail. Prior to PR #305 review finding #1+#2, + * `_handleChat` and `_handleInvocations` awaited `threadStore` without a + * try/catch — a backing-store failure (DB unreachable, permission error) + * would propagate the rejection without writing a response and the SSE + * client would hang until the upstream proxy timeout. + */ + +beforeEach(() => { + // biome-ignore lint/suspicious/noExplicitAny: test seam, mirrors other suites + (CacheManager as any).instance = { + get: vi.fn(), + set: vi.fn(), + getOrExecute: vi.fn(async (_k: unknown[], fn: () => Promise) => + fn(), + ), + generateKey: vi.fn(() => "test-key"), + }; +}); + +function mockReq(body: unknown, userId = "alice"): express.Request { + const headers: Record = { + "x-forwarded-user": userId, + "x-forwarded-access-token": "fake-token", + }; + return { + body, + headers, + header: (name: string) => headers[name.toLowerCase()], + } as unknown as express.Request; +} + +function mockRes() { + const json = vi.fn(); + const setHeader = vi.fn(); + let statusCode = 200; + const status = vi.fn((code: number) => { + statusCode = code; + return { json }; + }); + return { + res: { status, json, setHeader } as unknown as express.Response, + get statusCode() { + return statusCode; + }, + json, + }; +} + +function seedPlugin(): AgentsPlugin { + const plugin = new AgentsPlugin({ dir: false }); + // biome-ignore lint/suspicious/noExplicitAny: seed private state + (plugin as any).agents.set("default", { + name: "default", + instructions: "hi", + adapter: { async *run() {} }, + toolIndex: new Map(), + }); + // biome-ignore lint/suspicious/noExplicitAny: seed private state + (plugin as any).defaultAgentName = "default"; + return plugin; +} + +describe("POST /chat — threadStore failure", () => { + test("returns 500 when threadStore.get rejects (existing thread path)", async () => { + const plugin = seedPlugin(); + // biome-ignore lint/suspicious/noExplicitAny: stub threadStore for failure + (plugin as any).threadStore = { + get: vi.fn().mockRejectedValue(new Error("DB unreachable")), + create: vi.fn(), + addMessage: vi.fn(), + }; + + const { res, json } = mockRes(); + await ( + plugin as unknown as { + _handleChat: (r: express.Request, w: express.Response) => Promise; + } + )._handleChat(mockReq({ message: "hi", threadId: "t-1" }), res); + + expect(res.status).toHaveBeenCalledWith(500); + expect(json).toHaveBeenCalledWith({ error: "Thread operation failed" }); + }); + + test("returns 500 when threadStore.create rejects (new-thread path)", async () => { + const plugin = seedPlugin(); + // biome-ignore lint/suspicious/noExplicitAny: stub + (plugin as any).threadStore = { + get: vi.fn().mockResolvedValue(null), + create: vi.fn().mockRejectedValue(new Error("Disk full")), + addMessage: vi.fn(), + }; + + const { res, json } = mockRes(); + await ( + plugin as unknown as { + _handleChat: (r: express.Request, w: express.Response) => Promise; + } + )._handleChat(mockReq({ message: "hi" }), res); + + expect(res.status).toHaveBeenCalledWith(500); + expect(json).toHaveBeenCalledWith({ error: "Thread operation failed" }); + }); + + test("returns 500 when threadStore.addMessage rejects", async () => { + const plugin = seedPlugin(); + // biome-ignore lint/suspicious/noExplicitAny: stub + (plugin as any).threadStore = { + get: vi.fn().mockResolvedValue(null), + create: vi.fn().mockResolvedValue({ id: "t-new", messages: [] }), + addMessage: vi.fn().mockRejectedValue(new Error("Quota exceeded")), + }; + + const { res, json } = mockRes(); + await ( + plugin as unknown as { + _handleChat: (r: express.Request, w: express.Response) => Promise; + } + )._handleChat(mockReq({ message: "hi" }), res); + + expect(res.status).toHaveBeenCalledWith(500); + expect(json).toHaveBeenCalledWith({ error: "Thread operation failed" }); + }); +}); + +describe("POST /invocations — threadStore failure", () => { + test("returns 500 when threadStore.create rejects", async () => { + const plugin = seedPlugin(); + // biome-ignore lint/suspicious/noExplicitAny: stub + (plugin as any).threadStore = { + create: vi.fn().mockRejectedValue(new Error("DB unreachable")), + addMessage: vi.fn(), + }; + + const { res, json } = mockRes(); + await ( + plugin as unknown as { + _handleInvocations: ( + r: express.Request, + w: express.Response, + ) => Promise; + } + )._handleInvocations(mockReq({ input: "hi" }), res); + + expect(res.status).toHaveBeenCalledWith(500); + expect(json).toHaveBeenCalledWith({ error: "Thread operation failed" }); + }); + + test("returns 500 when threadStore.addMessage rejects mid-loop", async () => { + const plugin = seedPlugin(); + // biome-ignore lint/suspicious/noExplicitAny: stub + (plugin as any).threadStore = { + create: vi.fn().mockResolvedValue({ id: "t-new", messages: [] }), + addMessage: vi + .fn() + .mockResolvedValueOnce(undefined) + .mockRejectedValueOnce(new Error("Conflict")), + }; + + const { res, json } = mockRes(); + await ( + plugin as unknown as { + _handleInvocations: ( + r: express.Request, + w: express.Response, + ) => Promise; + } + )._handleInvocations( + mockReq({ + input: [ + { role: "user", content: "first" }, + { role: "user", content: "second" }, + ], + }), + res, + ); + + expect(res.status).toHaveBeenCalledWith(500); + expect(json).toHaveBeenCalledWith({ error: "Thread operation failed" }); + }); +}); From 5dfd5eb0473f099f0da59c2f035f02027113b3c6 Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Fri, 8 May 2026 15:32:53 +0200 Subject: [PATCH 08/12] feat(appkit): replace fromPlugin with tools(plugins) function form Drop the symbol-keyed fromPlugin(factory) API in favor of a function form on AgentDefinition.tools that receives a typed plugins map. The key win: each plugin appears exactly once in user code (only inside createApp({ plugins: [...] })), and plugins..toolkit() is fully autocompleted in the IDE. Before: import { analytics, fromPlugin } from "@databricks/appkit"; const support = createAgent({ tools: { ...fromPlugin(analytics) }, }); await createApp({ plugins: [analytics(), agents({ agents: { support } })] }); After: const support = createAgent({ tools(plugins) { return { ...plugins.analytics.toolkit() }; }, }); await createApp({ plugins: [analytics(), agents({ agents: { support } })] }); The dual tools: AgentTools | AgentToolsFn shape means simple agents keep the plain object form. The function runs once at agent setup; its return record replaces def.tools for the rest of the registered agent's lifetime. Typing relies on a RegisteredPlugins module-augmentation interface that core plugins (analytics, files, genie, lakebase) extend at their declaration sites. Third-party plugins fall back to a PluginToolkitProvider shape via the index signature; they still work at runtime, just without keyed autocomplete. Standalone runAgent invokes the function form against a Plugins map built lazily from RunAgentInput.plugins. Plugin instances are constructed on first plugins..toolkit() call and cached, so the same instance handles both spread-time and dispatch-time work. Auto-inherit semantics unchanged: declaring tools (object or function, even an empty record) opts out, mirroring the prior rule. Markdown agents are untouched. YAML cannot carry a function reference, so toolkits: [analytics] and ambient agents({ tools: {...} }) keep working as the markdown surface. fromPlugin, FromPluginMarker, FROM_PLUGIN_MARKER, FromPluginSpread, isFromPluginMarker, and the NamedPluginFactory.pluginName field (its only consumer) are deleted from beta exports and from-plugin.ts. Signed-off-by: MarioCadenas --- knip.json | 1 - packages/appkit/src/beta.ts | 7 +- packages/appkit/src/core/agent/from-plugin.ts | 97 --------- packages/appkit/src/core/agent/index.ts | 11 +- packages/appkit/src/core/agent/run-agent.ts | 184 +++++++++++------- .../src/core/agent/tests/create-agent.test.ts | 7 +- .../src/core/agent/tests/from-plugin.test.ts | 80 -------- .../src/core/agent/tests/run-agent.test.ts | 127 ++++-------- .../appkit/src/core/agent/toolkit-resolver.ts | 2 +- .../src/core/agent/tools/define-tool.ts | 3 +- packages/appkit/src/core/agent/types.ts | 97 ++++++++- packages/appkit/src/plugin/index.ts | 2 +- packages/appkit/src/plugin/to-plugin.ts | 23 +-- packages/appkit/src/plugins/agents/agents.ts | 116 +++++------ packages/appkit/src/plugins/agents/index.ts | 11 +- .../agents/tests/agents-plugin.test.ts | 133 +++++++++---- .../appkit/src/plugins/analytics/analytics.ts | 10 +- packages/appkit/src/plugins/files/plugin.ts | 6 + packages/appkit/src/plugins/genie/genie.ts | 6 + .../appkit/src/plugins/lakebase/lakebase.ts | 6 + 20 files changed, 442 insertions(+), 487 deletions(-) delete mode 100644 packages/appkit/src/core/agent/from-plugin.ts delete mode 100644 packages/appkit/src/core/agent/tests/from-plugin.test.ts diff --git a/knip.json b/knip.json index bf22afd6d..67e825eee 100644 --- a/knip.json +++ b/knip.json @@ -21,7 +21,6 @@ "packages/appkit/src/plugins/agents/**", "packages/appkit/src/core/agent/index.ts", "packages/appkit/src/core/agent/tools/index.ts", - "packages/appkit/src/core/agent/from-plugin.ts", "packages/appkit/src/core/agent/load-agents.ts", "packages/appkit/src/connectors/mcp/index.ts", "template/**", diff --git a/packages/appkit/src/beta.ts b/packages/appkit/src/beta.ts index 9295a43ed..724f5df12 100644 --- a/packages/appkit/src/beta.ts +++ b/packages/appkit/src/beta.ts @@ -53,19 +53,20 @@ export type { AgentsPluginConfig, AgentTool, AgentTools, + AgentToolsFn, AutoInheritToolsConfig, BaseSystemPromptOption, - FromPluginMarker, + Plugins, + PluginToolkitProvider, PromptContext, RegisteredAgent, + RegisteredPlugins, ResolvedToolEntry, ToolkitEntry, ToolkitOptions, } from "./plugins/agents"; export { agentIdFromMarkdownPath, - fromPlugin, - isFromPluginMarker, isToolkitEntry, loadAgentFromFile, loadAgentsFromDir, diff --git a/packages/appkit/src/core/agent/from-plugin.ts b/packages/appkit/src/core/agent/from-plugin.ts deleted file mode 100644 index b11285941..000000000 --- a/packages/appkit/src/core/agent/from-plugin.ts +++ /dev/null @@ -1,97 +0,0 @@ -import type { NamedPluginFactory } from "../../plugin/to-plugin"; -import type { ToolkitOptions } from "./types"; - -/** - * Symbol brand for the `fromPlugin` marker. Using a globally-interned symbol - * (`Symbol.for`) keeps the brand stable across module boundaries / bundle - * duplicates so `isFromPluginMarker` stays reliable. - */ -export const FROM_PLUGIN_MARKER = Symbol.for( - "@databricks/appkit.fromPluginMarker", -); - -/** - * A lazy reference to a plugin's tools, produced by {@link fromPlugin} and - * resolved to concrete `ToolkitEntry`s at `AgentsPlugin.setup()` time. - * - * The marker is spread under a unique symbol key so multiple calls to - * `fromPlugin` (even for the same plugin) coexist in an `AgentDefinition.tools` - * record without colliding. - */ -export interface FromPluginMarker { - readonly [FROM_PLUGIN_MARKER]: true; - readonly pluginName: string; - readonly opts: ToolkitOptions | undefined; -} - -/** - * Record shape returned by {@link fromPlugin} — a single symbol-keyed entry - * suitable for spreading into `AgentDefinition.tools`. - */ -export type FromPluginSpread = { readonly [key: symbol]: FromPluginMarker }; - -/** - * Reference a plugin's tools inside an `AgentDefinition.tools` record without - * naming the plugin instance. The returned spread-friendly object carries a - * symbol-keyed marker that the agents plugin resolves against registered - * `ToolProvider`s at setup time. - * - * The factory argument must come from `toPlugin` (or any function that - * carries a `pluginName` field). `fromPlugin` reads `factory.pluginName` - * synchronously — it does not construct an instance. - * - * If the referenced plugin is also registered in `createApp({ plugins })`, the - * same runtime instance is used for dispatch. If the plugin is missing, - * `AgentsPlugin.setup()` throws with a clear `Available: …` listing. - * - * @example - * ```ts - * import { analytics, createAgent, files, fromPlugin, tool } from "@databricks/appkit"; - * - * const support = createAgent({ - * instructions: "You help customers.", - * tools: { - * ...fromPlugin(analytics), - * ...fromPlugin(files, { only: ["uploads.read"] }), - * get_weather: tool({ ... }), - * }, - * }); - * ``` - * - * @param factory A plugin factory produced by `toPlugin`. Must expose a - * `pluginName` field. - * @param opts Optional toolkit scoping — `prefix`, `only`, `except`, `rename`. - * Same shape as the `.toolkit()` method. - */ -export function fromPlugin( - factory: F, - opts?: ToolkitOptions, -): FromPluginSpread { - if ( - !factory || - typeof factory.pluginName !== "string" || - !factory.pluginName - ) { - throw new Error( - "fromPlugin(): factory is missing pluginName. Pass a factory created by toPlugin().", - ); - } - const pluginName = factory.pluginName; - const marker: FromPluginMarker = { - [FROM_PLUGIN_MARKER]: true, - pluginName, - opts, - }; - return { [Symbol(`fromPlugin:${pluginName}`)]: marker }; -} - -/** - * Type guard for {@link FromPluginMarker}. - */ -export function isFromPluginMarker(value: unknown): value is FromPluginMarker { - return ( - typeof value === "object" && - value !== null && - (value as Record)[FROM_PLUGIN_MARKER] === true - ); -} diff --git a/packages/appkit/src/core/agent/index.ts b/packages/appkit/src/core/agent/index.ts index 96fb3eb8c..c346b5b44 100644 --- a/packages/appkit/src/core/agent/index.ts +++ b/packages/appkit/src/core/agent/index.ts @@ -8,13 +8,6 @@ export { buildToolkitEntries } from "./build-toolkit"; export { consumeAdapterStream } from "./consume-adapter-stream"; export { createAgent } from "./create-agent"; -export { - FROM_PLUGIN_MARKER, - type FromPluginMarker, - type FromPluginSpread, - fromPlugin, - isFromPluginMarker, -} from "./from-plugin"; export { agentIdFromMarkdownPath, type LoadContext, @@ -52,11 +45,15 @@ export { type AgentsPluginConfig, type AgentTool, type AgentTools, + type AgentToolsFn, type AutoInheritToolsConfig, type BaseSystemPromptOption, isToolkitEntry, + type Plugins, + type PluginToolkitProvider, type PromptContext, type RegisteredAgent, + type RegisteredPlugins, type ResolvedToolEntry, type ToolkitEntry, type ToolkitOptions, diff --git a/packages/appkit/src/core/agent/run-agent.ts b/packages/appkit/src/core/agent/run-agent.ts index 9118c025f..cec2e9410 100644 --- a/packages/appkit/src/core/agent/run-agent.ts +++ b/packages/appkit/src/core/agent/run-agent.ts @@ -9,7 +9,6 @@ import type { ToolProvider, } from "shared"; import { consumeAdapterStream } from "./consume-adapter-stream"; -import { isFromPluginMarker } from "./from-plugin"; import { resolveToolkitFromProvider } from "./toolkit-resolver"; import { type FunctionTool, @@ -17,7 +16,13 @@ import { isFunctionTool, } from "./tools/function-tool"; import { isHostedTool } from "./tools/hosted-tools"; -import type { AgentDefinition, AgentTool, ToolkitEntry } from "./types"; +import type { + AgentDefinition, + AgentTool, + AgentTools, + Plugins, + PluginToolkitProvider, +} from "./types"; import { isToolkitEntry } from "./types"; export interface RunAgentInput { @@ -26,11 +31,11 @@ export interface RunAgentInput { /** Abort signal for cancellation. */ signal?: AbortSignal; /** - * Optional plugin list used to resolve `fromPlugin` markers in `def.tools`. - * Required when the def contains any `...fromPlugin(factory)` spreads; - * ignored otherwise. `runAgent` constructs a fresh instance per plugin - * and dispatches tool calls against it as the service principal (no - * OBO — there is no HTTP request in standalone mode). + * Optional plugin list. Required when `def.tools` is the function form + * `(plugins) => Record` and the function dereferences + * any plugins. `runAgent` constructs a fresh instance per plugin and + * dispatches tool calls against it as the service principal (no OBO — + * there is no HTTP request in standalone mode). */ plugins?: PluginData[]; } @@ -53,8 +58,9 @@ export interface RunAgentResult { * that only exists inside the agents plugin. * - Sub-agents (`agents: { ... }` on the def) are executed as nested * `runAgent` calls with no shared thread state. - * - Plugin tools (`fromPlugin` markers or `ToolkitEntry` spreads) require - * passing `plugins: [...]` via `RunAgentInput`. + * - Plugin tools (used inside the function form via + * `plugins..toolkit(...)`) require passing `plugins: [...]` via + * `RunAgentInput`. */ export async function runAgent( def: AgentDefinition, @@ -186,51 +192,21 @@ type StandaloneEntry = }; /** - * Resolves `def.tools` (string-keyed entries + symbol-keyed `fromPlugin` - * markers) and `def.agents` (sub-agents) into a flat dispatch index. - * Symbol-keyed markers are resolved against `plugins`; missing references - * throw with an `Available: …` listing. + * Resolves `def.tools` (object or function form) and `def.agents` + * (sub-agents) into a flat dispatch index. The function form is invoked + * once per call against a {@link Plugins} map built lazily from + * `input.plugins`; missing references throw with an `Available: …` listing. */ function buildStandaloneToolIndex( def: AgentDefinition, plugins: PluginData[], ): Map { const index = new Map(); - const tools = def.tools; - - const symbolKeys = tools ? Object.getOwnPropertySymbols(tools) : []; - if (symbolKeys.length > 0) { - const providerCache = new Map(); - for (const sym of symbolKeys) { - const marker = (tools as Record)[sym]; - if (!isFromPluginMarker(marker)) continue; + const providerCache = new Map(); + const tools = resolveDefTools(def, plugins, providerCache); - const provider = resolveStandaloneProvider( - marker.pluginName, - plugins, - providerCache, - ); - const entries = resolveToolkitFromProvider( - marker.pluginName, - provider, - marker.opts, - ); - for (const [key, entry] of Object.entries(entries)) { - index.set(key, { - kind: "toolkit", - provider, - pluginName: entry.pluginName, - localName: entry.localName, - def: { ...entry.def, name: key }, - }); - } - } - } - - if (tools) { - for (const [key, tool] of Object.entries(tools)) { - index.set(key, classifyTool(key, tool)); - } + for (const [key, tool] of Object.entries(tools)) { + index.set(key, classifyTool(key, tool, providerCache)); } for (const [childKey, child] of Object.entries(def.agents ?? {})) { @@ -260,9 +236,78 @@ function buildStandaloneToolIndex( return index; } -function classifyTool(key: string, tool: AgentTool): StandaloneEntry { +/** + * Resolves `def.tools` to a plain record. The function form is invoked + * with a typed {@link Plugins} map built lazily over `plugins`; each + * `plugins.foo.toolkit(opts)` call constructs the underlying provider + * (cached) on first access. + */ +function resolveDefTools( + def: AgentDefinition, + plugins: PluginData[], + providerCache: Map, +): AgentTools { + if (typeof def.tools !== "function") { + return def.tools ?? {}; + } + const pluginsMap = buildStandalonePluginsMap(plugins, providerCache); + try { + return def.tools(pluginsMap); + } catch (err) { + const name = def.name ?? ""; + throw new Error( + `runAgent: agent '${name}' tools(plugins) callback threw: ${ + err instanceof Error ? err.message : String(err) + }`, + { cause: err instanceof Error ? err : undefined }, + ); + } +} + +/** + * Builds the typed {@link Plugins} map passed to the function form of + * `def.tools` in standalone mode. Plugin instances are constructed lazily + * (only when the user actually calls `plugins.foo.toolkit(...)`) and + * cached so the same instance is reused for dispatch downstream. + */ +function buildStandalonePluginsMap( + plugins: PluginData[], + providerCache: Map, +): Plugins { + const out: Record = {}; + for (const data of plugins) { + out[data.name] = { + toolkit: (opts) => { + const provider = resolveStandaloneProvider( + data.name, + plugins, + providerCache, + ); + return resolveToolkitFromProvider(data.name, provider, opts); + }, + }; + } + return out as Plugins; +} + +function classifyTool( + key: string, + tool: AgentTool, + providerCache: Map, +): StandaloneEntry { if (isToolkitEntry(tool)) { - return toolkitEntryToStandalone(key, tool); + // Toolkit entries inside the function form's returned record carry the + // provider name they came from, so we can resolve the provider on + // demand and dispatch through it. The cache is shared with the + // pluginsMap path so the same instance is reused. + const provider = providerCacheLookup(tool.pluginName, providerCache); + return { + kind: "toolkit", + provider, + pluginName: tool.pluginName, + localName: tool.localName, + def: { ...tool.def, name: key }, + }; } if (isFunctionTool(tool)) { return { @@ -284,29 +329,18 @@ function classifyTool(key: string, tool: AgentTool): StandaloneEntry { throw new Error(`runAgent: unrecognized tool shape at key "${key}"`); } -/** - * Pre-`fromPlugin` code could reach a `ToolkitEntry` by calling - * `.toolkit()` at module scope (which requires an instance). Those entries - * still flow through `def.tools` but without a provider we can dispatch - * against — runAgent cannot execute them and errors clearly. - */ -function toolkitEntryToStandalone( - key: string, - entry: ToolkitEntry, -): StandaloneEntry { - const def: AgentToolDefinition = { ...entry.def, name: key }; - return { - kind: "hosted", - def: { - ...def, - description: - `${def.description ?? ""} ` + - `[runAgent: this ToolkitEntry refers to plugin '${entry.pluginName}' but ` + - "runAgent cannot dispatch it without the plugin instance. Pass the " + - "plugin via plugins: [...] and use fromPlugin(factory) instead of " + - ".toolkit() spreads.]".trim(), - }, - }; +function providerCacheLookup( + pluginName: string, + cache: Map, +): ToolProvider { + const cached = cache.get(pluginName); + if (cached) return cached; + throw new Error( + `runAgent: tool refers to plugin '${pluginName}' but its instance was ` + + "not initialised by tools(plugins). This usually means the function " + + "form returned a stale ToolkitEntry without going through " + + "plugins[...].toolkit().", + ); } /** @@ -339,8 +373,8 @@ function resolveStandaloneProvider( if (!match) { const available = plugins.map((p) => p.name).join(", ") || "(none)"; throw new Error( - `runAgent: agent references plugin '${pluginName}' via fromPlugin(), but ` + - "that plugin is missing from RunAgentInput.plugins. " + + `runAgent: agent tools(plugins) referenced plugin '${pluginName}', ` + + "but that plugin is missing from RunAgentInput.plugins. " + `Available: ${available}.`, ); } @@ -353,7 +387,7 @@ function resolveStandaloneProvider( throw new Error( `runAgent: plugin '${pluginName}' is not a ToolProvider ` + "(missing getAgentTools/executeAgentTool). Only ToolProvider plugins " + - "are supported via fromPlugin() in runAgent.", + "are supported in standalone runAgent.", ); } cache.set(pluginName, instance); diff --git a/packages/appkit/src/core/agent/tests/create-agent.test.ts b/packages/appkit/src/core/agent/tests/create-agent.test.ts index df920369e..46668e1a0 100644 --- a/packages/appkit/src/core/agent/tests/create-agent.test.ts +++ b/packages/appkit/src/core/agent/tests/create-agent.test.ts @@ -28,7 +28,12 @@ describe("createAgent", () => { tools: { get_weather }, }); - expect(def.tools?.get_weather).toBe(get_weather); + // The object form preserves identity; we narrow with typeof to satisfy + // the dual ToolRecord | AgentToolsFn shape. + expect(typeof def.tools).toBe("object"); + if (typeof def.tools === "object") { + expect(def.tools.get_weather).toBe(get_weather); + } }); test("accepts sub-agents in a keyed record", () => { diff --git a/packages/appkit/src/core/agent/tests/from-plugin.test.ts b/packages/appkit/src/core/agent/tests/from-plugin.test.ts deleted file mode 100644 index eb31d0f7d..000000000 --- a/packages/appkit/src/core/agent/tests/from-plugin.test.ts +++ /dev/null @@ -1,80 +0,0 @@ -import { describe, expect, test } from "vitest"; -import { - FROM_PLUGIN_MARKER, - fromPlugin, - isFromPluginMarker, -} from "../../../core/agent/from-plugin"; - -function fakeFactory(name: string) { - const f = () => ({ name }); - Object.defineProperty(f, "pluginName", { value: name, enumerable: true }); - return f as typeof f & { readonly pluginName: string }; -} - -describe("fromPlugin", () => { - test("returns a spread-friendly object with a single symbol-keyed marker", () => { - const spread = fromPlugin(fakeFactory("analytics")); - - expect(Object.keys(spread)).toHaveLength(0); - const syms = Object.getOwnPropertySymbols(spread); - expect(syms).toHaveLength(1); - - const marker = (spread as Record)[syms[0]!]; - expect(isFromPluginMarker(marker)).toBe(true); - expect((marker as { pluginName: string }).pluginName).toBe("analytics"); - }); - - test("multiple calls produce distinct symbol keys (spreads coexist)", () => { - const spread = { - ...fromPlugin(fakeFactory("analytics")), - ...fromPlugin(fakeFactory("analytics")), - ...fromPlugin(fakeFactory("files")), - }; - - const syms = Object.getOwnPropertySymbols(spread); - expect(syms).toHaveLength(3); - }); - - test("passes opts through to the marker", () => { - const spread = fromPlugin(fakeFactory("analytics"), { - only: ["query"], - prefix: "q_", - }); - const sym = Object.getOwnPropertySymbols(spread)[0]!; - const marker = (spread as Record)[sym] as { - opts: { only: string[]; prefix: string }; - }; - expect(marker.opts.only).toEqual(["query"]); - expect(marker.opts.prefix).toBe("q_"); - }); - - test("throws when factory has no pluginName", () => { - const missing = () => ({ name: "nope" }); - expect(() => - fromPlugin(missing as unknown as { readonly pluginName: string }), - ).toThrow(/missing pluginName/); - }); - - test("FROM_PLUGIN_MARKER is a globally-interned symbol", () => { - expect(FROM_PLUGIN_MARKER).toBe( - Symbol.for("@databricks/appkit.fromPluginMarker"), - ); - }); -}); - -describe("isFromPluginMarker", () => { - test("returns true for real markers", () => { - const spread = fromPlugin(fakeFactory("analytics")); - const sym = Object.getOwnPropertySymbols(spread)[0]!; - expect(isFromPluginMarker((spread as Record)[sym])).toBe( - true, - ); - }); - - test("returns false for objects without the brand", () => { - expect(isFromPluginMarker({ pluginName: "x" })).toBe(false); - expect(isFromPluginMarker(null)).toBe(false); - expect(isFromPluginMarker(undefined)).toBe(false); - expect(isFromPluginMarker("string")).toBe(false); - }); -}); diff --git a/packages/appkit/src/core/agent/tests/run-agent.test.ts b/packages/appkit/src/core/agent/tests/run-agent.test.ts index 7a87a4cec..d59bda0c7 100644 --- a/packages/appkit/src/core/agent/tests/run-agent.test.ts +++ b/packages/appkit/src/core/agent/tests/run-agent.test.ts @@ -11,11 +11,9 @@ import type { import { describe, expect, test, vi } from "vitest"; import { z } from "zod"; import { createAgent } from "../create-agent"; -import { fromPlugin } from "../from-plugin"; import { runAgent } from "../run-agent"; import { mcpServer } from "../tools/hosted-tools"; import { tool } from "../tools/tool"; -import type { ToolkitEntry } from "../types"; function scriptedAdapter(events: AgentEvent[]): AgentAdapter { return { @@ -90,7 +88,7 @@ describe("runAgent", () => { expect(weatherFn).toHaveBeenCalledWith({ city: "NYC" }); }); - test("resolves fromPlugin markers against RunAgentInput.plugins", async () => { + test("function-form tools(plugins) resolves toolkit() against RunAgentInput.plugins", async () => { const pingExec = vi.fn(async () => "pong"); class FakePlugin implements ToolProvider { static manifest = { name: "ping" }; @@ -114,16 +112,6 @@ describe("runAgent", () => { executeAgentTool = pingExec; } - const factory = () => ({ - plugin: FakePlugin as unknown as PluginConstructor, - config: {}, - name: "ping" as const, - }); - Object.defineProperty(factory, "pluginName", { - value: "ping", - enumerable: true, - }); - let capturedCtx: AgentRunContext | null = null; const adapter: AgentAdapter = { async *run(_input, context) { @@ -135,16 +123,16 @@ describe("runAgent", () => { const def = createAgent({ instructions: "x", model: adapter, - tools: { - ...fromPlugin(factory as unknown as { readonly pluginName: string }), + tools(plugins) { + return { ...plugins.ping.toolkit() }; }, }); - const pluginData = factory() as PluginData< - PluginConstructor, - unknown, - string - >; + const pluginData: PluginData = { + plugin: FakePlugin as unknown as PluginConstructor, + config: {}, + name: "ping", + }; await runAgent(def, { messages: "hi", plugins: [pluginData] }); expect(capturedCtx).not.toBeNull(); @@ -154,13 +142,7 @@ describe("runAgent", () => { expect(pingExec).toHaveBeenCalled(); }); - test("throws with guidance when fromPlugin marker has no matching plugin", async () => { - const factory = () => ({ name: "absent" as const }); - Object.defineProperty(factory, "pluginName", { - value: "absent", - enumerable: true, - }); - + test("function-form throws with guidance when a referenced plugin is missing", async () => { const adapter: AgentAdapter = { async *run(_input, _context) { yield { type: "message_delta", content: "" }; @@ -170,55 +152,18 @@ describe("runAgent", () => { const def = createAgent({ instructions: "x", model: adapter, - tools: { - ...fromPlugin(factory as unknown as { readonly pluginName: string }), + tools(plugins) { + return { ...plugins.absent.toolkit() }; }, }); - await expect(runAgent(def, { messages: "hi" })).rejects.toThrow(/absent/); + // No plugins passed → plugins.absent is undefined → toolkit() call throws. await expect(runAgent(def, { messages: "hi" })).rejects.toThrow( - /Available:/, + /tools\(plugins\) callback threw/, ); }); - test("throws a clear error when a ToolkitEntry is invoked", async () => { - const toolkitEntry: ToolkitEntry = { - __toolkitRef: true, - pluginName: "analytics", - localName: "query", - def: { - name: "analytics.query", - description: "SQL", - parameters: { type: "object", properties: {} }, - }, - }; - - let capturedCtx: AgentRunContext | null = null; - const adapter: AgentAdapter = { - async *run(_input, context) { - capturedCtx = context; - yield { type: "message_delta", content: "" }; - }, - }; - - const def = createAgent({ - instructions: "x", - model: adapter, - tools: { "analytics.query": toolkitEntry }, - }); - - await runAgent(def, { messages: "hi" }); - expect(capturedCtx).not.toBeNull(); - await expect( - // biome-ignore lint/style/noNonNullAssertion: asserted above - capturedCtx!.executeTool("analytics.query", {}), - ).rejects.toThrow(/only usable via createApp/); - }); - - test("rejects fromPlugin against a plugin lacking ToolProvider methods", async () => { - // Pre-condition for #305 review finding #7: a plain plugin (no - // getAgentTools / executeAgentTool) referenced via fromPlugin must - // surface a clear error at standalone resolution, not at dispatch. + test("function-form rejects a plugin lacking ToolProvider methods", async () => { class NotAToolProvider { static manifest = { name: "noop" }; static DEFAULT_CONFIG = {}; @@ -231,20 +176,8 @@ describe("runAgent", () => { } } - const factory = () => ({ - plugin: NotAToolProvider as unknown as PluginConstructor, - config: {}, - name: "noop" as const, - }); - Object.defineProperty(factory, "pluginName", { - value: "noop", - enumerable: true, - }); - const adapter: AgentAdapter = { - async *run(_input, context) { - // Force resolution by attempting a dispatch. - await context.executeTool("noop.anything", {}).catch(() => undefined); + async *run(_input, _context) { yield { type: "message_delta", content: "" }; }, }; @@ -252,16 +185,16 @@ describe("runAgent", () => { const def = createAgent({ instructions: "x", model: adapter, - tools: { - ...fromPlugin(factory as unknown as { readonly pluginName: string }), + tools(plugins) { + return { ...plugins.noop.toolkit() }; }, }); - const pluginData = factory() as PluginData< - PluginConstructor, - unknown, - string - >; + const pluginData: PluginData = { + plugin: NotAToolProvider as unknown as PluginConstructor, + config: {}, + name: "noop", + }; await expect( runAgent(def, { messages: "hi", plugins: [pluginData] }), @@ -323,4 +256,20 @@ describe("runAgent", () => { capturedCtx!.executeTool("agent-helper", { input: "say hi" }); expect(result).toBe("child says hi"); }); + + test("function-form invoked exactly once per runAgent call", async () => { + const toolsFn = vi.fn(() => ({})); + const adapter: AgentAdapter = { + async *run(_input, _context) { + yield { type: "message_delta", content: "" }; + }, + }; + const def = createAgent({ + instructions: "x", + model: adapter, + tools: toolsFn, + }); + await runAgent(def, { messages: "hi" }); + expect(toolsFn).toHaveBeenCalledTimes(1); + }); }); diff --git a/packages/appkit/src/core/agent/toolkit-resolver.ts b/packages/appkit/src/core/agent/toolkit-resolver.ts index d49921269..97d1d9b30 100644 --- a/packages/appkit/src/core/agent/toolkit-resolver.ts +++ b/packages/appkit/src/core/agent/toolkit-resolver.ts @@ -26,7 +26,7 @@ type MaybeToolkitProvider = ToolProvider & { * * This helper is the single source of truth for "turn a provider into a * toolkit entry record" and is used by `AgentsPlugin.buildToolIndex` - * (both the `fromPlugin` resolution pass and auto-inherit) and by the + * (the `tools(plugins)` resolution pass and auto-inherit) and by the * standalone `runAgent` executor. */ export function resolveToolkitFromProvider( diff --git a/packages/appkit/src/core/agent/tools/define-tool.ts b/packages/appkit/src/core/agent/tools/define-tool.ts index dc269ba61..b51792f4c 100644 --- a/packages/appkit/src/core/agent/tools/define-tool.ts +++ b/packages/appkit/src/core/agent/tools/define-tool.ts @@ -20,7 +20,8 @@ export interface ToolEntry { * consider it safe enough to appear in every agent's tool record without an * explicit `tools:` declaration. Destructive or privilege-sensitive tools * should leave this unset so that they only reach agents that wire them - * explicitly (via `tools:`, `toolkits:`, or `fromPlugin({ only: [...] })`). + * explicitly (via `tools:` object/function form, markdown `toolkits:`, or + * `plugins..toolkit({ only: [...] })`). */ autoInheritable?: boolean; handler: ( diff --git a/packages/appkit/src/core/agent/types.ts b/packages/appkit/src/core/agent/types.ts index baa15967f..0e5b6fcef 100644 --- a/packages/appkit/src/core/agent/types.ts +++ b/packages/appkit/src/core/agent/types.ts @@ -6,7 +6,6 @@ import type { ToolAnnotations, } from "shared"; import type { McpHostPolicyConfig } from "../../connectors/mcp"; -import type { FromPluginMarker } from "./from-plugin"; import type { FunctionTool } from "./tools/function-tool"; import type { HostedTool } from "./tools/hosted-tools"; @@ -49,6 +48,66 @@ export interface ToolkitOptions { rename?: Record; } +/** + * Minimum shape every entry in the {@link Plugins} map must expose. Core + * plugins (analytics, files, genie, lakebase) implement this directly via + * their `.toolkit()` method. The agents plugin and standalone `runAgent` + * synthesize this shape for any registered plugin that doesn't implement + * `.toolkit()` directly (falling back to `getAgentTools()` walking). + */ +export interface PluginToolkitProvider { + toolkit(opts?: ToolkitOptions): Record; +} + +/** + * Module-augmentation interface. Core plugins extend this from their + * declaration site so the function form of `AgentDefinition.tools` + * autocompletes both available plugin keys and the methods on each + * plugin (notably `.toolkit()`). + * + * Third-party plugins may extend this interface to participate in the + * typed surface: + * + * ```ts + * declare module "@databricks/appkit" { + * interface RegisteredPlugins { + * myPlugin: MyPlugin; + * } + * } + * ``` + * + * Plugins not registered here still work at runtime (they appear under + * the index-signature fallback as {@link PluginToolkitProvider}), they + * just don't get keyed autocomplete. + */ +// biome-ignore lint/suspicious/noEmptyInterface: intentional augmentation seed +export interface RegisteredPlugins {} + +/** + * Plugin map passed to the function form of {@link AgentDefinition.tools}. + * Known names (extended via {@link RegisteredPlugins}) keep their concrete + * plugin class type; unknown names fall back to {@link PluginToolkitProvider}. + * + * @example + * ```ts + * const support = createAgent({ + * instructions: "...", + * tools(plugins) { + * return { + * get_weather: tool({ ... }), + * ...plugins.analytics.toolkit(), + * ...plugins.files.toolkit({ only: ["uploads.read"] }), + * }; + * }, + * }); + * ``` + */ +export type Plugins = { + readonly [K in keyof RegisteredPlugins]: RegisteredPlugins[K]; +} & { + readonly [key: string]: PluginToolkitProvider; +}; + /** * Context passed to `baseSystemPrompt` callbacks. */ @@ -65,13 +124,20 @@ export type BaseSystemPromptOption = /** * Per-agent tool record. String keys map to inline tools, toolkit entries, - * hosted tools, etc. Symbol keys hold `FromPluginMarker` references produced - * by `fromPlugin(factory)` spreads — these are resolved at - * `AgentsPlugin.setup()` time against registered `ToolProvider` plugins. + * hosted tools, etc. */ -export type AgentTools = { [key: string]: AgentTool } & { - [key: symbol]: FromPluginMarker; -}; +export type AgentTools = Record; + +/** + * Function form of `AgentDefinition.tools`. Receives the typed + * {@link Plugins} map and returns a tool record. Invoked exactly once at + * setup (or once per `runAgent` call in standalone mode); the result is + * cached as the agent's resolved tool record. + * + * Use the function form when an agent needs tools from registered plugins. + * The bare object form is fine when an agent only uses inline tools. + */ +export type AgentToolsFn = (plugins: Plugins) => AgentTools; export interface AgentDefinition { /** Filled in from the enclosing key when used in `agents: { foo: def }`. */ @@ -84,8 +150,18 @@ export interface AgentDefinition { * falls back to the plugin's `defaultModel`. */ model?: AgentAdapter | Promise | string; - /** Per-agent tool record. Key is the LLM-visible tool-call name. */ - tools?: AgentTools; + /** + * Per-agent tool record. Key is the LLM-visible tool-call name. + * + * Accepts either a plain record (for agents that only use inline tools) + * or a function `(plugins) => Record` that receives + * the typed {@link Plugins} map and returns a tool record (for agents + * that pull tools from registered plugins). + * + * The function is invoked once at agent setup; the result is cached. + * Don't put per-request logic in there. + */ + tools?: AgentTools | AgentToolsFn; /** Sub-agents, exposed as `agent-` tools on this agent. */ agents?: Record; /** Override the plugin's baseSystemPrompt for this agent only. */ @@ -108,7 +184,8 @@ export interface AgentDefinition { * with no explicit `tools:` declaration receive every registered ToolProvider * plugin tool whose author marked `autoInheritable: true`. Tools without that * flag — destructive, state-mutating, or privilege-sensitive — never spread - * automatically and must be wired via `tools:`, `toolkits:`, or `fromPlugin`. + * automatically and must be wired via `tools:` (object or function form) or + * markdown `toolkits:`. * * Defaults are `false` for both origins (safe-by-default): developers must * consciously opt an origin in to any auto-inherit behaviour. diff --git a/packages/appkit/src/plugin/index.ts b/packages/appkit/src/plugin/index.ts index 46a4eb94f..937652196 100644 --- a/packages/appkit/src/plugin/index.ts +++ b/packages/appkit/src/plugin/index.ts @@ -1,4 +1,4 @@ export type { ToPlugin } from "shared"; export type { ExecutionResult } from "./execution-result"; export { Plugin } from "./plugin"; -export { type NamedPluginFactory, toPlugin } from "./to-plugin"; +export { toPlugin } from "./to-plugin"; diff --git a/packages/appkit/src/plugin/to-plugin.ts b/packages/appkit/src/plugin/to-plugin.ts index c882f3000..785d96b9a 100644 --- a/packages/appkit/src/plugin/to-plugin.ts +++ b/packages/appkit/src/plugin/to-plugin.ts @@ -1,27 +1,15 @@ import type { PluginConstructor, PluginData, ToPlugin } from "shared"; -/** - * Factory function produced by {@link toPlugin}. Carries a static - * `pluginName` field so tooling (e.g. `fromPlugin`) can identify which - * plugin a factory references without constructing an instance. - */ -export type NamedPluginFactory = { - readonly pluginName: Name; -}; - /** * Wraps a plugin class so it can be passed to `createApp` with optional * config. Infers the config type from the constructor and the plugin name - * from the static `manifest.name` property, and stamps `pluginName` onto - * the returned factory function so `fromPlugin` can identify the plugin - * without needing to construct it. + * from the static `manifest.name` property. * * @internal */ export function toPlugin( plugin: T, -): ToPlugin[0], T["manifest"]["name"]> & - NamedPluginFactory { +): ToPlugin[0], T["manifest"]["name"]> { type Config = ConstructorParameters[0]; type Name = T["manifest"]["name"]; const pluginName = plugin.manifest.name as Name; @@ -32,10 +20,5 @@ export function toPlugin( config: config as Config, name: pluginName, }); - Object.defineProperty(factory, "pluginName", { - value: pluginName, - writable: false, - enumerable: true, - }); - return factory as ToPlugin & NamedPluginFactory; + return factory as ToPlugin; } diff --git a/packages/appkit/src/plugins/agents/agents.ts b/packages/appkit/src/plugins/agents/agents.ts index c80e6149c..ebd2cb12a 100644 --- a/packages/appkit/src/plugins/agents/agents.ts +++ b/packages/appkit/src/plugins/agents/agents.ts @@ -18,7 +18,6 @@ import type { import { AppKitMcpClient, buildMcpHostPolicy } from "../../connectors/mcp"; import { getWorkspaceClient } from "../../context"; import { consumeAdapterStream } from "../../core/agent/consume-adapter-stream"; -import { isFromPluginMarker } from "../../core/agent/from-plugin"; import { loadAgentsFromDir } from "../../core/agent/load-agents"; import { normalizeToolResult } from "../../core/agent/normalize-result"; import { @@ -35,7 +34,10 @@ import { import type { AgentDefinition, AgentsPluginConfig, + AgentTools, BaseSystemPromptOption, + Plugins, + PluginToolkitProvider, PromptContext, RegisteredAgent, ResolvedToolEntry, @@ -411,17 +413,16 @@ export class AgentsPlugin extends Plugin implements ToolProvider { src: AgentSource, ): Promise> { const index = new Map(); - const toolsRecord = def.tools ?? {}; - const hasExplicitTools = - def.tools !== undefined && - (Object.keys(toolsRecord).length > 0 || - Object.getOwnPropertySymbols(toolsRecord).length > 0); + const hasDeclaredTools = def.tools !== undefined; + const toolsRecord = this.resolveDefTools(agentName, def); const hasExplicitSubAgents = def.agents && Object.keys(def.agents).length > 0; const inheritDefaults = normalizeAutoInherit(this.config.autoInheritTools); + // Declaring `tools` (object or function, even an empty record) opts out + // of auto-inherit. Same rule for both forms — see plan decision (E1/I1). const shouldInherit = - !hasExplicitTools && + !hasDeclaredTools && !hasExplicitSubAgents && (src.origin === "file" ? inheritDefaults.file : inheritDefaults.code); @@ -454,11 +455,7 @@ export class AgentsPlugin extends Plugin implements ToolProvider { }); } - // 2. fromPlugin markers — resolve against registered ToolProviders first so - // explicit string-keyed tools can still overwrite on the same key. - this.resolveFromPluginMarkers(agentName, toolsRecord, index); - - // 3. Explicit tools (toolkit entries, function tools, hosted tools) + // 2. Explicit tools (toolkit entries, function tools, hosted tools) const hostedToCollect: import("../../core/agent/tools/hosted-tools").HostedTool[] = []; for (const [key, tool] of Object.entries(toolsRecord)) { @@ -495,6 +492,56 @@ export class AgentsPlugin extends Plugin implements ToolProvider { return index; } + /** + * Resolves an `AgentDefinition.tools` field to a plain tool record. The + * function form is invoked exactly once at agent setup with the typed + * {@link Plugins} map; the result replaces the function reference for the + * remainder of the registered agent's lifetime. + * + * Plain object form is returned as-is; an undefined `tools` returns an + * empty record. The function form is wrapped in a try/catch so a thrown + * callback fails registration with a useful message instead of leaking + * the raw stack. + */ + private resolveDefTools(agentName: string, def: AgentDefinition): AgentTools { + if (typeof def.tools !== "function") { + return def.tools ?? {}; + } + try { + return def.tools(this.buildPluginsMap()); + } catch (err) { + throw new Error( + `Agent '${agentName}': tools(plugins) callback threw: ${ + err instanceof Error ? err.message : String(err) + }`, + { cause: err instanceof Error ? err : undefined }, + ); + } + } + + /** + * Builds the typed {@link Plugins} map passed to the function form of + * `AgentDefinition.tools`. Each entry exposes the plugin instance directly + * (so user code can call typed instance methods including `.toolkit()`); + * plugins missing `.toolkit()` get a synthesized fallback that walks + * `getAgentTools()` via `resolveToolkitFromProvider`. + */ + private buildPluginsMap(): Plugins { + const out: Record = {}; + if (!this.context) return out as Plugins; + for (const { name, provider } of this.context.getToolProviders()) { + const direct = (provider as { toolkit?: unknown }).toolkit; + if (typeof direct === "function") { + out[name] = provider as unknown as PluginToolkitProvider; + } else { + out[name] = { + toolkit: (opts) => resolveToolkitFromProvider(name, provider, opts), + }; + } + } + return out as Plugins; + } + private async applyAutoInherit( agentName: string, index: Map, @@ -553,51 +600,6 @@ export class AgentsPlugin extends Plugin implements ToolProvider { } } - /** - * Walks the symbol-keyed `fromPlugin` markers in an agent's `tools` record - * and resolves each one against a registered `ToolProvider`. Throws with a - * helpful `Available: …` listing if a referenced plugin isn't registered. - */ - private resolveFromPluginMarkers( - agentName: string, - toolsRecord: Record, - index: Map, - ): void { - const symbolKeys = Object.getOwnPropertySymbols(toolsRecord); - if (symbolKeys.length === 0) return; - - const providers = this.context?.getToolProviders() ?? []; - - for (const sym of symbolKeys) { - const marker = (toolsRecord as Record)[sym]; - if (!isFromPluginMarker(marker)) continue; - - const providerEntry = providers.find((p) => p.name === marker.pluginName); - if (!providerEntry) { - const available = providers.map((p) => p.name).join(", ") || "(none)"; - throw new Error( - `Agent '${agentName}' references plugin '${marker.pluginName}' via ` + - `fromPlugin(), but that plugin is not registered in createApp. ` + - `Available: ${available}.`, - ); - } - - const entries = resolveToolkitFromProvider( - marker.pluginName, - providerEntry.provider, - marker.opts, - ); - for (const [key, entry] of Object.entries(entries)) { - index.set(key, { - source: "toolkit", - pluginName: entry.pluginName, - localName: entry.localName, - def: { ...entry.def, name: key }, - }); - } - } - } - private async connectHostedTools( hostedTools: import("../../core/agent/tools/hosted-tools").HostedTool[], index: Map, diff --git a/packages/appkit/src/plugins/agents/index.ts b/packages/appkit/src/plugins/agents/index.ts index f630cc681..96202c174 100644 --- a/packages/appkit/src/plugins/agents/index.ts +++ b/packages/appkit/src/plugins/agents/index.ts @@ -2,13 +2,6 @@ // the public package barrel (`@databricks/appkit`) and any callers that // already imported via `./plugins/agents` continue to resolve unchanged. export { buildToolkitEntries } from "../../core/agent/build-toolkit"; -export { - FROM_PLUGIN_MARKER, - type FromPluginMarker, - type FromPluginSpread, - fromPlugin, - isFromPluginMarker, -} from "../../core/agent/from-plugin"; export { agentIdFromMarkdownPath, type LoadContext, @@ -22,11 +15,15 @@ export { type AgentsPluginConfig, type AgentTool, type AgentTools, + type AgentToolsFn, type AutoInheritToolsConfig, type BaseSystemPromptOption, isToolkitEntry, + type Plugins, + type PluginToolkitProvider, type PromptContext, type RegisteredAgent, + type RegisteredPlugins, type ResolvedToolEntry, type ToolkitEntry, type ToolkitOptions, diff --git a/packages/appkit/src/plugins/agents/tests/agents-plugin.test.ts b/packages/appkit/src/plugins/agents/tests/agents-plugin.test.ts index 42cb6b127..da8dd6bd6 100644 --- a/packages/appkit/src/plugins/agents/tests/agents-plugin.test.ts +++ b/packages/appkit/src/plugins/agents/tests/agents-plugin.test.ts @@ -12,7 +12,6 @@ import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; import { z } from "zod"; import { CacheManager } from "../../../cache"; import { buildToolkitEntries } from "../../../core/agent/build-toolkit"; -import { fromPlugin } from "../../../core/agent/from-plugin"; import { defineTool, type ToolRegistry, @@ -26,12 +25,6 @@ import { isToolkitEntry } from "../../../core/agent/types"; // Import the class directly so we can construct it without a createApp import { AgentsPlugin } from "../agents"; -function namedFactory(name: string) { - const f = () => ({ name }); - Object.defineProperty(f, "pluginName", { value: name, enumerable: true }); - return f as typeof f & { readonly pluginName: string }; -} - interface FakeContext { providers: Array<{ name: string; provider: ToolProvider }>; getToolProviders(): Array<{ name: string; provider: ToolProvider }>; @@ -379,8 +372,8 @@ describe("AgentsPlugin", () => { expect(isToolkitEntry(null)).toBe(false); }); - describe("fromPlugin markers", () => { - test("spreading fromPlugin registers all tools from the referenced plugin", async () => { + describe("function-form tools(plugins)", () => { + test("function form spreads tools from plugins..toolkit()", async () => { const registry: ToolRegistry = { query: defineTool({ description: "q", @@ -402,7 +395,9 @@ describe("AgentsPlugin", () => { support: { instructions: "...", model: stubAdapter(), - tools: { ...fromPlugin(namedFactory("analytics")) }, + tools(plugins) { + return { ...plugins.analytics.toolkit() }; + }, }, }, }, @@ -417,7 +412,7 @@ describe("AgentsPlugin", () => { expect(agent?.toolIndex.has("analytics.query")).toBe(true); }); - test("mixed inline + fromPlugin tools coexist", async () => { + test("mixed inline tools + plugin toolkit() coexist in the function form", async () => { const registry: ToolRegistry = { query: defineTool({ description: "q", @@ -439,14 +434,16 @@ describe("AgentsPlugin", () => { support: { instructions: "...", model: stubAdapter(), - tools: { - ...fromPlugin(namedFactory("analytics")), - get_weather: tool({ - name: "get_weather", - description: "Weather", - schema: z.object({ city: z.string() }), - execute: async ({ city }) => `Sunny in ${city}`, - }), + tools(plugins) { + return { + ...plugins.analytics.toolkit(), + get_weather: tool({ + name: "get_weather", + description: "Weather", + schema: z.object({ city: z.string() }), + execute: async ({ city }) => `Sunny in ${city}`, + }), + }; }, }, }, @@ -463,14 +460,8 @@ describe("AgentsPlugin", () => { expect(agent?.toolIndex.has("get_weather")).toBe(true); }); - test("missing plugin throws at setup with Available: listing", async () => { - const ctx = fakeContext([ - { - name: "files", - provider: makeToolProvider("files", {}), - }, - ]); - + test("function-form callback that throws fails registration with a clear message", async () => { + const ctx = fakeContext([]); const plugin = instantiate( { dir: false, @@ -478,18 +469,22 @@ describe("AgentsPlugin", () => { support: { instructions: "...", model: stubAdapter(), - tools: { ...fromPlugin(namedFactory("analytics")) }, + tools(plugins) { + // Calling .toolkit() on a missing plugin throws because + // plugins.analytics is undefined under the index signature. + return { ...plugins.analytics.toolkit() }; + }, }, }, }, ctx, ); - await expect(plugin.setup()).rejects.toThrow(/analytics/); - await expect(plugin.setup()).rejects.toThrow(/Available:/); - await expect(plugin.setup()).rejects.toThrow(/files/); + await expect(plugin.setup()).rejects.toThrow( + /tools\(plugins\) callback threw/, + ); }); - test("symbol-only tools record disables auto-inherit", async () => { + test("function form opts out of auto-inherit even when other plugins are autoInheritable", async () => { const analyticsReg: ToolRegistry = { query: defineTool({ description: "q", @@ -501,6 +496,7 @@ describe("AgentsPlugin", () => { list: defineTool({ description: "l", schema: z.object({}), + autoInheritable: true, handler: () => [], }), }; @@ -523,7 +519,9 @@ describe("AgentsPlugin", () => { support: { instructions: "...", model: stubAdapter(), - tools: { ...fromPlugin(namedFactory("analytics")) }, + tools(plugins) { + return { ...plugins.analytics.toolkit() }; + }, }, }, }, @@ -537,10 +535,53 @@ describe("AgentsPlugin", () => { const agent = api.get("support"); const toolNames = Array.from(agent?.toolIndex.keys() ?? []); expect(toolNames.some((n) => n.startsWith("analytics."))).toBe(true); + // files is autoInheritable but the function form opted us out expect(toolNames.some((n) => n.startsWith("files."))).toBe(false); }); - test("falls back to getAgentTools() for providers without toolkit()", async () => { + test("empty function-form record still opts out of auto-inherit", async () => { + const filesReg: ToolRegistry = { + list: defineTool({ + description: "l", + schema: z.object({}), + autoInheritable: true, + handler: () => [], + }), + }; + const ctx = fakeContext([ + { + name: "files", + provider: makeToolProvider("files", filesReg), + }, + ]); + + const plugin = instantiate( + { + dir: false, + autoInheritTools: { code: true }, + agents: { + support: { + instructions: "...", + model: stubAdapter(), + tools(_plugins) { + return {}; + }, + }, + }, + }, + ctx, + ); + await plugin.setup(); + + const api = plugin.exports() as { + get: (name: string) => { toolIndex: Map } | null; + }; + const agent = api.get("support"); + // Nothing inherited even though files.list is autoInheritable. + expect(agent?.toolIndex.size).toBe(0); + }); + + test("function form falls back to getAgentTools() for providers without toolkit()", async () => { // Provider lacks .toolkit() — only getAgentTools/executeAgentTool. const bareProvider: ToolProvider = { getAgentTools: () => [ @@ -561,7 +602,9 @@ describe("AgentsPlugin", () => { support: { instructions: "...", model: stubAdapter(), - tools: { ...fromPlugin(namedFactory("bare")) }, + tools(plugins) { + return { ...plugins.bare.toolkit() }; + }, }, }, }, @@ -575,5 +618,25 @@ describe("AgentsPlugin", () => { const agent = api.get("support"); expect(agent?.toolIndex.has("bare.ping")).toBe(true); }); + + test("function form runs exactly once at setup", async () => { + const ctx = fakeContext([]); + const toolsFn = vi.fn(() => ({})); + const plugin = instantiate( + { + dir: false, + agents: { + support: { + instructions: "...", + model: stubAdapter(), + tools: toolsFn, + }, + }, + }, + ctx, + ); + await plugin.setup(); + expect(toolsFn).toHaveBeenCalledTimes(1); + }); }); }); diff --git a/packages/appkit/src/plugins/analytics/analytics.ts b/packages/appkit/src/plugins/analytics/analytics.ts index a16d6303a..e8f6212d0 100644 --- a/packages/appkit/src/plugins/analytics/analytics.ts +++ b/packages/appkit/src/plugins/analytics/analytics.ts @@ -314,8 +314,8 @@ export class AnalyticsPlugin extends Plugin implements ToolProvider { * Returns the plugin's tools as a keyed record of `ToolkitEntry` markers. * Called by the agents plugin (via `resolveToolkitFromProvider`) to spread * a filtered, renamed view of the plugin's tools into an agent's tool - * index. Most callers should go through `fromPlugin(analytics, opts)` at - * module scope instead of reaching for this directly. + * index. Inside the function form of `AgentDefinition.tools`, callers + * reach this method via `plugins.analytics.toolkit(opts)`. */ toolkit(opts?: import("../../core/agent/types").ToolkitOptions) { return buildToolkitEntries(this.name, this.tools, opts); @@ -339,3 +339,9 @@ export class AnalyticsPlugin extends Plugin implements ToolProvider { * @internal */ export const analytics = toPlugin(AnalyticsPlugin); + +declare module "../../core/agent/types" { + interface RegisteredPlugins { + analytics: AnalyticsPlugin; + } +} diff --git a/packages/appkit/src/plugins/files/plugin.ts b/packages/appkit/src/plugins/files/plugin.ts index 0542f6618..a71c138ea 100644 --- a/packages/appkit/src/plugins/files/plugin.ts +++ b/packages/appkit/src/plugins/files/plugin.ts @@ -1225,3 +1225,9 @@ export class FilesPlugin extends Plugin implements ToolProvider { * @internal */ export const files = Object.assign(toPlugin(FilesPlugin), { policy }); + +declare module "../../core/agent/types" { + interface RegisteredPlugins { + files: FilesPlugin; + } +} diff --git a/packages/appkit/src/plugins/genie/genie.ts b/packages/appkit/src/plugins/genie/genie.ts index 8248b2c4a..fe254d286 100644 --- a/packages/appkit/src/plugins/genie/genie.ts +++ b/packages/appkit/src/plugins/genie/genie.ts @@ -377,3 +377,9 @@ export class GeniePlugin extends Plugin implements ToolProvider { * @internal */ export const genie = toPlugin(GeniePlugin); + +declare module "../../core/agent/types" { + interface RegisteredPlugins { + genie: GeniePlugin; + } +} diff --git a/packages/appkit/src/plugins/lakebase/lakebase.ts b/packages/appkit/src/plugins/lakebase/lakebase.ts index 36c64c5ae..33527653d 100644 --- a/packages/appkit/src/plugins/lakebase/lakebase.ts +++ b/packages/appkit/src/plugins/lakebase/lakebase.ts @@ -225,3 +225,9 @@ export class LakebasePlugin extends Plugin implements ToolProvider { * @internal */ export const lakebase = toPlugin(LakebasePlugin); + +declare module "../../core/agent/types" { + interface RegisteredPlugins { + lakebase: LakebasePlugin; + } +} From a146c2514767b8b05ff8f077ecef4fb61873cb48 Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Fri, 8 May 2026 15:43:12 +0200 Subject: [PATCH 09/12] fix(appkit): narrow plugins map to PluginToolkitProvider, drop per-plugin augmentation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups on the tools(plugins) function form: 1. Each entry in the Plugins map is now typed as PluginToolkitProvider (just the .toolkit() method) instead of the full plugin class. Inside tools(plugins), plugins.analytics. shows only .toolkit() — the contract for tool composition — instead of leaking query, name, setup, injectRoutes, and every other instance method. 2. RegisteredPlugins ships pre-populated with the four core plugin keys (analytics, files, genie, lakebase) directly in core/agent/types.ts. The per-plugin "declare module ../../core/agent/types" blocks at the bottom of each core plugin file are gone. Adding a new core plugin now means adding one line to RegisteredPlugins, not duplicating an augmentation block. Third-party plugins still augment RegisteredPlugins from user code if they want their key in autocomplete; the index-signature fallback keeps unaugmented names working at runtime. Signed-off-by: MarioCadenas --- packages/appkit/src/core/agent/types.ts | 41 ++++++++++++------- .../appkit/src/plugins/analytics/analytics.ts | 6 --- packages/appkit/src/plugins/files/plugin.ts | 6 --- packages/appkit/src/plugins/genie/genie.ts | 6 --- .../appkit/src/plugins/lakebase/lakebase.ts | 6 --- 5 files changed, 26 insertions(+), 39 deletions(-) diff --git a/packages/appkit/src/core/agent/types.ts b/packages/appkit/src/core/agent/types.ts index 0e5b6fcef..4c7393e85 100644 --- a/packages/appkit/src/core/agent/types.ts +++ b/packages/appkit/src/core/agent/types.ts @@ -60,33 +60,44 @@ export interface PluginToolkitProvider { } /** - * Module-augmentation interface. Core plugins extend this from their - * declaration site so the function form of `AgentDefinition.tools` - * autocompletes both available plugin keys and the methods on each - * plugin (notably `.toolkit()`). + * Registry of plugin names that participate in keyed autocomplete inside + * the function form of `AgentDefinition.tools`. Each entry is typed as the + * minimal {@link PluginToolkitProvider} shape — the only surface guaranteed + * available inside `tools(plugins)`. Other plugin instance methods are + * intentionally hidden; `tools(plugins)` is for tool composition, not for + * reaching into a plugin's runtime API. * - * Third-party plugins may extend this interface to participate in the - * typed surface: + * Core AppKit plugins are listed here directly (no per-plugin module + * augmentation needed). Third-party plugins can opt into autocomplete by + * augmenting this interface from user code: * * ```ts - * declare module "@databricks/appkit" { + * declare module "@databricks/appkit/beta" { * interface RegisteredPlugins { - * myPlugin: MyPlugin; + * myPlugin: PluginToolkitProvider; * } * } * ``` * - * Plugins not registered here still work at runtime (they appear under - * the index-signature fallback as {@link PluginToolkitProvider}), they - * just don't get keyed autocomplete. + * Augmentation is purely a discoverability win — unknown plugin names + * still resolve at runtime via the index-signature fallback in + * {@link Plugins}. */ -// biome-ignore lint/suspicious/noEmptyInterface: intentional augmentation seed -export interface RegisteredPlugins {} +export interface RegisteredPlugins { + analytics: PluginToolkitProvider; + files: PluginToolkitProvider; + genie: PluginToolkitProvider; + lakebase: PluginToolkitProvider; +} /** * Plugin map passed to the function form of {@link AgentDefinition.tools}. - * Known names (extended via {@link RegisteredPlugins}) keep their concrete - * plugin class type; unknown names fall back to {@link PluginToolkitProvider}. + * Each entry exposes a `.toolkit(opts?)` method that returns a record of + * {@link ToolkitEntry} markers ready to be spread into a tool record. + * + * Plugins not declared in {@link RegisteredPlugins} still work at runtime + * via the index-signature fallback; they just don't appear in keyed + * autocomplete. * * @example * ```ts diff --git a/packages/appkit/src/plugins/analytics/analytics.ts b/packages/appkit/src/plugins/analytics/analytics.ts index e8f6212d0..fdcb16b43 100644 --- a/packages/appkit/src/plugins/analytics/analytics.ts +++ b/packages/appkit/src/plugins/analytics/analytics.ts @@ -339,9 +339,3 @@ export class AnalyticsPlugin extends Plugin implements ToolProvider { * @internal */ export const analytics = toPlugin(AnalyticsPlugin); - -declare module "../../core/agent/types" { - interface RegisteredPlugins { - analytics: AnalyticsPlugin; - } -} diff --git a/packages/appkit/src/plugins/files/plugin.ts b/packages/appkit/src/plugins/files/plugin.ts index a71c138ea..0542f6618 100644 --- a/packages/appkit/src/plugins/files/plugin.ts +++ b/packages/appkit/src/plugins/files/plugin.ts @@ -1225,9 +1225,3 @@ export class FilesPlugin extends Plugin implements ToolProvider { * @internal */ export const files = Object.assign(toPlugin(FilesPlugin), { policy }); - -declare module "../../core/agent/types" { - interface RegisteredPlugins { - files: FilesPlugin; - } -} diff --git a/packages/appkit/src/plugins/genie/genie.ts b/packages/appkit/src/plugins/genie/genie.ts index fe254d286..8248b2c4a 100644 --- a/packages/appkit/src/plugins/genie/genie.ts +++ b/packages/appkit/src/plugins/genie/genie.ts @@ -377,9 +377,3 @@ export class GeniePlugin extends Plugin implements ToolProvider { * @internal */ export const genie = toPlugin(GeniePlugin); - -declare module "../../core/agent/types" { - interface RegisteredPlugins { - genie: GeniePlugin; - } -} diff --git a/packages/appkit/src/plugins/lakebase/lakebase.ts b/packages/appkit/src/plugins/lakebase/lakebase.ts index 33527653d..36c64c5ae 100644 --- a/packages/appkit/src/plugins/lakebase/lakebase.ts +++ b/packages/appkit/src/plugins/lakebase/lakebase.ts @@ -225,9 +225,3 @@ export class LakebasePlugin extends Plugin implements ToolProvider { * @internal */ export const lakebase = toPlugin(LakebasePlugin); - -declare module "../../core/agent/types" { - interface RegisteredPlugins { - lakebase: LakebasePlugin; - } -} From 3d9006c2c06a15c178e1e9ace61a0f09dd8d05ca Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Fri, 8 May 2026 15:49:17 +0200 Subject: [PATCH 10/12] fix(appkit): drop RegisteredPlugins, type Plugins as plain string-keyed record MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hardcoding the registered plugin names in core/agent/types.ts was the wrong shape — AppKit cannot statically know which plugins the surrounding createApp call will register, so a curated list there was guaranteed to drift. Drop the RegisteredPlugins augmentation interface entirely. Plugins is now Record: every entry exposes .toolkit() at runtime, but plugin names do not autocomplete inside tools(plugins). Users refer to plugins by the same name they pass to createApp({ plugins: [...] }). Plugin-name autocomplete (so tools(plugins) sees plugins. -> analytics | files | ...) is a separate, larger design problem with real tradeoffs across inline-in-createApp, factory pattern, codegen, and user augmentation. Tracked separately; this commit unblocks the v5 stack with a runtime-correct, honest type that does not lie about what AppKit knows. Signed-off-by: MarioCadenas --- packages/appkit/src/beta.ts | 1 - packages/appkit/src/core/agent/index.ts | 1 - packages/appkit/src/core/agent/types.ts | 44 +++------------------ packages/appkit/src/plugins/agents/index.ts | 1 - 4 files changed, 5 insertions(+), 42 deletions(-) diff --git a/packages/appkit/src/beta.ts b/packages/appkit/src/beta.ts index 724f5df12..3726ca178 100644 --- a/packages/appkit/src/beta.ts +++ b/packages/appkit/src/beta.ts @@ -60,7 +60,6 @@ export type { PluginToolkitProvider, PromptContext, RegisteredAgent, - RegisteredPlugins, ResolvedToolEntry, ToolkitEntry, ToolkitOptions, diff --git a/packages/appkit/src/core/agent/index.ts b/packages/appkit/src/core/agent/index.ts index c346b5b44..e35d483d4 100644 --- a/packages/appkit/src/core/agent/index.ts +++ b/packages/appkit/src/core/agent/index.ts @@ -53,7 +53,6 @@ export { type PluginToolkitProvider, type PromptContext, type RegisteredAgent, - type RegisteredPlugins, type ResolvedToolEntry, type ToolkitEntry, type ToolkitOptions, diff --git a/packages/appkit/src/core/agent/types.ts b/packages/appkit/src/core/agent/types.ts index 4c7393e85..4ff7d31ed 100644 --- a/packages/appkit/src/core/agent/types.ts +++ b/packages/appkit/src/core/agent/types.ts @@ -59,45 +59,15 @@ export interface PluginToolkitProvider { toolkit(opts?: ToolkitOptions): Record; } -/** - * Registry of plugin names that participate in keyed autocomplete inside - * the function form of `AgentDefinition.tools`. Each entry is typed as the - * minimal {@link PluginToolkitProvider} shape — the only surface guaranteed - * available inside `tools(plugins)`. Other plugin instance methods are - * intentionally hidden; `tools(plugins)` is for tool composition, not for - * reaching into a plugin's runtime API. - * - * Core AppKit plugins are listed here directly (no per-plugin module - * augmentation needed). Third-party plugins can opt into autocomplete by - * augmenting this interface from user code: - * - * ```ts - * declare module "@databricks/appkit/beta" { - * interface RegisteredPlugins { - * myPlugin: PluginToolkitProvider; - * } - * } - * ``` - * - * Augmentation is purely a discoverability win — unknown plugin names - * still resolve at runtime via the index-signature fallback in - * {@link Plugins}. - */ -export interface RegisteredPlugins { - analytics: PluginToolkitProvider; - files: PluginToolkitProvider; - genie: PluginToolkitProvider; - lakebase: PluginToolkitProvider; -} - /** * Plugin map passed to the function form of {@link AgentDefinition.tools}. * Each entry exposes a `.toolkit(opts?)` method that returns a record of * {@link ToolkitEntry} markers ready to be spread into a tool record. * - * Plugins not declared in {@link RegisteredPlugins} still work at runtime - * via the index-signature fallback; they just don't appear in keyed - * autocomplete. + * AppKit does not statically know which plugins the surrounding + * `createApp` will register, so this is a plain string-keyed record. + * Refer to plugins by the name used in `createApp({ plugins: [...] })`; + * unknown names resolve to `undefined` at runtime. * * @example * ```ts @@ -113,11 +83,7 @@ export interface RegisteredPlugins { * }); * ``` */ -export type Plugins = { - readonly [K in keyof RegisteredPlugins]: RegisteredPlugins[K]; -} & { - readonly [key: string]: PluginToolkitProvider; -}; +export type Plugins = Record; /** * Context passed to `baseSystemPrompt` callbacks. diff --git a/packages/appkit/src/plugins/agents/index.ts b/packages/appkit/src/plugins/agents/index.ts index 96202c174..869333afb 100644 --- a/packages/appkit/src/plugins/agents/index.ts +++ b/packages/appkit/src/plugins/agents/index.ts @@ -23,7 +23,6 @@ export { type PluginToolkitProvider, type PromptContext, type RegisteredAgent, - type RegisteredPlugins, type ResolvedToolEntry, type ToolkitEntry, type ToolkitOptions, From ad78095aa32e9588944d9ea04c16eb2fe6e3734f Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Fri, 8 May 2026 16:31:34 +0200 Subject: [PATCH 11/12] fix(appkit): make countUserStreams O(1) via per-user counter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit countUserStreams() previously walked every entry in activeStreams on every /chat and /invocations request — O(n) over total concurrent streams across all users on the hot path. At scale that scan dominates chat-request latency under load. Add userStreamCounts: Map kept in sync with activeStreams via two new helpers: - trackStream(requestId, userId, controller) — sole writer that registers a stream and increments the user's counter. - untrackStream(requestId) — sole writer that removes a stream and decrements the counter; deletes the user's entry on the last stream to keep the map bounded across many distinct users; idempotent on unknown ids. countUserStreams() is now an O(1) Map.get(). All three previous mutation sites (_streamAgent setup, _streamAgent finally, _handleCancel) go through the helpers, so the counter cannot drift from the map. Tests: existing dos-limits seeders updated to use trackStream(); new test in dos-limits.test.ts asserts the invariant across track/untrack for multiple users plus idempotent untrack. Signed-off-by: MarioCadenas --- packages/appkit/src/plugins/agents/agents.ts | 54 +++++++++++++++--- .../plugins/agents/tests/dos-limits.test.ts | 55 +++++++++++++------ 2 files changed, 85 insertions(+), 24 deletions(-) diff --git a/packages/appkit/src/plugins/agents/agents.ts b/packages/appkit/src/plugins/agents/agents.ts index ebd2cb12a..0f841f22b 100644 --- a/packages/appkit/src/plugins/agents/agents.ts +++ b/packages/appkit/src/plugins/agents/agents.ts @@ -139,6 +139,13 @@ export class AgentsPlugin extends Plugin implements ToolProvider { string, { controller: AbortController; userId: string } >(); + /** + * Per-user stream count, kept in sync with `activeStreams` so the + * concurrent-stream rate limit check is O(1) instead of O(n) over every + * active stream on every request. Mutated only via {@link trackStream} + * and {@link untrackStream}. + */ + private userStreamCounts = new Map(); private mcpClient: AppKitMcpClient | null = null; private threadStore; private approvalGate = new ToolApprovalGate(); @@ -195,13 +202,44 @@ export class AgentsPlugin extends Plugin implements ToolProvider { }; } - /** Count active streams owned by a given user. */ + /** Count active streams owned by a given user. O(1). */ private countUserStreams(userId: string): number { - let n = 0; - for (const entry of this.activeStreams.values()) { - if (entry.userId === userId) n++; + return this.userStreamCounts.get(userId) ?? 0; + } + + /** + * Register a stream for `userId` and bump the per-user counter. Paired + * with {@link untrackStream}; the two helpers are the only writers to + * `activeStreams` + `userStreamCounts`, so the counter cannot drift from + * the map. + */ + private trackStream( + requestId: string, + userId: string, + controller: AbortController, + ): void { + this.activeStreams.set(requestId, { controller, userId }); + this.userStreamCounts.set( + userId, + (this.userStreamCounts.get(userId) ?? 0) + 1, + ); + } + + /** + * Remove a stream from the active map and decrement the per-user + * counter. Idempotent — calling twice for the same `requestId` is a + * no-op (the second call sees no entry and returns early). + */ + private untrackStream(requestId: string): void { + const entry = this.activeStreams.get(requestId); + if (!entry) return; + this.activeStreams.delete(requestId); + const next = (this.userStreamCounts.get(entry.userId) ?? 1) - 1; + if (next <= 0) { + this.userStreamCounts.delete(entry.userId); + } else { + this.userStreamCounts.set(entry.userId, next); } - return n; } async setup() { @@ -875,7 +913,7 @@ export class AgentsPlugin extends Plugin implements ToolProvider { const abortController = new AbortController(); const signal = abortController.signal; const requestId = randomUUID(); - this.activeStreams.set(requestId, { controller: abortController, userId }); + this.trackStream(requestId, userId, abortController); const tools = Array.from(registered.toolIndex.values()).map((e) => e.def); const approvalPolicy = this.resolvedApprovalPolicy; @@ -985,7 +1023,7 @@ export class AgentsPlugin extends Plugin implements ToolProvider { // Any pending approval gates for this stream are auto-denied so the // adapter can unwind if it was still waiting. this.approvalGate.abortStream(requestId); - this.activeStreams.delete(requestId); + this.untrackStream(requestId); // Stateless agents (e.g. autocomplete) don't persist history; drop // the thread so `InMemoryThreadStore` doesn't accumulate one record // per request. Swallow delete errors — the stream has already @@ -1246,7 +1284,7 @@ export class AgentsPlugin extends Plugin implements ToolProvider { return; } entry.controller.abort("Cancelled by user"); - this.activeStreams.delete(streamId); + this.untrackStream(streamId); this.approvalGate.abortStream(streamId); res.json({ cancelled: true }); } diff --git a/packages/appkit/src/plugins/agents/tests/dos-limits.test.ts b/packages/appkit/src/plugins/agents/tests/dos-limits.test.ts index 25700396f..e2bbcbe99 100644 --- a/packages/appkit/src/plugins/agents/tests/dos-limits.test.ts +++ b/packages/appkit/src/plugins/agents/tests/dos-limits.test.ts @@ -149,10 +149,7 @@ describe("POST /chat — per-user concurrent-stream limit", () => { const plugin = seedPlugin(); for (let i = 0; i < 5; i++) { // biome-ignore lint/suspicious/noExplicitAny: seeding - (plugin as any).activeStreams.set(`s${i}`, { - controller: new AbortController(), - userId: "alice", - }); + (plugin as any).trackStream(`s${i}`, "alice", new AbortController()); } const { res, setHeader, json } = mockRes(); @@ -175,10 +172,7 @@ describe("POST /chat — per-user concurrent-stream limit", () => { const plugin = seedPlugin(); for (let i = 0; i < 5; i++) { // biome-ignore lint/suspicious/noExplicitAny: seeding - (plugin as any).activeStreams.set(`s${i}`, { - controller: new AbortController(), - userId: "alice", - }); + (plugin as any).trackStream(`s${i}`, "alice", new AbortController()); } // Carol's request must not see a 429 even though alice is at-limit. @@ -204,10 +198,7 @@ describe("POST /chat — per-user concurrent-stream limit", () => { }); for (let i = 0; i < 2; i++) { // biome-ignore lint/suspicious/noExplicitAny: seeding - (plugin as any).activeStreams.set(`s${i}`, { - controller: new AbortController(), - userId: "alice", - }); + (plugin as any).trackStream(`s${i}`, "alice", new AbortController()); } const { res } = mockRes(); @@ -220,16 +211,48 @@ describe("POST /chat — per-user concurrent-stream limit", () => { expect(res.status).toHaveBeenCalledWith(429); }); + test("trackStream/untrackStream keep userStreamCounts in sync with activeStreams (O(1) counter)", async () => { + // Regression for the agentic review finding: countUserStreams() used + // to walk every active stream on every chat request (O(n) over total + // concurrent streams). The new path keeps a per-user counter that + // must mirror the underlying map across track/untrack and across + // multiple users. + const plugin = seedPlugin(); + // biome-ignore lint/suspicious/noExplicitAny: private state probe + const t = plugin as any; + + expect(t.countUserStreams("alice")).toBe(0); + + t.trackStream("s1", "alice", new AbortController()); + t.trackStream("s2", "alice", new AbortController()); + t.trackStream("s3", "bob", new AbortController()); + + expect(t.countUserStreams("alice")).toBe(2); + expect(t.countUserStreams("bob")).toBe(1); + expect(t.activeStreams.size).toBe(3); + + t.untrackStream("s1"); + expect(t.countUserStreams("alice")).toBe(1); + expect(t.activeStreams.has("s1")).toBe(false); + + // Untrack the last stream for a user → counter map drops the key + // entirely (avoids unbounded growth across many distinct users). + t.untrackStream("s3"); + expect(t.countUserStreams("bob")).toBe(0); + expect(t.userStreamCounts.has("bob")).toBe(false); + + // Idempotent — untracking a missing stream is a no-op. + t.untrackStream("s1"); + expect(t.countUserStreams("alice")).toBe(1); + }); + test("/invocations also honours maxConcurrentStreamsPerUser (no bypass)", async () => { // Regression for the agentic review finding: /invocations skipped the // rate-limit gate that /chat enforces, letting clients bypass the cap. const plugin = seedPlugin(); for (let i = 0; i < 5; i++) { // biome-ignore lint/suspicious/noExplicitAny: seeding - (plugin as any).activeStreams.set(`s${i}`, { - controller: new AbortController(), - userId: "alice", - }); + (plugin as any).trackStream(`s${i}`, "alice", new AbortController()); } const { res, setHeader, json } = mockRes(); From 11157fd67e3bdf073813bb6807d7e121ed5a9ff0 Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Fri, 8 May 2026 17:08:53 +0200 Subject: [PATCH 12/12] fix(appkit): apply xavier panel review feedback (4 medium) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address all four medium findings from the xavier review panel (correctness + security + performance) on PR #305. 1. applyToolkitOptions no longer returns literal "undefined" The rename branch used Object.hasOwn(rename, name) followed by an unguarded rename[name] return. When a developer wrote `rename: { query: featureFlag ? "x" : undefined }`, the explicit undefined satisfied hasOwn() and propagated as the toolkit key, producing a tool keyed literally "undefined" downstream. Drop hasOwn, use nullish coalescing + empty-string guard so explicit undefined and "" both fall through to the prefix path. Adds a new toolkit-options.test.ts covering all four knobs (prefix, only, except, rename) plus the regression case. 2. runAgent shares providerCache across sub-agent recursion Sub-agent tool dispatch used to call runAgent recursively, which built a fresh providerCache inside buildStandaloneToolIndex on every nested call. Plugins were re-instantiated per sub-agent invocation, in-instance state diverged between parent and child, and any per-call setup cost (pool open, client construction) ran for every nested level. Hoist the cache to the top-level runAgent and thread it through a private runAgentInternal() helper so all nested calls share the same instance map. 3. Plugin lookup names the missing plugin (proxy) The Plugins type is Record without noUncheckedIndexedAccess workspace-wide, so unknown keys typecheck as present but resolve to undefined at runtime. Accessing .toolkit() on undefined produced a generic "Cannot read properties of undefined (reading 'toolkit')" with no plugin name and no list of available plugins. New core/agent/plugins-map.ts exports createPluginsProxy() — wraps the resolved record so unknown string-key access throws a named error: " referenced plugin '', but it is not registered. Available: ...". Used by both standalone runAgent and the agents plugin's buildPluginsMap. Symbol access and well-known probes (then, toJSON, toString, valueOf, constructor) pass through untouched so Promise/JSON tooling doesn't trip the guard. 4. Standalone runAgent eagerly initialises plugin lifecycle resolveStandaloneProvider used to construct plugin instances lazily on first plugins[name].toolkit() call and never invoke attachContext()/setup(). First-party plugins like AnalyticsPlugin that depend on createApp's runtime (WorkspaceClient, ServiceContext, PluginContext) crashed mid-stream when their tools dereferenced getWorkspaceClient(), with stack traces far from the cause. The previous JSDoc only hinted with "(when they work at all)". New initStandalonePlugins() helper runs at the top of runAgent: constructs every plugin in input.plugins, calls attachContext({}), awaits setup(), and populates the shared cache. Failures wrap with a clear message naming the plugin and pointing the caller at createApp({ plugins: [..., agents(...)] }). Plugins with the default no-op setup() initialise cleanly; plugins that need createApp-only runtime fail at startup, not mid-conversation. The runAgent JSDoc is rewritten to spell out the trust boundary (no OBO, no approval gate, treat as trusted-prompt environment) and the new init contract. Tests: + 8 toolkit-options unit tests, + 3 run-agent tests covering the new behaviours (proxy names missing plugin, setup failure surfaces at entry not mid-stream, sub-agent recursion shares plugin instance with parent — verified by counting constructor calls and asserting same instance id from both parent and child tools). 2284 tests passing across the workspace. Signed-off-by: MarioCadenas --- packages/appkit/src/core/agent/plugins-map.ts | 67 ++++++ packages/appkit/src/core/agent/run-agent.ts | 202 ++++++++++++------ .../src/core/agent/tests/run-agent.test.ts | 162 ++++++++++++++ .../core/agent/tests/toolkit-options.test.ts | 63 ++++++ .../appkit/src/core/agent/toolkit-options.ts | 8 +- packages/appkit/src/plugins/agents/agents.ts | 12 +- 6 files changed, 445 insertions(+), 69 deletions(-) create mode 100644 packages/appkit/src/core/agent/plugins-map.ts create mode 100644 packages/appkit/src/core/agent/tests/toolkit-options.test.ts diff --git a/packages/appkit/src/core/agent/plugins-map.ts b/packages/appkit/src/core/agent/plugins-map.ts new file mode 100644 index 000000000..181b2f7df --- /dev/null +++ b/packages/appkit/src/core/agent/plugins-map.ts @@ -0,0 +1,67 @@ +import type { Plugins, PluginToolkitProvider } from "./types"; + +/** + * Wrap a plain `Record` so that accessing an + * unknown plugin name throws with a named, actionable error instead of the + * default `TypeError: Cannot read properties of undefined (reading 'toolkit')` + * that surfaces from chained access on a missing key. + * + * The {@link Plugins} type is a `Record` + * without `noUncheckedIndexedAccess` workspace-wide, so unknown keys type as + * present at compile time but resolve to `undefined` at runtime. The proxy + * closes that gap with a runtime error that names the missing plugin and + * lists what's available — same shape as the agents plugin's pre-existing + * "plugin is not registered" errors. + * + * @param entries - the resolved plugin map; the proxy serves these directly + * for known keys and throws for any other string key. + * @param contextLabel - prefix included in the error message; differentiates + * the runtime context (e.g. `"runAgent: tools(plugins)"` vs + * `"AgentsPlugin: tools(plugins)"`) so users know which path to debug. + */ +export function createPluginsProxy( + entries: Record, + contextLabel: string, +): Plugins { + return new Proxy(entries, { + get(target, prop, receiver) { + // Symbols and well-known string accessors (e.g. `Symbol.toPrimitive`, + // `then` checked by Promise.resolve, `toJSON`) must pass through so + // host code that probes the object doesn't hit the missing-plugin + // error. Only named string-key access that misses is treated as a + // user error. + if (typeof prop !== "string") { + return Reflect.get(target, prop, receiver); + } + if (prop in target) { + return Reflect.get(target, prop, receiver); + } + // Probes from runtime tooling (e.g. utility inspection, JSON + // serialization, async-iterator detection) hit common property names + // that are clearly not plugin lookups. Pass those through silently. + if ( + prop === "then" || + prop === "toJSON" || + prop === "constructor" || + prop === "toString" || + prop === "valueOf" + ) { + return undefined; + } + const available = Object.keys(target).join(", ") || "(none)"; + throw new Error( + `${contextLabel} referenced plugin '${prop}', but it is not registered. ` + + `Available: ${available}.`, + ); + }, + has(target, prop) { + return Reflect.has(target, prop); + }, + ownKeys(target) { + return Reflect.ownKeys(target); + }, + getOwnPropertyDescriptor(target, prop) { + return Reflect.getOwnPropertyDescriptor(target, prop); + }, + }) as Plugins; +} diff --git a/packages/appkit/src/core/agent/run-agent.ts b/packages/appkit/src/core/agent/run-agent.ts index cec2e9410..4dd4401ac 100644 --- a/packages/appkit/src/core/agent/run-agent.ts +++ b/packages/appkit/src/core/agent/run-agent.ts @@ -9,6 +9,7 @@ import type { ToolProvider, } from "shared"; import { consumeAdapterStream } from "./consume-adapter-stream"; +import { createPluginsProxy } from "./plugins-map"; import { resolveToolkitFromProvider } from "./toolkit-resolver"; import { type FunctionTool, @@ -52,23 +53,55 @@ export interface RunAgentResult { * inline tools, and drives the adapter's `run()` loop to completion. * * Limitations vs. running through the agents() plugin: - * - No OBO: there is no HTTP request, so plugin tools run as the service - * principal (when they work at all). - * - Hosted tools (MCP) are not supported — they require a live MCP client - * that only exists inside the agents plugin. - * - Sub-agents (`agents: { ... }` on the def) are executed as nested - * `runAgent` calls with no shared thread state. - * - Plugin tools (used inside the function form via + * - **No OBO and no approval gate** — there is no HTTP request, so plugin + * tools run as the service principal. The agents-plugin approval gate + * that prompts for human confirmation on `effect: "write" | "update" | + * "destructive"` tools is also absent. LLM-controlled tool arguments + * flow straight through to the SP. Treat standalone runAgent as a + * trusted-prompt environment (CI, batch eval, internal scripts) — not + * as an exposed user-facing surface. + * - **Hosted tools (MCP) are not supported** — they require a live MCP + * client that only exists inside the agents plugin's lifecycle. + * `runAgent` rejects them at index-build time with a clear error. + * - **Sub-agents** (`agents: { ... }` on the def) are executed as nested + * `runAgent` calls with no shared thread state. Plugin instances ARE + * shared across the recursion (same cache as the parent). + * - **Plugin tools** (used inside the function form via * `plugins..toolkit(...)`) require passing `plugins: [...]` via - * `RunAgentInput`. + * `RunAgentInput`. Each plugin in that array is constructed once, + * `attachContext({})` and `await setup()` are called eagerly, and the + * resulting instance is shared across the top-level run and all + * sub-agent recursions. Plugins whose `setup()` requires runtime that + * only `createApp` provides (e.g. `WorkspaceClient`, `ServiceContext`, + * `PluginContext`) throw at standalone-init time with a clear "use + * createApp instead" message — not mid-stream. */ export async function runAgent( def: AgentDefinition, input: RunAgentInput, +): Promise { + // Single shared cache for the whole call graph: parent + every nested + // sub-agent dispatch share constructed plugin instances. Without this, + // each nested `runAgent` would build its own cache, re-instantiate every + // plugin, and silently diverge in-instance state between parent and child + // (e.g. query result caches, connection pools). + const providerCache = new Map(); + await initStandalonePlugins(input.plugins ?? [], providerCache); + return runAgentInternal(def, input, providerCache); +} + +async function runAgentInternal( + def: AgentDefinition, + input: RunAgentInput, + providerCache: Map, ): Promise { const adapter = await resolveAdapter(def); const messages = normalizeMessages(input.messages, def.instructions); - const toolIndex = buildStandaloneToolIndex(def, input.plugins ?? []); + const toolIndex = buildStandaloneToolIndex( + def, + input.plugins ?? [], + providerCache, + ); const tools = Array.from(toolIndex.values()).map((e) => e.def); const signal = input.signal; @@ -97,7 +130,13 @@ export async function runAgent( signal, plugins: input.plugins, }; - const res = await runAgent(entry.agentDef, subInput); + // Reuse the same `providerCache` so sub-agent plugin tools dispatch + // through the same instances the parent constructed. + const res = await runAgentInternal( + entry.agentDef, + subInput, + providerCache, + ); return res.text; } throw new Error( @@ -131,6 +170,73 @@ export async function runAgent( return { text, events }; } +/** + * Eagerly construct every plugin in `input.plugins`, run the standard + * AppKit lifecycle (`attachContext({})` + `await setup()`), and populate + * `cache`. Failures here surface BEFORE any adapter work — mid-stream + * `getWorkspaceClient is not initialised`-style errors become a clear + * startup failure naming the plugin and pointing the user at `createApp`. + * + * Plugins that don't need runtime context (no overridden `setup`, or one + * that doesn't dereference `createApp`-only state) initialise cleanly and + * standalone runAgent works as documented. Plugins like analytics/files + * that depend on `WorkspaceClient` will throw the underlying error wrapped + * with the migration hint. + */ +async function initStandalonePlugins( + plugins: PluginData[], + cache: Map, +): Promise { + for (const data of plugins) { + if (cache.has(data.name)) continue; + const instance = new data.plugin({ + ...(data.config ?? {}), + name: data.name, + }); + if (!isStandaloneToolProvider(instance)) { + throw new Error( + `runAgent: plugin '${data.name}' is not a ToolProvider ` + + "(missing getAgentTools/executeAgentTool). Only ToolProvider plugins " + + "are supported in standalone runAgent.", + ); + } + if ( + typeof (instance as { attachContext?: unknown }).attachContext === + "function" + ) { + try { + ( + instance as { attachContext: (deps: Record) => void } + ).attachContext({}); + } catch (err) { + throw new Error( + `runAgent: plugin '${data.name}' attachContext() failed in ` + + "standalone mode. This plugin probably depends on createApp's " + + "runtime (WorkspaceClient, ServiceContext, PluginContext). Run " + + "via createApp({ plugins: [..., agents(...)] }) instead. " + + `Cause: ${err instanceof Error ? err.message : String(err)}`, + { cause: err instanceof Error ? err : undefined }, + ); + } + } + if (typeof (instance as { setup?: unknown }).setup === "function") { + try { + await (instance as { setup: () => Promise | void }).setup(); + } catch (err) { + throw new Error( + `runAgent: plugin '${data.name}' setup() failed in standalone ` + + "mode. This plugin probably depends on createApp's runtime " + + "(WorkspaceClient, ServiceContext, PluginContext). Run via " + + "createApp({ plugins: [..., agents(...)] }) instead. " + + `Cause: ${err instanceof Error ? err.message : String(err)}`, + { cause: err instanceof Error ? err : undefined }, + ); + } + } + cache.set(data.name, instance); + } +} + async function resolveAdapter(def: AgentDefinition): Promise { const { model } = def; if (!model) { @@ -194,15 +300,16 @@ type StandaloneEntry = /** * Resolves `def.tools` (object or function form) and `def.agents` * (sub-agents) into a flat dispatch index. The function form is invoked - * once per call against a {@link Plugins} map built lazily from - * `input.plugins`; missing references throw with an `Available: …` listing. + * once per call against a {@link Plugins} map drawn from the shared + * `providerCache` populated by {@link initStandalonePlugins}. Missing + * references throw a named "not registered" error via the proxy. */ function buildStandaloneToolIndex( def: AgentDefinition, plugins: PluginData[], + providerCache: Map, ): Map { const index = new Map(); - const providerCache = new Map(); const tools = resolveDefTools(def, plugins, providerCache); for (const [key, tool] of Object.entries(tools)) { @@ -238,9 +345,9 @@ function buildStandaloneToolIndex( /** * Resolves `def.tools` to a plain record. The function form is invoked - * with a typed {@link Plugins} map built lazily over `plugins`; each - * `plugins.foo.toolkit(opts)` call constructs the underlying provider - * (cached) on first access. + * with a typed {@link Plugins} map drawn from the pre-populated + * `providerCache`; each `plugins.foo.toolkit(opts)` lookup hits the cache + * directly (no construction at toolkit-call time). */ function resolveDefTools( def: AgentDefinition, @@ -266,9 +373,11 @@ function resolveDefTools( /** * Builds the typed {@link Plugins} map passed to the function form of - * `def.tools` in standalone mode. Plugin instances are constructed lazily - * (only when the user actually calls `plugins.foo.toolkit(...)`) and - * cached so the same instance is reused for dispatch downstream. + * `def.tools` in standalone mode. Reads pre-constructed instances from + * `providerCache` (populated eagerly by {@link initStandalonePlugins}) + * and wraps the result in a Proxy so unknown plugin names produce a + * named "not registered, Available: ..." error instead of bubbling up a + * generic `TypeError: Cannot read properties of undefined`. */ function buildStandalonePluginsMap( plugins: PluginData[], @@ -276,18 +385,13 @@ function buildStandalonePluginsMap( ): Plugins { const out: Record = {}; for (const data of plugins) { + const provider = providerCache.get(data.name); + if (!provider) continue; // initStandalonePlugins should have set this out[data.name] = { - toolkit: (opts) => { - const provider = resolveStandaloneProvider( - data.name, - plugins, - providerCache, - ); - return resolveToolkitFromProvider(data.name, provider, opts); - }, + toolkit: (opts) => resolveToolkitFromProvider(data.name, provider, opts), }; } - return out as Plugins; + return createPluginsProxy(out, "runAgent: tools(plugins)"); } function classifyTool( @@ -335,11 +439,12 @@ function providerCacheLookup( ): ToolProvider { const cached = cache.get(pluginName); if (cached) return cached; + const available = Array.from(cache.keys()).join(", ") || "(none)"; throw new Error( - `runAgent: tool refers to plugin '${pluginName}' but its instance was ` + - "not initialised by tools(plugins). This usually means the function " + - "form returned a stale ToolkitEntry without going through " + - "plugins[...].toolkit().", + `runAgent: tool refers to plugin '${pluginName}', but no instance was ` + + "initialised for that name. Add it to RunAgentInput.plugins, or — if " + + "this came from a hand-rolled ToolkitEntry — go through " + + `plugins[name].toolkit() instead. Available: ${available}.`, ); } @@ -360,36 +465,3 @@ function isStandaloneToolProvider(value: unknown): value is ToolProvider { typeof obj.executeAgentTool === "function" ); } - -function resolveStandaloneProvider( - pluginName: string, - plugins: PluginData[], - cache: Map, -): ToolProvider { - const cached = cache.get(pluginName); - if (cached) return cached; - - const match = plugins.find((p) => p.name === pluginName); - if (!match) { - const available = plugins.map((p) => p.name).join(", ") || "(none)"; - throw new Error( - `runAgent: agent tools(plugins) referenced plugin '${pluginName}', ` + - "but that plugin is missing from RunAgentInput.plugins. " + - `Available: ${available}.`, - ); - } - - const instance = new match.plugin({ - ...(match.config ?? {}), - name: pluginName, - }); - if (!isStandaloneToolProvider(instance)) { - throw new Error( - `runAgent: plugin '${pluginName}' is not a ToolProvider ` + - "(missing getAgentTools/executeAgentTool). Only ToolProvider plugins " + - "are supported in standalone runAgent.", - ); - } - cache.set(pluginName, instance); - return instance; -} diff --git a/packages/appkit/src/core/agent/tests/run-agent.test.ts b/packages/appkit/src/core/agent/tests/run-agent.test.ts index d59bda0c7..4d60a8c96 100644 --- a/packages/appkit/src/core/agent/tests/run-agent.test.ts +++ b/packages/appkit/src/core/agent/tests/run-agent.test.ts @@ -272,4 +272,166 @@ describe("runAgent", () => { await runAgent(def, { messages: "hi" }); expect(toolsFn).toHaveBeenCalledTimes(1); }); + + test("function-form names the missing plugin in the error (proxy)", async () => { + // Regression: previously `plugins.absent` was undefined, and accessing + // `.toolkit()` on it produced a generic "Cannot read properties of + // undefined" — no plugin name, no list of available names. The proxy + // now throws a clear "not registered. Available: ..." error. + const adapter: AgentAdapter = { + async *run(_input, _context) { + yield { type: "message_delta", content: "" }; + }, + }; + const def = createAgent({ + instructions: "x", + model: adapter, + tools(plugins) { + return { ...plugins.absent.toolkit() }; + }, + }); + await expect(runAgent(def, { messages: "hi" })).rejects.toThrow(/'absent'/); + await expect(runAgent(def, { messages: "hi" })).rejects.toThrow( + /not registered/, + ); + await expect(runAgent(def, { messages: "hi" })).rejects.toThrow( + /Available/, + ); + }); + + test("plugin setup() failure surfaces at runAgent entry, not mid-stream", async () => { + // Regression: previously plugins were constructed lazily during the + // function form's toolkit() call, and setup() was never invoked. That + // pushed any setup-time failure (e.g. `getWorkspaceClient is not + // initialised`) into mid-conversation tool dispatches with confusing + // stack traces. The fix: eagerly attachContext + setup at runAgent + // entry; failures wrap with a "use createApp instead" hint. + class BadSetupPlugin { + static manifest = { name: "bad" }; + static DEFAULT_CONFIG = {}; + name = "bad"; + constructor(public config: unknown) {} + async setup() { + throw new Error("WorkspaceClient not initialised"); + } + injectRoutes() {} + getEndpoints() { + return {}; + } + getAgentTools() { + return []; + } + async executeAgentTool() { + return null; + } + } + const adapter: AgentAdapter = { + async *run(_input, _context) { + // Should never reach the adapter. + yield { type: "message_delta", content: "this should not run" }; + }, + }; + const def = createAgent({ + instructions: "x", + model: adapter, + tools(plugins) { + return { ...plugins.bad.toolkit() }; + }, + }); + const pluginData: PluginData = { + plugin: BadSetupPlugin as unknown as PluginConstructor, + config: {}, + name: "bad", + }; + + await expect( + runAgent(def, { messages: "hi", plugins: [pluginData] }), + ).rejects.toThrow(/setup\(\) failed in standalone mode/); + await expect( + runAgent(def, { messages: "hi", plugins: [pluginData] }), + ).rejects.toThrow(/createApp/); + }); + + test("sub-agent recursion shares the same plugin instance with the parent", async () => { + // Regression: providerCache used to be per-call inside + // buildStandaloneToolIndex, so each nested runAgent constructed fresh + // plugin instances and parent/child diverged in-instance state. + let constructorCount = 0; + class StatefulPlugin { + static manifest = { name: "stateful" }; + static DEFAULT_CONFIG = {}; + name = "stateful"; + readonly id = ++constructorCount; + constructor(public config: unknown) {} + async setup() {} + injectRoutes() {} + getEndpoints() { + return {}; + } + getAgentTools() { + return [ + { + name: "whoami", + description: "return instance id", + parameters: { type: "object", properties: {} }, + }, + ]; + } + async executeAgentTool() { + return String(this.id); + } + } + + const childAdapter: AgentAdapter = { + async *run(_input, context) { + const id = await context.executeTool("stateful.whoami", {}); + yield { type: "message_delta", content: `child-id=${id}` }; + }, + }; + const parentAdapter: AgentAdapter = { + async *run(_input, context) { + const myId = await context.executeTool("stateful.whoami", {}); + const childOut = await context.executeTool("agent-child", { + input: "go", + }); + yield { + type: "message_delta", + content: `parent-id=${myId};${childOut}`, + }; + }, + }; + + const child = createAgent({ + instructions: "child", + model: childAdapter, + tools(plugins) { + return { ...plugins.stateful.toolkit() }; + }, + }); + const parent = createAgent({ + instructions: "parent", + model: parentAdapter, + agents: { child }, + tools(plugins) { + return { ...plugins.stateful.toolkit() }; + }, + }); + + const pluginData: PluginData = { + plugin: StatefulPlugin as unknown as PluginConstructor, + config: {}, + name: "stateful", + }; + + constructorCount = 0; + const result = await runAgent(parent, { + messages: "go", + plugins: [pluginData], + }); + + // Plugin constructed exactly once across parent + child. + expect(constructorCount).toBe(1); + // Both parent and child reported the same instance id. + expect(result.text).toBe("parent-id=1;child-id=1"); + }); }); diff --git a/packages/appkit/src/core/agent/tests/toolkit-options.test.ts b/packages/appkit/src/core/agent/tests/toolkit-options.test.ts new file mode 100644 index 000000000..763aa4f55 --- /dev/null +++ b/packages/appkit/src/core/agent/tests/toolkit-options.test.ts @@ -0,0 +1,63 @@ +import { describe, expect, test } from "vitest"; +import { applyToolkitOptions } from "../toolkit-options"; + +describe("applyToolkitOptions", () => { + test("default prefix is `${pluginName}.`", () => { + expect(applyToolkitOptions("query", "analytics")).toBe("analytics.query"); + }); + + test("empty `prefix` drops the namespace", () => { + expect(applyToolkitOptions("query", "analytics", { prefix: "" })).toBe( + "query", + ); + }); + + test("explicit prefix overrides the default", () => { + expect(applyToolkitOptions("query", "analytics", { prefix: "sql_" })).toBe( + "sql_query", + ); + }); + + test("`only` allowlist filters out unmatched names", () => { + expect( + applyToolkitOptions("destructive", "analytics", { only: ["query"] }), + ).toBeNull(); + expect(applyToolkitOptions("query", "analytics", { only: ["query"] })).toBe( + "analytics.query", + ); + }); + + test("`except` denylist drops matched names", () => { + expect( + applyToolkitOptions("query", "analytics", { except: ["query"] }), + ).toBeNull(); + }); + + test("`rename` wins over the prefix path", () => { + expect( + applyToolkitOptions("query", "analytics", { + rename: { query: "sql_query" }, + }), + ).toBe("sql_query"); + }); + + test("`rename: { name: undefined }` falls through to the prefix path", () => { + // Regression: prior implementation used `Object.hasOwn(rename, name) ? + // rename[name] : ...` which returned `undefined` when the key was + // present-but-undefined (e.g. from a ternary that didn't fire). That + // produced a tool keyed literally `"undefined"` downstream. + expect( + applyToolkitOptions("query", "analytics", { + rename: { query: undefined as unknown as string }, + }), + ).toBe("analytics.query"); + }); + + test("`rename: { name: '' }` falls through (empty string is not a valid key)", () => { + expect( + applyToolkitOptions("query", "analytics", { + rename: { query: "" }, + }), + ).toBe("analytics.query"); + }); +}); diff --git a/packages/appkit/src/core/agent/toolkit-options.ts b/packages/appkit/src/core/agent/toolkit-options.ts index c8b1dedd1..6ac075425 100644 --- a/packages/appkit/src/core/agent/toolkit-options.ts +++ b/packages/appkit/src/core/agent/toolkit-options.ts @@ -21,11 +21,15 @@ export function applyToolkitOptions( pluginName: string, opts: ToolkitOptions = {}, ): string | null { + // `only`/`except` take precedence: filter first, then derive the key. if (opts.only && !opts.only.includes(localName)) return null; if (opts.except?.includes(localName)) return null; - const rename = opts.rename ?? {}; - if (Object.hasOwn(rename, localName)) return rename[localName]; + // `rename` accepts string overrides; explicit `undefined` (e.g. from a + // ternary that didn't fire) and empty strings fall through to the prefix + // path so we never produce a tool keyed literally `"undefined"` or `""`. + const renamed = opts.rename?.[localName]; + if (typeof renamed === "string" && renamed.length > 0) return renamed; const prefix = opts.prefix ?? `${pluginName}.`; return `${prefix}${localName}`; diff --git a/packages/appkit/src/plugins/agents/agents.ts b/packages/appkit/src/plugins/agents/agents.ts index 0f841f22b..3c20d6165 100644 --- a/packages/appkit/src/plugins/agents/agents.ts +++ b/packages/appkit/src/plugins/agents/agents.ts @@ -20,6 +20,7 @@ import { getWorkspaceClient } from "../../context"; import { consumeAdapterStream } from "../../core/agent/consume-adapter-stream"; import { loadAgentsFromDir } from "../../core/agent/load-agents"; import { normalizeToolResult } from "../../core/agent/normalize-result"; +import { createPluginsProxy } from "../../core/agent/plugins-map"; import { buildBaseSystemPrompt, composeSystemPrompt, @@ -563,10 +564,17 @@ export class AgentsPlugin extends Plugin implements ToolProvider { * (so user code can call typed instance methods including `.toolkit()`); * plugins missing `.toolkit()` get a synthesized fallback that walks * `getAgentTools()` via `resolveToolkitFromProvider`. + * + * Wrapped in {@link createPluginsProxy} so that accessing an unknown + * plugin name throws a named "not registered, Available: ..." error + * instead of bubbling up a generic `Cannot read properties of undefined` + * from the agent's `tools(plugins)` callback. */ private buildPluginsMap(): Plugins { const out: Record = {}; - if (!this.context) return out as Plugins; + if (!this.context) { + return createPluginsProxy(out, `Agent '${this.name}': tools(plugins)`); + } for (const { name, provider } of this.context.getToolProviders()) { const direct = (provider as { toolkit?: unknown }).toolkit; if (typeof direct === "function") { @@ -577,7 +585,7 @@ export class AgentsPlugin extends Plugin implements ToolProvider { }; } } - return out as Plugins; + return createPluginsProxy(out, `Agent '${this.name}': tools(plugins)`); } private async applyAutoInherit(