Skip to content

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

Open
Unshure wants to merge 1 commit intostrands-agents:mainfrom
Unshure:reorder-callback
Open

fix: make callback the first parameter for adding hook callback#545
Unshure wants to merge 1 commit intostrands-agents:mainfrom
Unshure:reorder-callback

Conversation

@Unshure
Copy link
Member

@Unshure Unshure commented Feb 18, 2026

Description

Breaking Change This reorders the parameters for the HookProvider addCallback method to align with the feedback in the python sdk pr: strands-agents/sdk-python#1706

Related Issues

N/A

Documentation PR

TODO

Type of Change

Breaking change

Testing

How have you tested the change?

  • I ran npm run check

Checklist

  • I have read the CONTRIBUTING document
  • I have added any necessary tests that prove my fix is effective or my feature works
  • I have updated the documentation accordingly
  • I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@github-actions github-actions bot added the strands-running <strands-managed> Whether or not an agent is currently running label Feb 18, 2026
@github-actions
Copy link

Issue: The referenced Python SDK PR #1706 is still open and not yet merged.

Suggestion: Consider waiting until the Python SDK PR is finalized and merged to ensure alignment. If the Python PR changes direction during review, this change would need to be updated accordingly.

@github-actions
Copy link

Assessment: Comment

This PR correctly reorders the addCallback parameters to (callback, eventType) for consistency with the Python SDK. The mechanical changes look correct and comprehensive.

Review Feedback
  • API Review Process: This is a breaking change to a public API (HookRegistry.addCallback). Per API Bar Raising guidelines, consider adding the needs-api-review label and documenting the API change in the PR description with before/after examples.
  • Dependency on Python SDK: The referenced Python SDK PR #1706 is still open. Consider coordinating merge timing.
  • Documentation: The Documentation PR is marked as "TODO" - ensure external docs at strandsagents.com are updated before or alongside this merge.

The code changes themselves are clean and all usages have been updated consistently.

@github-actions
Copy link

Code Review - PR #545

Assessment: Comment

This PR correctly reorders the addCallback parameters to (callback, eventType) for consistency with the Python SDK. The mechanical changes look correct and all usages have been updated.

Review Feedback
  • API Review Process: This is a breaking change to a public API (HookRegistry.addCallback). Per API Bar Raising guidelines, consider adding the needs-api-review label and documenting the API change in the PR description with before/after examples.
  • Dependency on Python SDK: The referenced Python SDK PR #1706 is still open and not yet merged. Consider coordinating merge timing to ensure alignment if that PR changes direction.
  • Documentation: The Documentation PR is marked as "TODO" - ensure external docs at strandsagents.com are updated before or alongside this merge.

The code changes are clean and comprehensive.

@github-actions github-actions bot removed the strands-running <strands-managed> Whether or not an agent is currently running label Feb 18, 2026
@zastrowm
Copy link
Member

Can we just remove HookRegistry and migrate to plugins? IMHO there's no need for agent.hooks in typescript if we're adding callbacks and plugins to the top level.

export interface HookRegistry {
is only those 3 methods and I'd rather avoid a breaking change and a migration later to Plugins if we can do it all in one go

@Unshure
Copy link
Member Author

Unshure commented Feb 18, 2026

Can we just remove HookRegistry and migrate to plugins? IMHO there's no need for agent.hooks in typescript if we're adding callbacks and plugins to the top level.

export interface HookRegistry {

is only those 3 methods and I'd rather avoid a breaking change and a migration later to Plugins if we can do it all in one go

@zastrowm Are we adding callbacks to the top level? I thought we would still expose agent.hook just like in python?

@zastrowm
Copy link
Member

Can we just remove HookRegistry and migrate to plugins? IMHO there's no need for agent.hooks in typescript if we're adding callbacks and plugins to the top level.

export interface HookRegistry {

is only those 3 methods and I'd rather avoid a breaking change and a migration later to Plugins if we can do it all in one go

@zastrowm Are we adding callbacks to the top level? I thought we would still expose agent.hook just like in python?

In TS, we don't expose the ability to fire hooks and kept more apis as an implementation detail. I haven't seen any usage of folks firing custom events yet, and given the number of changes of hooks went through in python I figured it's better not to expose these items until we have use cases

So in TS, you already don't have access to callbacks; the only apis we have are addCallback, addHook, removeHook; and all 3 are effectively not needed once we have addHook and plugins on agent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants