dispatch: broadcast inbound_claim to global plugin listeners#71988
dispatch: broadcast inbound_claim to global plugin listeners#71988flowolforg wants to merge 3 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR adds a broadcast block in Confidence Score: 3/5Not safe to merge — existing tests for the exact fallback scenarios this PR targets will fail. A P1 issue exists: the new broadcast guard condition is satisfied in at least three existing test cases that explicitly assert src/auto-reply/reply/dispatch-from-config.ts — the broadcast block itself, and the corresponding test file which needs updated assertions and a new positive test. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/auto-reply/reply/dispatch-from-config.ts
Line: 651-661
Comment:
**Broadcast fires in existing fallback tests, breaking their assertions**
Two existing tests in `dispatch-from-config.test.ts` mock `hasHooks` to return `true` for `"inbound_claim"` and set `runInboundClaimForPluginOutcome` to return `missing_plugin` / `no_handler` (which is what sets `pluginFallbackReason`), then explicitly assert:
```
expect(hookMocks.runner.runInboundClaim).not.toHaveBeenCalled();
```
With this new block, both conditions on line 651 are satisfied in those tests, so `runInboundClaim` **is** called and those assertions will fail:
- "falls back to OpenClaw once per startup when a bound plugin is missing" (lines 2959 and 2986)
- "falls back to OpenClaw when the bound plugin is loaded but has no inbound_claim handler" (line 3044)
The tests need to be updated to either relax the `not.toHaveBeenCalled()` assertions or mock `hasHooks` to return `false` for `inbound_claim` in the pure-fallback scenarios. There is also no positive test that verifies the broadcast fires and is handled correctly in the intended scenario.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "dispatch: broadcast inbound_claim to glo..." | Re-trigger Greptile |
| if (pluginFallbackReason && hookRunner?.hasHooks("inbound_claim")) { | ||
| const broadcastResult = await hookRunner.runInboundClaim( | ||
| inboundClaimEvent, | ||
| inboundClaimContext, | ||
| ); | ||
| if (broadcastResult?.handled) { | ||
| markIdle("plugin_broadcast_claim"); | ||
| recordProcessed("completed", { reason: "plugin-broadcast-handled" }); | ||
| return { queuedFinal: false, counts: dispatcher.getQueuedCounts() }; | ||
| } | ||
| } |
There was a problem hiding this comment.
Two existing tests in dispatch-from-config.test.ts mock hasHooks to return true for "inbound_claim" and set runInboundClaimForPluginOutcome to return missing_plugin / no_handler (which is what sets pluginFallbackReason), then explicitly assert:
expect(hookMocks.runner.runInboundClaim).not.toHaveBeenCalled();
With this new block, both conditions on line 651 are satisfied in those tests, so runInboundClaim is called and those assertions will fail:
- "falls back to OpenClaw once per startup when a bound plugin is missing" (lines 2959 and 2986)
- "falls back to OpenClaw when the bound plugin is loaded but has no inbound_claim handler" (line 3044)
The tests need to be updated to either relax the not.toHaveBeenCalled() assertions or mock hasHooks to return false for inbound_claim in the pure-fallback scenarios. There is also no positive test that verifies the broadcast fires and is handled correctly in the intended scenario.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/auto-reply/reply/dispatch-from-config.ts
Line: 651-661
Comment:
**Broadcast fires in existing fallback tests, breaking their assertions**
Two existing tests in `dispatch-from-config.test.ts` mock `hasHooks` to return `true` for `"inbound_claim"` and set `runInboundClaimForPluginOutcome` to return `missing_plugin` / `no_handler` (which is what sets `pluginFallbackReason`), then explicitly assert:
```
expect(hookMocks.runner.runInboundClaim).not.toHaveBeenCalled();
```
With this new block, both conditions on line 651 are satisfied in those tests, so `runInboundClaim` **is** called and those assertions will fail:
- "falls back to OpenClaw once per startup when a bound plugin is missing" (lines 2959 and 2986)
- "falls back to OpenClaw when the bound plugin is loaded but has no inbound_claim handler" (line 3044)
The tests need to be updated to either relax the `not.toHaveBeenCalled()` assertions or mock `hasHooks` to return `false` for `inbound_claim` in the pure-fallback scenarios. There is also no positive test that verifies the broadcast fires and is handled correctly in the intended scenario.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Addressed in second commit (54f0a75): updated the three test assertions at lines 2959, 2986, and 3044 from .not.toHaveBeenCalled() to .toHaveBeenCalledTimes(1). All checks-node-auto-reply-reply-dispatch tests pass.
The only remaining CI failure is checks-fast-protocol which is unrelated to this PR — it flags out-of-date generated Swift protocol files (modelrun/promptmode added to AgentParams on main without re-running pnpm protocol:gen).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e000107aaa
ℹ️ 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".
| markIdle("plugin_broadcast_claim"); | ||
| recordProcessed("completed", { reason: "plugin-broadcast-handled" }); | ||
| return { queuedFinal: false, counts: dispatcher.getQueuedCounts() }; |
There was a problem hiding this comment.
Commit dedupe state before returning on broadcast claim
When pluginFallbackReason is set and a global inbound_claim handler returns { handled: true }, this new early return exits before the normal dedupe cleanup path (commitInboundDedupe in the success tail, or releaseInboundDedupe in catch). That leaves the claimed key stuck in the in-flight set, so retries of the same inbound message are treated as permanently inflight and skipped. This regression is specific to plugin-bound fallback turns handled by the new broadcast block.
Useful? React with 👍 / 👎.
| if (broadcastResult?.handled) { | ||
| markIdle("plugin_broadcast_claim"); |
There was a problem hiding this comment.
Send synthetic reply from broadcast inbound_claim handlers
The broadcast path now treats broadcastResult.handled as terminal but ignores broadcastResult.reply. inbound_claim is documented/typed to support synthetic replies, and the targeted plugin-bound path already delivers result.reply, so a global listener that handles via { handled: true, reply: ... } will silently drop its reply here. In fallback conversations this can produce a handled turn with no plugin response.
Useful? React with 👍 / 👎.
This comment was marked as low quality.
This comment was marked as low quality.
|
Codex review: keeping this open for maintainer follow-up; there is still a little grit to resolve. Keep this PR open. Current main still does not implement the requested fallback Best possible solution: Keep the PR open for normal maintainer review. The best path is to land a narrow dispatch fix that broadcasts What I checked:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against f76c8322d302. |
…penclaw#48434) When plugin dispatch falls through (missing_plugin or no_handler), broadcast the inbound_claim hook to global listeners so listen-only plugins can observe unbound inbound messages. Guarded by pluginFallbackReason to avoid firing for normal core conversations.
- Commit inbound dedupe before the broadcast-handled early return so the claim does not stay pinned in inFlight forever (Codex review concern). The other plugin-bound early returns above have the same latent issue but are out of scope for this PR. - Add positive test for the broadcast-handled short-circuit path (Greptile P1 / QC review request): asserts runInboundClaim is called exactly once, runMessageReceived is not invoked, and the agent path is not entered.
54f0a75 to
9176cc0
Compare
|
Rebased onto main and addressed reviewer feedback in
Greptile P1 / QC: positive test for the broadcast-handled short-circuit added ( Codex — dedupe cleanup: the broadcast-handled early return now commits Codex — synthetic-reply delivery: |
|
CI status update for Resolved: Test of interest: Two failures remain, both pre-existing on main and unrelated to this PR:
Both failures reproduce on multiple recent main pushes without any code from this branch involved.
|
|
Closing — superseded by
Thanks to the reviewers (Greptile, Codex/clawsweeper, QC) — all concerns are moot now that the use case is covered by an existing, purpose-built hook. |
Summary
Fixes #48434
inbound_claimfor plugin-bound conversations. Global listeners (listen-only plugins) never receive events for conversations where plugin dispatch fell through.inbound_claimto global plugin listeners whenpluginFallbackReasonis set (i.e. plugin dispatch fell through withmissing_pluginorno_handler).pluginFallbackReason(not!pluginOwnedBinding) to avoid broadcasting for normal core conversations that were never plugin-bound.Supersedes #71644 (closed as dirty due to corrupted diff).
Change Type
Test Plan
does not broadcast inbound claims without a core-owned plugin bindingvalidates that the guard prevents firing for non-plugin conversations.checks-node-auto-reply-reply-agent-dispatchandchecks-node-coreshould pass.