Greptile Summary

This PR adds an agent:turn:end internal hook event emitted from recordTurnCompletion() on both the success and error paths, along with a type guard (isAgentTurnEndEvent), a new AgentTurnEndHookContext type, and manager-level tests covering both paths.

• The AgentTurnEndHookContext type does not include the optional errorCode field that the PR description explicitly advertises, meaning failure-path consumers receive success: false with no way to distinguish error codes without additional lookups.
• fireAndForgetHook (no timeout, no concurrency limit) is used instead of the existing fireAndForgetBoundedHook (2 s timeout, max-16 concurrency), leaving hook handlers that hang indefinitely able to accumulate stuck promises across turns.

Confidence Score: 4/5

Safe to merge with minor improvements; no correctness or security issues found.

Only P2 findings: a missing errorCode field in the hook context and use of the unbounded fireAndForgetHook instead of fireAndForgetBoundedHook. No logic errors, no security concerns, and the new tests cover both success and failure paths.

src/acp/control-plane/manager.core.ts (fireAndForgetHook choice) and src/hooks/internal-hooks.ts (missing errorCode in context type)

This is a comment left during a code review.
Path: src/hooks/internal-hooks.ts
Line: 171-178

Comment:
**`errorCode` missing from hook context despite PR description**

The PR description explicitly states "The hook carries `sessionKey`, `success`, `durationMs`, and optional `errorCode` on failures," but `AgentTurnEndHookContext` has no `errorCode` field and the `createInternalHookEvent` call in `manager.core.ts` does not pass it. Hook consumers receiving `success: false` cannot determine the failure reason (e.g., `ACP_TURN_FAILED` vs `ACP_SESSION_INIT_FAILED`) without re-examining state elsewhere. Adding the optional field now avoids a future breaking change to the hook contract.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/acp/control-plane/manager.core.ts
Line: 1685-1694

Comment:
**No timeout protection on hook execution**

`fireAndForgetHook` simply attaches a `.catch()` to the already-running promise — it has no timeout, no concurrency cap, and no queue limit. `triggerInternalHook` awaits each handler sequentially, so a single misbehaving or slow handler will keep the promise pending indefinitely, silently accumulating stuck microtasks across many turns. `fireAndForgetBoundedHook` already exists and enforces a 2 s timeout, max-16 concurrency, and max-256 queue depth. It expects a factory `() => Promise<unknown>`, so the call site would need to wrap `triggerInternalHook(...)` in an arrow function instead of passing the already-constructed promise directly.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "test(acp): cover agent turn end hook emi..." | Re-trigger Greptile}

Codex review: needs changes before merge.

Keep open. Current main still does not implement the intended agent:turn:end internal hook, and this PR remains the focused implementation attempt. It is not ready to close or merge as-is because the proposed payload drops the advertised failure errorCode, and the hook dispatch path uses the unbounded fire-and-forget helper even though a bounded helper exists.

Required change before merge:

Keep this PR open for maintainer review or an in-place update. The best path is to emit agent:turn:end from AcpSessionManager.recordTurnCompletion() on both success and failure, include sessionKey, success, durationMs, and optional errorCode, use bounded non-blocking dispatch or document a deliberate exception, cover success/failure manager emission and the type guard, and update hook docs if the event becomes documented operator-facing behavior.

Best possible solution:

Keep this PR open for maintainer review or an in-place update. The best path is to emit agent:turn:end from AcpSessionManager.recordTurnCompletion() on both success and failure, include sessionKey, success, durationMs, and optional errorCode, use bounded non-blocking dispatch or document a deliberate exception, cover success/failure manager emission and the type guard, and update hook docs if the event becomes documented operator-facing behavior.

Acceptance criteria:

• pnpm test src/hooks/internal-hooks.test.ts src/acp/control-plane/manager.test.ts
• pnpm check:changed in Testbox before handoff if the PR is updated

What I checked:

• Current ACP completion path has no hook emission: Successful and failed ACP turns call recordTurnCompletion() with timing/error inputs only; there is no sessionKey, createInternalHookEvent(), or triggerInternalHook() call on current main. (src/acp/control-plane/manager.core.ts:851, 213f92a9ef0e)
• Current completion recorder is stats-only: recordTurnCompletion() computes duration, updates completed/failed counts, records error codes, and returns. It does not expose a per-turn hook seam. (src/acp/control-plane/manager.core.ts:1655, 213f92a9ef0e)
• No current-main turn-end hook symbols: Exact searches for agent:turn:end, AgentTurnEnd, isAgentTurnEnd, and turn:end returned no matches in source, docs, packages, UI, or plugins on current main. (213f92a9ef0e)
• Internal hooks docs do not list the event: The documented internal hook event table lists command, session, agent bootstrap, gateway, and message events, but not agent:turn:end. Public page: https://docs.openclaw.ai/automation/hooks. (docs/automation/hooks.md:36, 213f92a9ef0e)
• Existing plugin hook is related but not equivalent: Current main has a typed plugin agent_end event with success/error/duration fields, but searches show it is wired through agent/plugin runners, not the ACP manager recordTurnCompletion() seam requested here. (src/plugins/hook-types.ts:272, 213f92a9ef0e)
• Latest release also lacks the hook: The latest release tag has the same stats-only ACP completion path and no exact agent:turn:end/AgentTurnEnd symbols. (src/acp/control-plane/manager.core.ts:1655, be8c24633aaa)

