Skip to content

fix(outbound): prevent internal sink from intercepting action-only plugins#94869

Closed
lakshitsoni26 wants to merge 2 commits into
openclaw:mainfrom
lakshitsoni26:fix/plugin-channel-routing
Closed

fix(outbound): prevent internal sink from intercepting action-only plugins#94869
lakshitsoni26 wants to merge 2 commits into
openclaw:mainfrom
lakshitsoni26:fix/plugin-channel-routing

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 that rely solely on actions: { handleAction } and do not have 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 channel plugins (like Lansenger) from silently failing or misrouting messages.

What is the intended outcome?

  • Valid custom channels are properly recognized by the routing logic and receive outbound messages through their native interfaces.

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 without an outbound object by checking plugin?.actions?.handleAction, instead of instantly failing.

What should reviewers focus on?

  • The updated conditional logic in src/infra/outbound/internal-source-reply.ts around getLoadedChannelPlugin and the capability checks.

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, it fixes the plugin routing bug as defined in #94597.

Real behavior proof (required for external PRs)

  • Behavior or issue addressed: Valid plugins lacking outbound capability were incorrectly sent to the internal sink.
  • Real environment tested: Local dev environment with the Lansenger channel plugin.
  • Exact steps or command run after this patch: Triggered a tool execution that uses message(action=send) without specifying the to parameter from within the Lansenger 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 Screenshot 2026-06-19 at 1 29 49 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

Tests and validation

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

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 710 tests across the 37 test files in src/infra/outbound pass perfectly with this change, confirming that there are no regressions to the existing routing and channel resolution logic.

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 intended plugins.

Current review state

What is the next action?
Maintainer review and merge.

What is still waiting on author, maintainer, CI, or external proof?
None, all local tests and type checks have successfully passed.

Which bot or reviewer comments were addressed?
None yet.

@lakshitsoni26
fix(mcp): evaluate environment variables at transport connection time
73f8792 
Fixes openclaw#93144

Prior to this change,  templates in MCP config fields (such as
`env` and `headers`) were resolved during Gateway startup. As a result,
any dynamic rotation of underlying environment variables required a full
Gateway restart.

This commit shifts the environment variable substitution logic to the MCP
transport connection layer. We now intercept the SDK's fetch invocations
(for HTTP/SSE) and child process spawn routines (for stdio), dynamically
substituting variables based on the live `process.env` state at the
exact moment of request, cleanly solving the credential rotation limits.
@lakshitsoni26
fix(infra): allow plugins without outbound capability to handle chann…
04c25e9 
…el selection
@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: S 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

Thanks for the contribution. I reviewed the branch, and this PR is not a good landing base for OpenClaw.

This PR should close as a dirty branch: the useful outbound routing fix is small and relevant, but most of the branch is an unrelated MCP dynamic-credential change already open separately, so this is not a safe landing candidate.

So I’m closing this PR rather than keeping an unmergeable branch open. A new narrow PR that carries only the useful part is welcome.

Review details

Best possible solution:

Keep the MCP dynamic-env work in #94167 and review a narrow outbound-only patch for #94597 with focused coverage and real external-channel proof.

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

Yes at source level for the linked outbound bug: current main checks the internal sink before normal plugin routing, and the issue report plus screenshots show the claimed failure path. I did not establish a fresh real Lansenger channel reproduction in this review.

Is this the best way to solve the issue?

No. The outbound idea may be useful, but this branch is not the best solution because it bundles unrelated MCP credential semantics; the maintainable path is a narrow outbound-only PR plus separate MCP review.

Security review:

Security review needs attention: The branch introduces security-sensitive MCP credential resolution changes that are unrelated to the outbound routing fix.

  • [medium] Keep dynamic MCP credentials out of this routing PR — src/agents/mcp-transport.ts:97
    The first commit skips config-time substitution for MCP env/headers and re-reads process.env in MCP transport code, changing when credential-bearing values are applied; that belongs in the dedicated MCP security/product review path, not this outbound routing PR.
    Confidence: 0.93

AGENTS.md: found and applied where relevant.

What I checked:

  • PR diff scope: The diff shows one small outbound change plus a separate five-file MCP dynamic env/header substitution commit that is outside the stated outbound-routing scope. (src/agents/mcp-transport.ts:97, 04c25e987ca7)
  • Separate MCP PR already exists: The same MCP commit is already the entirety of open pull request fix(mcp): evaluate environment variables at transport connection time #94167, which is labeled with compatibility, auth-provider, and security-boundary merge risk. (src/agents/mcp-transport.ts:97, 73f87924c617)
  • Linked outbound issue remains the canonical bug: The linked issue is still open and describes targetless message-tool sends from an external plugin channel being intercepted by the internal source-reply sink.
  • Current source-reply boundary: Current main calls the internal source-reply sink check before normal message-action channel resolution, so the outbound portion is plausibly relevant but compatibility-sensitive. (src/infra/outbound/message-action-runner.ts:1414, dbd5689ea199)
  • Current actions-only channel contract: Current channel-resolution tests explicitly assert that an actions-only plugin is not treated as send-capable after bootstrap, which makes the proposed sink bypass an owner-boundary decision needing focused coverage. (src/infra/outbound/channel-resolution.test.ts:383, dbd5689ea199)
  • Proof inspection: The attached screenshots show local Vitest/test-script output, including a passing routing test, but they do not show a real external Lansenger channel delivery after the patch.

Likely related people:

  • glenn-agent: Current blame for the touched outbound and MCP transport files points to the recent merged PR that carried these areas into current main, though the commit was broad so ownership confidence is limited. (role: recent area contributor; confidence: medium; commits: 2f1275549816; files: src/infra/outbound/internal-source-reply.ts, src/infra/outbound/message-action-runner.ts, src/agents/mcp-transport.ts)
  • Peter Steinberger: Recent history shows related work on source-reply routing, MCP operator workflows, and config env substitution behavior. (role: adjacent feature owner; confidence: high; commits: 1955f42bfeb2, 38d3d11cbc0c, 617c65849856; files: src/infra/outbound/message-action-runner.ts, src/agents/mcp-transport.ts, src/config/env-substitution.ts)
  • Vincent Koc: Recent commits touched adjacent outbound/source-reply delivery and tool payload behavior that intersects the linked message-tool routing bug. (role: recent adjacent contributor; confidence: medium; commits: 0ea08076c3b5, a22a1edc8faa; files: src/infra/outbound/internal-source-reply.ts, src/infra/outbound/message-action-runner.ts)
  • Pavan Kumar Gondhi: Recent MCP HTTP redirect/header scrub work is directly relevant to any request-time MCP header substitution change. (role: adjacent MCP HTTP safety contributor; confidence: medium; commits: 3c6259ebb70c, 47eb2d48d434; files: src/agents/mcp-http-fetch.ts, src/agents/mcp-transport.ts, src/agents/mcp-transport.test.ts)

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

@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: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. labels Jun 19, 2026
@clawsweeper

clawsweeper Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper applied the proposed close for this PR.

@clawsweeper clawsweeper Bot closed this Jun 19, 2026
@lakshitsoni26
lakshitsoni26 deleted the fix/plugin-channel-routing branch June 19, 2026 13:26
This was referenced Jun 19, 2026
Open
Closed
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: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. 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 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