Skip to content

fix(agents): merge config and per-call onStepStart hooks for sub-agent forwarding#59

Merged
zrosenbauer merged 4 commits intomainfrom
fix/agents-merge-onStepStart-hook
Mar 25, 2026
Merged

fix(agents): merge config and per-call onStepStart hooks for sub-agent forwarding#59
zrosenbauer merged 4 commits intomainfrom
fix/agents-merge-onStepStart-hook

Conversation

@zrosenbauer
Copy link
Member

Summary

  • Bug: The base agent only forwarded params.onStepStart to sub-agents via parentCtx, dropping config.onStepStart. This was inconsistent with onStepFinish, which correctly merged both config and per-call hooks via buildMergedHook.
  • Fix: Use buildMergedHook(log, config.onStepStart, params.onStepStart) in the parentCtx construction, matching the onStepFinish pattern.
  • Type addition: Adds onStepStart to AgentConfig so config-level step-start hooks can be defined (previously it only existed on GenerateParams).

Test plan

  • Added integration test: parent config and per-call onStepStart both fire for flow sub-agent steps
  • All existing lifecycle tests pass (37/37)
  • Typecheck passes

zrosenbauer and others added 2 commits March 25, 2026 12:54
…t forwarding

The base agent was only forwarding `params.onStepStart` to sub-agents via
parentCtx, ignoring `config.onStepStart`. This meant config-level onStepStart
hooks were silently dropped when forwarding to sub-agents, unlike onStepFinish
which correctly merged both hooks via buildMergedHook.

Also adds onStepStart to AgentConfig type for parity with onStepFinish.

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
@changeset-bot
Copy link

changeset-bot bot commented Mar 25, 2026

🦋 Changeset detected

Latest commit: 33042e2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@funkai/agents Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@coderabbitai
Copy link

coderabbitai bot commented Mar 25, 2026

Caution

Review failed

Pull request was closed or merged during review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: dcd0f10b-f7c0-48ab-8306-78709a41a91b

📥 Commits

Reviewing files that changed from the base of the PR and between b5c8192 and 33042e2.

📒 Files selected for processing (1)
  • packages/agents/src/integration/lifecycle.test.ts

📝 Walkthrough

Walkthrough

Adds an optional onStepStart lifecycle hook to AgentConfig, ensures config-level onStepStart is merged/forwarded into sub-agents (matching onStepFinish), and adds an integration test plus a changeset documenting the fix.

Changes

Cohort / File(s) Summary
Type Definitions
packages/agents/src/core/agents/types.ts
Add optional `onStepStart?: (event: StepStartEvent) => void
Hook Merging Logic
packages/agents/src/core/agents/base/agent.ts
Change parent context wiring to merge config-level onStepStart with per-call hook using buildMergedHook(log, config.onStepStart, params.onStepStart) so config hook is propagated to sub-agents (mirrors onStepFinish).
Tests & Release Notes
.changeset/fix-merge-onstepstart.md, packages/agents/src/integration/lifecycle.test.ts
Add changeset describing the patch; add integration test asserting both config-level and per-call onStepStart fire for delegated sub-agent steps and that the config-level hook runs before the per-call hook.

Sequence Diagram(s)

sequenceDiagram
    participant Caller
    participant ParentAgent as Parent Agent
    participant Merger as Hook Merger
    participant SubAgent as Sub-agent
    participant ConfigHook as Config onStepStart
    participant PerCallHook as Per-call onStepStart

    Caller->>ParentAgent: generate(params { onStepStart: PerCallHook })
    ParentAgent->>Merger: buildMergedHook(config.onStepStart, params.onStepStart)
    Merger-->>ParentAgent: mergedHook
    ParentAgent->>SubAgent: invoke with mergedHook
    SubAgent->>ParentAgent: step "work" start
    ParentAgent->>mergedHook: call StepStartEvent
    mergedHook->>ConfigHook: invoke (if present)
    mergedHook->>PerCallHook: invoke (if present)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: merging config and per-call onStepStart hooks for sub-agent forwarding, which is the core fix in this PR.
Description check ✅ Passed The description clearly explains the bug (config-level onStepStart being dropped), the fix (using buildMergedHook), and the type addition, with test plan details demonstrating thorough validation.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/agents/src/integration/lifecycle.test.ts`:
- Around line 1623-1626: The current assertion only checks for at least one
"work" event but the test intends to verify both the config-level and per-call
onStepStart hooks fired; update the assertion on the filtered stepEvents
(variable subSteps) to require two or more events (e.g., use
toBeGreaterThanOrEqual(2) or toBeGreaterThan(1)) so the test confirms both hooks
executed for the sub-flow.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: fee277c4-c8ca-485a-bd62-371bd27fa66b

📥 Commits

Reviewing files that changed from the base of the PR and between e60a34d and bb2eb7a.

📒 Files selected for processing (4)
  • .changeset/fix-merge-onstepstart.md
  • packages/agents/src/core/agents/base/agent.ts
  • packages/agents/src/core/agents/types.ts
  • packages/agents/src/integration/lifecycle.test.ts

FlowAgent doesn't structurally satisfy Agent at the type level (missing
model property), but works correctly at runtime. Added oxlint-disable
comment explaining the cast.

Co-Authored-By: Claude <noreply@anthropic.com>
@vercel
Copy link

vercel bot commented Mar 25, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
funkai Ready Ready Preview, Comment Mar 25, 2026 5:12pm

Request Review

Assert >= 2 events instead of > 0 to explicitly verify both config
and per-call onStepStart hooks fired for the sub-flow step.

Co-Authored-By: Claude <noreply@anthropic.com>
@zrosenbauer zrosenbauer merged commit 7f62f8f into main Mar 25, 2026
2 of 3 checks passed
@zrosenbauer zrosenbauer deleted the fix/agents-merge-onStepStart-hook branch March 25, 2026 17:08
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.

1 participant