Skip to content

feat(hooks): add agent and tool lifecycle boundaries#18889

Closed
vincentkoc wants to merge 1 commit into
openclaw:mainfrom
vincentkoc:vincentkoc-code/hooks-agent-tool-lifecycle-r3
Closed

feat(hooks): add agent and tool lifecycle boundaries#18889
vincentkoc wants to merge 1 commit into
openclaw:mainfrom
vincentkoc:vincentkoc-code/hooks-agent-tool-lifecycle-r3

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Feb 17, 2026 •
edited by greptile-apps Bot
Loading

Copy link
Copy Markdown
Member

Why

Agent execution lacked complete lifecycle boundaries around thinking, response generation, and tool execution, reducing traceability for observability and policy systems. This PR adds those boundaries and ensures tool lifecycle hooks carry toolCallId context when available. lobster-biscuit

Split Context

This PR was accidentally closed after a messed up rebase #12583 which was originally split from closed umbrella PR #9761: #9761.

Detailed Changes

  • Added internal agent lifecycle hook emissions:
    • agent:thinking:start / agent:thinking:end
    • agent:response:start / agent:response:end
    • agent:tool:start / agent:tool:end
  • Added runAfterToolCallHook wiring for tool execution lifecycle completion
  • Propagated toolCallId through before/after tool hook contexts where available
  • Extended client tool definition adapter to pass toolCallId and run after-tool hook flow
  • Added focused tests for agent lifecycle emissions and tool lifecycle behavior

Related Links, Issues and Resolution

Greptile Summary

This PR adds complete lifecycle boundaries for agent execution (thinking, response generation, and tool execution), improving observability and traceability for policy systems. The implementation propagates toolCallId through tool lifecycle hooks and ensures consistent hook firing across different execution paths.

Key improvements:

  • Split lifecycle hook logic into dedicated lifecycle-hooks.ts module
  • Ensured agent:thinking:start/end fire for all agent runs
  • Added conditional agent:response:start/end hooks that only fire when response output is generated
  • Propagated toolCallId to before_tool_call and after_tool_call hooks
  • Added runAfterToolCallHook wiring for both success and error paths in tool execution
  • Comprehensive test coverage validates hook emissions in different scenarios

Confidence Score: 5/5

  • Safe to merge - well-structured refactoring with comprehensive test coverage
  • The changes are well-architected with proper error handling throughout. The new lifecycle-hooks.ts module cleanly separates concerns, all hook emission paths include error handling that logs warnings without failing execution, and the test coverage validates the core lifecycle hook behavior. The toolCallId propagation is handled consistently with proper fallbacks.
  • No files require special attention

Last reviewed commit: d48824c

@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation agents Agent runtime and tooling labels Feb 17, 2026
@vincentkoc vincentkoc mentioned this pull request Feb 17, 2026
Closed
@vincentkoc
vincentkoc marked this pull request as ready for review February 17, 2026 05:29
This was referenced Feb 17, 2026
Closed
Closed
Closed
This was referenced Feb 26, 2026
Closed
Closed
@vincentkoc vincentkoc self-assigned this Mar 3, 2026
@mantisai-bot mantisai-bot mentioned this pull request Mar 6, 2026
Closed
@vincentkoc vincentkoc mentioned this pull request Mar 8, 2026
Closed
@vincentkoc
vincentkoc force-pushed the vincentkoc-code/hooks-agent-tool-lifecycle-r3 branch from d48824c to d942194 Compare March 8, 2026 04:58
@openclaw-barnacle openclaw-barnacle Bot added size: M maintainer Maintainer-authored PR and removed docs Improvements or additions to documentation size: L labels Mar 8, 2026
@EuphorieHQ EuphorieHQ mentioned this pull request Mar 11, 2026
Closed
3 tasks
@steipete steipete mentioned this pull request Apr 26, 2026
Closed
This was referenced Apr 27, 2026
Open
Closed
Closed
@clawsweeper clawsweeper Bot mentioned this pull request May 17, 2026
Open
@clawsweeper clawsweeper Bot added the rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. label May 19, 2026
@clawsweeper

