feat: add --session-key support to system wake/event#34944
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abc53e3142
ℹ️ 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".
| if (p.mode === "now") { | ||
| requestHeartbeatNow({ reason: "wake", sessionKey }); | ||
| } |
There was a problem hiding this comment.
Wake targeted sessions on next-heartbeat cron requests
When sessionKey is provided, this path only calls requestHeartbeatNow for mode === "now"; in next-heartbeat mode it just enqueues into that specific session. The heartbeat runner only reads the main (or statically configured heartbeat.session) key unless a forced session key is passed, so non-main session events can sit indefinitely until another targeted wake happens. As a result, system event --session-key <non-main> --mode next-heartbeat can silently stall.
Useful? React with 👍 / 👎.
| if (value.mode === "now") { | ||
| requestHeartbeatNow({ reason: "hook:wake" }); | ||
| // Only scope the heartbeat nudge when an explicit session key was | ||
| // provided. Omitting sessionKey preserves fan-out to all agents. | ||
| if (sessionKey) { | ||
| requestHeartbeatNow({ reason: "hook:wake", sessionKey }); |
There was a problem hiding this comment.
Wake hook-targeted sessions for next-heartbeat mode
This logic also scopes heartbeat nudges to mode === "now" only. For hook wake payloads/mappings that set a non-main sessionKey with next-heartbeat, the event is queued in that session but no targeted run is ever requested, and interval heartbeats do not scan arbitrary session keys. Those hook events can therefore remain unprocessed until some later explicit wake for the same session.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR adds an optional Key observations:
Confidence Score: 4/5
Last reviewed commit: abc53e3 |
| const wakeResult = dispatchWakeHook(normalized.value); | ||
| if (!wakeResult.ok) { | ||
| sendJson(res, 400, { ok: false, error: wakeResult.error }); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Dead code in no-sessionKey branch.
When normalized.value.sessionKey is absent, dispatchWakeHook is called without a sessionKey argument. Inside dispatchWakeHook (in server/hooks.ts) the if (value.sessionKey) guard is skipped, so canonicalizeWakeSessionKey is never called and the function unconditionally returns { ok: true }. The if (!wakeResult.ok) check below can therefore never be true in this branch.
The same pattern repeats in the mapping path (lines 494–501). Consider removing the dead-code guard, or adding a brief comment explaining why it is intentionally defensive:
| const wakeResult = dispatchWakeHook(normalized.value); | |
| if (!wakeResult.ok) { | |
| sendJson(res, 400, { ok: false, error: wakeResult.error }); | |
| return true; | |
| } | |
| const wakeResult = dispatchWakeHook(normalized.value); | |
| // dispatchWakeHook only returns ok:false when a sessionKey canonicalization | |
| // fails; without a sessionKey it always returns ok:true. | |
| if (!wakeResult.ok) { | |
| sendJson(res, 400, { ok: false, error: wakeResult.error }); | |
| return true; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/server-http.ts
Line: 413-417
Comment:
Dead code in no-sessionKey branch.
When `normalized.value.sessionKey` is absent, `dispatchWakeHook` is called without a `sessionKey` argument. Inside `dispatchWakeHook` (in `server/hooks.ts`) the `if (value.sessionKey)` guard is skipped, so `canonicalizeWakeSessionKey` is never called and the function unconditionally returns `{ ok: true }`. The `if (!wakeResult.ok)` check below can therefore never be `true` in this branch.
The same pattern repeats in the mapping path (lines 494–501). Consider removing the dead-code guard, or adding a brief comment explaining why it is intentionally defensive:
```suggestion
const wakeResult = dispatchWakeHook(normalized.value);
// dispatchWakeHook only returns ok:false when a sessionKey canonicalization
// fails; without a sessionKey it always returns ok:true.
if (!wakeResult.ok) {
sendJson(res, 400, { ok: false, error: wakeResult.error });
return true;
}
```
How can I resolve this? If you propose a fix, please make it concise.abc53e3 to
20af7fa
Compare
20af7fa to
9953e04
Compare
9953e04 to
c6c50be
Compare
|
Superseded by #35231 |
Summary
--session-keyflag tosystem wakeandsystem eventCLI commandssessionKeythrough HTTP endpoints (/wake,/system-event), hook dispatch, and cron wakenildefaults for optional paramscanonicalizeWakeParamshelper in session-utils for consistent wake parameter handlingTest plan
vitest runon all 10 test files)pnpm protocol:checkpasses (schema + Swift codegen in sync)git diff --stat origin/mainshows only expected files (22 files, no unrelated changes).tldr/,vite.config.ts,process-respawn.test.ts,store.ts, or PRD files in diff🤖 Generated with Claude Code