Skip to content

fix: make callback the first parameter for adding hook callback #545

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.

Sign up for GitHub

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
Unshure wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: make callback the first parameter for adding hook callback #545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 4 additions & 4 deletions docs/TESTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,12 +321,12 @@ When testing hook behavior, you **MUST** use `agent.hooks.addCallback()` for reg
// ✅ CORRECT - Use agent.hooks.addCallback() for single callbacks
const agent = new Agent({ model, tools: [tool] })

agent.hooks.addCallback(BeforeToolCallEvent, (event: BeforeToolCallEvent) => {
agent.hooks.addCallback((event: BeforeToolCallEvent) => {
event.toolUse = {
...event.toolUse,
input: { value: 42 },
}
})
}, BeforeToolCallEvent)

// ✅ CORRECT - Use MockHookProvider to record and verify hook invocations
const hookProvider = new MockHookProvider()
Expand All @@ -337,11 +337,11 @@ expect(hookProvider.invocations).toContainEqual(new BeforeInvocationEvent({ agen
// ❌ WRONG - Do NOT create inline HookProvider objects
const switchToolHook = {
registerCallbacks: (registry: HookRegistry) => {
registry.addCallback(BeforeToolCallEvent, (event: BeforeToolCallEvent) => {
registry.addCallback((event: BeforeToolCallEvent) => {
if (event.toolUse.name === 'tool1') {
event.tool = tool2
}
})
}, BeforeToolCallEvent)
},
}
```
Expand Down
4 changes: 2 additions & 2 deletions src/__fixtures__/mock-hook-provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,9 @@ export class MockHookProvider implements HookProvider {
const eventTypes = this.includeModelEvents ? [...lifecycleEvents, ...modelEvents] : lifecycleEvents

for (const eventType of eventTypes) {
registry.addCallback(eventType, (e) => {
registry.addCallback((e) => {
this.invocations.push(e)
})
}, eventType)
}
}

Expand Down
24 changes: 12 additions & 12 deletions src/agent/__tests__/agent.hook.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,12 +306,12 @@ describe('Agent Hooks Integration', () => {
.addTurn({ type: 'textBlock', text: 'Success after retry' })

const agent = new Agent({ model })
agent.hooks.addCallback(AfterModelCallEvent, (event: AfterModelCallEvent) => {
agent.hooks.addCallback((event: AfterModelCallEvent) => {
callCount++
if (callCount === 1 && event.error) {
event.retry = true
}
})
}, AfterModelCallEvent)

const result = await agent.invoke('Test')

Expand All @@ -333,12 +333,12 @@ describe('Agent Hooks Integration', () => {
.addTurn({ type: 'textBlock', text: 'Second response after retry' })

const agent = new Agent({ model })
agent.hooks.addCallback(AfterModelCallEvent, (event: AfterModelCallEvent) => {
agent.hooks.addCallback((event: AfterModelCallEvent) => {
callCount++
if (callCount === 1 && !event.error) {
event.retry = true
}
})
}, AfterModelCallEvent)

const result = await agent.invoke('Test')

Expand All @@ -364,12 +364,12 @@ describe('Agent Hooks Integration', () => {
.addTurn({ type: 'textBlock', text: 'Done' })

const agent = new Agent({ model, tools: [tool] })
agent.hooks.addCallback(AfterToolCallEvent, (event: AfterToolCallEvent) => {
agent.hooks.addCallback((event: AfterToolCallEvent) => {
hookCallCount++
if (hookCallCount === 1 && event.error) {
event.retry = true
}
})
}, AfterToolCallEvent)

const result = await agent.invoke('Test')

Expand Down Expand Up @@ -414,15 +414,15 @@ describe('Agent Hooks Integration', () => {
.addTurn({ type: 'textBlock', text: 'Done' })

const agent = new Agent({ model, tools: [tool] })
agent.hooks.addCallback(BeforeToolCallEvent, () => {
agent.hooks.addCallback(() => {
beforeCount++
})
agent.hooks.addCallback(AfterToolCallEvent, (event: AfterToolCallEvent) => {
}, BeforeToolCallEvent)
agent.hooks.addCallback((event: AfterToolCallEvent) => {
afterCount++
if (afterCount === 1) {
event.retry = true
}
})
}, AfterToolCallEvent)

await agent.invoke('Test')

Expand All @@ -448,12 +448,12 @@ describe('Agent Hooks Integration', () => {
.addTurn({ type: 'textBlock', text: 'Done' })

const agent = new Agent({ model, tools: [tool] })
agent.hooks.addCallback(AfterToolCallEvent, (event: AfterToolCallEvent) => {
agent.hooks.addCallback((event: AfterToolCallEvent) => {
hookCallCount++
if (hookCallCount === 1) {
event.retry = true
}
})
}, AfterToolCallEvent)

const result = await agent.invoke('Test')

Expand Down
8 changes: 4 additions & 4 deletions src/conversation-manager/sliding-window-conversation-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,17 +65,17 @@ export class SlidingWindowConversationManager implements HookProvider {
*/
public registerCallbacks(registry: HookRegistry): void {
// Apply sliding window management after each invocation
registry.addCallback(AfterInvocationEvent, (event) => {
registry.addCallback((event) => {
this.applyManagement(event.agent.messages)
})
}, AfterInvocationEvent)

// Handle context overflow errors
registry.addCallback(AfterModelCallEvent, (event) => {
registry.addCallback((event) => {
if (event.error instanceof ContextWindowOverflowError) {
this.reduceContext(event.agent.messages, event.error)
event.retry = true
}
})
}, AfterModelCallEvent)
}

/**
Expand Down
72 changes: 36 additions & 36 deletions src/hooks/__tests__/registry.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ describe('HookRegistryImplementation', () => {
describe('addCallback', () => {
it('registers callback for event type', async () => {
const callback = vi.fn()
registry.addCallback(BeforeInvocationEvent, callback)
registry.addCallback(callback, BeforeInvocationEvent)

await registry.invokeCallbacks(new BeforeInvocationEvent({ agent: mockAgent }))

Expand All @@ -27,8 +27,8 @@ describe('HookRegistryImplementation', () => {
const callback1 = vi.fn()
const callback2 = vi.fn()

registry.addCallback(BeforeInvocationEvent, callback1)
registry.addCallback(BeforeInvocationEvent, callback2)
registry.addCallback(callback1, BeforeInvocationEvent)
registry.addCallback(callback2, BeforeInvocationEvent)

await registry.invokeCallbacks(new BeforeInvocationEvent({ agent: mockAgent }))

Expand All @@ -40,8 +40,8 @@ describe('HookRegistryImplementation', () => {
const beforeCallback = vi.fn()
const afterCallback = vi.fn()

registry.addCallback(BeforeInvocationEvent, beforeCallback)
registry.addCallback(AfterInvocationEvent, afterCallback)
registry.addCallback(beforeCallback, BeforeInvocationEvent)
registry.addCallback(afterCallback, AfterInvocationEvent)

await registry.invokeCallbacks(new BeforeInvocationEvent({ agent: mockAgent }))

Expand All @@ -61,8 +61,8 @@ describe('HookRegistryImplementation', () => {

const provider: HookProvider = {
registerCallbacks: (reg) => {
reg.addCallback(BeforeInvocationEvent, beforeCallback)
reg.addCallback(AfterInvocationEvent, afterCallback)
reg.addCallback(beforeCallback, BeforeInvocationEvent)
reg.addCallback(afterCallback, AfterInvocationEvent)
},
}

Expand All @@ -87,7 +87,7 @@ describe('HookRegistryImplementation', () => {
// Verify _currentProvider is cleared by registering another provider successfully
const workingProvider: HookProvider = {
registerCallbacks: (reg) => {
reg.addCallback(BeforeInvocationEvent, vi.fn())
reg.addCallback(vi.fn(), BeforeInvocationEvent)
},
}

Expand All @@ -105,8 +105,8 @@ describe('HookRegistryImplementation', () => {
callOrder.push(2)
})

registry.addCallback(BeforeInvocationEvent, callback1)
registry.addCallback(BeforeInvocationEvent, callback2)
registry.addCallback(callback1, BeforeInvocationEvent)
registry.addCallback(callback2, BeforeInvocationEvent)

await registry.invokeCallbacks(new BeforeInvocationEvent({ agent: mockAgent }))

Expand All @@ -122,8 +122,8 @@ describe('HookRegistryImplementation', () => {
callOrder.push(2)
})

registry.addCallback(AfterInvocationEvent, callback1)
registry.addCallback(AfterInvocationEvent, callback2)
registry.addCallback(callback1, AfterInvocationEvent)
registry.addCallback(callback2, AfterInvocationEvent)

await registry.invokeCallbacks(new AfterInvocationEvent({ agent: mockAgent }))

Expand All @@ -137,7 +137,7 @@ describe('HookRegistryImplementation', () => {
completed = true
})

registry.addCallback(BeforeInvocationEvent, callback)
registry.addCallback(callback, BeforeInvocationEvent)

await registry.invokeCallbacks(new BeforeInvocationEvent({ agent: mockAgent }))

Expand All @@ -149,7 +149,7 @@ describe('HookRegistryImplementation', () => {
throw new Error('Hook failed')
})

registry.addCallback(BeforeInvocationEvent, callback)
registry.addCallback(callback, BeforeInvocationEvent)

await expect(registry.invokeCallbacks(new BeforeInvocationEvent({ agent: mockAgent }))).rejects.toThrow(
'Hook failed'
Expand All @@ -162,8 +162,8 @@ describe('HookRegistryImplementation', () => {
})
const callback2 = vi.fn()

registry.addCallback(BeforeInvocationEvent, callback1)
registry.addCallback(BeforeInvocationEvent, callback2)
registry.addCallback(callback1, BeforeInvocationEvent)
registry.addCallback(callback2, BeforeInvocationEvent)

await expect(registry.invokeCallbacks(new BeforeInvocationEvent({ agent: mockAgent }))).rejects.toThrow(
'First callback failed'
Expand All @@ -182,8 +182,8 @@ describe('HookRegistryImplementation', () => {
callOrder.push('async')
})

registry.addCallback(BeforeInvocationEvent, syncCallback)
registry.addCallback(BeforeInvocationEvent, asyncCallback)
registry.addCallback(syncCallback, BeforeInvocationEvent)
registry.addCallback(asyncCallback, BeforeInvocationEvent)

await registry.invokeCallbacks(new BeforeInvocationEvent({ agent: mockAgent }))

Expand All @@ -201,7 +201,7 @@ describe('HookRegistryImplementation', () => {
it('returns cleanup function that removes the callback', async () => {
const callback = vi.fn()

const cleanup = registry.addCallback(BeforeInvocationEvent, callback)
const cleanup = registry.addCallback(callback, BeforeInvocationEvent)
cleanup()

await registry.invokeCallbacks(new BeforeInvocationEvent({ agent: mockAgent }))
Expand All @@ -212,7 +212,7 @@ describe('HookRegistryImplementation', () => {
it('cleanup function is idempotent', async () => {
const callback = vi.fn()

const cleanup = registry.addCallback(BeforeInvocationEvent, callback)
const cleanup = registry.addCallback(callback, BeforeInvocationEvent)
cleanup()
cleanup()
cleanup()
Expand All @@ -226,8 +226,8 @@ describe('HookRegistryImplementation', () => {
const callback1 = vi.fn()
const callback2 = vi.fn()

const cleanup1 = registry.addCallback(BeforeInvocationEvent, callback1)
registry.addCallback(BeforeInvocationEvent, callback2)
const cleanup1 = registry.addCallback(callback1, BeforeInvocationEvent)
registry.addCallback(callback2, BeforeInvocationEvent)
cleanup1()

await registry.invokeCallbacks(new BeforeInvocationEvent({ agent: mockAgent }))
Expand All @@ -241,7 +241,7 @@ describe('HookRegistryImplementation', () => {

const provider: HookProvider = {
registerCallbacks: (reg) => {
reg.addCallback(BeforeInvocationEvent, callback)
reg.addCallback(callback, BeforeInvocationEvent)
},
}

Expand All @@ -261,8 +261,8 @@ describe('HookRegistryImplementation', () => {

const provider: HookProvider = {
registerCallbacks: (reg) => {
reg.addCallback(BeforeInvocationEvent, beforeCallback)
reg.addCallback(AfterInvocationEvent, afterCallback)
reg.addCallback(beforeCallback, BeforeInvocationEvent)
reg.addCallback(afterCallback, AfterInvocationEvent)
},
}

Expand All @@ -281,7 +281,7 @@ describe('HookRegistryImplementation', () => {

const provider: HookProvider = {
registerCallbacks: (reg) => {
reg.addCallback(BeforeInvocationEvent, callback)
reg.addCallback(callback, BeforeInvocationEvent)
},
}

Expand All @@ -299,7 +299,7 @@ describe('HookRegistryImplementation', () => {

const provider1: HookProvider = {
registerCallbacks: (reg) => {
reg.addCallback(BeforeInvocationEvent, callback)
reg.addCallback(callback, BeforeInvocationEvent)
},
}

Expand All @@ -321,13 +321,13 @@ describe('HookRegistryImplementation', () => {

const provider1: HookProvider = {
registerCallbacks: (reg) => {
reg.addCallback(BeforeInvocationEvent, callback1)
reg.addCallback(callback1, BeforeInvocationEvent)
},
}

const provider2: HookProvider = {
registerCallbacks: (reg) => {
reg.addCallback(BeforeInvocationEvent, callback2)
reg.addCallback(callback2, BeforeInvocationEvent)
},
}

Expand All @@ -347,11 +347,11 @@ describe('HookRegistryImplementation', () => {

const provider: HookProvider = {
registerCallbacks: (reg) => {
reg.addCallback(BeforeInvocationEvent, providerCallback)
reg.addCallback(providerCallback, BeforeInvocationEvent)
},
}

registry.addCallback(BeforeInvocationEvent, directCallback)
registry.addCallback(directCallback, BeforeInvocationEvent)
registry.addHook(provider)
registry.removeHook(provider)

Expand All @@ -366,7 +366,7 @@ describe('HookRegistryImplementation', () => {

const provider: HookProvider = {
registerCallbacks: (reg) => {
reg.addCallback(BeforeInvocationEvent, callback)
reg.addCallback(callback, BeforeInvocationEvent)
},
}

Expand All @@ -391,16 +391,16 @@ describe('HookRegistryImplementation', () => {

const provider: HookProvider = {
registerCallbacks: (reg) => {
reg.addCallback(BeforeInvocationEvent, callback1)
reg.addCallback(BeforeInvocationEvent, callback2)
reg.addCallback(callback1, BeforeInvocationEvent)
reg.addCallback(callback2, BeforeInvocationEvent)
},
}

registry.addHook(provider)
registry.removeHook(provider)

const cleanup = registry.addCallback(BeforeInvocationEvent, callback1)
registry.addCallback(BeforeInvocationEvent, callback2)
const cleanup = registry.addCallback(callback1, BeforeInvocationEvent)
registry.addCallback(callback2, BeforeInvocationEvent)
cleanup()

await registry.invokeCallbacks(new BeforeInvocationEvent({ agent: mockAgent }))
Expand Down
Loading
Loading