fix: narrow reply-root dedupe to queued reruns#47923
fix: narrow reply-root dedupe to queued reruns#47923livingghost wants to merge 3 commits intoopenclaw:mainfrom
Conversation
Track recent reply roots across queued and routed sends so the same inbound thread does not trigger duplicate agent replies. Surface the reply-root key into message hooks so plugin-side send guards can keep using the same identifier.
Avoid dropping fresh inbound messages that happen to share a reply root, prefer explicit reply targets over thread roots, and only mark a reply root as sent after an outbound delivery actually occurs.
Greptile SummaryThis PR narrows reply-root deduplication to only suppress queued reruns of the same causal reply target, fixing a regression where fresh inbound turns sharing a reply root with an earlier message were incorrectly dropped. Key changes:
One style concern: in Confidence Score: 4/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/infra/outbound/deliver.ts
Line: 437-441
Comment:
**`hookMetadata` spread can silently override canonical delivery fields**
`...params.hookMetadata` is placed *after* `channel`, `accountId`, and `mediaUrls`. Since `hookMetadata` is typed as `Record<string, unknown>`, any future caller that accidentally includes a `channel`, `accountId`, or `mediaUrls` key in the metadata would silently override the canonical delivery values that plugins receive in the `message_sending` hook — without any compile-time warning.
The current callers from `route-reply.ts` don't have conflicting keys, but the contract is fragile. Reversing the spread order ensures the canonical delivery values always win:
```suggestion
metadata: {
...params.hookMetadata,
channel: params.channel,
accountId: params.accountId,
mediaUrls: params.payloadSummary.mediaUrls,
},
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 78264ed |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| channel: params.channel, | ||
| accountId: params.accountId, | ||
| mediaUrls: params.payloadSummary.mediaUrls, | ||
| ...params.hookMetadata, | ||
| }, |
There was a problem hiding this comment.
hookMetadata spread can silently override canonical delivery fields
...params.hookMetadata is placed after channel, accountId, and mediaUrls. Since hookMetadata is typed as Record<string, unknown>, any future caller that accidentally includes a channel, accountId, or mediaUrls key in the metadata would silently override the canonical delivery values that plugins receive in the message_sending hook — without any compile-time warning.
The current callers from route-reply.ts don't have conflicting keys, but the contract is fragile. Reversing the spread order ensures the canonical delivery values always win:
| channel: params.channel, | |
| accountId: params.accountId, | |
| mediaUrls: params.payloadSummary.mediaUrls, | |
| ...params.hookMetadata, | |
| }, | |
| metadata: { | |
| ...params.hookMetadata, | |
| channel: params.channel, | |
| accountId: params.accountId, | |
| mediaUrls: params.payloadSummary.mediaUrls, | |
| }, |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/infra/outbound/deliver.ts
Line: 437-441
Comment:
**`hookMetadata` spread can silently override canonical delivery fields**
`...params.hookMetadata` is placed *after* `channel`, `accountId`, and `mediaUrls`. Since `hookMetadata` is typed as `Record<string, unknown>`, any future caller that accidentally includes a `channel`, `accountId`, or `mediaUrls` key in the metadata would silently override the canonical delivery values that plugins receive in the `message_sending` hook — without any compile-time warning.
The current callers from `route-reply.ts` don't have conflicting keys, but the contract is fragile. Reversing the spread order ensures the canonical delivery values always win:
```suggestion
metadata: {
...params.hookMetadata,
channel: params.channel,
accountId: params.accountId,
mediaUrls: params.payloadSummary.mediaUrls,
},
```
How can I resolve this? If you propose a fix, please make it concise.![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: 78264ed959
ℹ️ 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".
| if (hasRecentSentReplyRoot(recentSentReplyRootKey)) { | ||
| await touchActiveSessionEntry(); | ||
| typing.cleanup(); | ||
| return undefined; |
There was a problem hiding this comment.
This guard drops every follow-up while a run is active whenever the reply-root key was seen recently, without checking whether the inbound turn is actually the same message (or even the same reply target). Because resolveReplyRootId can fall back to RootMessageId, providers that only expose a thread root will map multiple legitimate user messages to the same key, so new messages in that thread can be silently discarded for up to the TTL window instead of being queued.
Useful? React with 👍 / 👎.
| return buildRecentSentReplyRootKey({ | ||
| scopeKey: run.run.sessionKey ?? run.run.sessionId, | ||
| agentId: run.run.agentId, | ||
| channel: run.originatingChannel, |
There was a problem hiding this comment.
The follow-up-side key uses run.originatingChannel as-is, but routeReply records sent keys with normalizeChannelId(channel). If inbound context carries a non-canonical channel string (for example different casing), the recorded key and lookup key diverge, so queued reruns won't be suppressed even though the same reply root was just sent.
Useful? React with 👍 / 👎.
Keep queued reply-root suppression scoped to real retry targets, skip thread-root fallbacks, and ensure hook metadata preserves canonical delivery fields.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bcaaaa229e
ℹ️ 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".
| const replyRootId = run.replyRootId?.trim(); | ||
| if (replyRootId && run.replyRootSource !== "thread-root") { | ||
| return { kind: "reply-root", value: replyRootId }; | ||
| } |
There was a problem hiding this comment.
resolveRunDedupeIdentity now prioritizes replyRootId over messageId, so queued dedupe collapses distinct turns that reply to the same parent message. Since runPreparedReply now populates replyRootId from ReplyToId* first, two different inbound messages in one thread (different provider message IDs) are treated as duplicates under followup/collect queueing and only one is kept. This drops legitimate user input; replyRootId should be a fallback identity only when a stable message ID is missing.
Useful? React with 👍 / 👎.
1 participant
Summary
Describe the problem and fix in 2–5 bullets:
Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
Security Impact (required)
Yes/No) NoYes/No) NoYes/No) NoYes/No) NoYes/No) NoYes, explain risk + mitigation:Repro + Verification
Environment
Steps
Expected
Actual
Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
ReplyToIdFullshould beatRootMessageId.Review Conversations
If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.
Compatibility / Migration
Yes/No) YesYes/No) NoYes/No) NoFailure Recovery (if this breaks)
78264ed95and07db1c8eefromopenclaw-pr3src/auto-reply/reply/and outbound hook metadata wiringRisks and Mitigations
List only real risks for this PR. Add/remove entries as needed. If none, write
None.