feat: add --session-key support to system wake/event#35231
Conversation
Greptile SummaryThis PR adds an optional Two minor style/UX concerns were identified:
The core logic is sound and well-tested (103 tests across 9 files), with proper validation ordering and no security issues detected. These findings are non-blocking style/UX concerns, not correctness bugs. Confidence Score: 4/5
Last reviewed commit: 84a7b17 |
| const isNodePresenceLine = text.startsWith("Node:"); | ||
| let targetSessionKey: string | undefined; | ||
| if (!isNodePresenceLine && rawSessionKey) { | ||
| try { | ||
| targetSessionKey = canonicalizeWakeSessionKey(rawSessionKey); | ||
| } catch (err) { | ||
| respond(false, undefined, errorShape(ErrorCodes.INVALID_REQUEST, (err as Error).message)); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
When the event text starts with "Node:", any provided sessionKey parameter is not validated and has no effect — the handler always routes Node presence events to mainSessionKey (line 137). The caller receives { ok: true } with no indication their routing hint was ignored.
While this behavior appears intentional (Node presence events are system-generated and should go to the main session), callers expecting targeted dispatch may be confused. Consider either:
- Explicitly reject
sessionKeyfor Node presence lines with a descriptive error message - Document that Node presence events always route to the main session, so callers understand the exception
This would improve clarity for future maintainers and callers reading the API.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/server-methods/system.ts
Line: 49-58
Comment:
When the event text starts with `"Node:"`, any provided `sessionKey` parameter is not validated and has no effect — the handler always routes Node presence events to `mainSessionKey` (line 137). The caller receives `{ ok: true }` with no indication their routing hint was ignored.
While this behavior appears intentional (Node presence events are system-generated and should go to the main session), callers expecting targeted dispatch may be confused. Consider either:
1. **Explicitly reject** `sessionKey` for Node presence lines with a descriptive error message
2. **Document** that Node presence events always route to the main session, so callers understand the exception
This would improve clarity for future maintainers and callers reading the API.
How can I resolve this? If you propose a fix, please make it concise.| public struct WakeParams: Codable, Sendable { | ||
| public let mode: AnyCodable | ||
| public let text: String | ||
| public let sessionkey: String? |
There was a problem hiding this comment.
The property sessionkey uses all-lowercase naming, which violates Swift camelCase conventions. The public API exposes this as wakeParams.sessionkey instead of the idiomatic wakeParams.sessionKey. Since CodingKeys is already explicitly defined, renaming is safe and straightforward. Rename the property to follow Swift conventions. This same naming issue appears throughout both GatewayModels.swift files — consider updating all affected structs for consistency.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/macos/Sources/OpenClawProtocol/GatewayModels.swift
Line: 694
Comment:
The property `sessionkey` uses all-lowercase naming, which violates Swift camelCase conventions. The public API exposes this as `wakeParams.sessionkey` instead of the idiomatic `wakeParams.sessionKey`. Since `CodingKeys` is already explicitly defined, renaming is safe and straightforward. Rename the property to follow Swift conventions. This same naming issue appears throughout both GatewayModels.swift files — consider updating all affected structs for consistency.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
@Takhoffman Would you be willing to review this? It touches the gateway session-key routing, hooks dispatch, and heartbeat wake paths — areas you've been active in recently (custom session key inheritance, session delivery invariants, cron hardening). TL;DR: adds an optional |
|
@vincentkoc Would love your review on this — it touches hooks dispatch, gateway wake paths, and session key routing, which fall under your ownership area. Summary in the PR description. All tests pass, Greptile 4/5. Thanks! |
|
This pull request has been automatically marked as stale due to inactivity. |
|
Closing due to inactivity. |
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 #35125 and #34944.
Entry Points
openclaw system event "text" --session-key <key>wake:{ method: "wake", params: { text, mode, sessionKey } }system-event:{ method: "system-event", params: { text, sessionKey, ... } }/hooks/wake:POST { text, mode, sessionKey }— gated by policy{ action: "wake", sessionKey: "hook:{{payload.id}}" }Core Behavior
sessionKey: Canonicalize, validate, enqueue event to that session,requestHeartbeatNowto wake itsessionKey: Preserve existing fan-out (no behavioral change)Safety
hooks.allowRequestSessionKey+ prefix allowlistdispatchWakeHookreturns result object (errors propagated)Changes since v5 (#35125)
system-eventWS handler now callsrequestHeartbeatNowfor targeted sessions — previously events to non-main sessions would silently stall (same bug that was fixed inwakeand hooks paths in v5, but missed insystem-event)cron.ts,server/hooks.ts,system.ts) verified to haverequestHeartbeatNowafterenqueueSystemEventTesting
Closes #35125, closes #34944