Skip to content

fix(message): route WebChat message tool sends to source sink#96850

Closed
harjothkhara wants to merge 1 commit into
openclaw:mainfrom
harjothkhara:oss/96840-webchat-message-tool-sink
Closed

fix(message): route WebChat message tool sends to source sink#96850
harjothkhara wants to merge 1 commit into
openclaw:mainfrom
harjothkhara:oss/96840-webchat-message-tool-sink

Conversation

@harjothkhara

@harjothkhara harjothkhara commented Jun 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes #96840.

Targetless message.send calls created for WebChat source turns were still using automatic source delivery, so the outbound runner required an explicit target even though WebChat should provide the internal source-reply sink for message-tool sends.

Why This Change Was Made

The fix keeps ordinary WebChat room-event turns on automatic source delivery, but defaults WebChat-constructed (internal-channel) message tools to message_tool_only unless a caller explicitly supplies another source delivery mode. That keeps the existing gateway routing behavior intact while giving targetless WebChat message-tool sends the internal sink they need.

User Impact

WebChat agents can call message.send without an explicit target when replying to the current source, matching the documented/source-backed behavior. Explicit automatic source delivery remains preserved for callers that request it.

Real behavior proof

Behavior addressed: Targetless message(action="send") from an active WebChat run failed with Action send requires a target instead of being projected into the active chat via the internal source-reply sink (#96840, per docs/web/webchat.md). After this patch, internal-channel (WebChat) message tools default sourceReplyDeliveryMode to message_tool_only, so a targetless send routes through the internal sink.

Real environment tested: Local openclaw source checkout in isolated git worktrees — PR branch oss/96840-webchat-message-tool-sink (6c80752285) versus base main (c68484acc4) — Node + pnpm, node scripts/run-vitest.mjs with external boundaries mocked.

Exact steps or command run after this patch: Ran the focused regression test against base main (fix absent) and against the PR branch via the repo's vitest runner, then the sibling/contract suite on the branch:

# RED (PR test on main source) / GREEN (PR branch)
node scripts/run-vitest.mjs src/agents/tools/message-tool.test.ts \
  --testNamePattern 'internal source-reply sink|explicit automatic source delivery for webchat' \
  --reporter dot --maxWorkers 1
# Sibling + contract suite (PR branch)
node scripts/run-vitest.mjs \
  src/agents/tools/message-tool.test.ts \
  src/infra/outbound/message-action-runner.send-validation.test.ts \
  src/gateway/tool-resolution.test.ts --reporter dot --maxWorkers 2

Evidence after fix: Red-green plus a source/contract trace:

  • RED on main (fix absent): Tests 2 failed | 2 passed — "defaults webchat message tools to the internal source-reply sink" fails with AssertionError: expected undefined to be 'message_tool_only'.
  • GREEN on branch: Test Files 2 passed (2) · Tests 4 passed.
  • Sibling + contract on branch: Test Files 6 passed (6) · Tests 249 passed (249) (full message-tool + message-action-runner.send-validation + gateway/tool-resolution).
  • Contract: applyImplicitSourceReplySendPolicy (src/infra/outbound/message-action-runner.ts:604) sets bestEffort=true only when action==="send" && sourceReplyDeliveryMode==="message_tool_only" and no explicit foreign target — bypassing the Action send requires a target throw (src/infra/outbound/message-action-normalization.ts:76). This patch makes internal-channel message tools default to that mode.

Observed result after fix: Targetless WebChat message.send resolves with sourceReplyDeliveryMode=message_tool_only and is routed to the internal source-reply sink with no target required. Ordinary WebChat room-event turns remain on automatic, and explicit sourceReplyDeliveryMode overrides are preserved — both verified by the tests above.

What was not tested: A live WebChat send and Control-UI visual capture were out of scope; the repo's gate is focused red-green plus a source/contract trace. The full monorepo vitest suite was not run — coverage was scoped to the affected message-tool module plus the runner send-validation and gateway tool-resolution contracts (249 tests). Non-internal-channel providers are unchanged and exercised by their existing passing tests.

AI assistance disclosure: Codex helped identify the source-sink boundary and implement the patch; the red-green and source/contract proof above were produced with Claude.

@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: XS labels Jun 25, 2026
@clawsweeper

clawsweeper Bot commented Jun 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 25, 2026, 7:21 PM ET / 23:21 UTC.

Summary
The PR defaults internal/WebChat-created message tools to message_tool_only when no explicit source delivery mode is supplied and adds focused regression tests for that default and explicit automatic mode.

PR surface: Source +9, Tests +31. Total +40 across 2 files.

Reproducibility: yes. from source inspection: current main leaves implicit WebChat/internal message tools without message_tool_only, while the internal sink requires that mode before target validation. I did not run a live WebChat session in this read-only review.

Review metrics: 1 noteworthy metric.

  • Delivery-mode default: 1 added. The diff adds one implicit message_tool_only default for internal/WebChat message tools, which changes targetless-send routing and needs both mode-contract coverage and visible delivery proof before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #96840
Summary: This PR is the open candidate fix for the canonical WebChat targetless message.send bug, with related prior sink work and adjacent test-alignment work.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🦪 silver shellfish
Patch quality: 🧂 unranked krab
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Separate tool-local targetless send routing from run-level message-tool-only final-answer guidance and add focused regression coverage.
  • [P1] Add redacted live WebChat or Control UI proof showing targetless message.send delivered in the same active chat.
  • Update the PR body with proof so ClawSweeper can re-review automatically; if it does not, ask a maintainer to comment @clawsweeper re-review.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body provides red-green Vitest/source-contract output with mocked external boundaries, but no live WebChat or Control UI output, screenshot, recording, linked artifact, copied live output, or redacted log showing after-fix delivery. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Mantis proof suggestion
A short WebChat or Control UI run would directly prove the visible runtime path that mocked tests cannot show. A maintainer can ask Mantis to capture proof by posting this exact PR comment:

@openclaw-mantis visual task: verify a WebChat run can use targetless message.send and the reply appears in the same chat.

Risk before merge

  • [P1] Merging as-is can make automatic WebChat runs with an explicitly available message tool expose message-tool-only guidance while the run-level final-answer suppression path remains automatic, which can produce duplicate or misleading visible reply behavior.
  • [P1] The PR changes a user-visible message-delivery path but still lacks live WebChat or Control UI proof showing a targetless message.send delivered in the same active chat.

Maintainer options:

  1. Fix the mode split before merge (recommended)
    Preserve targetless internal WebChat send routing while keeping message-tool-only description, terminal suppression, and transcript mirroring tied to the actual run-level delivery mode.
  2. Require visible WebChat proof after repair
    Ask for redacted live WebChat or Control UI output showing targetless message.send delivered in the same active chat before clearing the proof gate.
  3. Accept delivery-risk consciously
    Maintainers could accept the current source-level tests only, but that owns the risk of duplicated or misleading WebChat visible replies in real automatic runs.

Next step before merge

  • [P1] The author or maintainer needs to address the delivery-contract finding and add real WebChat proof; automation cannot supply contributor real-environment proof for this external PR.

Security
Cleared: The diff only changes message-tool routing logic and focused tests; it does not touch dependencies, workflows, credentials, permissions, lockfiles, package metadata, or artifact execution paths.

Review findings

  • [P1] Keep automatic WebChat runs out of message-tool-only guidance — src/agents/tools/message-tool.ts:1221
Review details

Best possible solution:

Keep the targetless internal WebChat send repair, but separate local send routing from run-level message-tool-only final-answer semantics and add coverage plus redacted live WebChat proof before merge.

Do we have a high-confidence way to reproduce the issue?

Yes from source inspection: current main leaves implicit WebChat/internal message tools without message_tool_only, while the internal sink requires that mode before target validation. I did not run a live WebChat session in this read-only review.

Is this the best way to solve the issue?

No. The targetless send-routing repair is plausible, but the current patch uses the derived tool-local mode as the message-tool-only description/final-answer contract even when the run-level WebChat delivery mode remains automatic.

Full review comments:

  • [P1] Keep automatic WebChat runs out of message-tool-only guidance — src/agents/tools/message-tool.ts:1221
    This passes the derived internal-channel mode into the message tool description, so a WebChat run where the message tool is available but the run-level mode is still automatic will tell the model that final answers stay private. The subscriber and payload paths only suppress/mirror message-tool source replies when the run-level sourceReplyDeliveryMode is message_tool_only, so such a run can deliver a targetless message.send and still auto-deliver the final answer. Please keep the local send-routing default separate from message-tool-only final-answer guidance, or promote the run-level mode deliberately with coverage for automatic WebChat runs.
    Confidence: 0.84

Overall correctness: patch is incorrect
Overall confidence: 0.84

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 95b97e5b0b5e.

Label changes

Label changes:

  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦪 silver shellfish and patch quality is 🧂 unranked krab.
  • add status: 🛠️ actively grinding: The PR author has acted after the latest ClawSweeper review and work remains. Needs real behavior proof before merge: The PR body provides red-green Vitest/source-contract output with mocked external boundaries, but no live WebChat or Control UI output, screenshot, recording, linked artifact, copied live output, or redacted log showing after-fix delivery. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • remove rating: 🦪 silver shellfish: Current PR rating is rating: 🧂 unranked krab, so this older rating label is no longer current.
  • remove status: 📣 needs proof: Current PR status label is status: 🛠️ actively grinding.

Label justifications:

  • P2: This is a normal-priority WebChat message-delivery bug fix with a narrow runtime and test surface.
  • merge-risk: 🚨 message-delivery: The PR changes how targetless internal WebChat message-tool sends route and can leave tool delivery semantics out of sync with automatic final delivery.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦪 silver shellfish and patch quality is 🧂 unranked krab.
  • status: 🛠️ actively grinding: The PR author has acted after the latest ClawSweeper review and work remains. Needs real behavior proof before merge: The PR body provides red-green Vitest/source-contract output with mocked external boundaries, but no live WebChat or Control UI output, screenshot, recording, linked artifact, copied live output, or redacted log showing after-fix delivery. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +9, Tests +31. Total +40 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 12 3 +9
Tests 1 31 0 +31
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 43 3 +40

What I checked:

Likely related people:

  • pashpashpash: Authored and merged the internal UI message-tool sink and WebChat/TUI docs contract that this PR builds on. (role: feature-history owner; confidence: high; commits: 78eb92e62277; files: src/infra/outbound/message-action-runner.ts, src/infra/outbound/internal-source-reply.ts, docs/web/webchat.md)
  • steipete: Recent commit history and related PR context changed WebChat delivery prompt/source behavior around the automatic versus message-tool-only boundary. (role: recent related behavior owner; confidence: high; commits: 0cfccdb0c7ee; files: src/gateway/tool-resolution.ts, src/gateway/tool-resolution.test.ts, src/auto-reply/reply/source-reply-delivery-mode.ts)
  • vincentkoc: Recent history touched the internal source-reply and message-action runner paths that gate targetless current-source sends. (role: recent adjacent owner; confidence: medium; commits: 0ea08076c3b5; files: src/infra/outbound/internal-source-reply.ts, src/infra/outbound/message-action-runner.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. labels Jun 25, 2026
@harjothkhara
harjothkhara marked this pull request as ready for review June 25, 2026 20:34
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. label Jun 25, 2026
@harjothkhara

harjothkhara commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

/review

@clawsweeper

clawsweeper Bot commented Jun 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@harjothkhara

harjothkhara commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

Setting expectations on the proof gate: the six-field real-behavior-proof block is in the PR body and the Real behavior proof check is green, but I was not able to produce the requested live WebChat / Control-UI capture — I don't have a running WebChat surface to drive a targetless message.send and screenshot the in-chat delivery.

What the PR is verified against instead (in the body):

  • Red-green: defaults webchat message tools to the internal source-reply sink fails on main (AssertionError: expected undefined to be 'message_tool_only') and passes on this branch; 249/249 across message-tool + message-action-runner.send-validation + gateway/tool-resolution.
  • Contract trace: applyImplicitSourceReplySendPolicy (src/infra/outbound/message-action-runner.ts) sets bestEffort=true only when action==='send' && sourceReplyDeliveryMode==='message_tool_only' with no explicit foreign target — bypassing the Action send requires a target throw (message-action-normalization.ts). This patch makes internal-channel (WebChat) message tools default to that mode.

So the fix is verified at the test + source-contract level, just not via a live in-chat capture. Happy for a maintainer to run the live WebChat verification or apply the proof override if that's sufficient.

@zhangguiping-xydt zhangguiping-xydt mentioned this pull request Jun 27, 2026
Merged
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 🛠️ actively grinding The PR author has acted after the latest ClawSweeper review and work remains. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 27, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 29, 2026
Closed
@harjothkhara

harjothkhara commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Closing as superseded by #97167, which fixes the same targetless WebChat message.send source-reply sink routing (#96840) and is further along — proof: sufficient, ready for maintainer look. Deferring to the canonical fix.

@clawsweeper clawsweeper Bot mentioned this pull request Jun 29, 2026
Closed
@steipete

steipete commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Canonical fix #97167 has now landed on main as 29371cfe1fed763df59b56d15b39793c832f939c.

It preserves automatic WebChat final replies, routes targetless message.send through the current-run internal sink, and proves the full tool-to-sink/transcript/UI path. This duplicate remains correctly closed as superseded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

agents Agent runtime and tooling merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: XS status: 🛠️ actively grinding The PR author has acted after the latest ClawSweeper review and work remains.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: Targetless message.send fails with 'Action send requires a target' in WebChat despite docs stating source-reply sink should handle it

2 participants

@harjothkhara @steipete