clawsweeper Bot commented May 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: found issues before merge. Reviewed July 5, 2026, 8:46 AM ET / 12:46 UTC.

Summary
The PR adds internal agent lifecycle hook emissions around thinking/response phases, propagates client-tool toolCallId values, and adds focused lifecycle/tool hook tests.

PR surface: Source +151, Tests +197. Total +348 across 7 files.

Reproducibility: yes. at source level for the PR defects. Current main runs through src/agents/embedded-agent-runner, has no tracked Pi-era target paths from the PR, and preserves client-tool source order through reserve/complete/discard slots in the active runner.

Review metrics: 3 noteworthy metrics.

  • Active runtime target: 7 touched PR files target removed Pi-era paths. The submitted branch needs a current-runtime rebase before it can affect OpenClaw's active embedded agent execution path.
  • Advertised lifecycle actions: 6 advertised, 4 helper-emitted. Hook event names become observable contracts, so advertised tool start/end events need implementation or removal before merge.
  • Client-tool attribution model: current reserve/complete/discard recorder vs PR single callback path. Current code protects parallel client-tool ordering, so the rebase must preserve that state association.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🌊 off-meta tidepool
Patch quality: 🧂 unranked krab
Result: blocked by patch quality or review findings.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Rebase the lifecycle changes onto src/agents/embedded-agent-runner and src/agents/agent-tool-definition-adapter.ts.
  • Preserve the current client-tool reserve/complete/discard slot model and its parallel-order behavior.
  • [P2] Implement agent:tool:start/end on the active tool path or remove those events from the PR contract.

Risk before merge

  • [P1] Merging this head as-is would not wire the feature into the active embedded-agent runtime because the patch still targets removed Pi-era files.
  • [P1] The lifecycle action names, payload fields, timing, and ordering become a compatibility contract for hook consumers once released.
  • [P1] A naive rebase could regress current client-tool reserve/complete/discard slot attribution and mis-associate parallel client-hosted tool calls with run state.
  • [P1] The PR advertises agent:tool:start and agent:tool:end, but the submitted helper only implements thinking and response events.

Maintainer options:

  1. Rebase onto active runtime (recommended)
    Move the lifecycle and client-tool ID work to src/agents/embedded-agent-runner and src/agents/agent-tool-definition-adapter.ts while preserving current reserve/complete/discard ordering.
  2. Approve hook contract explicitly
    Maintainers can accept the event names, payload fields, timing, and ordering as a new compatibility surface before implementation lands.
  3. Pause for hook API design
    If lifecycle semantics are not settled, pause this PR and keep the remaining API decision in the related hook trackers instead of merging a partial contract.

Next step before merge

  • [P2] Protected maintainer feature PR needs maintainer API-contract judgment plus an author or maintainer rebase rather than cleanup closure or an automated repair lane.

Maintainer decision needed

  • Question: Should OpenClaw accept these internal agent lifecycle hook events as a compatibility contract after rebasing them onto the active embedded runner?
  • Rationale: The remaining blocker is not only mechanical rebase work; event names, payload fields, ordering, session attribution, and tool state semantics become an API surface for hook consumers.
  • Likely owner: vincentkoc — vincentkoc owns the current branch and related hook-series history, so he is the best first owner for deciding whether to rebase and carry the hook contract forward.
  • Options:
    • Rebase and accept contract (recommended): Move the implementation to the active runner and adapter, preserve slot ordering, add missing tool events, and treat the lifecycle semantics as approved for merge.
    • Pause for canonical hook design: Hold this PR while maintainers settle lifecycle and managed hook semantics in the related hook trackers.
    • Close this branch later: If the hook direction is not wanted in core, close this PR after stating the supported plugin or internal-hook alternative.

Security
Cleared: No concrete security or supply-chain regression was found; the remaining concerns are hook API compatibility and session/tool-state correctness.