Likely related people:

• steipete: Recent commit history for src/acp/control-plane/manager.core.ts shows ACP turn completion and idle/error transition work, including 277cc640b19b near the same run-turn completion path. Earlier hook infrastructure and the original fireAndForgetHook helper also trace to steipete commits. (role: recent ACP manager and hooks maintainer; confidence: medium; commits: 277cc640b19b, caae34cbafab, faba508fe0ae; files: src/acp/control-plane/manager.core.ts, src/hooks/fire-and-forget.ts, src/hooks/internal-hooks.ts)
• vincentkoc: Recent history for hook contracts and asynchronous hook dispatch points to vincentkoc, including the commit that added fireAndForgetBoundedHook() and recent internal-hook contract extraction work. The provided related context also identifies vincentkoc on adjacent agent/tool lifecycle hook work. (role: recent internal hook/runtime helper maintainer; confidence: medium; commits: 8154337cb6ff, 97d1b88e3faa, df9d26eb4335; files: src/hooks/fire-and-forget.ts, src/hooks/internal-hooks.ts, src/acp/control-plane/manager.core.ts)
• osolmaz: The ACP manager file and its observability/turn-stat scaffolding appear to originate in feat: ACP thread-bound agents, which makes osolmaz a lower-confidence historical routing candidate for the ACP manager boundary. (role: original ACP manager introducer; confidence: low; commits: a7d56e3554d0; files: src/acp/control-plane/manager.core.ts)

Remaining risk / open question:

• Merging the PR unchanged would create a new hook contract without the advertised optional errorCode, making failure classification harder for hook consumers and future additions more awkward.
• Merging the PR unchanged would dispatch a per-turn hook through unbounded fire-and-forget execution while triggerInternalHook() awaits handlers sequentially and a bounded helper already exists.
• Hook timing still needs maintainer judgment because the proposed emission point is before the final ACP session state transition to idle or error.
• The current PR is a feature/API surface change, so docs may need an update if maintainers decide the event is operator-facing rather than purely internal.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 213f92a9ef0e.

@mantisai-bot Hi! This is a great hook which I also need for an implementation I have for a security plugin. I'd love to see the relatively minor issues flagged addressed and am happy to help/collaborate or just cheerlead. Hope this one goes in!

Codex review: needs real behavior proof before merge. Reviewed June 1, 2026, 11:32 AM ET / 15:32 UTC.

Summary
The PR adds documented ACP agent:turn:end and agent:turn:save internal hook events, save-outcome callback wiring for manager/direct/dispatch ACP turns, and focused tests/docs.

PR surface: Source +338, Tests +952, Docs +9. Total +1299 across 17 files.

Reproducibility: not applicable. this is a feature PR rather than a bug report. Source checks show current main and v2026.5.28 do not have the requested hook surface, while the PR tests exercise the proposed paths.

Review metrics: 2 noteworthy metrics.

• Hook/API surfaces: 2 documented events, 1 save callback contract added. The change creates a consumer-visible hook lifecycle contract, so names, payload fields, and timing need maintainer acceptance before merge.
• ACP completion ordering: 1 completion path changed from synchronous stats recording to awaited save callback. This is the session-state timing change that ordinary tests may not fully settle for existing ACP operators.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🐚 platinum hermit
Result: blocked until stronger real behavior proof is added.

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

Rank-up moves:

• [P1] Add redacted terminal/log output from a real direct or dispatch ACP turn showing both agent:turn:end and agent:turn:save payloads after the patch.
• [P1] Get maintainer acceptance for the split event names, payload fields, and awaited save timing before merge.

Proof guidance:

• [P1] Needs stronger real behavior proof before merge: The PR body includes terminal proof for production transcript persistence returning saved, but it does not show a real direct or dispatch ACP turn emitting both new hook events and payloads after the fix. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

• [P1] The documented event names, payload fields, and timing guarantees become a hook/API contract for internal hook consumers once merged.
• [P1] ACP runTurn() now awaits the save callback before task terminal marking and idle/error transition, so slow or failing save layers can shift observable session-state timing even though hook handlers are bounded.
• [P1] The supplied terminal proof exercises transcript persistence only; it does not yet show a real direct or dispatch ACP turn emitting both new hook event payloads.

Maintainer options:

1. Require API and proof sign-off before merge (recommended)
   Have maintainers explicitly accept the agent:turn:end/agent:turn:save names, payloads, and awaited save timing, then require real direct or dispatch ACP hook-output proof before landing.
