Skip to content

fix(web): drop SSE plan_update/approval_needed events without thread_id - #2986

Merged
serrrfirat merged 1 commit into
stagingfrom
fix/web-plan-update-thread-leak
Apr 28, 2026
Merged

serrrfirat merged 1 commit into
stagingfrom
fix/web-plan-update-thread-leak

Conversation

@italic-jinxin

@italic-jinxin italic-jinxin commented Apr 27, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Plan checklists and approval cards from one conversation could leak into another conversation's chat view when the user switched threads, because two SSE handlers (plan_update, approval_needed) used short-circuit guards that treated missing thread_id as "applies to current view".
  • Replaced both with strict isCurrentThread(data.thread_id) filtering — matching the convention every other thread-scoped handler in sse.js (response, stream_chunk, tool_started, …) already uses.
  • Also guarded approval_needed's else branch on data.thread_id so a null value no longer pollutes the unreadThreads Map.

Root cause

PlanUpdateTool::execute at src/tools/builtin/plan.rs:143 emits:

thread_id: ctx.conversation_id.map(|id| id.to_string()),
When the tool runs in a context without a chat conversation (mission runner, routine engine, heartbeat-driven flows), ctx.conversation_id is None, so the SSE event arrives with thread_id: null.

The old frontend guard:

if (data.thread_id && !isCurrentThread(data.thread_id)) return;
short-circuits on the falsy thread_id and falls through to renderPlanChecklist(data), which renders into the currently-displayed conversation. Same shape on approval_needed via forCurrentThread = !hasThread || isCurrentThread(...).

The fix is at the correct boundary: isCurrentThread(null) === false per chat.js:1-5, so !isCurrentThread(data.thread_id) filters thread_id-less events out instead of routing them to the active view.

Change Type

  • Bug fix
  • New feature
  • Refactor
  • Documentation
  • CI/Infrastructure
  • Security
  • Dependencies

Linked Issue

fixes #2833

Validation

  • cargo fmt --all -- --check
  • cargo clippy --all --benches --tests --examples --all-features -- -D warnings
  • cargo build
  • Relevant tests pass:
  • cargo test --features integration if database-backed or integration behavior changed
  • Manual testing:
  • If a coding agent was used and supports it, review-pr or pr-shepherd --fix was run before requesting review

Security Impact

Database Impact

Blast Radius

Rollback Plan

Review Follow-Through


Review track:

The plan_update and approval_needed handlers used short-circuit guards
(`data.thread_id && !isCurrentThread(...)` and
`!hasThread || isCurrentThread(...)`) that fell through to render in
the current view when thread_id was missing. When a backend tool path
emitted PlanUpdate with ctx.conversation_id = None (mission/routine
execution), the plan checklist appeared in whatever conversation the
user had switched to.

Use `isCurrentThread(data.thread_id)` directly — it returns false for
falsy input, matching the strict convention every other thread-scoped
handler (response, stream_chunk, tool_started, ...) already uses.

Also guard approval_needed's else branch with `data.thread_id` so a
null value no longer leaks into the unreadThreads Map as a key.

[skip-regression-check] frontend static-JS only; no JS test
infrastructure in repo, equivalent to src/channels/web/static/ exempt.
@italic-jinxin italic-jinxin self-assigned this Apr 27, 2026
@github-actions github-actions Bot added size: XS < 10 changed lines (excluding docs) risk: low Changes to docs, tests, or low-risk modules contributor: experienced 6-19 merged PRs labels Apr 27, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the Server-Sent Events (SSE) event listeners in sse.js to simplify conditional logic. The approval_needed listener was updated to remove redundant variables and clarify the handling of background thread updates, while the plan_update listener was streamlined by removing an unnecessary check for data.thread_id. I have no feedback to provide as there were no review comments to evaluate.

@serrrfirat
serrrfirat enabled auto-merge (squash) April 28, 2026 05:48
@serrrfirat
serrrfirat disabled auto-merge April 28, 2026 05:48
@serrrfirat
serrrfirat merged commit 93d0305 into staging Apr 28, 2026
17 checks passed
@serrrfirat
serrrfirat deleted the fix/web-plan-update-thread-leak branch April 28, 2026 05:48
@henrypark133 henrypark133 mentioned this pull request Apr 29, 2026
This was referenced May 7, 2026
theredspoon pushed a commit to theredspoon/ironclaw that referenced this pull request Jun 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor: experienced 6-19 merged PRs risk: low Changes to docs, tests, or low-risk modules size: XS < 10 changed lines (excluding docs)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Issue: Cross-Conversation Response Contamination When Switching Conversations

2 participants