Review findings

  • [P1] Rebase lifecycle wiring onto the active runner — src/agents/pi-embedded-runner/run/attempt.ts:133-138
  • [P1] Preserve client-tool slot attribution — src/agents/pi-embedded-runner/run/attempt.ts:1177-1178
  • [P2] Implement or remove the advertised tool events — src/agents/pi-embedded-runner/run/lifecycle-hooks.ts:45-63
Review details

Best possible solution:

Rebase the useful lifecycle work onto the active embedded runner and adapter, preserve current client-tool slot ordering, and ship only after maintainers accept the hook names, payloads, timing, and complete tool-event coverage.

Do we have a high-confidence way to reproduce the issue?

Yes, at source level for the PR defects. Current main runs through src/agents/embedded-agent-runner, has no tracked Pi-era target paths from the PR, and preserves client-tool source order through reserve/complete/discard slots in the active runner.

Is this the best way to solve the issue?

No, not as submitted. The direction is plausible, but the best fix is a current-runtime rebase plus explicit maintainer acceptance of the lifecycle hook contract and complete tool-event coverage.

Full review comments:

  • [P1] Rebase lifecycle wiring onto the active runner — src/agents/pi-embedded-runner/run/attempt.ts:133-138
    This PR imports and wires lifecycle helpers under src/agents/pi-embedded-runner, but current main runs through src/agents/embedded-agent-runner and no longer has the Pi-era target directory. Move the hook wiring to the active runtime before merge so the feature affects current behavior.
    Confidence: 0.97
  • [P1] Preserve client-tool slot attribution — src/agents/pi-embedded-runner/run/attempt.ts:1177-1178
    The PR records a single detected client tool call with an ID, while current main reserves, completes, and discards per-toolCallId slots before async hooks finish to keep parallel client-hosted calls ordered. Carry the change through the current recorder shape or pending tool calls can be associated with the wrong run state after rebase.
    Confidence: 0.91
  • [P2] Implement or remove the advertised tool events — src/agents/pi-embedded-runner/run/lifecycle-hooks.ts:45-63
    The PR advertises agent:tool:start and agent:tool:end, but the added lifecycle helper only emits thinking and response events. Add tool start/end emission on the active tool execution path or narrow the PR contract and tests to the events actually implemented.
    Confidence: 0.89
  • [P2] Preserve lifecycle hook ordering — src/agents/pi-embedded-runner/run/attempt.ts:1650
    emitThinkingStart is dispatched without awaiting it, and response start also fire-and-forgets from the assistant-message callback. A slow async start handler can complete after the end handler, so lifecycle hooks need ordered delivery or a clearly documented asynchronous contract before they ship.
    Confidence: 0.82

Overall correctness: patch is incorrect
Overall confidence: 0.93

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 601b80c5f4c1.

Label changes

Label justifications:

  • P2: This is meaningful agent hook/API feature work with limited but non-trivial runtime and compatibility impact.
  • merge-risk: 🚨 compatibility: The PR introduces lifecycle hook action names, payload fields, timing, and ordering that can become a compatibility contract for hook consumers.
  • merge-risk: 🚨 session-state: The PR changes toolCallId propagation around client-hosted tool calls, which current main uses to associate pending tool-call state with a run/session.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🌊 off-meta tidepool and patch quality is 🧂 unranked krab.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The external-contributor proof gate does not apply to this MEMBER maintainer PR, though current-runtime proof after rebase would still help before merge.
Evidence reviewed

PR surface:

Source +151, Tests +197. Total +348 across 7 files.

View PR surface stats
Area Files Added Removed Net
Source 5 160 9 +151
Tests 2 201 4 +197
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 7 361 13 +348

What I checked:

