fix(gateway): prefer current route delivery context#69010
Conversation
Greptile SummaryThis PR fixes a routing bug in the gateway agent handler where stale persisted session Confidence Score: 5/5Safe to merge — the fix is a single targeted merge-order change with a solid regression test and no security implications. The root cause analysis is accurate, the reversal of mergeDeliveryContext(requestDeliveryHint, deliveryFields.deliveryContext) correctly makes the live request primary, mergeDeliveryContext already handles cross-channel field isolation via the channelsConflict guard, and the currentChannelId: resolvedTo addition is straightforward. The regression test exercises the exact bug shape reported. No P0/P1 issues found. No files require special attention. Reviews (1): Last reviewed commit: "fix(gateway): prefer current route deliv..." | Re-trigger Greptile |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a557b56825
ℹ️ 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".
5825aed to
e9c89b3
Compare
|
Aisle / review follow-up:
Verification:
Notes on broader local gates: local |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9c89b3c43
ℹ️ 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".
e6911ce to
d8f4d09
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8f4d09a34
ℹ️ 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".
d8f4d09 to
af75067
Compare
|
同样遇到此问题，等待修复，请尽快合并。 |
|
ProjectClownfish pushed a narrow repair to this branch so the original contributor path can stay canonical. Source PR: #69010 |
af75067 to
21a07b1
Compare
🔒 Aisle Security AnalysisWe found 1 potential security issue(s) in this PR:
1. 🟡 Mixed accountId/to when replacing session delivery route can misdeliver messages
DescriptionWhen
This can create an inconsistent route where a new Vulnerable code: normalizeDeliveryContext({
channel: requestDeliveryHint?.channel ?? sessionDelivery?.channel,
to: requestDeliveryHint?.to ?? sessionDelivery?.to,
accountId: requestDeliveryHint?.accountId ?? sessionDelivery?.accountId,
threadId: requestDeliveryHint?.threadId,
})RecommendationRequire route fields to be updated atomically when changing the target (or explicitly clear coupled fields) to avoid pairing For example, when
One possible safe approach: if (requestTargetDiffers) {
if (requestDeliveryHint?.to == null) {
// cannot change target without explicit to
return error;
}
// either require accountId explicitly or clear it
const effective = normalizeDeliveryContext({
channel: requestDeliveryHint.channel ?? sessionDelivery?.channel,
to: requestDeliveryHint.to,
accountId: requestDeliveryHint.accountId, // no fallback
threadId: requestDeliveryHint.threadId, // no fallback
});
}Also consider validating that Analyzed PR: #69010 at commit Last updated on: 2026-04-28T18:49:49Z |
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Keep open: this PR still targets a real wrong-channel gateway routing bug, but it is not merge-ready because the branch is stale against current main, still has an account/target mixing path, and lacks after-fix real behavior proof. Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Canonical path: Close this stale PR. The latest review rated it F, the branch still lacks merge-ready proof, and there has been no human follow-up after the durable review. So I’m closing this here because the remaining work is already tracked in the canonical issue. Review detailsBest possible solution: Close this stale PR. The latest review rated it F, the branch still lacks merge-ready proof, and there has been no human follow-up after the durable review. Do we have a high-confidence way to reproduce the issue? Yes, from source and linked logs rather than a fresh local run: the linked issue shows Discord traffic delivered via Feishu, and the PR/current-main code paths show how stale session delivery context and turn-source routing decide the destination. I did not run a live Discord/Feishu reproduction in this read-only review. Is this the best way to solve the issue? No, not in its current form. The bug belongs in the gateway/outbound route owner path, but this branch must be rebased onto current main and must avoid pairing a fresh target with stale account metadata. Security review: Security review needs attention: The diff still has a concrete same-channel account/target mixing risk that can misdeliver messages across configured channel accounts.
AGENTS.md: found and applied where relevant. What I checked:
Likely related people:
Codex review notes: model internal, reasoning high; reviewed against 8ed6c78b7891. |
vincentkoc
added
clawsweeper
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
openclaw-barnacle
Bot
added
the
triage: needs-real-behavior-proof
clawsweeper
Bot
added
P1
|
This pull request has been automatically marked as stale due to inactivity. |
clawsweeper
Bot
added
rating: 🌊 off-meta tidepool
clawsweeper
Bot
added
rating: 🧂 unranked krab
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
openclaw-barnacle
Bot
added
the
triage: needs-pr-context
clawsweeper
Bot
added
merge-risk: 🚨 security-boundary
|
This pull request has been automatically marked as stale due to inactivity. |
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: fix(gateway): prefer current route delivery context This is item 1/1 in the current shard. Shard 15/22. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
3 participants
Summary
channel,to,accountId,threadId) wins over stale persisted sessiondeliveryContextfields when the channels conflict.to/accountId/threadIdvalues, avoiding mixed-channel routes.channel=last, and same-channel existing-session routes.runContext.currentChannelIdfrom the resolved delivery target so downstream message-tool fallback inference sees the active route.Linked Issue/PR
resolvedTo->runContext.currentChannelIdpropagation, but that PR is closed unmerged and does not address stale delivery-context precedence.Root Cause / Diagnosis
src/gateway/server-methods/agent.tsbuiltrequestDeliveryHintfrom the active gateway request, but then merged it with the stored sessiondeliveryContextin a way that made persisted fields authoritative. That let stale persisted fields, such as Feishuchannel/to, override a current Discord request before the agent command and session update were assembled. The same path also did not pass the resolved target intorunContext.currentChannelId, so tool fallback routing could miss the current route even whenresolvedTowas known.A second edge case appears when the request changes only
channel: route-field fallback must not pair a new channel with stalelastTo/lastAccountId/lastThreadIdfrom the old channel.A third edge case appears when the request and session refer to the same target with different channel-owned encodings, such as Slack
C1andchannel:C1: raw string comparison can falsely treat that as a target change and clear the stored thread.Fix
toas a target change, preserving stored threads for equivalent target spellings.channel=last, normal follow-ups, and existing subagent routes behave as before.resolvedTointorunContext.currentChannelIdalongside the existing channel/account/thread context.C1andchannel:C1are equivalent.Duplicate Sweep
Searched GitHub issues/PRs for combinations of
#68378, Discord/Feishu wrong-channel delivery, staledeliveryContext, gateway agent delivery,currentChannelId, and multi-channel session routing. Closest results:currentChannelIdpropagation only; closed unmerged; no stale context precedence fix or regression for Bug: Gateway sends Discord session messages to Feishu channel (critical message routing bug) #68378.chat.send/cron canonical route derivation; closed unmerged; different call sites.Conclusion: no duplicate or already-satisfactory PR found for #68378.
Attribution
resolvedTointorunContext.currentChannelId; this PR incorporates that same missing-context piece and adds the route-precedence fix needed for Bug: Gateway sends Discord session messages to Feishu channel (critical message routing bug) #68378.Testing
Fresh
origin/mainbase:64089fd15ed4ba8296612b1a622d11b5662c8b96pnpm test -- src/gateway/server-methods/agent.test.ts -t "uses the current request route"pnpm test -- src/gateway/server-methods/agent.test.tspnpm test -- src/gateway/server-methods/agent.test.ts -t "preserves the stored thread when the request target is canonically equivalent"pnpm test -- src/gateway/server-methods/agent.test.ts src/gateway/server.agent.gateway-server-agent-a.test.ts src/gateway/server.agent.gateway-server-agent-b.test.ts src/gateway/server.agent.subagent-delivery-context.test.tsThe final command covers the CI shard failures observed after the first draft and now passes locally with 71 tests.
Scoped lint note:
pnpm lint -- src/gateway/server-methods/agent.ts src/gateway/server-methods/agent.test.tsis currently blocked before linting these files by the existingsrc/agents/skills/*plugin SDK boundary type errors already present onorigin/main.Review Follow-up
5825aede6c7568f8cec2af2653d4851bf3296d78.d8f4d09a343f30d82df2bdeea9541a707923a72d.af7506771a0fd734024ecc2b51b23b7945f27b21.Security Impact
The change narrows routing behavior to prefer the active request route only when it conflicts with stale persisted delivery metadata, and preserves stored thread context for equivalent same-channel target spellings.