[Context] Avoid replaying current Slack bot thread context#68402
[Context] Avoid replaying current Slack bot thread context#68402bek91 wants to merge 6 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR fixes two related Slack thread-context issues: (1) bot-authored messages were being replayed into new thread sessions via The bot-message fix pre-filters Confidence Score: 5/5Safe to merge — changes are focused, correct, and fully covered by new/updated tests. Both fixes are logically sound. The only remaining findings are P2 cleanup suggestions (dead code branches after the bot pre-filter, and uniqueUserIds being derived from the pre-filter list). Neither affects correctness. No files require special attention.
|
martingarramon
left a comment
There was a problem hiding this comment.
LGTM on the core change — filtering bot-authored history out of first-turn seeding is the right move, and the combined omittedHistoryCount + omittedBotHistoryCount log math is correct (the two sets are non-overlapping since threadHistoryWithoutBots feeds filterSupplementalContextItems).
Two small questions:
-
ThreadStarterBody parity? The filter targets
threadHistorybut doesn't touchThreadStarterBody. If a bot kicks off a thread that the gateway later joins, the starter itself would still seed. Intentional scope limit, or a follow-up? -
Fallback if the filter ever leaks. Removing the role branch means any bot message that slips through
!historyMsg.botId(e.g., a future Slack SDK shape wherebotIdis absent but the author is effectively a bot) would now be labeled(user)rather than(assistant). Low-probability, but a one-line defense-in-depth (const role = historyMsg.botId ? "assistant" : "user"kept) or a comment noting the filter is now the single source of truth would make the intent explicit.
Non-blocking. Test updates match the behavior change.
One housekeeping note: the pre-commit hook failures you called out in the PR body (Discord + qa-lab) are tracked now at #69006 — you can reference it if you want a canonical link.
|
Follow-up after review:
I also rewrote the PR body to match the repo template and to document the current validation state plus the unrelated full-repo |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b937a41df7
ℹ️ 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".
|
Also related to #68383. This latest follow-up commit fixes the |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22ba3df0cf
ℹ️ 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".
|
LGTM. The new "omits bot-authored starter text and history" test covers both surface points I flagged — ThreadStarterBody parity (via |
|
Follow-on |
Summary
Describe the problem and fix in 2–5 bullets:
runPreparedReply()could duplicateThreadStarterBodyby emitting both the structured untrusted block and a plain-text[Thread starter - for context]prelude.ThreadStarterBodyfallback when noThreadHistoryBodyexists.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause (if applicable)
get-reply-run.tsindependently added a second plain-text starter prelude when onlyThreadStarterBodyexisted.codex review --base origin/maincaught that as over-broad, so this update narrows the filter to the current Slack bot identity only.Regression Test Plan (if applicable)
extensions/slack/src/monitor/message-handler/prepare-thread-context.test.ts,extensions/slack/src/monitor/message-handler/prepare.thread-context-allowlist.test.ts,extensions/slack/src/monitor/message-handler/prepare.test.ts,src/auto-reply/reply/get-reply-run.media-only.test.tspnpm test:extension slack.User-visible / Behavior Changes
ThreadStarterBody/ThreadHistoryBody.[Thread starter - for context]prelude.Diagram (if applicable)
Security Impact (required)
No)No)No)No)No)Yes, explain risk + mitigation:Repro + Verification
Environment
pnpm/ Vitest checkoutchannels.slack.replyToMode=all,channels.slack.groupPolicy=open, thread bootstrap enabled withinitialHistoryLimit=20Steps
ThreadStarterBody/ThreadHistoryBody.Expected
Actual
Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
ThreadStarterBodyprelude regression.AI Assistance
pnpm build/pnpm checkstill fail on unrelated existing issues noted below.codex review --base origin/mainwas run locally and its actionable finding was addressed before this update.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)No)Risks and Mitigations
pnpm test:extension slack, and documented the unrelated failures below.Validation Notes
Passed locally:
codex review --base origin/mainpnpm exec vitest run extensions/slack/src/monitor/message-handler/prepare-thread-context.test.ts extensions/slack/src/monitor/message-handler/prepare.thread-context-allowlist.test.ts extensions/slack/src/monitor/message-handler/prepare.test.ts src/auto-reply/reply/get-reply-run.media-only.test.tspnpm test:extension slackAttempted but currently failing outside this diff:
pnpm buildextensions/lobster/src/lobster-runner.ts: could not resolve@clawdbot/lobster/corepnpm checkextensions/discord/src/monitor/gateway-plugin*.ts:firstHeartbeatTimeouttypingextensions/qa-lab/src/providers/aimock/server.ts:@copilotkit/aimock/ implicitany#69006for the Discord / qa-lab failures