GoogleChat: respect replyToMode for thread replies#43112
Conversation
Preserve Google Chat thread references through the shared reply pipeline so off/first/all modes behave correctly without forcing implicit thread targets.
openclaw-barnacle
Bot
added
channel: googlechat
Greptile SummaryThis PR fixes the root cause of Google Chat always threading replies regardless of Key observations:
Confidence Score: 4/5
Last reviewed commit: 1bbbf96 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| path: "/googlechat", | ||
| mediaMaxMb: 5, | ||
| }); | ||
|
|
||
| try { | ||
| const handled = await handleGoogleChatWebhookRequest( | ||
| createWebhookRequest(createMessageEvent(threadName)), | ||
| createMockServerResponse(), | ||
| ); | ||
|
|
||
| expect(handled).toBe(true); | ||
| expect(recordSessionMetaFromInbound).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| ctx: expect.objectContaining({ | ||
| CurrentMessageId: threadName, | ||
| ReplyToId: threadName, | ||
| MessageThreadId: threadName, | ||
| }), | ||
| }), | ||
| ); | ||
| } finally { | ||
| unregister(); | ||
| } | ||
| }); | ||
|
|
||
| it("uses the preserved inbound thread for immediate replies", async () => { | ||
| const threadName = "spaces/AAA/threads/thread-2"; | ||
| sendGoogleChatMessageMock.mockResolvedValue({ | ||
| messageName: "spaces/AAA/messages/reply-1", | ||
| }); | ||
| const core = createCore({ | ||
| onDispatchReply: async ({ dispatcherOptions }) => { | ||
| await dispatcherOptions.deliver({ text: "Threaded reply", replyToId: threadName }); | ||
| }, | ||
| }); | ||
| const unregister = registerGoogleChatWebhookTarget({ | ||
| account: createAccount({ typingIndicator: "none" }), | ||
| config: createConfig(), |
There was a problem hiding this comment.
Misleading test config vs. test intent
createConfig() hardcodes replyToMode: "off", yet this test explicitly delivers { text: "Threaded reply", replyToId: threadName } — a combination that could never occur in production because the shared reply pipeline would have already stripped replyToId before calling deliver when the mode is "off".
The test is technically exercising a valid low-level path (that deliverGoogleChatReply passes replyToId through when it is present), but it can mislead a future maintainer into thinking that replyToMode: "off" still allows an explicit replyToId to reach the transport layer. Consider either (a) using a config with replyToMode: "all" for this test, or (b) adding a comment clarifying that this test exercises the delivery function in isolation, independent of the replyToMode filter.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/googlechat/src/monitor.threading.test.ts
Line: 243-280
Comment:
**Misleading test config vs. test intent**
`createConfig()` hardcodes `replyToMode: "off"`, yet this test explicitly delivers `{ text: "Threaded reply", replyToId: threadName }` — a combination that could never occur in production because the shared reply pipeline would have already stripped `replyToId` before calling `deliver` when the mode is `"off"`.
The test is technically exercising a valid low-level path (that `deliverGoogleChatReply` passes `replyToId` through when it is present), but it can mislead a future maintainer into thinking that `replyToMode: "off"` still allows an explicit `replyToId` to reach the transport layer. Consider either (a) using a config with `replyToMode: "all"` for this test, or (b) adding a comment clarifying that this test exercises the delivery function in isolation, independent of the `replyToMode` filter.
How can I resolve this? If you propose a fix, please make it concise.| let editableTypingMessageName = typingMessageName; | ||
| if (editableTypingMessageName && resolvedThreadId) { | ||
| try { | ||
| await deleteGoogleChatMessage({ | ||
| account, | ||
| messageName: editableTypingMessageName, | ||
| }); | ||
| } catch (err) { | ||
| runtime.error?.(`Google Chat typing cleanup failed: ${String(err)}`); | ||
| } | ||
| editableTypingMessageName = undefined; | ||
| } |
There was a problem hiding this comment.
Typing placeholder leaked on delete failure for threaded replies
editableTypingMessageName is unconditionally set to undefined after the try/catch block regardless of whether the delete succeeded. On delete failure, the typing message remains visible as _<bot> is typing..._ in the root conversation indefinitely — no fallback update clears it. This is consistent with the media-path behaviour, but unlike the media path there is no fallback updateGoogleChatMessage call here either.
This is likely intentional (avoiding a root-level message update when the real reply belongs in a thread), but could be worth leaving a brief code comment to that effect — otherwise it looks like the error handler is incomplete compared to the non-threaded path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/googlechat/src/monitor.ts
Line: 464-475
Comment:
**Typing placeholder leaked on delete failure for threaded replies**
`editableTypingMessageName` is unconditionally set to `undefined` after the try/catch block regardless of whether the delete succeeded. On delete failure, the typing message remains visible as `_<bot> is typing..._` in the root conversation indefinitely — no fallback update clears it. This is consistent with the media-path behaviour, but unlike the media path there is no fallback `updateGoogleChatMessage` call here either.
This is likely intentional (avoiding a root-level message update when the real reply belongs in a thread), but could be worth leaving a brief code comment to that effect — otherwise it looks like the error handler is incomplete compared to the non-threaded path.
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: 1bbbf96147
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| (channelId === "slack" && threadId != null && threadId !== "" ? String(threadId) : undefined); | ||
| const resolvedThreadId = channelId === "slack" ? null : (threadId ?? null); | ||
| const resolvedThreadId = | ||
| channelId === "slack" || channelId === "googlechat" ? null : (threadId ?? null); |
There was a problem hiding this comment.
Setting resolvedThreadId to null for Google Chat drops caller-supplied threadId even when the payload has no replyToId, so routeReply posts to the root space instead of the intended thread. This affects existing call sites that pass only threadId (for example sendResetSessionNotice in src/auto-reply/reply/get-reply-run.ts:108-122 and ACP routing in src/auto-reply/reply/dispatch-acp-delivery.ts:152-159), which means thread-scoped notices/tool replies can silently lose thread context after this change. A safer behavior is to avoid unconditional dropping and preserve thread targeting when no payload-level reply reference is present.
Useful? React with 👍 / 👎.
1 participant
Summary
threadIdpath, soreplyToModefiltering onreplyToIddid not actually disable or limit threading.channels.googlechat.replyToMode: \"off\"still threaded replies, and\"first\"could keep threading follow-up payloads after the first reply.threadIdas an unconditional explicit thread target.AI-assisted: yes. Testing: fully tested locally.
Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
Google Chat now respects
channels.googlechat.replyToModein the shared reply pipeline:offkeeps replies in the root conversationfirstonly threads the first replyallpreserves threaded replies consistentlySecurity Impact (required)
NoNoNoNoNoYes, explain risk + mitigation:Repro + Verification
Environment
channels.googlechat.replyToMode=off|first|allSteps
message.thread.nameset.replyToMode=off,first, andall.Expected
offstrips implicit threading.firstuses the thread only once.allpreserves the Google Chat thread reference consistently.Actual
threadIdpath even afterreplyToIdhad been filtered out.Evidence
Human Verification (required)
pnpm install,pnpm build,pnpm check,pnpm testreplyToMode=off,replyToMode=first, immediate monitor replies, routed shared-pipeline replies, typing-placeholder behavior for threaded repliesReview Conversations
Compatibility / Migration
YesNoNoFailure Recovery (if this breaks)
1bbbf961extensions/googlechat/src/monitor.ts,src/auto-reply/reply/route-reply.ts,src/auto-reply/reply/agent-runner-utils.tsreplyToMode=off, or later replies continuing to thread whenreplyToMode=firstRisks and Mitigations
CurrentMessageIdfor Google Chat, so regressions would show up as incorrect implicit threading rather than explicit send failures.Made with Cursor