feat(runtime): expose process lifecycle hooks to embedding hosts - #514
feat(runtime): expose process lifecycle hooks to embedding hosts#514MertBasar0 wants to merge 3 commits into
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
Codex review: needs maintainer review before merge. Reviewed August 24, 2026, 9:50 AM ET / 13:50 UTC. ClawSweeper reviewWhat this changesThe PR adds optional lifecycle callbacks to the embedding runtime so hosts can observe or gate ACP agent process spawn, spawn failure, and exit events. Merge readinessKeep open for maintainer review: the implementation is a focused, well-proven embedding feature, but it establishes a durable public lifecycle contract whose names, scopes, ordering, and admission semantics need explicit product sponsorship. Priority: P2 Review scores
Verification
Live VerificationCommand: Result: PASS (completed) Assertions:
How this fits togetherACPX’s embedding runtime creates ACP client processes for sessions, controls, and health probes. The new observer flows from runtime options into each client creation path, where it receives correlated child-process lifecycle events. flowchart LR
Host[Embedding host] --> Runtime[ACPX runtime]
Runtime --> Scope[Session or probe scope]
Scope --> Client[ACP client]
Client --> Admission[Lifecycle admission]
Admission --> Process[Agent process]
Process --> Events[Spawn failure or exit events]
Events --> Host
Decision needed
Why: The patch is technically coherent, but choosing a stable host-facing contract and its blocking semantics is a product boundary that cannot be inferred from tests alone. Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Sponsor the lifecycle API only if maintainers want ACPX to own this narrow host-observability seam, keeping persistence, lease policy, and process-tree cleanup outside core as the PR describes. Do we have a high-confidence way to reproduce the issue? Not applicable as a bug report; the submitted real-host trace demonstrates the proposed runtime behavior on the PR head. Is this the best way to solve the issue? Unclear: the implementation is narrow and matches ACPX’s backend-oriented vision, but maintainers must first choose whether this exact public lifecycle contract should exist. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against d4c16ab32154. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (5 earlier review cycles)
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
Related: #499
Context: openclaw/openclaw#128006
What Problem This Solves
Embedding hosts can launch ACP agents through
AcpClientorAcpRuntime, but they currently have no supported way to durably record process ownership around the exact spawn boundary. Hosts that need crash recovery or process-lease reconciliation must either wrap ACPX internals or accept a gap between preparing a launch and learning the child PID.Why This Change Was Made
This adds an optional lifecycle contract with immutable, correlated events for pre-spawn admission, successful spawn, spawn failure, and process exit. Runtime-created clients also receive stable session/probe scopes.
The two admission hooks are deliberately awaited: rejecting before spawn prevents launch, while rejecting after spawn terminates the child before returning the error. Failure and exit observers remain best-effort and cannot mask the underlying process outcome. Exit notification is ordered after spawned admission settles, avoiding a stale-lease race when a child exits quickly.
This is an embedding seam only. It does not add persistence, process-tree cleanup, or host-specific lease policy to ACPX, and therefore does not claim to close #499.
User Impact
Embedding developers can now implement durable host-owned launch tracking without wrapping ACPX spawn internals. Existing clients are unaffected because every lifecycle hook is optional.
Evidence
pnpm run checkpasses in a clean Linux checkout: formatting, TypeScript checks, type-aware lint, package and replay-viewer builds, 942 repository tests, and 130 coverage tests.onSpawnFailednonblocking while preserving synchronous invocation, correlation data, and logging for synchronous throws and asynchronous rejections.@agentclientprotocol/codex-acp1.6.2 adapter passes; redacted runtime proof is included below.openclaw/acpxmainat commit time.Real embedding-host proof
Environment: WSL2 Linux x64, Node v24.15.0, PR head
08a8601, published@agentclientprotocol/codex-acp1.6.2, existing Codex ChatGPT login. No credential values were logged. The temporary external host imported the built publicdist/runtime.jsAPI and usedcreateAcpRuntime,createRuntimeStore, andcreateAgentRegistry; it was not added to the PR.The first scenario initialized a real ACP session through
codex-acp, then closed it. The second used a real Node child that exited with code 17 while a slow but successfulonSpawnedadmission hook was pending. The third used a real OS spawn failure whileonSpawnFaileddeliberately returned a Promise that never settles.The successful adapter PID and the admission-test PID were both confirmed exited after the run. No model prompt or inference was needed; this proof exercises the embedding boundary, real adapter process lifecycle, ACP session initialization, correlation, the repaired startup-exit path, and the repaired spawn-failure semantics.
AI assistance
The implementation was AI-assisted. I reviewed the resulting lifecycle contract, failure semantics, tests, generated diff, and real embedding-host trace, and validated it with the checks listed above.