refactor: convert printer to hook-based implementation #277
Conversation
- Remove Printer interface (no longer needed) - Make AgentPrinter implement HookProvider interface - Agent registers AgentPrinter as hook when printer config is true - Remove direct _printer?.processEvent() calls from Agent - Wrap ToolResultBlock in ModelStreamEventHook for printer processing - Update tests to use hooks.addHook() pattern - Maintain backward compatibility (printer?: boolean still works) Resolves: #270
zastrowm
temporarily deployed
to
auto-approve
GitHub Actions
Inactive
src/agent/agent.ts
Outdated
| toolResultBlocks.push(toolResultBlock) | ||
|
|
||
| // Wrap ToolResultBlock in hook event for printer | ||
| yield new ModelStreamEventHook({ agent: this, event: toolResultBlock }) |
There was a problem hiding this comment.
This shows a gap in our hook implementation. This doesn't make sense to yield a tool result block for ModelStreamEventHook, but it also doesn't make sense that we introduce ToolResultBlockHook just for toolResults.
Instead, update ModelStreamEventHook to only be created for non-ContentBlocks, and let's add a new hook for ContentBlocks that is used for both Model ContentBlocks and ToolContent blocks. Then we would yield that here instead. Make sure to add associated test for the new hook, mimicking our existing hook documentation
There was a problem hiding this comment.
Addressed! Created ContentBlockHook for unified content block handling. ModelStreamEventHook now only accepts ModelStreamEvent, and printer subscribes to both hooks.
|
/strands implement |
github-actions
bot
added
the
strands-running
- Create ContentBlockHook for handling content blocks from model and tools - Update ModelStreamEventHook to only accept ModelStreamEvent (no ContentBlock) - Agent yields ContentBlockHook for content blocks from model streaming - Agent yields ContentBlockHook for tool result blocks - AgentPrinter subscribes to both ModelStreamEventHook and ContentBlockHook - Add 3 tests for ContentBlockHook following existing hook test patterns This addresses architectural concern where ToolResultBlock shouldn't be wrapped in ModelStreamEventHook. ContentBlockHook provides a unified way to handle content blocks regardless of their source.
github-actions
bot
removed
the
strands-running
src/agent/agent.ts
Outdated
| // Yield hook event for observability | ||
| yield new ModelStreamEventHook({ agent: this, event }) | ||
| // Yield appropriate hook event for observability | ||
| if ('type' in event && typeof event.type === 'string' && event.type.endsWith('Block')) { |
There was a problem hiding this comment.
Rather than doing this much work, let's add a base class to all content blocks, BaseContentBlock for now. Make it non-public and add a TODO to the code to api-bar-raise it.
Then do the type check here for that using instanceof rather than this event.type checking
src/agent/printer.ts
Outdated
| * Handle model stream events and process them for printing. | ||
| * @param event - The model stream event to handle | ||
| */ | ||
| private handleModelStreamEvent = (event: ModelStreamEventHook): void => { |
There was a problem hiding this comment.
Just use inline lambdas in the registerCallbacks method and just forward the events processEvent directly
| it('returns false for _shouldReverseCallbacks', () => { | ||
| const agent = new Agent() | ||
| const contentBlock = new TextBlock('Hello world') | ||
| const hookEvent = new ContentBlockHook({ agent, block: contentBlock }) |
There was a problem hiding this comment.
We need tests in agent.hook.test.ts as well to verify the lifecyles. Follow the existing conventions and add tests there too
|
/strands implement |
github-actions
bot
added
the
strands-running
- Create BaseContentBlock abstract class in separate file - Update all content block classes to extend BaseContentBlock - Replace string-based type checking with instanceof in agent - Simplify printer callbacks to use inline lambdas - Add lifecycle tests for ContentBlockHook - Fix circular dependency by moving BaseContentBlock to own file This addresses PR feedback to use class-based type discrimination instead of string checking and simplify callback registration.
Updates Based on Second Review FeedbackI've implemented all three requested changes: 1. Use BaseContentBlock for Type Discrimination ✅
2. Simplify Printer Callbacks ✅
3. Add Lifecycle Tests ✅
Validation Results
CommitsThe changes are in commit Ready for follow-up review! |
github-actions
bot
removed
the
strands-running
2 participants
Resolves: #270
Overview
Refactors the printer implementation from direct integration into Agent to a hook-based approach. This is an internal refactoring with NO PUBLIC API CHANGES - the
printer?: booleanconfig remains, and default behavior stays the same.Key Changes
Printerinterface (no longer needed)AgentPrinterimplementHookProviderinterfaceAgentPrinteras a hook internally whenprinterconfig is true (default)_printer?.processEvent(event)calls from AgentModelStreamEventHookto receive all printing eventsToolResultBlockinModelStreamEventHookwhen yieldingImplementation Details
The printer now follows the same pattern as
SlidingWindowConversationManager:HookProviderinterfaceModelStreamEventHookeventsModelStreamEventandContentBlocktypes through the hook systemThis makes the printer extensible via hooks while maintaining complete backward compatibility with the existing public API.
Testing
Files Modified
src/agent/printer.ts- Removed interface, added HookProvider implementationsrc/agent/agent.ts- Removed printer field, register as hook insteadsrc/agent/__tests__/printer.test.ts- Updated test setup to use hookssrc/agent/__tests__/agent.test.ts- Updated printer-related tests