feat: add --session-key support to system wake/event#35125
Conversation
Greptile SummaryThis PR adds an optional Key changes:
One functional bug found: the Confidence Score: 3/5
Last reviewed commit: c6c50be |
| } else { | ||
| enqueueSystemEvent(text, { sessionKey }); | ||
| enqueueSystemEvent(text, { sessionKey: targetSessionKey ?? mainSessionKey }); | ||
| } |
There was a problem hiding this comment.
Targeted system-event silently stalls — missing requestHeartbeatNow
When targetSessionKey is set (the caller passed a sessionKey pointing to a non-main session), the event is enqueued to that session but requestHeartbeatNow is never called with that session key.
The PR description explicitly identifies this as the root-cause bug that was fixed in the other two paths: "Fixed: next-heartbeat mode with non-main sessionKey now always triggers requestHeartbeatNow — previously events would silently stall because the heartbeat runner doesn't scan arbitrary session keys."
Both cron.ts (line 67) and dispatchWakeHook in server/hooks.ts (line 51) were updated to call requestHeartbeatNow for targeted sessions. This path was missed.
Without the heartbeat nudge, events enqueued to a non-main session will never be consumed — the heartbeat runner doesn't poll arbitrary session keys, so the targeted session stays asleep indefinitely.
The fix should mirror what was done in cron.ts: after the enqueueSystemEvent call on line 142, add a guard that checks if (targetSessionKey) and then calls requestHeartbeatNow with that session key. Also add the import for requestHeartbeatNow from ../../infra/heartbeat-wake.js at the top of the file.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/server-methods/system.ts
Line: 141-143
Comment:
**Targeted `system-event` silently stalls — missing `requestHeartbeatNow`**
When `targetSessionKey` is set (the caller passed a `sessionKey` pointing to a non-main session), the event is enqueued to that session but `requestHeartbeatNow` is never called with that session key.
The PR description explicitly identifies this as the root-cause bug that was fixed in the other two paths: "Fixed: next-heartbeat mode with non-main sessionKey now always triggers requestHeartbeatNow — previously events would silently stall because the heartbeat runner doesn't scan arbitrary session keys."
Both `cron.ts` (line 67) and `dispatchWakeHook` in `server/hooks.ts` (line 51) were updated to call `requestHeartbeatNow` for targeted sessions. This path was missed.
Without the heartbeat nudge, events enqueued to a non-main session will never be consumed — the heartbeat runner doesn't poll arbitrary session keys, so the targeted session stays asleep indefinitely.
The fix should mirror what was done in `cron.ts`: after the `enqueueSystemEvent` call on line 142, add a guard that checks `if (targetSessionKey)` and then calls `requestHeartbeatNow` with that session key. Also add the import for `requestHeartbeatNow` from `../../infra/heartbeat-wake.js` at the top of the file.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6c50beff8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| } else { | ||
| enqueueSystemEvent(text, { sessionKey }); | ||
| enqueueSystemEvent(text, { sessionKey: targetSessionKey ?? mainSessionKey }); |
There was a problem hiding this comment.
Wake targeted system-event sessions after enqueueing
When system-event is called with a non-main sessionKey, this branch enqueues into that specific session but never triggers requestHeartbeatNow for it, so the event can remain undelivered indefinitely unless another unrelated wake targets the same session. This regression affects the new sessionKey path specifically: unlike the updated wake/hook flows, non-main session queues are not polled by default heartbeat fan-out, so targeted system events silently stall.
Useful? React with 👍 / 👎.
|
Superseded by #35231 |
What
Add optional
--session-keyparameter to all wake/system-event entry points so callers can deliver events to a specific agent session instead of default fan-out.Supersedes #34944 with fixes for review feedback.
Entry Points
openclaw system event "text" --session-key <key>wakemethod:{ method: "wake", params: { text, mode, sessionKey } }system-eventmethod:{ method: "system-event", params: { text, sessionKey, ... } }/hooks/wake:POST { text, mode, sessionKey }— gated byhooks.allowRequestSessionKeypolicy{ action: "wake", sessionKey: "hook:{{payload.id}}" }Core Behavior
sessionKey: Canonicalize, validate, enqueue event to that specific session, heartbeat-wake that sessionsessionKey: Preserve existing fan-out behavior — all agents get nudged (no behavioral change)Safety
agent:ops) are rejectedsessionKeygated byhooks.allowRequestSessionKeypolicy + prefix allowlistdispatchWakeHookreturns result object (errors propagated, not swallowed)Changes since v4 (#34944)
next-heartbeatmode with non-mainsessionKeynow always triggersrequestHeartbeatNow— previously events would silently stall because the heartbeat runner doesn't scan arbitrary session keys!wakeResult.okguards in no-sessionKey branchesTesting
Closes #34944