Refactor inbound room events into core#82606
Conversation
|
Codex review: needs changes before merge. Summary Reproducibility: yes. for the review findings: source inspection shows current main exposes inboundTurnKind and the PR head only forwards the new inboundEventKind/action header names. The PR itself is a feature/refactor rather than a user bug report. Real behavior proof Next step before merge Security Review findings
Review detailsBest possible solution: Keep the core classifier direction, add compatibility for the public action context and MCP loopback header during the rename, then let maintainers decide the protocol/config break before merge. Do we have a high-confidence way to reproduce the issue? Yes for the review findings: source inspection shows current main exposes inboundTurnKind and the PR head only forwards the new inboundEventKind/action header names. The PR itself is a feature/refactor rather than a user bug report. Is this the best way to solve the issue? No as written: centralizing classification is a reasonable direction, but dropping public action-context and MCP loopback compatibility is not the safest migration path. Add deprecated aliases/fallbacks while keeping the new canonical names. Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 3b2cd0dd1a85. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87245f2fdb
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| sessionKey: input.sessionKey, | ||
| sessionId: input.sessionId, | ||
| inboundTurnKind: input.inboundTurnKind, | ||
| inboundEventKind: input.inboundEventKind, |
There was a problem hiding this comment.
Preserve
inboundTurnKind in plugin action context
handlePluginAction now forwards only inboundEventKind to dispatchChannelMessageAction, so existing channel plugins that still read ctx.inboundTurnKind will silently see undefined after this update. That breaks backward compatibility for already-published plugins (for example, plugins that branch room-event delivery/idempotency by this field) even when they otherwise run unchanged on a newer host. Keep a compatibility alias in the dispatched context (send both keys) until the old key is fully deprecated.
Useful? React with 👍 / 👎.
| normalizeMessageChannel(getHeader(req, "x-openclaw-message-channel")) ?? undefined, | ||
| accountId: normalizeOptionalString(getHeader(req, "x-openclaw-account-id")), | ||
| inboundTurnKind: normalizeMcpInboundTurnKind(getHeader(req, "x-openclaw-inbound-turn-kind")), | ||
| inboundEventKind: normalizeMcpInboundEventKind(getHeader(req, "x-openclaw-inbound-event-kind")), |
There was a problem hiding this comment.
Accept legacy MCP inbound-turn header during transition
resolveMcpRequestContext only reads x-openclaw-inbound-event-kind now. Any existing MCP client/proxy that still sends x-openclaw-inbound-turn-kind will lose room-event classification, which changes tool scoping and source-reply behavior (room events no longer enforced as message-tool-only). Add a fallback read of the old header to avoid silent behavior regressions during upgrade.
Useful? React with 👍 / 👎.
87245f2 to
6d00eab
Compare
|
Maintainer rebase/fixup done for landing. Proof:
Applied |
Summary
src/channels/inbound-event/*, with shared classification, context, event-kind, and media helpers.InboundEventKind/ inbound-event terminology.messages.groupChat.unmentionedInbound: "room_event"and deliberately removes the pre-shipambientTurnsspelling with no runtime alias or migration.inboundEventKind, including MCP header/env rename tox-openclaw-inbound-event-kind/OPENCLAW_MCP_INBOUND_EVENT_KIND, plus Android/Swift generated model updates.openclaw/plugin-sdk/channel-inbound; all bundled plugins are migrated to the modern API.Fixes #81317.
Behavior addressed
Core, plugin-agnostic inbound room-event classification for unmentioned always-on group/channel chatter. Mentions, DMs, commands, abort requests, and native commands remain
user_request; configured unmentioned group/channel chatter can becomeroom_event, run as quiet context, avoid normal visible source replies, avoid Discord status/ack reactions, and speak visibly only through the message tool.Implementation notes
src/channels/inbound-event/classification.tsownsclassifyChannelInboundEventandresolveUnmentionedGroupInboundPolicy.src/channels/inbound-event/context.tsbuilds inbound event prompt context and emitsInboundEventKind.inbound_event_kind: room_eventandvisible_reply_contract: message_tool_only, suppress transcript-only assistant persistence, and force source reply delivery tomessage_tool_only.inboundTurnKindand returnInboundTurnKind; this is the explicit public-plugin-API compat exception, not an internal compat layer.Codex review
/Users/steipete/Projects/agent-scripts/skills/codex-review/scripts/codex-reviewagainst the branch diff before the final fixup.buildChannelTurnContextnow returnsInboundTurnKind, Slack/Discord room events staymessage_tool_only, Slack/Discord abort phrases stayuser_request, and Discord room events do not emit ack/status reactions./Users/steipete/Projects/agent-scripts/skills/codex-review/scripts/codex-reviewon the local fix diff returnedcodex-review clean: no accepted/actionable findings reported.ambientTurns: this branch has not shipped, repo policy keeps runtime config canonical, and legacy repair belongs in doctor/migration paths only.Verification
git diff --checkpnpm docs:listpnpm config:docs:checkpnpm plugin-sdk:api:checkpnpm protocol:checkpnpm check:test-typespnpm buildpnpm test src/plugin-sdk/channel-inbound.test.ts extensions/discord/src/monitor/message-handler.process.test.tspnpm test src/plugin-sdk/channel-inbound.test.ts src/channels/inbound-event/classification.test.ts src/channels/inbound-event/context.test.ts src/channels/inbound-event/media.test.ts src/config/zod-schema.visible-replies.test.ts src/auto-reply/reply/prompt-prelude.test.ts src/auto-reply/reply/get-reply-run.media-only.test.ts src/auto-reply/reply/dispatch-from-config.test.ts src/gateway/mcp-http.test.ts src/gateway/tool-resolution.test.ts src/gateway/server-methods/send.test.ts extensions/telegram/src/bot-message-context.require-mention.test.ts extensions/telegram/src/bot-message-context.reactions.test.ts extensions/telegram/src/inbound-event-delivery.test.ts extensions/telegram/src/bot-message-dispatch.test.ts extensions/telegram/src/action-runtime.test.ts extensions/discord/src/monitor/message-handler.preflight.test.ts extensions/discord/src/monitor/message-handler.process.test.ts extensions/slack/src/monitor/message-handler/prepare.test.ts src/auto-reply/reply/source-reply-delivery-mode.test.tstbx_01krrehj2sys4bmh2qxwh01z78, Actions run https://github.com/openclaw/openclaw/actions/runs/25962778077.pnpm check:changedpassed through conflict-marker, changelog attribution, wildcard reexport, duplicate coverage, dependency/patch guard, typecheck core/core tests/extensions/extension tests, lint core/extensions/scripts. Testbox app lint stopped only because the image lackedswiftlint.pnpm lint:swiftlocally passed. Existing non-fatal macOS type body length warnings remain; iOS lint was clean.Real behavior proof
Behavior addressed: Unmentioned always-on group/channel chatter is classified in core as a quiet
room_event, while explicit requests keepuser_requestbehavior.Real environment tested: Local macOS checkout plus Blacksmith Testbox through Crabbox (
tbx_01krrehj2sys4bmh2qxwh01z78).Exact steps or command run after this patch: Ran the local docs/config/protocol/SDK/type/build checks, focused Vitest coverage across core/Telegram/Slack/Discord/gateway reply behavior, final
codex-review, and Testboxpnpm check:changedvianode scripts/crabbox-wrapper.mjs.Evidence after fix: Focused Vitest passed 603 tests before final review fixup; post-fix targeted
pnpm test src/plugin-sdk/channel-inbound.test.ts extensions/discord/src/monitor/message-handler.process.test.tspassed 61 tests; Testbox check lanes were green until the missingswiftlintbinary; localpnpm lint:swiftpassed.Observed result after fix: Core emits
InboundEventKind, bundled plugins use the core classifier, Discord no longer leaks visible reactions for room events even with explicit status reactions, and public legacy SDK callers still seeInboundTurnKind.What was not tested: Live Telegram, Slack, or Discord provider E2E. Testbox app lint could not complete because
swiftlintis missing from that image; the same Swift lint lane was covered locally.