Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 24 additions & 3 deletions packages/appkit/src/core/appkit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,20 @@ import { createLogger } from "../logging/logger";
import { ResourceRegistry, ResourceType } from "../registry";
import type { TelemetryConfig } from "../telemetry";
import { TelemetryManager } from "../telemetry";
import { isToolProvider, PluginContext } from "./plugin-context";

const logger = createLogger("appkit");

export class AppKit<TPlugins extends InputPluginMap> {
#pluginInstances: Record<string, BasePlugin> = {};
#setupPromises: Promise<void>[] = [];
#context: PluginContext;

private constructor(config: { plugins: TPlugins }) {
const { plugins, ...globalConfig } = config;

this.#context = new PluginContext();

const pluginEntries = Object.entries(plugins);

const corePlugins = pluginEntries.filter(([_, p]) => {
Expand All @@ -38,20 +42,24 @@ export class AppKit<TPlugins extends InputPluginMap> {

for (const [name, pluginData] of corePlugins) {
if (pluginData) {
this.createAndRegisterPlugin(globalConfig, name, pluginData);
this.createAndRegisterPlugin(globalConfig, name, pluginData, {
context: this.#context,
});
}
}

for (const [name, pluginData] of normalPlugins) {
if (pluginData) {
this.createAndRegisterPlugin(globalConfig, name, pluginData);
this.createAndRegisterPlugin(globalConfig, name, pluginData, {
context: this.#context,
});
}
}

for (const [name, pluginData] of deferredPlugins) {
if (pluginData) {
this.createAndRegisterPlugin(globalConfig, name, pluginData, {
plugins: this.#pluginInstances,
context: this.#context,
});
}
}
Expand All @@ -73,8 +81,20 @@ export class AppKit<TPlugins extends InputPluginMap> {
};
const pluginInstance = new Plugin(baseConfig);

if (typeof pluginInstance.attachContext === "function") {
pluginInstance.attachContext({
context: this.#context,
telemetryConfig: baseConfig.telemetry,
});
}

this.#pluginInstances[name] = pluginInstance;

this.#context.registerPlugin(name, pluginInstance);
if (isToolProvider(pluginInstance)) {
this.#context.registerToolProvider(name, pluginInstance);
}

this.#setupPromises.push(pluginInstance.setup());

const self = this;
Expand Down Expand Up @@ -203,6 +223,7 @@ export class AppKit<TPlugins extends InputPluginMap> {
const instance = new AppKit(mergedConfig);

await Promise.all(instance.#setupPromises);
await instance.#context.emitLifecycle("setup:complete");

const handle = instance as unknown as PluginMap<T>;

Expand Down
287 changes: 287 additions & 0 deletions packages/appkit/src/core/plugin-context.ts
Copy link
Copy Markdown
Member

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

# File Issue Reviewer Conf. Route
1 plugin-context.ts:226 Lifecycle hook Set mutated during iteration. emitLifecycle iterates for (const fn of hooks) over a Set. If a callback calls context.onLifecycle() for the same event, the Set is mutated mid-iteration. Per ECMAScript spec, newly added Set entries during for...of WILL be visited — this can cause unexpected extra executions or infinite loops. adversarial, correctness 0.88 safe_auto -> review-fixer
2 plugin.ts:79 attachContext missing from EXCLUDED_FROM_PROXY. The EXCLUDED_FROM_PROXY set lists lifecycle methods that asUser() should not proxy. attachContext is absent, meaning plugin.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. api-contract, adversarial 0.82 safe_auto -> review-fixer

P2 — Moderate

# File Issue Reviewer Conf. Route
3 plugin-context.ts:174,178 OTel span status uses magic numbers instead of SpanStatusCode enum. span.setStatus({ code: 0 }) and span.setStatus({ code: 2 }) use raw integers. The codebase pattern in TelemetryInterceptor uses SpanStatusCode.OK / SpanStatusCode.ERROR. Works by coincidence but fragile and inconsistent. reliability 0.95 safe_auto -> review-fixer
4 plugin-context.ts:119 getPlugins() 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. security 0.70 safe_auto -> review-fixer
5 plugin-context.ts:168 Unsafe (entry.plugin as any).asUser(req) cast bypasses type safety. ToolProvider doesn't declare asUser; it lives on the Plugin base class. The as any cast defeats TypeScript's ability to catch signature drift. The existing isToolProvider() guard could be extended post-asUser for runtime safety. kieran-ts, security 0.85 gated_auto -> downstream-resolver
6 plugin-context.ts:85 Double registerAsRouteTarget call 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. adversarial 0.75 manual -> human
7 plugin-context.ts:103 Tool provider name collision silently accepted. registerToolProvider uses Map.set() without checking for duplicates. A second plugin with the same name silently overwrites the first, making the first plugin's tools unreachable. adversarial 0.72 manual -> human
8 plugin-context.ts:161 Hardcoded 30s timeout in executeTool not configurable. Different tools have different performance profiles (ML inference vs. quick lookups). No way to override per-call or per-context. maintainability 0.75 advisory -> human
9 appkit.ts:46,85 Double context injection: constructor config + attachContext. Context is passed in extraData to the constructor (line 46) AND then via attachContext (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. maintainability, testing 0.70 advisory -> human
10 plugin.ts:216 Unsafe cast (config as Record<string, unknown>).context in Plugin constructor. BasePluginConfig doesn't declare a context field. The cast bypasses TypeScript to extract context from an enriched config object. Future config changes could silently break this. kieran-ts 0.92 gated_auto -> downstream-resolver

P3 — Low

# File Issue Reviewer Conf. Route
11 plugin-context.ts:18 ToolProviderEntry interface adds unnecessary indirection. Wraps plugin + name, but name is already the Map key. Could store ToolProvider directly and derive name from the key. maintainability 0.80 safe_auto -> review-fixer

Applied Fixes (Plan)

The following safe_auto findings should be fixed:

Fix 1: Snapshot lifecycle hooks before iteration (Finding #1)

File: packages/appkit/src/core/plugin-context.ts:226

Change:

for (const fn of hooks) {

To:

for (const fn of [...hooks]) {

This creates an array snapshot so mutations during iteration don't affect the loop.

Fix 2: Add attachContext to EXCLUDED_FROM_PROXY (Finding #2)

File: packages/appkit/src/plugin/plugin.ts:79-92

Add "attachContext" to the Set:

const EXCLUDED_FROM_PROXY = new Set([
  // Lifecycle methods
  "setup",
  "shutdown",
  "attachContext",   // <-- add this
  "injectRoutes",
  ...
]);

Fix 3: Use SpanStatusCode enum (Finding #3)

File: packages/appkit/src/core/plugin-context.ts:174,178

Import SpanStatusCode from the telemetry module and replace magic numbers:

import { SpanStatusCode } from "@opentelemetry/api";
// ...
span.setStatus({ code: SpanStatusCode.OK });
// ...
span.setStatus({ code: SpanStatusCode.ERROR, message: ... });

Check how TelemetryInterceptor imports it — it uses import { SpanStatusCode } from "../../telemetry".

Fix 4: Return defensive copy from getPlugins() (Finding #4)

File: packages/appkit/src/core/plugin-context.ts:119-121

Change:

getPlugins(): Map<string, BasePlugin> {
  return this.plugins;
}

To:

getPlugins(): ReadonlyMap<string, BasePlugin> {
  return this.plugins;
}

TypeScript's ReadonlyMap prevents set/delete/clear at compile time. No runtime overhead.

Fix 5: Remove ToolProviderEntry indirection (Finding #11)

File: packages/appkit/src/core/plugin-context.ts

  • Remove the ToolProviderEntry interface (lines 18-21)
  • Change toolProviders to Map<string, BasePlugin & ToolProvider>
  • Update registerToolProvider to store the plugin directly
  • Update getToolProviders to map entries to { name, provider } from the Map

Residual Actionable Work

These findings should be addressed but require human judgment:

  1. Finding chore: tune tsdown configuration to reduce bundle size #5 — The as any cast in executeTool: Consider adding a HasAsUser interface or runtime check post-asUser using isToolProvider().
  2. Finding WIP: feat: add volume serving plugin #6 — Guard registerAsRouteTarget against double calls: Add if (this.routeTarget) throw new Error(...).
  3. Finding chore: add release to NPM workflow #7 — Warn on tool provider name collision: Add if (this.toolProviders.has(name)) logger.warn(...).
  4. Finding fix: npm publishing #10 — Unsafe config context extraction: Consider an InternalPluginConfig type or removing constructor context extraction in favor of attachContext-only path.

Testing Gaps

The following test coverage gaps were identified across reviewers:

  1. executeTool timeout/signal combination — No test verifies the 30s timeout fires, or that AbortSignal.any() correctly combines user signal + timeout signal.
  2. emitLifecycle warning for unapplied routes — No test for the setup:complete warning when routes are buffered but no server registered.
  3. Plugin.attachContext unit tests — No dedicated tests for attachContext config merging, idempotence, or telemetry binding.
  4. ServerPlugin.attachContext + registerAsRouteTarget — No test verifies attachContext triggers route target registration.
  5. AppKit ToolProvider auto-registration — No test verifies isToolProvider() check + registerToolProvider() in createAndRegisterPlugin.
  6. executeTool telemetry span assertions — Tests mock the span but never assert setStatus/recordException/end are called with correct values.
  7. Double registerAsRouteTarget behavior — No test for what happens when called twice.
  8. Lifecycle hook mutation during iteration — No test for hooks that register new hooks during execution.

Agent-Native Gaps

The agent-native reviewer identified that while PluginContext.executeTool() provides the right infrastructure for tool execution, it is not yet wired to AgentRunContext or exposed via HTTP endpoints for agent consumption. This is expected to be addressed in a subsequent PR in the agent/v2 series 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

  • Suppressed: 0 findings below 0.60 confidence
  • Failed reviewers: 0
  • Untracked files: None
  • Project standards: Full compliance with CLAUDE.md (conventional commits, vitest, Biome, correct file placement, shared types)

Verdict: Ready with fixes

The architecture is sound. The PluginContext mediator is well-designed with proper separation of concerns. Five safe_auto fixes should be applied (lifecycle Set snapshot, EXCLUDED_FROM_PROXY, SpanStatusCode enum, ReadonlyMap, ToolProviderEntry cleanup). Four manual/gated_auto findings are recommended for human review. Eight testing gaps should be addressed before merge.

Fix order:

  1. Fix chore: rework TelemetryManager to use Node SDK #1 (P1 — Set mutation safety)
  2. Fix feat: reexport shadcn components and theme #2 (P1 — EXCLUDED_FROM_PROXY)
  3. Fix chore: fix ci runners #3 (P2 — SpanStatusCode)
  4. Fix feat: arrow stream #4 (P2 — ReadonlyMap)
  5. Fix chore: tune tsdown configuration to reduce bundle size #5 (P3 — ToolProviderEntry cleanup)

Verification

After applying fixes:

pnpm check:fix && pnpm -r typecheck && pnpm test

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"
);
}
Loading