refactor(5/12): change handler contract to event-based emission#323
refactor(5/12): change handler contract to event-based emission#323cameroncooke wants to merge 1 commit intorefactor/build-test-utility-extractionfrom
Conversation
089f6a3 to
fa1ece2
Compare
344aeba to
f7616d2
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fa1ece2. Configure here.
| throw error; | ||
| } | ||
| }; | ||
| return impl as ToolHandler; |
There was a problem hiding this comment.
Duplicated context setup logic across handler factories
Low Severity
createValidatedHandler and createSessionAwareHandler duplicate the entire ToolHandlerContext bootstrapping sequence: isToolHandlerContext check, internal session creation, ctx/context resolution, handlerContextStorage.run, ZodError handling, and sessionToTestResult finalization. If the context propagation or session lifecycle logic changes, both paths need identical updates, creating a maintenance risk.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit fa1ece2. Configure here.


Summary
This is PR 5 of 12 in a stacked PR series that decouples the rendering pipeline from MCP transport. Depends on PR 4 (utility extraction). This is the most architecturally significant PR in the stack -- it changes the fundamental contract between tool handlers and the runtime.
The contract change
Before: Tool handlers returned a
ToolResponseobject containing pre-rendered MCP content:```typescript
handler: (params) => Promise
```
After: Tool handlers receive a context object and emit events through it, returning nothing:
```typescript
handler: (params, ctx: ToolHandlerContext) => Promise
```
The
ToolHandlerContextprovides:emit(event: PipelineEvent): Push a structured event into the render sessionattach(image: ImageAttachment): Attach binary content (screenshots)This decouples tools from rendering entirely. A tool says "the build succeeded" via
ctx.emit(statusLine('success', 'Build succeeded'))without knowing whether the output will be rendered as colored terminal text, JSON, or MCP protocol content.Tool invoker refactor
The invoker (
tool-invoker.ts) is significantly simplified:postProcessToolResponse,emitNextStepsEvent,renderNextStepsIntoContent,finalizeResultToolHandlerContext, passes it to the handler, then emits next-steps into the same render session after the handler completesPendingBuildResultpattern -- the handler just awaits the pipeline and the render session accumulates events progressivelyTyped tool factory
typed-tool-factory.tsupdated to produce handlers matching the new signature. Session-aware wrappers now thread the context through.Core manifest and schema
import-resource-module.tsadded for resource manifest loading (split from tool module loading)plugin-types.tsupdated to reflect new handler signaturesDeleted
typed-tool-factory-consolidation.test.ts(tested removed consolidation logic)load-manifest.test.ts(moved to schema test coverage)Stack navigation
ctx.emit)Note for reviewers
PRs 6-9 (tool migrations) depend on this contract change. Each tool handler is updated from
return toolResponse([...])toctx.emit(...)calls. Those are mechanical transformations but they cannot compile without this PR landing first. If reviewing the stack incrementally, this PR is the critical design decision point.Test plan
npx vitest runpasses -- invoker tests updated for new contract