Skip to content

fix(outbound): prevent internal sink from intercepting action-only pl…#94952

Open
lakshitsoni26 wants to merge 3 commits into
openclaw:mainfrom
lakshitsoni26:fix/clean-routing-fix
Open

fix(outbound): prevent internal sink from intercepting action-only pl…#94952
lakshitsoni26 wants to merge 3 commits into
openclaw:mainfrom
lakshitsoni26:fix/clean-routing-fix

Conversation

@lakshitsoni26

@lakshitsoni26 lakshitsoni26 commented Jun 19, 2026

Copy link
Copy Markdown

Summary

What problem does this PR solve?
Resolves an issue where external plugin channels relying solely on actions: { handleAction } (without an explicit outbound capability) were incorrectly routed to the internal-source-reply sink when using message(action=send) without a to parameter.

Why does this matter now?
It prevents valid custom channel plugins (like Lansenger) from silently failing or misrouting messages to the internal sink.

What is the intended outcome?
Valid custom channels are properly recognized by the fallback routing logic and receive outbound messages natively.

What is intentionally out of scope?
Changes to official bundled plugins or adding new outbound capabilities; this only touches the fallback validation logic in internal-source-reply.ts.

What does success look like?
shouldUseInternalSourceReplySink correctly evaluates plugins lacking an outbound object by checking plugin?.actions?.handleAction instead of instantly failing when resolveOutboundChannelPlugin returns undefined.

What should reviewers focus on?
The updated conditional logic in src/infra/outbound/internal-source-reply.ts around getLoadedChannelPlugin and the fallback capability checks.

Summary guidance

This PR description is the contributor's durable explanation of the change. Write it for human maintainers first; ClawSweeper and Barnacle use the same text to understand intent, proof, risk, and current review state.

Describe the intent and outcome in 2-5 bullets. Avoid restating the diff; reviewers and bots can read the changed files.

If this PR fixes a plugin beta-release blocker, title it fix(<plugin-id>): beta blocker - <summary> and link the matching Beta blocker: <plugin-name> - <summary> issue labeled beta-blocker. Contributors cannot label PRs, so the title is the PR-side signal for maintainers and automation.

Linked context

Which issue does this close?

Closes #94597

Which issues, PRs, or discussions are related?

Related #94597

Was this requested by a maintainer or owner?
Yes, this is the requested clean and narrow PR isolating only the routing fix, replacing the previous dirty branch containing unrelated MCP changes.

Linked context guidance

Link the issue, PR, discussion, maintainer request, or owner request that explains why this PR should exist. Maintainer context helps reviewers and automation distinguish intended work from drive-by churn.

Real behavior proof (required for external PRs)

  • Behavior or issue addressed: Valid custom channel plugins lacking an outbound capability were incorrectly captured by the internal sink.
  • Real environment tested: Local dev environment testing custom plugin routing bounds.
  • Exact steps or command run after this patch: Triggered a test tool execution using message(action=send) without specifying the to parameter from within the custom channel context.
  • Evidence after fix (screenshot, recording, terminal capture, console output, redacted runtime log, linked artifact, or copied live output):
Screenshot 2026-06-19 at 1 36 07 PM
  • Observed result after fix: Message correctly arrived at the external channel rather than being captured by the internal source-reply policy.
  • What was not tested: Unrelated official plugins that already define the outbound capability (they were unaffected).
  • Proof limitations or environment constraints: None.
  • Before evidence (optional but encouraged):
Screenshot 2026-06-19 at 1 04 53 PM
Real behavior proof guidance

External contributors must show after-fix evidence from a real OpenClaw setup. Unit tests, mocks, lint, typechecks, snapshots, and CI are supplemental only.

Screenshots are encouraged even for CLI, console, text, or log changes. Terminal screenshots, copied live output, redacted runtime logs, recordings, and linked artifacts count.

If your environment cannot produce the ideal proof, explain that under Proof limitations or environment constraints so reviewers and ClawSweeper can direct the next step properly.

Be mindful of private information like IP addresses, API keys, phone numbers, non-public endpoints, or other private details when providing evidence.

Tests and validation

Which commands did you run?
pnpm test src/infra/outbound
pnpm check:test-types

Screenshot 2026-06-19 at 1 29 49 PM

What regression coverage was added or updated?
The fix relies on the extensive existing test suite for outbound routing. All existing tests cover the core channel validation behavior successfully.

What failed before this fix, if known?
Plugins lacking plugin.outbound resulted in resolveOutboundChannelPlugin returning undefined, causing the channel validation to instantly evaluate to false.

If no test was added, why not?
All 709 tests across the 36 test files in src/infra/outbound pass perfectly with this change, confirming no regressions to the existing routing logic. We did not permanently add the temporary bug-repro test file because it uses a mocked custom plugin which interferes with standard test-suite channel bootstrapping rules.

Testing guidance

List focused commands, not every incidental check. CI is useful support, but external PRs still need real behavior proof above when behavior changes.

Risk checklist

Did user-visible behavior change? (Yes/No)
Yes (fixes broken routing for custom plugins).

Did config, environment, or migration behavior change? (Yes/No)
No.

Did security, auth, secrets, network, or tool execution behavior change? (Yes/No)
No.

What is the highest-risk area?
The channel fallback logic in internal-source-reply.ts.

How is that risk mitigated?
By explicitly ensuring we only return false if both plugin?.actions?.handleAction and plugin?.outbound are falsy, which strictly narrows the fix to the exact intended plugins without disrupting official ones.

Risk guidance

Use this for author judgment that is not obvious from the diff. ClawSweeper can see touched files, but it cannot know which behavior you think is risky, why the risk is acceptable, or what mitigation reviewers should verify.

Current review state

What is the next action?
Maintainer review and merge.

What is still waiting on author, maintainer, CI, or external proof?
Waiting for CI to pass.

Which bot or reviewer comments were addressed?
Created a clean branch isolating only the routing fix, per maintainer feedback on the previous dirty branch.

Review state guidance

Keep this as the durable state for review progress. If useful information appears in comments, fold the current next action or blocker back here so maintainers and ClawSweeper do not need to reconstruct state from comment history.

@openclaw-barnacle openclaw-barnacle Bot added size: XS proof: supplied External PR includes structured after-fix real behavior proof. labels Jun 19, 2026
@clawsweeper

clawsweeper Bot commented Jun 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 28, 2026, 8:27 AM ET / 12:27 UTC.

Summary
The PR changes the internal source-reply sink predicate and adds focused predicate tests for action-capable channel plugins.

PR surface: Source +14, Tests +81. Total +95 across 2 files.

Reproducibility: yes. at source level: current main checks the internal-source predicate before normal channel routing and plugin dispatch, matching the reported targetless external-plugin message_tool_only flow. I did not establish a live Lansenger failure in this read-only review.

Review metrics: 1 noteworthy metric.

  • Fallback Predicate Changed: 1 sink gate broadened. This predicate decides whether targetless message_tool_only sends stay in the private sink or enter plugin delivery.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #94597
Summary: The linked open issue is the canonical message-loss report; this PR is its active candidate fix, with an earlier dirty branch superseded and an adjacent maintainer test PR covering part of the sink boundary.

Members:

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

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
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:

  • Gate the sink escape on activated outbound send capability or explicit send-action support.
  • [P1] Add a full targetless runMessageAction regression plus setup-shell or non-send cases.
  • [P1] Attach redacted after-fix proof from Lansenger or an equivalent real external channel.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The supplied screenshots show local Vitest output and a temporary routing test, not after-fix delivery through Lansenger or an equivalent real external channel; the contributor should attach redacted terminal output, logs, screenshot, or video, and updating the PR body should trigger a fresh ClawSweeper review. 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.

Risk before merge

  • [P1] The PR may let loaded plugins with non-activated outbound metadata bypass the private internal source-reply sink even when they cannot actually perform the targetless send.
  • [P1] The supplied proof is local test output rather than redacted after-fix delivery through Lansenger or an equivalent real external channel.

Maintainer options:

  1. Gate On Activated Send (recommended)
    Require the sink escape to use an activated outbound send surface or explicit send-action support, then cover the full runner path before merge.
  2. Accept Broader Loaded-Plugin Escape
    Maintainers can intentionally allow any loaded outbound metadata to bypass the sink, but that needs an explicit compatibility decision with tests for setup shells and non-send plugins.
  3. Pause For Real Channel Proof
    Pause review until the contributor attaches redacted Lansenger or equivalent external-channel after-fix delivery proof.

Next step before merge

  • [P1] Human review remains needed because the PR changes a compatibility-sensitive sink boundary and still needs contributor real behavior proof.

Security
Cleared: No concrete supply-chain, secrets, permissions, or code-execution regression was found in the two-file routing/test diff.

Review findings

  • [P1] Gate the sink escape on an activated send surface — src/infra/outbound/internal-source-reply.ts:99-104
  • [P2] Prove the full targetless send path — src/infra/outbound/internal-source-reply.test.ts:52-53
  • [P2] Remove the explicit any cast blocking lint — src/infra/outbound/internal-source-reply.ts:102
Review details

Best possible solution:

Gate the sink escape on an activated send surface or explicit send-action capability, prove it through the full targetless runMessageAction path, and require redacted real external-channel delivery proof before merge.

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

Yes at source level: current main checks the internal-source predicate before normal channel routing and plugin dispatch, matching the reported targetless external-plugin message_tool_only flow. I did not establish a live Lansenger failure in this read-only review.

Is this the best way to solve the issue?

No. The PR is in the right area, but the best fix should reuse or extract the activated send/action capability contract, keep non-send or setup-only plugins on the sink, and prove the full runner path.