2. Accept the new hook contract intentionally
   Maintainers can land the split as the durable-readiness contract if they are comfortable owning it as a documented internal hook API and with the save callback delaying ACP state transitions.
3. Pause if the lifecycle boundary is unsettled
   If maintainers want a different event name, permission model, or non-awaited save shape, pause this branch and keep the remaining work in the hook/API design discussion instead.

Next step before merge

• [P1] Remaining action is maintainer API/timing acceptance plus contributor proof, not a narrow automation-safe repair.

Security
Cleared: No concrete security or supply-chain regression was found; the diff does not touch dependencies, workflows, secrets, or command execution, and the new hook payloads do not embed transcript text.

Hi maintainers — I’ve gone back through the earlier review feedback and believe the gaps are now covered in the current head 72ca7a44f2548bb172d8e1b5f004b7b8df50e8c3.

Covered since the earlier comments:

• AgentTurnEndHookContext now includes optional errorCode.
• recordTurnCompletion() now propagates failure codes into the agent:turn:end payload, including ACP_TURN_FAILED in the failure test/proof path.
• Hook dispatch now uses fireAndForgetBoundedHook(() => triggerInternalHook(...)) rather than unbounded fire-and-forget execution.
• isAgentTurnEndEvent() validates the new event shape, including optional string errorCode.
• Docs and changelog mention the new hook surface.
• The stale review threads for those items are resolved.

Fresh local validation run:

• pnpm test src/hooks/internal-hooks.test.ts src/acp/control-plane/manager.test.ts
• pnpm exec oxfmt --check --threads=1 src/acp/control-plane/manager.core.ts src/acp/control-plane/manager.test.ts src/hooks/internal-hooks.ts src/hooks/internal-hooks.test.ts CHANGELOG.md docs/automation/hooks.md
• pnpm check:changed

Current PR checks are green, merge state is CLEAN, and the proof labels are present (proof: supplied, proof: sufficient).

Could you please take another look when you get a chance? Happy to adjust if there’s any remaining API/timing concern around the ACP completion hook seam.

@zeroaltitude Appreciate the cheerleading 🙂 genuinely helps. Same here — hope this one lands!

Hey :)
@mantisai-bot is my OC deployment.
I'm happy to see this thing getting some traction! If anyone needs anything done, please ping me as that's more reliable than trying to get to Mantis. It's trying its best but those hits don't always reach me.

@JonathanBraverDev @zeroaltitude — adding a use-case perspective from the field.

We run OpenClaw as a persistent agent runtime (Cophy) with a tiered memory system (Core/Episodic/Archive layers). Right now, post-turn memory consolidation is wired to session-level signals, which means we occasionally miss the write window when a turn ends abruptly or the session compacts mid-conversation. An agent:turn:end hook would let us trigger memory flush deterministically at the seam where it actually matters.

Concretely, the pattern we want:

agent:turn:end → check if turn produced L1/L2 information → write to memory files → confirm persistence

Without a reliable end-of-turn signal, we have to either poll file state or rely on the agent remembering to flush before context compaction — both are fragile. The errorCode addition in the latest revision is particularly useful: we can skip the flush on ACP_TURN_FAILED and avoid writing partial/inconsistent state.

One question for the implementation: does agent:turn:end fire before or after the session transcript is committed? The ordering matters for memory systems that want to reference the turn content in the flush payload.

Hope this lands — it fills a real gap for anyone building stateful agent workflows on top of OpenClaw.

Context: Running OpenClaw as a persistent agent runtime, exploring memory continuity across sessions.

@clawsweeper re-review

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

• State: Superseded
• Detail: A newer re-review for this item started before this run finished, so GitHub cancelled this older run. Check the latest ClawSweeper run for the current result.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/26721498646
• Updated: 2026-05-31T18:58:59.669Z

@clawsweeper re-review

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

• State: Complete
• Detail: The targeted re-review finished, the durable review comment was updated, and the synced verdict was routed.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/26721704063
• Updated: 2026-05-31T19:13:35.531Z

@clawsweeper re-review

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

• State: Complete
• Detail: The targeted re-review finished, the durable review comment was updated, and the synced verdict was routed.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/26722012945
• Updated: 2026-05-31T19:28:10.973Z

@icophy I've Split the hooks by the guarantee to avoid ambiguity. Note that you will need agent:turn:save post split.

@clawsweeper re-review

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

• State: Complete
• Detail: The targeted re-review finished, the durable review comment was updated, and the synced verdict was routed.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/26734132122
• Updated: 2026-06-01T03:57:42.051Z

Closing this battlefield branch in favor of two review-friendly sibling PRs based on current main:

• feat(hooks): add agent turn end hook #89152 — clean agent:turn:end lifecycle hook only.
• feat(hooks): add ACP turn transcript save hook #89154 — ACP agent:turn:save outcome hook and write-safe save evidence only.

This PR bundled lifecycle notification, save durability semantics, direct ACP transcript persistence, and several rounds of review hardening into one branch. The split keeps the safe lifecycle hook separate from the more delicate save-outcome contract.