-
Notifications
You must be signed in to change notification settings - Fork 13
feat(appkit): plugin infrastructure — attachContext + PluginContext mediator #303
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
MarioCadenas
wants to merge
1
commit into
agent/v2/2-tool-primitives
Choose a base branch
from
agent/v2/3-plugin-infra
base: agent/v2/2-tool-primitives
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,287 @@ | ||
| import type express from "express"; | ||
| import type { BasePlugin, ToolProvider } from "shared"; | ||
| import { createLogger } from "../logging/logger"; | ||
| import { TelemetryManager } from "../telemetry"; | ||
|
|
||
| const logger = createLogger("plugin-context"); | ||
|
|
||
| interface BufferedRoute { | ||
| method: string; | ||
| path: string; | ||
| handlers: express.RequestHandler[]; | ||
| } | ||
|
|
||
| interface RouteTarget { | ||
| addExtension(fn: (app: express.Application) => void): void; | ||
| } | ||
|
|
||
| interface ToolProviderEntry { | ||
| plugin: BasePlugin & ToolProvider; | ||
| name: string; | ||
| } | ||
|
|
||
| type LifecycleEvent = "setup:complete" | "server:ready" | "shutdown"; | ||
|
|
||
| /** | ||
| * Mediator for inter-plugin communication. | ||
| * | ||
| * Created by AppKit core and passed to every plugin. Plugins request | ||
| * capabilities from the context instead of holding direct references | ||
| * to sibling plugin instances. | ||
| * | ||
| * Capabilities: | ||
| * - Route mounting with buffering (order-independent) | ||
| * - Typed ToolProvider registry (live, not snapshot-based) | ||
| * - User-scoped tool execution with automatic telemetry | ||
| * - Lifecycle hooks for plugin coordination | ||
| */ | ||
| export class PluginContext { | ||
| private routeBuffer: BufferedRoute[] = []; | ||
| private routeTarget: RouteTarget | null = null; | ||
| private toolProviders = new Map<string, ToolProviderEntry>(); | ||
| private plugins = new Map<string, BasePlugin>(); | ||
| private lifecycleHooks = new Map< | ||
| LifecycleEvent, | ||
| Set<() => void | Promise<void>> | ||
| >(); | ||
| private telemetry = TelemetryManager.getProvider("plugin-context"); | ||
|
|
||
| /** | ||
| * Register a route on the root Express application. | ||
| * | ||
| * If a route target (server plugin) has registered, the route is applied | ||
| * immediately. Otherwise it is buffered and flushed when a route target | ||
| * becomes available. | ||
| */ | ||
| addRoute( | ||
| method: string, | ||
| path: string, | ||
| ...handlers: express.RequestHandler[] | ||
| ): void { | ||
| if (this.routeTarget) { | ||
| this.applyRoute({ method, path, handlers }); | ||
| } else { | ||
| this.routeBuffer.push({ method, path, handlers }); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Register middleware on the root Express application. | ||
| * | ||
| * Same buffering semantics as `addRoute`. | ||
| */ | ||
| addMiddleware(path: string, ...handlers: express.RequestHandler[]): void { | ||
| if (this.routeTarget) { | ||
| this.applyMiddleware(path, handlers); | ||
| } else { | ||
| this.routeBuffer.push({ method: "use", path, handlers }); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Called by the server plugin to opt in as the route target. | ||
| * Flushes all buffered routes via the server's `addExtension`. | ||
| */ | ||
| registerAsRouteTarget(target: RouteTarget): void { | ||
| this.routeTarget = target; | ||
|
|
||
| for (const route of this.routeBuffer) { | ||
| if (route.method === "use") { | ||
| this.applyMiddleware(route.path, route.handlers); | ||
| } else { | ||
| this.applyRoute(route); | ||
| } | ||
| } | ||
| this.routeBuffer = []; | ||
| } | ||
|
|
||
| /** | ||
| * Register a plugin that implements the ToolProvider interface. | ||
| * Called by AppKit core after constructing each plugin. | ||
| */ | ||
| registerToolProvider(name: string, plugin: BasePlugin & ToolProvider): void { | ||
| this.toolProviders.set(name, { plugin, name }); | ||
| } | ||
|
|
||
| /** | ||
| * Register a plugin instance. | ||
| * Called by AppKit core after constructing each plugin. | ||
| */ | ||
| registerPlugin(name: string, instance: BasePlugin): void { | ||
| this.plugins.set(name, instance); | ||
| } | ||
|
|
||
| /** | ||
| * Returns all registered plugin instances keyed by name. | ||
| * Used by the server plugin for route injection, client config, | ||
| * and shutdown coordination. | ||
| */ | ||
| getPlugins(): Map<string, BasePlugin> { | ||
| return this.plugins; | ||
| } | ||
|
|
||
| /** | ||
| * Returns all registered ToolProvider plugins. | ||
| * Always returns the current set — not a frozen snapshot. | ||
| */ | ||
| getToolProviders(): Array<{ name: string; provider: ToolProvider }> { | ||
| return Array.from(this.toolProviders.values()).map((entry) => ({ | ||
| name: entry.name, | ||
| provider: entry.plugin, | ||
| })); | ||
| } | ||
|
|
||
| /** | ||
| * Execute a tool on a ToolProvider plugin with automatic user scoping | ||
| * and telemetry. | ||
| * | ||
| * The context: | ||
| * 1. Resolves the plugin by name | ||
| * 2. Calls `asUser(req)` for user-scoped execution | ||
| * 3. Wraps the call in a telemetry span with a 30s timeout | ||
| */ | ||
| async executeTool( | ||
| req: express.Request, | ||
| pluginName: string, | ||
| toolName: string, | ||
| args: unknown, | ||
| signal?: AbortSignal, | ||
| ): Promise<unknown> { | ||
| const entry = this.toolProviders.get(pluginName); | ||
| if (!entry) { | ||
| throw new Error( | ||
| `PluginContext: unknown plugin "${pluginName}". Available: ${Array.from(this.toolProviders.keys()).join(", ")}`, | ||
| ); | ||
| } | ||
|
|
||
| const tracer = this.telemetry.getTracer(); | ||
| const operationName = `executeTool:${pluginName}.${toolName}`; | ||
|
|
||
| return tracer.startActiveSpan(operationName, async (span) => { | ||
| const timeout = 30_000; | ||
| const timeoutSignal = AbortSignal.timeout(timeout); | ||
| const combinedSignal = signal | ||
| ? AbortSignal.any([signal, timeoutSignal]) | ||
| : timeoutSignal; | ||
|
|
||
| try { | ||
| const userPlugin = (entry.plugin as any).asUser(req); | ||
| const result = await (userPlugin as ToolProvider).executeAgentTool( | ||
| toolName, | ||
| args, | ||
| combinedSignal, | ||
| ); | ||
| span.setStatus({ code: 0 }); | ||
| return result; | ||
| } catch (error) { | ||
| span.setStatus({ | ||
| code: 2, | ||
| message: | ||
| error instanceof Error ? error.message : "Tool execution failed", | ||
| }); | ||
| span.recordException( | ||
| error instanceof Error ? error : new Error(String(error)), | ||
| ); | ||
| throw error; | ||
| } finally { | ||
| span.end(); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Register a lifecycle hook callback. | ||
| */ | ||
| onLifecycle(event: LifecycleEvent, fn: () => void | Promise<void>): void { | ||
| let hooks = this.lifecycleHooks.get(event); | ||
| if (!hooks) { | ||
| hooks = new Set(); | ||
| this.lifecycleHooks.set(event, hooks); | ||
| } | ||
| hooks.add(fn); | ||
| } | ||
|
|
||
| /** | ||
| * Emit a lifecycle event, calling all registered callbacks. | ||
| * Errors in individual callbacks are logged but do not prevent | ||
| * other callbacks from running. | ||
| * | ||
| * @internal Called by AppKit core only. | ||
| */ | ||
| async emitLifecycle(event: LifecycleEvent): Promise<void> { | ||
| const hooks = this.lifecycleHooks.get(event); | ||
| if (!hooks) return; | ||
|
|
||
| if ( | ||
| event === "setup:complete" && | ||
| this.routeBuffer.length > 0 && | ||
| !this.routeTarget | ||
| ) { | ||
| logger.warn( | ||
| "%d buffered routes were never applied — no server plugin registered as route target", | ||
| this.routeBuffer.length, | ||
| ); | ||
| } | ||
|
|
||
| for (const fn of hooks) { | ||
| try { | ||
| await fn(); | ||
| } catch (error) { | ||
| logger.error("Lifecycle hook '%s' failed: %O", event, error); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Returns all registered plugin names. | ||
| */ | ||
| getPluginNames(): string[] { | ||
| return Array.from(this.plugins.keys()); | ||
| } | ||
|
|
||
| /** | ||
| * Check if a plugin with the given name is registered. | ||
| */ | ||
| hasPlugin(name: string): boolean { | ||
| return this.plugins.has(name); | ||
| } | ||
|
|
||
| private applyRoute(route: BufferedRoute): void { | ||
| if (!this.routeTarget) return; | ||
| this.routeTarget.addExtension((app) => { | ||
| const method = route.method.toLowerCase() as keyof express.Application; | ||
| if (typeof app[method] === "function") { | ||
| (app[method] as (...a: unknown[]) => void)( | ||
| route.path, | ||
| ...route.handlers, | ||
| ); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| private applyMiddleware( | ||
| path: string, | ||
| handlers: express.RequestHandler[], | ||
| ): void { | ||
| if (!this.routeTarget) return; | ||
| this.routeTarget.addExtension((app) => { | ||
| app.use(path, ...handlers); | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Type guard: checks whether a plugin implements the ToolProvider interface. | ||
| */ | ||
| export function isToolProvider( | ||
| plugin: unknown, | ||
| ): plugin is BasePlugin & ToolProvider { | ||
| return ( | ||
| typeof plugin === "object" && | ||
| plugin !== null && | ||
| "getAgentTools" in plugin && | ||
| typeof (plugin as ToolProvider).getAgentTools === "function" && | ||
| "executeAgentTool" in plugin && | ||
| typeof (plugin as ToolProvider).executeAgentTool === "function" | ||
| ); | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agentic review:
Findings
P1 — High
plugin-context.ts:226emitLifecycleiteratesfor (const fn of hooks)over aSet. If a callback callscontext.onLifecycle()for the same event, the Set is mutated mid-iteration. Per ECMAScript spec, newly added Set entries duringfor...ofWILL be visited — this can cause unexpected extra executions or infinite loops.safe_auto -> review-fixerplugin.ts:79attachContextmissing fromEXCLUDED_FROM_PROXY. TheEXCLUDED_FROM_PROXYset lists lifecycle methods thatasUser()should not proxy.attachContextis absent, meaningplugin.asUser(req).attachContext(...)would run in user context and could mutate the plugin's telemetry, cache, and context bindings. Other lifecycle methods (setup,shutdown,injectRoutes) are already excluded.safe_auto -> review-fixerP2 — Moderate
plugin-context.ts:174,178SpanStatusCodeenum.span.setStatus({ code: 0 })andspan.setStatus({ code: 2 })use raw integers. The codebase pattern inTelemetryInterceptorusesSpanStatusCode.OK/SpanStatusCode.ERROR. Works by coincidence but fragile and inconsistent.safe_auto -> review-fixerplugin-context.ts:119getPlugins()returns mutable internal Map. Any caller can mutate the plugin registry (set,delete,clear). Server plugin iterates over it but external access could corrupt state.safe_auto -> review-fixerplugin-context.ts:168(entry.plugin as any).asUser(req)cast bypasses type safety.ToolProviderdoesn't declareasUser; it lives on thePluginbase class. Theas anycast defeats TypeScript's ability to catch signature drift. The existingisToolProvider()guard could be extended post-asUserfor runtime safety.gated_auto -> downstream-resolverplugin-context.ts:85registerAsRouteTargetcall silently overwrites. No guard prevents a second call. The first target's extensions registered after the second call would be lost. In practice only ServerPlugin calls this, but a defensive guard prevents silent misconfiguration.manual -> humanplugin-context.ts:103registerToolProviderusesMap.set()without checking for duplicates. A second plugin with the same name silently overwrites the first, making the first plugin's tools unreachable.manual -> humanplugin-context.ts:161executeToolnot configurable. Different tools have different performance profiles (ML inference vs. quick lookups). No way to override per-call or per-context.advisory -> humanappkit.ts:46,85attachContext. Context is passed inextraDatato the constructor (line 46) AND then viaattachContext(line 85). The Plugin constructor extracts it at line 216. Both paths set the same context instance so it's not a bug, but the dual path is confusing and creates maintenance risk.advisory -> humanplugin.ts:216(config as Record<string, unknown>).contextin Plugin constructor.BasePluginConfigdoesn't declare acontextfield. The cast bypasses TypeScript to extract context from an enriched config object. Future config changes could silently break this.gated_auto -> downstream-resolverP3 — Low
plugin-context.ts:18ToolProviderEntryinterface adds unnecessary indirection. Wraps plugin + name, but name is already the Map key. Could storeToolProviderdirectly and derive name from the key.safe_auto -> review-fixerApplied Fixes (Plan)
The following
safe_autofindings should be fixed:Fix 1: Snapshot lifecycle hooks before iteration (Finding #1)
File:
packages/appkit/src/core/plugin-context.ts:226Change:
To:
This creates an array snapshot so mutations during iteration don't affect the loop.
Fix 2: Add
attachContextto EXCLUDED_FROM_PROXY (Finding #2)File:
packages/appkit/src/plugin/plugin.ts:79-92Add
"attachContext"to the Set:Fix 3: Use SpanStatusCode enum (Finding #3)
File:
packages/appkit/src/core/plugin-context.ts:174,178Import
SpanStatusCodefrom the telemetry module and replace magic numbers:Check how
TelemetryInterceptorimports it — it usesimport { SpanStatusCode } from "../../telemetry".Fix 4: Return defensive copy from getPlugins() (Finding #4)
File:
packages/appkit/src/core/plugin-context.ts:119-121Change:
To:
TypeScript's
ReadonlyMappreventsset/delete/clearat compile time. No runtime overhead.Fix 5: Remove ToolProviderEntry indirection (Finding #11)
File:
packages/appkit/src/core/plugin-context.tsToolProviderEntryinterface (lines 18-21)toolProviderstoMap<string, BasePlugin & ToolProvider>registerToolProviderto store the plugin directlygetToolProvidersto map entries to{ name, provider }from the MapResidual Actionable Work
These findings should be addressed but require human judgment:
as anycast inexecuteTool: Consider adding aHasAsUserinterface or runtime check post-asUserusingisToolProvider().registerAsRouteTargetagainst double calls: Addif (this.routeTarget) throw new Error(...).if (this.toolProviders.has(name)) logger.warn(...).InternalPluginConfigtype or removing constructor context extraction in favor ofattachContext-only path.Testing Gaps
The following test coverage gaps were identified across reviewers:
AbortSignal.any()correctly combines user signal + timeout signal.setup:completewarning when routes are buffered but no server registered.attachContextconfig merging, idempotence, or telemetry binding.attachContexttriggers route target registration.isToolProvider()check +registerToolProvider()increateAndRegisterPlugin.setStatus/recordException/endare called with correct values.registerAsRouteTargetbehavior — No test for what happens when called twice.Agent-Native Gaps
The agent-native reviewer identified that while
PluginContext.executeTool()provides the right infrastructure for tool execution, it is not yet wired toAgentRunContextor exposed via HTTP endpoints for agent consumption. This is expected to be addressed in a subsequent PR in theagent/v2series and is not a blocker for this infrastructure PR.Learnings & Past Solutions
No relevant past solutions found in
docs/solutions/. The learnings researcher confirmed the implementation patterns (route buffering, live registry, lifecycle hooks) are sound and well-tested.Coverage
Verdict: Ready with fixes
The architecture is sound. The PluginContext mediator is well-designed with proper separation of concerns. Five
safe_autofixes should be applied (lifecycle Set snapshot, EXCLUDED_FROM_PROXY, SpanStatusCode enum, ReadonlyMap, ToolProviderEntry cleanup). Fourmanual/gated_autofindings are recommended for human review. Eight testing gaps should be addressed before merge.Fix order:
Verification
After applying fixes: