fix: route wake hooks to explicit sessions#70268
Conversation
Greptile SummaryThis PR routes wake-hook signals to explicit session targets instead of always enqueuing into the default main session. It extends Confidence Score: 4/5Safe to merge with one minor policy-enforcement gap addressed. All core routing logic is correct and well-tested. The one finding is the missing src/gateway/server-http.ts — the Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/gateway/server-http.ts
Line: 797-802
Comment:
**Missing `allowedSessionKeyPrefixes` guard for auto-computed agent main session key**
When a mapped wake action has `agentId` but no explicit `sessionKey`, the code computes `agent:${targetAgentId}:main` and dispatches without checking it against `hooksConfig.sessionPolicy.allowedSessionKeyPrefixes`. The `if (mapped.action.sessionKey)` branch just above (lines 769–796) performs this guard after normalization, as does the agent hook dispatch path. An operator who configures a strict prefix policy expecting all wake targets to be constrained will have that constraint silently bypassed for `agentId`-only wake mappings.
The fix is to mirror the same prefix check used in the `sessionKey`-present branch: after computing `normalizedWakeSessionKey`, read `hooksConfig.sessionPolicy.allowedSessionKeyPrefixes` and call `isSessionKeyAllowedByPrefix` / `getHookSessionKeyPrefixError` before calling `dispatchWakeHook`.
How can I resolve this? If you propose a fix, please make it concise.Reviews (2): Last reviewed commit: "fix: canonicalize wake hook session targ..." | Re-trigger Greptile |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e3a32445e
ℹ️ 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".
| enqueueSystemEvent(value.text, { sessionKey, trusted: false }); | ||
| if (value.mode === "now") { | ||
| requestHeartbeatNow({ reason: "hook:wake" }); | ||
| requestHeartbeatNow({ reason: "hook:wake", sessionKey }); |
There was a problem hiding this comment.
Do not pass arbitrary wake sessionKey values directly to requestHeartbeatNow here, because the event is queued under the raw key while heartbeat targeting canonicalizes non-agent: keys (e.g. hook:*) to an agent-scoped key in resolveHeartbeatSession; that means the wake run checks a different session than the one that received the event, so explicit /hooks/wake and mapped wake sessions with non-agent: keys can be left unprocessed (and can be skipped entirely when no main agent exists).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 091eec222d. I now canonicalize the wake hook session target before both queueing the system event and requesting the heartbeat, and added regressions for direct /hooks/wake plus mapped wake routes with non-agent session keys.
|
Local proof / test notes:
This is the proof bar I wanted before treating explicit wake-session routing as real rather than just code-plausible. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 091eec222d
ℹ️ 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".
| enqueueSystemEvent(value.text, { sessionKey, trusted: false }); | ||
| if (value.mode === "now") { | ||
| requestHeartbeatNow({ reason: "hook:wake" }); | ||
| requestHeartbeatNow({ reason: "hook:wake", sessionKey }); |
There was a problem hiding this comment.
When /hooks/wake now accepts an explicit sessionKey, dispatchWakeHook always forwards that key to requestHeartbeatNow; for non-agent canonical keys (for example global in non-global session scope), the heartbeat runner reinterprets the forced target in resolveHeartbeatSession and ends up polling a different queue than the one enqueueSystemEvent wrote to, so mode: "now" can return 200 while the wake event stays unprocessed until a later unrelated run. This is newly reachable because wake hooks can now carry explicit session keys.
Useful? React with 👍 / 👎.
|
Maintainer pass: this is close, but I would not land until the prefix-policy gap is fixed. In Concrete fix: after computing |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: faa83f3dfe
ℹ️ 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".
| return toAgentStoreSessionKey({ | ||
| agentId: targetAgentId, | ||
| requestKey: trimmed, | ||
| }); |
There was a problem hiding this comment.
normalizeHookDispatchSessionKey now rewrites every non-agent: key through toAgentStoreSessionKey, which converts global into agent:<target>:global. In the new mapped wake flow (src/gateway/server-http.ts lines 797-801), this means a wake mapping with agentId and no explicit sessionKey in session.scope: "global" enqueues to agent:<agent>:global, but the heartbeat runner’s global-scope path only processes global, so the wake event can remain unprocessed indefinitely. Keeping global (and unknown) un-rebound avoids this stuck-queue behavior.
Useful? React with 👍 / 👎.
faa83f3 to
5a9390a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a9390acba
ℹ️ 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".
| const requestedSessionKey = value.sessionKey || resolveMainSessionKeyFromConfig(); | ||
| const { canonicalKey: sessionKey } = loadSessionEntry(requestedSessionKey); | ||
| enqueueSystemEvent(value.text, { sessionKey, trusted: false }); | ||
| if (value.mode === "now") { | ||
| requestHeartbeatNow({ reason: "hook:wake" }); | ||
| requestHeartbeatNow({ reason: "hook:wake", sessionKey }); |
There was a problem hiding this comment.
When session.scope is "global", this path canonicalizes explicit wake keys through loadSessionEntry, so values like hook:relay:123 become agent-scoped keys before enqueue/wake. The heartbeat runner’s global-scope branch ignores forced session targets and only drains global, so /hooks/wake with an explicit non-global sessionKey can return 200 in mode: "now" while the queued event remains unprocessed. This is newly reachable because this commit adds request/session routing for wake hooks; explicit wake keys should be rejected or coerced to global under global scope.
Useful? React with 👍 / 👎.
|
Quick maintainer-facing update on the current head (
I re-ran the focused local proof on this head:
If this shape matches what you wanted from the policy fix, I think it is ready to land. |
|
Follow-up for the second Codex global-scope wake feedback: fixed in What changed:
Added regressions in
Local verification on the pushed head:
|
|
Codex review: needs real behavior proof before merge. Reviewed July 3, 2026, 2:56 AM ET / 06:56 UTC. Summary PR surface: Source +151, Tests +356. Total +507 across 7 files. Reproducibility: yes. Source inspection on current main shows mapped wake actions pass only Review metrics: 2 noteworthy metrics.
Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Rebase onto current main, use the current Do we have a high-confidence way to reproduce the issue? Yes. Source inspection on current main shows mapped wake actions pass only Is this the best way to solve the issue? No, not as submitted. Threading wake routing through existing hook policy helpers is the right layer, but this head must be rebased, documented, and publicly proven before merge. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 8ed6c78b7891. Label changesLabel justifications:
Evidence reviewedPR surface: Source +151, Tests +356. Total +507 across 7 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
aba81fc to
a029bf0
Compare
|
Updated this PR onto current What changed in the rebase/adaptation:
Local validation on the rebased head
The PR diff now touches the current hook modules/tests only: |
|
Follow-up update: I fixed the residual global-scope prefix-policy footgun noted in second-mind review. Behavior after this push:
Validation on new head
|
clawsweeper
Bot
added
rating: 🦪 silver shellfish
openclaw-barnacle
Bot
added
the
triage: needs-pr-context
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Thanks @howardclaw26. Closing this stale implementation branch, not the underlying bug; #64556 remains open as the canonical tracker. This unchanged head is dirty, still calls the removed |
2 participants
Summary
/hooks/wakepayloads carry an optionalsessionKeyaction: "wake"mappingsessionKey/agentIdrouting instead of always wakingmainWhy
The current gateway silently ignores wake-hook session targeting and always enqueues into the default main session, even when the operator provided a wake session key or configured a wake mapping with agent/session routing.
This is what bit the GitHub webhook relay dogfood path: the relay could wake Howard, but not the intended existing session, so the signal was quieter than intended and could drift into the wrong place.
Fixes #64556.
Notes
agentIdand no explicitsessionKeynow target that agent's main session instead of falling back to the default main agent session/hooks/waketreatssessionKeyas an exact explicit target and does not do agent rebinding; mapped wake keeps the existing target-agent normalization pathValidation
corepack pnpm exec vitest run --config test/vitest/vitest.gateway.config.ts src/gateway/hooks.test.ts src/gateway/hooks-mapping.test.ts src/gateway/server.hooks.test.tscheck:changedgate (including typecheck, lint, import-cycle checks, and the gateway test shard)composer-2-fast) second-mind review on the final diff: ship, with the config-guard gap fixed before opening