Full review comments:

  • [P1] Gate the sink escape on an activated send surface — src/infra/outbound/internal-source-reply.ts:99-104
    After resolveOutboundChannelPlugin(...allowBootstrap: true) fails, this fallback accepts any loaded plugin.outbound object. Current resolution deliberately rejects setup shells and non-activated outbound metadata in that mode, so this can move targetless message_tool_only replies out of the private sink even when the plugin still cannot perform the send.
    Confidence: 0.91
  • [P2] Prove the full targetless send path — src/infra/outbound/internal-source-reply.test.ts:52-53
    These tests only call the sink predicate with a mocked registry. They do not prove that runMessageAction infers the current target and reaches plugin send dispatch for an action-only external plugin, so the reported user path could remain broken while this unit passes.
    Confidence: 0.84
  • [P2] Remove the explicit any cast blocking lint — src/infra/outbound/internal-source-reply.ts:102
    The PR head's check-lint job fails on this cast with typescript(no-explicit-any). Replace it with the existing channel action type or a small typed helper so the required lint lane can pass.
    Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.9

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 08d15ec32db3.

Label changes

Label justifications:

  • P1: The PR targets a current user-facing external plugin channel workflow where targetless sends may fail delivery.
  • merge-risk: 🚨 compatibility: The diff changes a compatibility-sensitive source-reply fallback predicate for configured plugin channels.
  • merge-risk: 🚨 message-delivery: The sink decision controls whether targetless source replies stay private or enter plugin delivery, so an imprecise capability check can suppress or misroute messages.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The supplied screenshots show local Vitest output and a temporary routing test, not after-fix delivery through Lansenger or an equivalent real external channel; the contributor should attach redacted terminal output, logs, screenshot, or video, and updating the PR body should trigger a fresh ClawSweeper review. 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 +14, Tests +81. Total +95 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 15 1 +14
Tests 1 81 0 +81
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 96 1 +95

Acceptance criteria:

  • [P1] pnpm test src/infra/outbound/internal-source-reply.test.ts src/infra/outbound/message-action-runner.plugin-dispatch.test.ts src/infra/outbound/message-action-runner.core-send.test.ts.
  • [P1] pnpm check:test-types.
  • [P1] pnpm lint --threads=8.

What I checked:

Likely related people:

  • vincentkoc: Introduced internal-source-reply.ts and recently changed message-action-runner and channel-resolution in the outbound path. (role: recent area contributor; confidence: high; commits: 0ea08076c3b5, a22a1edc8faa, 21125352d824; files: src/infra/outbound/internal-source-reply.ts, src/infra/outbound/message-action-runner.ts, src/infra/outbound/channel-resolution.ts)
  • ZengWen-DT: Recent merged work added activation-aware plugin session target and outbound bootstrap behavior that this predicate must preserve. (role: recent channel-resolution contributor; confidence: medium; commits: 6470bb76253b; files: src/infra/outbound/channel-resolution.ts)
  • mushuiyu886: Recent session-key routing changes touched the same internal source-reply sink and malformed route handling. (role: recent adjacent contributor; confidence: medium; commits: 65fec9d787e3; files: src/infra/outbound/internal-source-reply.ts, src/infra/outbound/message-action-runner.send-validation.test.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: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. labels Jun 19, 2026
This was referenced Jun 20, 2026
Closed
Open
@clawsweeper

clawsweeper Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

@lakshitsoni26 thanks for the PR. ClawSweeper is still waiting on real behavior proof before this can move forward.

Useful proof can be a screenshot, short video, terminal output, copied live output, linked artifact, or redacted logs that show the changed behavior after the fix. Please redact private tokens, phone numbers, private endpoints, customer data, and anything else sensitive.

Once proof is added to the PR body or a comment, ClawSweeper or a maintainer can re-check it.

@clawsweeper clawsweeper Bot mentioned this pull request Jul 8, 2026
Closed
@clawsweeper

clawsweeper Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

@lakshitsoni26 thanks for the PR. ClawSweeper is still waiting on real behavior proof before this can move forward.

Useful proof can be a screenshot, short video, terminal output, copied live output, linked artifact, or redacted logs that show the changed behavior after the fix. Please redact private tokens, phone numbers, private endpoints, customer data, and anything else sensitive.

Once proof is added to the PR body or a comment, ClawSweeper or a maintainer can re-check it.

@openclaw-barnacle

openclaw-barnacle Bot commented Jul 26, 2026

Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@openclaw-barnacle openclaw-barnacle Bot added the stale Marked as stale due to inactivity label Jul 26, 2026
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

merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. P1 High-priority user-facing bug, regression, or broken workflow. proof: supplied External PR includes structured after-fix real behavior proof. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S stale Marked as stale due to inactivity status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

message(action=send) without to fails for plugin channels under message_tool_only

1 participant

@lakshitsoni26