Uh oh!
There was an error while loading. Please reload this page.
Fix #1472: bound the mailbox escalation-toast dedupe set - #1484
Conversation
The `seen` Set in `activateMailboxEscalationToasts` was add-only and activation-scoped: one permanent entry per escalated mailbox row for the lifetime of the extension host. Evict on the row leaving the escalated set — the same signal that would let a legitimate re-escalation re-notify, mirroring how `activateGateToasts` prunes a builder that leaves the blocked set. The overview exposes no per-row mailbox ids, only the workspace-level `mailboxEscalated` flag, so `false` (nothing in this workspace is escalated) is the finest-grained signal available client-side: at that point every id in the set has left it, and the set is dropped whole. A MAX_SEEN=500 oldest-first cap backstops a window that stays escalated all day and never observes that `false`. Regression tests pin both paths (verified failing without the fix): an id re-toasts after de-escalation, and the cap evicts oldest-first while the workspace never de-escalates. Also covered: no prune while still escalated, and no prune on an empty cache. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CMAP non-blocking finding (claude lane): Tower also reports `mailboxEscalated: false` when it cannot read the mailbox, so the doc comment's "no held row is escalated" was marginally stronger than the server guarantees. Say so, and why it is harmless — a row escalates exactly once server-side and there is no SSE replay, so an early prune leaves no second event to dedupe against. Also commits the builder thread with the CMAP verdicts and the disposition of each non-blocking note. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mohidmakhdoomi
commented
Aug 17, 2026
Architect integration review (risk tier: Low — isolated VSCode module)Verdict: APPROVE (pending maintainer review — we are not maintainers; maintainer merges.) Verified directly against the diff:
Builder CMAP: gemini/codex/claude all APPROVE. No findings to address. Parked for maintainer approval + merge. |
The pr gate was approved and porch reports protocol complete. Porch's final merge task is deliberately skipped: a maintainer merges this PR, not the builder. GitHub concurs — REVIEW_REQUIRED / BLOCKED. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
waleedkadous
left a comment
There was a problem hiding this comment.
Excellent, tight fix — thank you. Both integration reviews (codex + claude) verified every load-bearing claim against source: the server-side one-way escalated flag means a cleared id has no second event to dedupe against, the overview cache's last-write-wins refresh closes the stale-snapshot race, and the shape matches gate-toast.ts so notifications/ is now consistent. Approving.
Two small things you might consider, neither blocking:
- The identical add-only
seenset lives inbuilder-spawn-handler.ts:11(keyed on terminalId, no eviction) — the very module this PR's comment cites as its model. Worth a follow-up issue under #1483 rather than widening this PR. - A half-sentence noting why this module deliberately does not persist its seen-set to
workspaceStatethe waygate-toast.tsdoes (no replay → nothing to re-toast) would save a future reader the question.
Uh oh!
There was an error while loading. Please reload this page.
Summary
The VSCode mailbox escalation toast deduped "already shown" escalations with a
seenSet that only ever grew — one permanent entry per escalated mailbox row for the lifetime of the extension host. It is now bounded, evicting on the mailbox row leaving the escalated set.Fixes#1472
Root Cause
apps/vscode/src/notifications/mailbox-escalation-toast.ts—const seen = new Set<string>()is activation-scoped, andseen.add(payload.mailboxId)was its only mutation: nodelete, noclear, anywhere in the module. Add-only container + extension-host lifetime = unbounded growth. Negligible per entry, but it never drains.Fix
Evict on the mailbox row leaving the escalated set — the same signal that would let a legitimate re-escalation re-notify — mirroring how
activateGateToastsprunes a builder that leaves the blocked set:OverviewCache(already in scope at the single call site inextension.ts) and subscribes toonDidChange.OverviewDatacarries no per-row mailbox ids, only the workspace-levelmailboxEscalatedflag, sofalse— nothing held in this workspace is escalated — is the finest-grained "left the escalated set" signal available client-side. At that point every id in the set has left it, so the set is dropped whole. A null/absent cache read says nothing about the escalated set and never prunes.MAX_SEEN = 500with oldest-first eviction (aSetiterates in insertion order) backstops the pathological window that stays escalated all day and so never observes thatfalse.No protocol or server change; the module and its call site are the whole surface.
Two notes checked against the server rather than assumed:
db/mailbox.tssetsescalated = 1once, guarded byescalated = 0, and never resets it; terminal rows are pruned. So the samemailboxIdcannot legitimately re-escalate — eviction here is purely about memory, never about suppressing a notification.OverviewCache.refresh()is last-write-wins by sequence, and the escalation SSE event itself triggers a refresh whose request starts after Tower flagged the row, so an older in-flightmailboxEscalated: falseresponse can never commit after it.Test Plan
src/__tests__/mailbox-escalation-toast.test.ts: an id re-toasts after de-escalation; the cap evicts oldest-first while the workspace never de-escalates; no prune while still escalated; no prune on an empty cache. Verified failing without the fix (2 failed / 10 passed with the eviction neutered) and passing with it (12/12).pnpm check-typesclean; porchbuildcheck green.pnpm lintreports one pre-existing warning in the unrelatedsrc/commands/tunnel.ts.🤖 Generated with Claude Code