fix(mattermost): restore threadRootId priority in thread replies#55186
fix(mattermost): restore threadRootId priority in thread replies#55186hnykda wants to merge 1 commit intoopenclaw:mainfrom
Conversation
…fectiveReplyToId Commit aaba1ae ("honor replyToMode off for threaded messages") added a replyToMode !== "off" guard to the threadRootId branch in resolveMattermostEffectiveReplyToId(). This conflates two concerns: 1. Session routing (should DM threads fork their own session?) 2. Reply delivery (should the bot reply inside the existing thread?) replyToMode controls (1), but the guard also kills (2): when replyToMode is "off" (the default for DMs), effectiveReplyToId becomes undefined, so all downstream call sites (typing indicators, reply delivery via resolveMattermostReplyRootId) lose the thread root. The bot then either posts to the main channel or gets a 400 Invalid RootId error. Fix: remove the replyToMode guard so threadRootId always flows through. replyToMode still controls session forking via resolveMattermostThreadSessionContext. Fixes openclaw#30977
Greptile SummaryThis PR makes a targeted one-line fix to Confidence Score: 4/5Safe to merge once the stale unit test is updated to reflect the new behaviour. The one-line logic fix is correct and well-reasoned. The only concrete issue is a pre-existing test that now asserts the removed behaviour and will fail — a straightforward update. No other tests, types, or call-sites appear affected. extensions/mattermost/src/mattermost/monitor.test.ts — the test 'suppresses existing thread roots when replyToMode is off' (line 172) must be updated.
|
| Filename | Overview |
|---|---|
| extensions/mattermost/src/mattermost/monitor.ts | Removes the replyToMode !== 'off' guard in resolveMattermostEffectiveReplyToId so threadRootId is always forwarded. Logic fix is correct but the corresponding test in monitor.test.ts still asserts the old behaviour and will fail. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: extensions/mattermost/src/mattermost/monitor.ts
Line: 178-179
Comment:
**Test still asserts the old (removed) behaviour**
The existing test at `monitor.test.ts:172` is titled *"suppresses existing thread roots when replyToMode is off"* and asserts:
```ts
resolveMattermostEffectiveReplyToId({
kind: "channel",
postId: "post-123",
replyToMode: "off",
threadRootId: "thread-root-456",
})
// → toBeUndefined()
```
After this one-line change, the function returns `"thread-root-456"` for that input, so the test will fail. The test should be updated (or replaced) to reflect the new intended behaviour — i.e. `threadRootId` is always forwarded regardless of `replyToMode`. A complementary test specifically covering the DM-thread-reply scenario from the bug report (`kind: "direct"`, `replyToMode: "off"`, `threadRootId` set) would also make the regression guard explicit.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(mattermost): restore threadRootId pr..." | Re-trigger Greptile
| if (threadRootId) { | ||
| return threadRootId; |
There was a problem hiding this comment.
The existing test at monitor.test.ts:172 is titled "suppresses existing thread roots when replyToMode is off" and asserts:
resolveMattermostEffectiveReplyToId({
kind: "channel",
postId: "post-123",
replyToMode: "off",
threadRootId: "thread-root-456",
})
// → toBeUndefined()After this one-line change, the function returns "thread-root-456" for that input, so the test will fail. The test should be updated (or replaced) to reflect the new intended behaviour — i.e. threadRootId is always forwarded regardless of replyToMode. A complementary test specifically covering the DM-thread-reply scenario from the bug report (kind: "direct", replyToMode: "off", threadRootId set) would also make the regression guard explicit.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/mattermost/src/mattermost/monitor.ts
Line: 178-179
Comment:
**Test still asserts the old (removed) behaviour**
The existing test at `monitor.test.ts:172` is titled *"suppresses existing thread roots when replyToMode is off"* and asserts:
```ts
resolveMattermostEffectiveReplyToId({
kind: "channel",
postId: "post-123",
replyToMode: "off",
threadRootId: "thread-root-456",
})
// → toBeUndefined()
```
After this one-line change, the function returns `"thread-root-456"` for that input, so the test will fail. The test should be updated (or replaced) to reflect the new intended behaviour — i.e. `threadRootId` is always forwarded regardless of `replyToMode`. A complementary test specifically covering the DM-thread-reply scenario from the bug report (`kind: "direct"`, `replyToMode: "off"`, `threadRootId` set) would also make the regression guard explicit.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aeec08080c
ℹ️ 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".
| if (threadRootId) { | ||
| return threadRootId; |
There was a problem hiding this comment.
Keep session routing honoring replyToMode off
resolveMattermostEffectiveReplyToId() now returns threadRootId unconditionally, but this value is also consumed by resolveMattermostThreadSessionContext() as the threadId for session key derivation. That means replyToMode: "off" no longer keeps sessions linear when a root_id is present, including direct chats where resolveMattermostReplyToMode() forces off; threaded DM messages will now fork to ...:thread:<root> sessions and lose continuity with the main DM session history. If the goal is only to preserve reply delivery in existing threads, delivery root selection needs to be decoupled from session-key threading.
Useful? React with 👍 / 👎.
|
Closing this as duplicate or superseded after Codex automated review. Close #55186 as superseded. Current main still needs Mattermost routing work, but this one-line PR is not the right landing path because Best possible solution: Close #55186 as superseded and continue with a decoupled Mattermost fix: preserve What I checked:
So I’m closing this here and keeping the remaining discussion on the canonical linked item. Codex Review notes: model gpt-5.5, reasoning high; reviewed against 06d409dc2738. |
Fixes #30977
Root Cause
Commit aaba1ae ("honor replyToMode off for threaded messages") added a
replyToMode !== "off"guard toresolveMattermostEffectiveReplyToId(). This conflates session routing (should DM threads fork sessions?) with reply delivery (should the bot reply in the thread?).Since DMs default to
replyToMode: "off", the guard makeseffectiveReplyToIdundefined for all DM thread replies. Downstream call sites lose the thread root, so replies go to the main channel or fail with 400 Invalid RootId.Fix
Remove the guard so
threadRootIdalways flows through to delivery.replyToModestill controls session forking viaresolveMattermostThreadSessionContext— unchanged.One-line change.