Likely related people:

  • vincentkoc: vincentkoc authored and is assigned to this PR, authored the head commit, and has recent hook contract/tool correlation commits in the same feature area. (role: proposed implementation owner and hook-series contributor; confidence: high; commits: d9421946e987, 7f5a5a34dbf8, 747902a26a63; files: src/agents/pi-embedded-runner/run/attempt.ts, src/agents/pi-embedded-runner/run/lifecycle-hooks.ts, src/agents/pi-tool-definition-adapter.ts)
  • Sanjay Santhanam: Current-main blame for the active embedded runner, active tool adapter, and after_tool_call handler points to 6cc534b, which owns the current runtime shape this PR must rebase onto. (role: recent active runner contributor; confidence: medium; commits: 6cc534bbad7b; files: src/agents/embedded-agent-runner/run/attempt.ts, src/agents/agent-tool-definition-adapter.ts, src/agents/embedded-agent-subscribe.handlers.tools.ts)
  • Peter Steinberger: Peter Steinberger introduced the internal hooks system and recently restored hook contract docs, making him a relevant owner for the internal hook API direction. (role: adjacent internal hook surface introducer; confidence: medium; commits: faba508fe0ae, 53bdac249dbd; files: src/hooks/internal-hooks.ts, src/hooks/internal-hook-types.ts, docs/automation/hooks.md)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (6 earlier review cycles)
  • reviewed 2026-07-04T06:52:14.087Z sha d942194 :: found issues before merge. :: [P1] Rebase lifecycle wiring onto the active runner | [P1] Preserve client-tool slot attribution | [P2] Implement or remove the advertised tool events | [P2] Preserve lifecycle hook ordering
  • reviewed 2026-07-04T12:40:59.783Z sha d942194 :: found issues before merge. :: [P1] Rebase lifecycle wiring onto the active runner | [P1] Preserve client-tool slot attribution | [P2] Implement or remove the advertised tool events | [P2] Preserve lifecycle hook ordering
  • reviewed 2026-07-04T13:08:36.283Z sha d942194 :: found issues before merge. :: [P1] Rebase lifecycle wiring onto the active runner | [P1] Preserve client-tool slot attribution | [P2] Implement or remove the advertised tool events | [P2] Preserve lifecycle hook ordering
  • reviewed 2026-07-04T18:11:15.111Z sha d942194 :: found issues before merge. :: [P1] Rebase lifecycle wiring onto the active runner | [P1] Preserve client-tool slot attribution | [P2] Implement or remove the advertised tool events | [P2] Preserve lifecycle hook ordering
  • reviewed 2026-07-05T08:15:54.347Z sha d942194 :: found issues before merge. :: [P1] Rebase lifecycle wiring onto the active runner | [P1] Preserve client-tool slot attribution | [P2] Implement or remove the advertised tool events | [P2] Preserve lifecycle hook ordering
  • reviewed 2026-07-05T12:11:11.204Z sha d942194 :: found issues before merge. :: [P1] Rebase lifecycle wiring onto the active runner | [P1] Preserve client-tool slot attribution | [P2] Implement or remove the advertised tool events | [P2] Preserve lifecycle hook ordering

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. labels May 22, 2026
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jun 10, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 19, 2026
Closed
5 tasks
@steipete

steipete commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Closing as superseded by the current public plugin-hook lifecycle.

The old branch targets retired pi-* runner paths and adds a second internal agent:thinking/response/tool event stack. Current main already owns these boundaries through the typed plugin surface: model_call_started/model_call_ended, llm_input/llm_output, agent_end, and before_tool_call/after_tool_call. Tool-call identity is carried through src/agents/agent-tool-definition-adapter.ts, and the current hook wiring has focused lifecycle and once-only after-tool coverage. Replaying this branch would duplicate policy and telemetry on obsolete paths rather than improve the canonical hooks.

Thanks for the early lifecycle work, @vincentkoc.

@steipete steipete closed this Jul 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@vincentkoc vincentkoc

Labels

agents Agent runtime and tooling maintainer Maintainer-authored PR merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: M status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Milestone

No milestone

2 participants

@vincentkoc @steipete