feat(appkit): plugin infrastructure — attachContext + PluginContext mediator#303
feat(appkit): plugin infrastructure — attachContext + PluginContext mediator#303MarioCadenas wants to merge 1 commit intoagent/v2/2-tool-primitivesfrom
Conversation
b328cf2 to
5a7a4df
Compare
a5642df to
e26795b
Compare
5a7a4df to
a384b1e
Compare
e26795b to
d73e138
Compare
a384b1e to
68e05d3
Compare
d73e138 to
26f43e5
Compare
68e05d3 to
b765708
Compare
26f43e5 to
2ffa31d
Compare
b765708 to
6712ce7
Compare
3a4deb7 to
ca9cfca
Compare
6712ce7 to
7077eb0
Compare
863439e to
cca914f
Compare
7d7f66a to
22e267c
Compare
cca914f to
a3d2cc6
Compare
724e48a to
39e7091
Compare
f495962 to
c15858b
Compare
0e06004 to
a70bbcc
Compare
aa95c27 to
71a986d
Compare
a70bbcc to
3343203
Compare
71a986d to
82d8ea0
Compare
3343203 to
85663cf
Compare
a8418ea to
c9c986d
Compare
a63b7a4 to
47c1c68
Compare
c9c986d to
5fde8aa
Compare
47c1c68 to
6f83621
Compare
4a388b1 to
a7ebc57
Compare
pkosiec
left a comment
There was a problem hiding this comment.
LGTM but please verify the agentic review findings before merge 👍
There was a problem hiding this comment.
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
ToolProviderEntryinterface (lines 18-21) - Change
toolProviderstoMap<string, BasePlugin & ToolProvider> - Update
registerToolProviderto store the plugin directly - Update
getToolProvidersto map entries to{ name, provider }from the Map
Residual Actionable Work
These findings should be addressed but require human judgment:
- Finding chore: tune tsdown configuration to reduce bundle size #5 — The
as anycast inexecuteTool: Consider adding aHasAsUserinterface or runtime check post-asUserusingisToolProvider(). - Finding WIP: feat: add volume serving plugin #6 — Guard
registerAsRouteTargetagainst double calls: Addif (this.routeTarget) throw new Error(...). - Finding chore: add release to NPM workflow #7 — Warn on tool provider name collision: Add
if (this.toolProviders.has(name)) logger.warn(...). - Finding fix: npm publishing #10 — Unsafe config context extraction: Consider an
InternalPluginConfigtype or removing constructor context extraction in favor ofattachContext-only path.
Testing Gaps
The following test coverage gaps were identified across reviewers:
- executeTool timeout/signal combination — No test verifies the 30s timeout fires, or that
AbortSignal.any()correctly combines user signal + timeout signal. - emitLifecycle warning for unapplied routes — No test for the
setup:completewarning when routes are buffered but no server registered. - Plugin.attachContext unit tests — No dedicated tests for
attachContextconfig merging, idempotence, or telemetry binding. - ServerPlugin.attachContext + registerAsRouteTarget — No test verifies
attachContexttriggers route target registration. - AppKit ToolProvider auto-registration — No test verifies
isToolProvider()check +registerToolProvider()increateAndRegisterPlugin. - executeTool telemetry span assertions — Tests mock the span but never assert
setStatus/recordException/endare called with correct values. - Double
registerAsRouteTargetbehavior — No test for what happens when called twice. - 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:
- Fix chore: rework TelemetryManager to use Node SDK #1 (P1 — Set mutation safety)
- Fix feat: reexport shadcn components and theme #2 (P1 — EXCLUDED_FROM_PROXY)
- Fix chore: fix ci runners #3 (P2 — SpanStatusCode)
- Fix feat: arrow stream #4 (P2 — ReadonlyMap)
- Fix chore: tune tsdown configuration to reduce bundle size #5 (P3 — ToolProviderEntry cleanup)
Verification
After applying fixes:
pnpm check:fix && pnpm -r typecheck && pnpm testa2c725e to
8b67c5a
Compare
…nContext mediator
Third layer: the substrate every downstream PR relies on. No user-
facing API changes here; the surface for this PR is the mediator
pattern, lifecycle semantics, and factory stamping.
`Plugin` constructors become pure — no `CacheManager.getInstanceSync()`,
no `TelemetryManager.getProvider()`, no `PluginContext` wiring inside
`constructor()`. That work moves to a new lifecycle method:
```ts
interface BasePlugin {
attachContext?(deps: {
context?: unknown;
telemetryConfig?: TelemetryOptions;
}): void;
}
```
`createApp` calls `attachContext()` on every plugin after all
constructors have run, before `setup()`. This lets factories return
`PluginData` tuples at module scope without pulling core services into
the import graph — a prerequisite for later PRs that construct agent
definitions before `createApp`.
`packages/appkit/src/core/plugin-context.ts` — new class that mediates
all inter-plugin communication:
- **Route buffering**: `addRoute()` / `addMiddleware()` buffer until
the server plugin calls `registerAsRouteTarget()`, then flush via
`addExtension()`. Eliminates plugin-ordering fragility.
- **ToolProvider registry**: `registerToolProvider(name, plugin)` +
live `getToolProviders()`. Typed discovery of tool-exposing plugins.
- **User-scoped tool execution**: `executeTool(req, pluginName,
localName, args, signal?)` resolves the provider, wraps in
`asUser(req)` for OBO, opens a telemetry span, applies a 30s
timeout, dispatches, returns.
- **Lifecycle hooks**: `onLifecycle('setup:complete' | 'server:ready'
| 'shutdown', cb)` + `emitLifecycle(event)`. Callback errors don't
block siblings.
`packages/appkit/src/plugin/to-plugin.ts` — the factory now attaches a
read-only `pluginName` property to the returned function. Later PRs'
`fromPlugin(factory)` reads it to identify which plugin a factory
refers to without needing to construct an instance. `NamedPluginFactory`
type exported for consumers who want to type-constrain factories.
`ServerPlugin.setup()` no longer calls `extendRoutes()` synchronously.
It subscribes to the `setup:complete` lifecycle event via
`PluginContext` and starts the HTTP server there. This ensures that
any deferred-phase plugin (agents plugin in a later PR) has had a
chance to register routes via `PluginContext.addRoute()` before the
server binds. Removes the `plugins` field from `ServerConfig` (routes
are now discovered via the context, not a config snapshot).
- 25 new PluginContext tests (route buffering, tool provider registry,
executeTool paths, lifecycle hooks, plugin metadata)
- Updated AppKit lifecycle tests to inject `context` instead of
`plugins`
- Full appkit vitest suite: 1237 tests passing
- Typecheck clean across all 8 workspace projects
Signed-off-by: MarioCadenas <[email protected]>
a7ebc57 to
91e66e1
Compare
Third layer: the substrate every downstream PR relies on. No user-
facing API changes here; the surface for this PR is the mediator
pattern, lifecycle semantics, and factory stamping.
Split Plugin construction from context binding
Pluginconstructors become pure — noCacheManager.getInstanceSync(),no
TelemetryManager.getProvider(), noPluginContextwiring insideconstructor(). That work moves to a new lifecycle method:createAppcallsattachContext()on every plugin after allconstructors have run, before
setup(). This lets factories returnPluginDatatuples at module scope without pulling core services intothe import graph — a prerequisite for later PRs that construct agent
definitions before
createApp.PluginContext mediator
packages/appkit/src/core/plugin-context.ts— new class that mediatesall inter-plugin communication:
addRoute()/addMiddleware()buffer untilthe server plugin calls
registerAsRouteTarget(), then flush viaaddExtension(). Eliminates plugin-ordering fragility.registerToolProvider(name, plugin)+live
getToolProviders(). Typed discovery of tool-exposing plugins.executeTool(req, pluginName, localName, args, signal?)resolves the provider, wraps inasUser(req)for OBO, opens a telemetry span, applies a 30stimeout, dispatches, returns.
onLifecycle('setup:complete' | 'server:ready' | 'shutdown', cb)+emitLifecycle(event). Callback errors don'tblock siblings.
toPluginstampspluginNamepackages/appkit/src/plugin/to-plugin.ts— the factory now attaches aread-only
pluginNameproperty to the returned function. Later PRs'fromPlugin(factory)reads it to identify which plugin a factoryrefers to without needing to construct an instance.
NamedPluginFactorytype exported for consumers who want to type-constrain factories.
Server plugin defers start to
setup:completeServerPlugin.setup()no longer callsextendRoutes()synchronously.It subscribes to the
setup:completelifecycle event viaPluginContextand starts the HTTP server there. This ensures thatany deferred-phase plugin (agents plugin in a later PR) has had a
chance to register routes via
PluginContext.addRoute()before theserver binds. Removes the
pluginsfield fromServerConfig(routesare now discovered via the context, not a config snapshot).
Test plan
executeTool paths, lifecycle hooks, plugin metadata)
contextinstead ofpluginsSigned-off-by: MarioCadenas [email protected]
PR Stack
agents()plugin +createAgent(def)+ markdown-driven agents — feat(appkit): agents() plugin, createAgent(def), and markdown-driven agents #304fromPlugin()DX +runAgentplugins arg + toolkit-resolver — feat(appkit): fromPlugin() DX, runAgent plugins arg, shared toolkit-resolver #305Demo
agent-demo.mp4