fix(slack): drop synthetic thread_ts to avoid invalid_thread_ts - #212
fix(slack): drop synthetic thread_ts to avoid invalid_thread_ts#212LIU9293 wants to merge 1 commit into
Conversation
![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: 7b7cfa16b1
ℹ️ 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".
| // a task/cron run, fall back to posting at the top of the channel rather | ||
| // than letting Slack reject the call with `invalid_thread_ts`. | ||
| const threadIsSynthetic = isSyntheticOwner(threadId); | ||
| const threadField = threadIsSynthetic ? {} : { thread_ts: threadId }; |
There was a problem hiding this comment.
When a task/cron run emits an ask_user question, this drops the synthetic thread_ts and posts the Slack question as a new top-level message, but the pending question is still stored under the synthetic thread id in request-run.ts (setPendingQuestion(request.channelId, request.threadId, ...)). Slack button handling later derives the thread from the posted message (body.message?.thread_ts || body.message?.ts in commands.ts), so a click/reply on this top-level question is routed under the real Slack timestamp instead of the synthetic thread and getPendingQuestion will not find it. In that scenario the user's answer starts a fresh turn instead of resuming the blocked task/cron run; the fallback needs to seed/mirror the synthetic session or otherwise map the posted question's ts back before exposing interactive/plain questions top-level.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — flagging this as an explicit follow-up rather than fixing in this PR.
This PR is intentionally narrow: stop posting an invalid thread_ts so Slack stops rejecting cron/task questions with invalid_thread_ts (Sentry ODE-DEAMON-7). Before this change, the question was lost entirely; after it, the question is at least visible at the top of the channel.
You're right that interactive button clicks / threaded replies under the new top-level message route through body.message?.thread_ts || body.message?.ts and so won't match the synthetic-thread pendingQuestion written by request-run.ts:495. That means: with this PR, the user sees the question but pressing a button starts a fresh turn instead of resuming the blocked cron/task run — strictly better than the previous "silently swallowed" failure mode, but still incomplete.
The proper fix needs one of:
- Mirror
pendingQuestionunder both the synthetic threadId and the posted message'stsonce we know it - Or re-key the pending-question entry after
chat.postMessagereturns the realts - Or seed the cron/task session early (right after the first
sendChannelMessage) so the synthetic id is gone by the timepostSlackQuestionruns
That's a session/state-routing change that belongs in its own PR. I'd rather land this fix now to stop the Sentry bleed and tackle the routing in a focused follow-up than block on it here. Will open a tracking issue.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd11394fa4
ℹ️ 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 result = await slackApp.client.chat.postMessage({ | ||
| channel: rawChannelId, | ||
| thread_ts: threadId, | ||
| ...(threadIsSynthetic ? {} : { thread_ts: threadId }), |
There was a problem hiding this comment.
When threadIsSynthetic is true this call is now a top-level channel post, but the token was still resolved with getSlackBotToken(channelId, threadId) before this branch. In multi-workspace installs where a scheduled task/cron fires before that channel has been seen by the message router, getSlackBotToken can fall back to the first registered token instead of the token for rawChannelId; the existing top-level helper avoids that by consulting getWorkspaceBotTokenForChannel(channelId) first. In that context the new fallback can still fail with channel_not_found/not_in_channel even though dropping thread_ts was supposed to make the message deliverable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 6da6431. Token resolution now mirrors sendChannelMessage when threadIsSynthetic is true:
getSlackBotTokenForProcessor(processorId)
?? (threadIsSynthetic
? (getWorkspaceBotTokenForChannel(channelId) ?? getSlackBotToken(channelId))
: getSlackBotToken(channelId, threadId));
The real-thread path is unchanged so existing registry bindings keep winning for in-flight replies. Suite still green (408 pass, 1 skip).
Addresses Codex P2 review on PR #212. When threadIsSynthetic is true, sendMessage degenerates to a top-level channel post (no thread_ts), but the token was still being resolved with getSlackBotToken(channelId, threadId) first. In multi-workspace installs where a scheduled task/cron fires before that channel has been seen by the message router, the registry has no entry for the fake thread_ts and getSlackBotToken can fall back to the first registered token instead of the token for rawChannelId — leading to a follow-on channel_not_found / not_in_channel even though dropping thread_ts was supposed to make the message deliverable. Mirror the token-resolution chain that sendChannelMessage already uses for top-level posts: processor token -> workspace-by-channel token -> getSlackBotToken for the synthetic path. The real-thread path is unchanged so registry bindings for in-flight thread replies still win. Tests: full suite still green (408 pass, 1 skip, 0 fail).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6da643122b
ℹ️ 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 result = await client.chat.postMessage({ | ||
| channel: channelId, | ||
| thread_ts: threadId, | ||
| ...threadField, |
There was a problem hiding this comment.
When this synthetic branch turns a task/cron ask_user question into a top-level Slack post, it still uses the token selected by createSlackAdapter.sendQuestion, which calls getSlackBotToken(channelId, threadId) with the fake task:/cron-job: id rather than doing the workspace-first channel lookup used for top-level sends. In a multi-workspace install where the scheduled channel has not yet been bound in the Slack registry, that can fall back to the first registered bot token, so these newly top-level Block Kit/plain question posts fail with channel_not_found/not_in_channel even though sendMessage was fixed; the fresh evidence is that the question path still passes the old token into this spread.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 1d4e434. The sendQuestion adapter now mirrors the token-resolution logic from sendMessage (client.ts:321): when threadId is a synthetic placeholder, resolve via getWorkspaceBotTokenForChannel(channelId) first, falling back to getSlackBotToken(channelId). The real-thread path is unchanged so existing registry bindings keep winning for in-flight replies.
const threadIsSynthetic = isSyntheticOwner(threadId);
const token = getSlackBotTokenForProcessor(processorId)
?? (threadIsSynthetic
? (getWorkspaceBotTokenForChannel(channelId) ?? getSlackBotToken(channelId))
: getSlackBotToken(channelId, threadId));
Full suite still green (408 pass, 1 skip).
Mirror the token-resolution logic from sendMessage in the sendQuestion adapter. When the thread id is a synthetic placeholder (task: / cron-job: / cron:), postSlackQuestion drops thread_ts and degenerates to a top-level channel post; in that case the registry has no entry for the fake thread_ts, and getSlackBotToken(channelId, fakeTs) can fall back to the first registered workspace token in multi-workspace installs. Use getWorkspaceBotTokenForChannel(channelId) first so the posted Block Kit / plain-text question lands with the token bound to the actual channel. Addresses Codex review comment on PR #212.
Addresses Codex P2 review on PR #212. When threadIsSynthetic is true, sendMessage degenerates to a top-level channel post (no thread_ts), but the token was still being resolved with getSlackBotToken(channelId, threadId) first. In multi-workspace installs where a scheduled task/cron fires before that channel has been seen by the message router, the registry has no entry for the fake thread_ts and getSlackBotToken can fall back to the first registered token instead of the token for rawChannelId — leading to a follow-on channel_not_found / not_in_channel even though dropping thread_ts was supposed to make the message deliverable. Mirror the token-resolution chain that sendChannelMessage already uses for top-level posts: processor token -> workspace-by-channel token -> getSlackBotToken for the synthetic path. The real-thread path is unchanged so registry bindings for in-flight thread replies still win. Tests: full suite still green (408 pass, 1 skip, 0 fail).
Mirror the token-resolution logic from sendMessage in the sendQuestion adapter. When the thread id is a synthetic placeholder (task: / cron-job: / cron:), postSlackQuestion drops thread_ts and degenerates to a top-level channel post; in that case the registry has no entry for the fake thread_ts, and getSlackBotToken(channelId, fakeTs) can fall back to the first registered workspace token in multi-workspace installs. Use getWorkspaceBotTokenForChannel(channelId) first so the posted Block Kit / plain-text question lands with the token bound to the actual channel. Addresses Codex review comment on PR #212.
1d4e434 to
a109625
Compare
|
Rebased onto latest
Full |
|
Addressed the remaining P1 review comment in dc9842c. When a synthetic task/cron question is posted top-level and Slack returns a real message ts, request-run now mirrors the pending question state from the synthetic session onto that real Slack thread. Button/reply routing via the posted message ts can now find the pending question and resume the blocked run instead of starting a fresh turn.\n\nTargeted validation: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc9842c207
ℹ️ 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".
| ?? (threadIsSynthetic | ||
| ? (getWorkspaceBotTokenForChannel(channelId) ?? getSlackBotToken(channelId)) | ||
| : getSlackBotToken(channelId, threadId)); |
There was a problem hiding this comment.
When threadIsSynthetic is true, this correctly resolves the channel's workspace token for the top-level question, but the returned Slack ts is never registered as a message/thread token. The button path later calls handleButtonSelection, which looks up the token from the clicked selection message or the question thread ts; for a scheduled task/cron question in a multi-workspace install where the channel has not already been bound by the message router, that lookup falls back to an empty/default processor, so multi-question follow-ups or error replies after the first button click can be sent with the wrong workspace token. Please store the resolved token against the postSlackQuestion result when the synthetic post creates the real thread.
Useful? React with 👍 / 👎.
|
Addressed the Codex P2 review comment in 3dff52d. After Targeted validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3dff52d3f6
ℹ️ 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".
| // multi-workspace installs can mis-route follow-ups through the wrong | ||
| // processor (Codex P2 on PR #212). | ||
| if (questionMessageTs) { | ||
| slackAuthRegistry.setMessageBotToken(channelId, questionMessageTs, token); |
There was a problem hiding this comment.
In a multi-workspace install where a task/cron question is posted as a new top-level Slack message and the user clicks a button, this new binding still won't be found by the button path: commands.ts passes the selection reply's ts as messageTs and the question ts as threadId, while handleButtonSelection checks getMessageBotToken(channel, selectionTs) and then getThreadBotToken(channel, questionTs). Since this line stores the question ts only in the message-token map, the lookup can still fall back to the default/first workspace processor and mis-route follow-up questions or error replies; the fresh evidence is that the attempted fix writes the wrong registry map for the value later used as the thread id.
Useful? React with 👍 / 👎.
Follow-up to 3dff52d, addressing the remaining Codex P2 comment on PR #212 (ODE-DEAMON-7). When a task/cron `ask_user` question is posted top-level (because the thread id is a synthetic `task:` / `cron-job:` placeholder), the button click path uses `body.message?.thread_ts || body.message?.ts` to derive the thread id. For a top-level question message there is no `thread_ts`, so `threadId` resolves to the question message's own ts. `handleButtonSelection` then looks up the bot token via: 1) getMessageBotToken(channel, selectionMessageTs) — selection reply is a fresh chat.postMessage in commands.ts with no registry binding, so this misses. 2) getThreadBotToken(channel, threadId) — where threadId is the question's own ts. The previous fix only registered the question ts against the message-token map, so step (2) still missed in multi-workspace installs where the channel had not been bound by the message router yet, and follow-up questions / error replies could be sent with the wrong workspace token. Also register the resolved token under setThreadBotToken(questionTs) so the button path's fallback finds it. Test suite: bun test packages/ims/slack/ — 20 pass, 0 fail. Typecheck: clean.
|
Addressed the remaining Codex P2 review comment in ed946f8. The previous binding at Fix mirrors both bindings: Targeted validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed946f87d6
ℹ️ 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".
| saveSession({ | ||
| ...syntheticSession, | ||
| threadId: realThreadId, | ||
| pendingQuestion, | ||
| }); |
There was a problem hiding this comment.
When a synthetic task/cron question is mirrored to the real Slack question thread, this spread also copies the synthetic session's activeRequest into the real-thread session. The active request lifecycle still completes/fails only the original synthetic key (for example completeActiveRequest(context.channelId, context.threadId) in request-run.ts), so after the user answers and the run finishes the real-thread copy remains state: "processing". If the daemon restarts or the user sends stop in that real thread, recovery/stop handling will treat the already-finished run as pending and update/delete the stale status message or abort the session; the mirror should omit activeRequest and only seed the routing/session fields plus pendingQuestion.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 8022b7b.
mirrorPendingQuestionToRealThread now destructures the synthetic session and drops activeRequest before saving the real-thread copy. The run lifecycle stays keyed on the synthetic (channelId, threadId) pair, so the real thread no longer holds a stale state: "processing" record after the user answers and the run finishes. Recovery/stop handling on the real thread will find no activeRequest and take the no-op path.
Also exported the helper so packages/core/test/pending-question.test.ts can drive it directly:
- Case 1: with an
activeRequeston the synthetic session, the mirrored real-thread session hasactiveRequest === undefinedwhilesessionId/pendingQuestionmirror through. - Case 2: with a pre-existing real-thread session, only
pendingQuestionis updated; the existingactiveRequestis preserved.
Targeted validation:
bun test packages/core/test/pending-question.test.ts packages/ims/slack/api.test.ts— 11 pass, 0 failbun test packages/ims/slack/ packages/core/— 116 pass, 1 skip, 0 failbun run typecheck— clean
|
Follow-up for ODE-DEAMON-7 in dc240c4. I reviewed the remaining synthetic-thread Slack paths and found / still forwarded / placeholders as , which can hit the same failure before a real Slack thread exists. This patch now drops for synthetic owners there too, matching and .\n\nTargeted validation:\n- bun test v1.3.10 (30e609e0) (5 pass)\n- (clean) |
Follow-up to dc9842c, addressing the remaining Codex P2 review comment on PR #212 (ODE-DEAMON-7). `mirrorPendingQuestionToRealThread` used a bare spread of the synthetic session when seeding the real-thread copy, which also carried the synthetic session's `activeRequest` onto the real thread. The run's lifecycle (complete/fail/stop, plus recovery on daemon restart) still keys on the synthetic `(channelId, threadId)` pair, so the mirrored copy would be left in `state: "processing"` after the user answered and the run finished. Recovery would then try to update / delete an already-final status message on the real thread, and `stop` in the real thread would abort a session that is no longer running. Fix: destructure and drop `activeRequest` when copying the synthetic session onto the real thread; only mirror the routing/identity fields plus the new `pendingQuestion` the human replier needs to resume the correct run. Export `mirrorPendingQuestionToRealThread` so the test file can drive it directly rather than reconstructing the full ask_user pipeline. Tests: - bun test packages/core/test/pending-question.test.ts packages/ims/slack/api.test.ts (11 pass) - bun test packages/ims/slack/ packages/core/ (116 pass, 1 skip, 0 fail) - bun run typecheck (clean)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8022b7b43b
ℹ️ 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".
| mirrorPendingQuestionToRealThread({ | ||
| channelId: request.channelId, | ||
| syntheticThreadId: request.threadId, | ||
| realThreadId: questionMessageTs, |
There was a problem hiding this comment.
When a synthetic task/cron question posts successfully, this only mirrors the pending question to questionMessageTs; the running request/context still keeps replyThreadId as the synthetic task:/cron-job: id. After the user answers, the status rotation and final publish paths continue calling sendMessage with that synthetic id, so Slack drops thread_ts and posts the resumed status/final answer as new top-level channel messages instead of in the question thread the user is interacting with.
Useful? React with 👍 / 👎.
Task/cron runs address a run by a synthetic placeholder thread id
(`task:{id}` / `cron-job:{id}:{run}`) before Slack has assigned a real
`thread_ts`. Forwarding that string as `thread_ts` makes Slack reject the
call with `invalid_thread_ts`, so the message is silently lost and
reported as a delivery failure (Sentry ODE-DEAMON-7, 11 events).
Add a shared `threadTsField()` helper next to `isSyntheticOwner` that
omits `thread_ts` for synthetic ids, and use it at the two remaining
Slack call sites that still forwarded a caller-supplied thread id:
- `sendMessage` (`chat.postMessage`) — intermediate agent output during a
scheduled run. Also resolve the bot token via the channel's workspace
for synthetic ids, since the registry has no entry for a fake
`thread_ts` and `getSlackBotToken` can otherwise return the first
registered token in multi-workspace installs. Skip binding a real token
to the placeholder thread id for the same reason.
- `slackFileUpload` (`files.completeUploadExternal`) — `ode send file`
invoked from a scheduled run.
The fallback is a top-level channel post, which keeps the message visible
rather than dropping it.
Rebased onto main: the `request-run.ts` pending-question mirroring and the
streaming/question call sites from the earlier version of this change are
already covered on main (the streaming/question surfaces were removed in
the CLI-integration refactor), so only these two paths remain.
8022b7b to
e903e9d
Compare
|
Rebased onto latest Review findings — what main already absorbedTwo-thirds of the old diff is now dead weight:
The What actually still needs fixingI re-verified ODE-DEAMON-7 is still live on
ChangeExtracted This also made the tests better. The old version tested via Token resolution and the Validation
Note on resolving the Sentry issueLast event was 2026-07-08. I am not resolving ODE-DEAMON-7 yet — the fix is not merged, and the 2-day-quiet window should be measured after it ships, not against a gap that likely just reflects no cron run hitting the race. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e903e9de90
ℹ️ 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".
| // not valid Slack timestamps. `ode send file` invoked from a scheduled | ||
| // task/cron run before a real thread exists would otherwise fail with | ||
| // `invalid_thread_ts`; upload to the channel top level instead. | ||
| ...(threadTsField(args.threadId)), |
There was a problem hiding this comment.
When a scheduled task or cron run calls ode send file with a synthetic thread ID, this change makes the upload a top-level channel post, but uploadSlackFile still selects its token through getSlackBotToken(channelId, syntheticThreadId). In a multi-workspace daemon after restart, before the inbound router has bound that channel, this falls back to the first registered workspace token; all three upload API calls can therefore fail with authorization or channel_not_found errors for channels in another workspace. Resolve the token from the configured channel workspace first, as the synthetic sendMessage path now does.
Useful? React with 👍 / 👎.
1 participant
What
Stop forwarding synthetic placeholder thread ids (
task:{id}/cron-job:{id}:{run}/cron:{id}) as Slackthread_ts. When the value is synthetic, post at the top of the channel instead.Why
Sentry ODE-DEAMON-7 — "IM slack send failed: An API error occurred: invalid_thread_ts". The most recent events (2026-06-02) all have:
channel_id: C0ATGCJ0YK0thread_id: cron-job:a86fbdc5-01df-4caf-9e0c-c0c199f00379:1780441200000op: sendA task / cron run starts with a synthetic placeholder thread id; the scheduler only learns the real Slack
tsafter it posts the top-level result viasendChannelMessage. In between, any intermediate output the agent runtime emits flows throughim.sendMessage(channelId, syntheticThreadId, ...)→chat.postMessage({ thread_ts: "cron-job:..." })→ Slack rejects withinvalid_thread_ts, the message is lost, and Sentry captures a delivery failure.Sibling PR #211 already auto-disables cron jobs that hit a permanent channel error (e.g.
channel_not_found). This PR fixes the other mode of the same Sentry group: messages that would have landed in the channel except for the bogusthread_ts.Design notes
thread_ts). That keeps the message visible rather than silently swallowed.isSyntheticOwneralready exists inpackages/ims/shared/synthetic-owner.tswith full test coverage of the prefix set (task:/cron-job:/cron:); reuse it here so the matcher stays in one place.packages/ims/slack/client.ts:sendMessage(plus the bot-token binding helper, which must not bind a real token to a fake thread id) andpackages/ims/slack/api.tspostSlackQuestion(both the plain-text fallback and the Block Kit branch).thread_tsanalogues are routed through platform-specific resolvers that already special-case synthetic owners.Tests
packages/ims/slack/api.test.tsmock the bolt app and assert:cron-job:...thread id →postMessagecalled withoutthread_ts(Block Kit branch)task:...thread id → same (plain-text fallback)1717000000.000200) →thread_tspreservedbun testfull suite: 408 pass, 1 skip, 0 fail.Out of scope
channel_not_foundmode addressed by fix(cron): auto-disable cron jobs whose target channel is unreachable #211).