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 |
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.
Keep wake target aligned with queued session key
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.
Avoid scoping wake heartbeat to non-agent session keys
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.
Preserve global key when rebinding hook session targets
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.
Guard global scope before waking explicit sessions
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 June 7, 2026, 1:06 AM ET / 05:06 UTC. Summary PR surface: Source +151, Tests +356. Total +507 across 7 files. Reproducibility: unclear. The review failed before ClawSweeper could establish a reproduction path. Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Risk before merge
Maintainer options:
Next step before merge
Review detailsBest possible solution: Retry the Codex review after fixing the execution failure. Do we have a high-confidence way to reproduce the issue? Unclear. The review failed before ClawSweeper could establish a reproduction path. Is this the best way to solve the issue? Unclear. Retry the review first so ClawSweeper can evaluate the actual issue and fix direction. AGENTS.md: unclear because the file could not be read completely. Codex review notes: model gpt-5.5, reasoning high; reviewed against fa614d0907e8. Label changesLabel changes:
Label 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
|
The cron `wake` MCP tool currently forwards only `{mode, text}` to the
gateway. Every wake then enqueues a system event with no sessionKey /
agentId, so the cron service falls back to the heartbeat / main
default. Wakes scheduled from a non-main session (Telegram thread,
Discord channel, multi-agent setup) silently route to the wrong
conversation lane — and on CLI runtimes the woken session burns
tokens generating output that no caller can route back.
Origin capture (closes the upstream half of openclaw#46886 and openclaw#64556):
- `src/agents/tools/cron-tool.ts` — the `wake` case now resolves
`opts.agentSessionKey` through `resolveInternalSessionKey` and
`resolveSessionAgentId`, matching the existing `add` action at
L569-583. Explicit `sessionKey` / `agentId` params on the tool
call take precedence over the inferred values so cross-session
wakes remain expressible.
- `src/gateway/protocol/schema/agent.ts` — `WakeParamsSchema` now
declares optional `sessionKey` and `agentId` (NonEmptyString)
so the gateway-level validator types them explicitly. The
schema's `additionalProperties: true` continues to accept
forward-compat metadata unchanged.
- `src/gateway/server-methods/cron.ts` — the wake handler reads
both fields, trims them, and forwards to `context.cron.wake`.
- `src/cron/service.ts` + `src/cron/service/ops.ts` +
`src/cron/service/timer.ts` — `wake()` accepts optional
`sessionKey`/`agentId` and threads them into
`enqueueSystemEvent` and `requestHeartbeatNow`. Missing /
whitespace-only fields fall through to the dep's default so
pre-existing call sites with no origin keep behaving the same
way (backwards compatible).
Tests:
- `src/cron/service/wake-origin.test.ts` (new, 6 tests) —
direct seams on `wake()`: origin forwarded on `mode: "now"`,
queued on `mode: "next-heartbeat"`, default fallback when
origin omitted, whitespace-only origin treated as omitted,
empty text still rejected.
- `src/gateway/protocol/index.test.ts` — extends
`validateWakeParams` coverage with the new optional fields
accepted + empty-string rejected.
Out of scope (deliberate split):
- Channel/thread/topic capture on the job's `delivery` block —
follow-up PR once this contract lands. The minimum-viable fix
here is session routing, which unblocks the CLI runtime's
`--resume <session>` path and the embedded session-resolution
path without a schema rewrite of the delivery contract.
- The 5 stalled wake-related PRs (openclaw#70268, openclaw#57199, openclaw#82767,
openclaw#79869, openclaw#63096) each fix downstream specifics. This PR fixes
the upstream origin-capture they all silently assume.
The cron `wake` MCP tool currently forwards only `{mode, text}` to the
gateway. Every wake then enqueues a system event with no sessionKey /
agentId, so the cron service falls back to the heartbeat / main
default. Wakes scheduled from a non-main session (Telegram thread,
Discord channel, multi-agent setup) silently route to the wrong
conversation lane — and on CLI runtimes the woken session burns
tokens generating output that no caller can route back.
Origin capture (closes the upstream half of openclaw#46886 and openclaw#64556):
- `src/agents/tools/cron-tool.ts` — the `wake` case now resolves
`opts.agentSessionKey` through `resolveInternalSessionKey` and
`resolveSessionAgentId`, matching the existing `add` action at
L569-583. Explicit `sessionKey` / `agentId` params on the tool
call take precedence over the inferred values so cross-session
wakes remain expressible.
- `src/gateway/protocol/schema/agent.ts` — `WakeParamsSchema` now
declares optional `sessionKey` and `agentId` (NonEmptyString)
so the gateway-level validator types them explicitly. The
schema's `additionalProperties: true` continues to accept
forward-compat metadata unchanged.
- `src/gateway/server-methods/cron.ts` — the wake handler reads
both fields, trims them, and forwards to `context.cron.wake`.
- `src/cron/service.ts` + `src/cron/service/ops.ts` +
`src/cron/service/timer.ts` — `wake()` accepts optional
`sessionKey`/`agentId` and threads them into
`enqueueSystemEvent` and `requestHeartbeatNow`. Missing /
whitespace-only fields fall through to the dep's default so
pre-existing call sites with no origin keep behaving the same
way (backwards compatible).
Tests:
- `src/cron/service/wake-origin.test.ts` (new, 6 tests) —
direct seams on `wake()`: origin forwarded on `mode: "now"`,
queued on `mode: "next-heartbeat"`, default fallback when
origin omitted, whitespace-only origin treated as omitted,
empty text still rejected.
- `src/gateway/protocol/index.test.ts` — extends
`validateWakeParams` coverage with the new optional fields
accepted + empty-string rejected.
Out of scope (deliberate split):
- Channel/thread/topic capture on the job's `delivery` block —
follow-up PR once this contract lands. The minimum-viable fix
here is session routing, which unblocks the CLI runtime's
`--resume <session>` path and the embedded session-resolution
path without a schema rewrite of the delivery contract.
- The 5 stalled wake-related PRs (openclaw#70268, openclaw#57199, openclaw#82767,
openclaw#79869, openclaw#63096) each fix downstream specifics. This PR fixes
the upstream origin-capture they all silently assume.
The cron `wake` MCP tool currently forwards only `{mode, text}` to the
gateway. Every wake then enqueues a system event with no sessionKey /
agentId, so the cron service falls back to the heartbeat / main
default. Wakes scheduled from a non-main session (Telegram thread,
Discord channel, multi-agent setup) silently route to the wrong
conversation lane — and on CLI runtimes the woken session burns
tokens generating output that no caller can route back.
Origin capture (closes the upstream half of openclaw#46886 and openclaw#64556):
- `src/agents/tools/cron-tool.ts` — the `wake` case now resolves
`opts.agentSessionKey` through `resolveInternalSessionKey` and
`resolveSessionAgentId`, matching the existing `add` action at
L569-583. Explicit `sessionKey` / `agentId` params on the tool
call take precedence over the inferred values so cross-session
wakes remain expressible.
- `src/gateway/protocol/schema/agent.ts` — `WakeParamsSchema` now
declares optional `sessionKey` and `agentId` (NonEmptyString)
so the gateway-level validator types them explicitly. The
schema's `additionalProperties: true` continues to accept
forward-compat metadata unchanged.
- `src/gateway/server-methods/cron.ts` — the wake handler reads
both fields, trims them, and forwards to `context.cron.wake`.
- `src/cron/service.ts` + `src/cron/service/ops.ts` +
`src/cron/service/timer.ts` — `wake()` accepts optional
`sessionKey`/`agentId` and threads them into
`enqueueSystemEvent` and `requestHeartbeatNow`. Missing /
whitespace-only fields fall through to the dep's default so
pre-existing call sites with no origin keep behaving the same
way (backwards compatible).
Tests:
- `src/cron/service/wake-origin.test.ts` (new, 6 tests) —
direct seams on `wake()`: origin forwarded on `mode: "now"`,
queued on `mode: "next-heartbeat"`, default fallback when
origin omitted, whitespace-only origin treated as omitted,
empty text still rejected.
- `src/gateway/protocol/index.test.ts` — extends
`validateWakeParams` coverage with the new optional fields
accepted + empty-string rejected.
Out of scope (deliberate split):
- Channel/thread/topic capture on the job's `delivery` block —
follow-up PR once this contract lands. The minimum-viable fix
here is session routing, which unblocks the CLI runtime's
`--resume <session>` path and the embedded session-resolution
path without a schema rewrite of the delivery contract.
- The 5 stalled wake-related PRs (openclaw#70268, openclaw#57199, openclaw#82767,
openclaw#79869, openclaw#63096) each fix downstream specifics. This PR fixes
the upstream origin-capture they all silently assume.
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
|
This pull request has been automatically marked as stale due to inactivity. |
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