diff --git a/apps/vscode/src/__tests__/mailbox-escalation-toast.test.ts b/apps/vscode/src/__tests__/mailbox-escalation-toast.test.ts index 866d07049..e4879b050 100644 --- a/apps/vscode/src/__tests__/mailbox-escalation-toast.test.ts +++ b/apps/vscode/src/__tests__/mailbox-escalation-toast.test.ts @@ -18,7 +18,7 @@ vi.mock('vscode', () => ({ }, })); -const { activateMailboxEscalationToasts } = await import('../notifications/mailbox-escalation-toast.js'); +const { activateMailboxEscalationToasts, MAX_SEEN } = await import('../notifications/mailbox-escalation-toast.js'); type SSEHandler = (e: { type: string; data: string }) => void; @@ -51,10 +51,35 @@ function escalationEvent(overrides: Record = {}): string { return JSON.stringify({ type: 'mailbox-escalation', body: JSON.stringify(payload) }); } -function activate(cm: ReturnType) { +/** + * Minimal OverviewCache double: the workspace-level `mailboxEscalated` flag plus a + * manual change hook, so a test can drive the de-escalation prune (#1472) the same + * way Tower's overview refresh would. + */ +function makeOverviewCache(initial: { mailboxEscalated: boolean } | null = null) { + let data = initial; + const listeners: (() => void)[] = []; + return { + getData: () => data, + onDidChange: (fn: () => void) => { + listeners.push(fn); + return { dispose() {} }; + }, + /** Simulate an overview refresh committing a new escalated state. */ + set: (next: { mailboxEscalated: boolean } | null) => { + data = next; + for (const fn of listeners) { fn(); } + }, + }; +} + +function activate( + cm: ReturnType, + cache: ReturnType = makeOverviewCache(), +) { const ctx = makeCtx(); - // Structural fakes stand in for vscode.ExtensionContext / ConnectionManager. - activateMailboxEscalationToasts(ctx as any, cm as any); + // Structural fakes stand in for vscode.ExtensionContext / ConnectionManager / OverviewCache. + activateMailboxEscalationToasts(ctx as any, cm as any, cache as any); return ctx; } @@ -129,3 +154,71 @@ describe('activateMailboxEscalationToasts', () => { expect(h.showWarningMessage).not.toHaveBeenCalled(); }); }); + +/** + * Issue #1472: the dedupe set must be bounded. Its eviction key is the mailbox row + * leaving the escalated set — observed here as the workspace-level + * `mailboxEscalated` flag going false — with a {@link MAX_SEEN} cap as backstop. + * Each test observes eviction the only way it is observable from outside: an id + * that was deduped before is toasted again after it has been evicted. + */ +describe('activateMailboxEscalationToasts — bounded dedupe set (#1472)', () => { + it('drops seen ids once no held row in the workspace is escalated', () => { + const cm = makeConnectionManager('/ws'); + const cache = makeOverviewCache({ mailboxEscalated: true }); + activate(cm, cache); + + cm.fire(escalationEvent({ mailboxId: 'a' })); + expect(h.showWarningMessage).toHaveBeenCalledTimes(1); + + // The row resolved: nothing is escalated any more, so 'a' has left the set. + cache.set({ mailboxEscalated: false }); + + cm.fire(escalationEvent({ mailboxId: 'a' })); + expect(h.showWarningMessage).toHaveBeenCalledTimes(2); + }); + + it('keeps seen ids while the workspace is still escalated', () => { + const cm = makeConnectionManager('/ws'); + const cache = makeOverviewCache({ mailboxEscalated: true }); + activate(cm, cache); + + cm.fire(escalationEvent({ mailboxId: 'a' })); + cache.set({ mailboxEscalated: true }); + cm.fire(escalationEvent({ mailboxId: 'a' })); + + expect(h.showWarningMessage).toHaveBeenCalledTimes(1); + }); + + it('does not prune on an empty cache — no data says nothing about the escalated set', () => { + const cm = makeConnectionManager('/ws'); + const cache = makeOverviewCache(null); + activate(cm, cache); + + cm.fire(escalationEvent({ mailboxId: 'a' })); + cache.set(null); + cm.fire(escalationEvent({ mailboxId: 'a' })); + + expect(h.showWarningMessage).toHaveBeenCalledTimes(1); + }); + + it('caps the set when the window never de-escalates, evicting oldest-first', () => { + const cm = makeConnectionManager('/ws'); + // Permanently escalated: the prune above never runs, so only the cap bounds it. + const cache = makeOverviewCache({ mailboxEscalated: true }); + activate(cm, cache); + + for (let i = 0; i < MAX_SEEN; i++) { + cm.fire(escalationEvent({ mailboxId: `id-${i}` })); + } + expect(h.showWarningMessage).toHaveBeenCalledTimes(MAX_SEEN); + + // One past the cap evicts the oldest id, which can therefore toast again; + // the newest id is still deduped. + cm.fire(escalationEvent({ mailboxId: 'overflow' })); + cm.fire(escalationEvent({ mailboxId: 'id-0' })); + cm.fire(escalationEvent({ mailboxId: 'overflow' })); + + expect(h.showWarningMessage).toHaveBeenCalledTimes(MAX_SEEN + 2); + }); +}); diff --git a/apps/vscode/src/extension.ts b/apps/vscode/src/extension.ts index 0fcacfcad..f458660bf 100644 --- a/apps/vscode/src/extension.ts +++ b/apps/vscode/src/extension.ts @@ -1519,7 +1519,7 @@ export async function activate(context: vscode.ExtensionContext) { // Spec 1313 Phase 8: toast when a held message crosses the escalation age // (the `mailbox-escalation` SSE event). Visibility only — read/dismiss via // `afx inbox`. Respects `codev.mailboxEscalationToasts.enabled`. - activateMailboxEscalationToasts(context, connectionManager); + activateMailboxEscalationToasts(context, connectionManager, overviewCache); // Auto-open builder terminals on Tower spawn events const builderSpawnHandler = new BuilderSpawnHandler(connectionManager, terminalManager, outputChannel); diff --git a/apps/vscode/src/notifications/mailbox-escalation-toast.ts b/apps/vscode/src/notifications/mailbox-escalation-toast.ts index 7b5d74c9a..e067c5e2f 100644 --- a/apps/vscode/src/notifications/mailbox-escalation-toast.ts +++ b/apps/vscode/src/notifications/mailbox-escalation-toast.ts @@ -3,6 +3,16 @@ import type { MailboxEscalationPayload } from '@cluesmith/codev-types'; import { parseSseEnvelope, parseSseBody } from '../sse-envelope.js'; import { escalationToastText, escalationMatchesWorkspace } from '../mailbox-indicators.js'; import type { ConnectionManager } from '../connection-manager.js'; +import type { OverviewCache } from '../views/overview-data.js'; + +/** + * Backstop bound on the dedupe set for a window that stays escalated all day, so + * the de-escalation prune below never gets to run. Oldest-first eviction (a `Set` + * iterates in insertion order); an evicted id can only re-toast if Tower re-emits + * an escalation for that same row, which the one-way server-side `escalated` flag + * makes impossible. Sized far above any plausible live escalated set. + */ +export const MAX_SEEN = 500; /** * Spec 1313 Phase 8: toast on `mailbox-escalation`. @@ -22,13 +32,44 @@ import type { ConnectionManager } from '../connection-manager.js'; * - gated by `codev.mailboxEscalationToasts.enabled` (default true) — the same * mute affordance `codev.gateToasts.enabled` gives the gate toasts. The * persistent status-bar count/attention state is unaffected by the mute. + * + * Issue #1472: the dedupe set is BOUNDED. Its eviction key is the mailbox 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 carries no per-row mailbox ids, only + * the workspace-level `mailboxEscalated` flag, so `false` (no held row in this + * workspace is escalated) is the finest-grained signal available here: at that + * point every id in the set has left the escalated set, and the set is dropped + * whole. A {@link MAX_SEEN} cap backstops a window that never sees that `false`. + * (Tower also reports `false` when it cannot read the mailbox at all, which prunes + * a little early — harmless: a row escalates exactly once server-side and there is + * no SSE replay, so an evicted id has no second event to be deduped against.) + * + * The prune cannot fire on a stale snapshot: `OverviewCache.refresh()` is + * last-write-wins by sequence, and the escalation event itself triggers a refresh + * whose request starts after Tower flagged the row, so an older in-flight + * `mailboxEscalated: false` response can never commit after it. */ export function activateMailboxEscalationToasts( context: vscode.ExtensionContext, connectionManager: ConnectionManager, + cache: OverviewCache, ): void { const seen = new Set(); + context.subscriptions.push( + cache.onDidChange(() => { + const data = cache.getData(); + // No data yet (or a transient read) says nothing about the escalated set. + if (!data) { + return; + } + if (!data.mailboxEscalated) { + seen.clear(); + } + }), + ); + context.subscriptions.push( connectionManager.onSSEEvent(({ data }) => { const enabled = vscode.workspace @@ -56,6 +97,10 @@ export function activateMailboxEscalationToasts( return; } seen.add(payload.mailboxId); + while (seen.size > MAX_SEEN) { + const oldest = seen.values().next().value as string; + seen.delete(oldest); + } void vscode.window.showWarningMessage(escalationToastText(payload)); }), diff --git a/codev/projects/bugfix-1472-vscode-bound-the-mailbox-escal/status.yaml b/codev/projects/bugfix-1472-vscode-bound-the-mailbox-escal/status.yaml new file mode 100644 index 000000000..09bcfa29d --- /dev/null +++ b/codev/projects/bugfix-1472-vscode-bound-the-mailbox-escal/status.yaml @@ -0,0 +1,22 @@ +id: bugfix-1472 +title: vscode-bound-the-mailbox-escal +protocol: bugfix +phase: verified +plan_phases: [] +current_plan_phase: null +gates: + pr: + status: approved + requested_at: '2026-08-17T23:14:58.384Z' + approved_at: '2026-08-17T23:24:05.907Z' +iteration: 1 +build_complete: false +history: [] +started_at: '2026-08-17T23:03:26.300Z' +updated_at: '2026-08-17T23:24:09.666Z' +pr_history: + - phase: pr + pr_number: 1484 + branch: builder/bugfix-1472 + created_at: '2026-08-17T23:14:54.493Z' +pr_ready_for_human: false diff --git a/codev/state/bugfix-1472_thread.md b/codev/state/bugfix-1472_thread.md new file mode 100644 index 000000000..0ab4c927d --- /dev/null +++ b/codev/state/bugfix-1472_thread.md @@ -0,0 +1,116 @@ +# bugfix-1472 — VSCode mailbox escalation-toast `seen` Set is unbounded + +Issue #1472 · protocol BUGFIX (strict) · branch `builder/bugfix-1472` + +## Architect constraints (2026-08-17) + +- We are **not** cluesmith/codev maintainers: open the PR, address review feedback, + then **park it for the maintainer** — do not merge it myself. Report protocol-complete. +- Design note from tracking issue #1483: prefer eviction keyed on **the mailbox row leaving + the escalated set** (the same signal that would let a legitimate re-escalation re-notify) + over a bare LRU cap, *if that signal is available in the toast module*. + +## Investigate + +**Root cause** — `apps/vscode/src/notifications/mailbox-escalation-toast.ts:30`: +`const seen = new Set()` lives for the whole extension-host activation. Line 58 +`seen.add(payload.mailboxId)` is the only mutation in the file — grep confirms **zero** +`seen.delete` / `seen.clear` anywhere. So every escalated mailbox row the window ever +observes leaves a permanent entry. Add-only container, activation-scoped lifetime = +unbounded growth. Reproduction is static and conclusive: there is no code path that can +ever remove an entry. + +**Is the de-escalation signal available?** Yes, at workspace granularity: + +- `OverviewData.mailboxEscalated: boolean` (packages/types/src/api.ts) — "at least one held + row in this workspace has crossed the escalation age". `OverviewData` carries **no + per-row mailbox ids** (only `heldCount` + this flag), so `false` is the finest-grained + "left the escalated set" signal a client can see. When it is false, *every* id in `seen` + has left the escalated set → the whole set can be dropped. +- The signal reaches a notification module the same way `activateGateToasts` gets it: + `OverviewCache.onDidChange` + `getData()` (`apps/vscode/src/views/overview-data.ts`). + `overviewCache` is already in scope at the `activateMailboxEscalationToasts(...)` call + site (extension.ts:1522), so this is a one-argument signature change, one call site. +- Precedent to mirror: `gate-toast.ts` keeps a `(builderId, gateName)` seen-set and prunes + entries once they leave the blocked set — same shape of fix. + +**Server-side semantics checked** (so the doc comment is accurate, not assumed): +`db/mailbox.ts` sets `escalated = 1` once, guarded by `escalated = 0`, on a still-held row; +the flag is never reset and terminal rows are pruned. So the same `mailboxId` cannot +legitimately re-escalate — eviction here is purely about memory, never about re-notifying. + +**Stale-snapshot race** — `OverviewCache.refresh()` is last-write-wins by `latestSeq`, and +the escalation SSE event itself triggers a refresh whose request starts *after* the row was +flagged in the DB. So an older in-flight `mailboxEscalated: false` response cannot commit +after the escalation's own `true` response and wrongly clear `seen`. + +**Fix shape** (~40 LOC + tests, well inside the BUGFIX ceiling): +1. Inject `OverviewCache`; on `onDidChange`, clear `seen` when `data.mailboxEscalated` is + false (the preferred eviction key — the row leaving the escalated set). +2. Keep a hard cap with oldest-first eviction as a backstop, for the pathological window + where the workspace is *continuously* escalated all day so the flag never falls false. + Insertion-ordered `Set` makes this trivial. +3. Regression tests that fail without the fix: re-toast after de-escalation, and the cap + bounding a continuously-escalated window. + +Signal: PHASE_COMPLETE. + +## Fix + +Implemented as planned, commit `850242bf` (~50 LOC of source + 4 tests): + +- `mailbox-escalation-toast.ts` takes `OverviewCache` and prunes `seen` whole on + `mailboxEscalated === false`; `MAX_SEEN = 500` oldest-first cap as backstop. +- `extension.ts` — the single call site passes `overviewCache` (already in scope). + +**Verified failing without the fix**, not assumed: with the eviction neutered the suite went +2 failed / 10 passed; restored, 12/12. Full vscode unit suite 72 files / 850 tests green; +`pnpm check-types` clean; `pnpm lint` has one pre-existing warning in the unrelated +`src/commands/tunnel.ts`. + +Environment note for the next builder in this worktree: it starts with **no `node_modules`**. +`pnpm install` at the worktree root, then `pnpm --filter @cluesmith/codev-types build && +pnpm --filter @cluesmith/codev-sdk build` — without the package builds, 18 vscode unit files +fail on `Cannot find package '@cluesmith/codev-sdk/...'`, which looks like a code failure and +is not one. + +## PR + +PR #1484 — https://github.com/cluesmith/codev/pull/1484. Per the architect's constraint we are +not maintainers here: the PR is parked for a maintainer to merge; I do not merge it. + +`consult` did not auto-detect the project from this worktree (it listed every project and +exited); `--issue 1472 --project-id bugfix-1472` is required. + +### CMAP (PR #1484) + +gemini = APPROVE (HIGH) · codex = APPROVE (HIGH) · claude = APPROVE (HIGH). No blocking issues +from any lane. The claude lane died once on a transient API 500 and was re-run. + +Non-blocking notes and what I did with them: + +- *Doc slightly stronger than the server guarantees* — Tower also reports + `mailboxEscalated: false` when it cannot read the mailbox at all. **Fixed**: the doc comment + now says so, and why it is harmless (one escalation per row, no SSE replay). +- *Uncommitted thread file* — **fixed**, committed with the PR. +- *A null-workspace window toasts every workspace but reads Tower's fallback-workspace flag, so + it can prune on a foreign `false`* — pre-existing quirk of `escalationMatchesWorkspace`'s + deliberate null-matches-everything rule; this fix neither introduces nor worsens it, and it + is unobservable given single-emission. **Left alone** — widening it into a workspace-scoping + change is outside a BUGFIX. +- *The `while (seen.size > MAX_SEEN)` loop can only iterate once* — deliberate; **left as is** + (the reviewer agreed). + +## Protocol complete — PR parked, NOT merged + +Human approved the `pr` gate (relayed by the architect 2026-08-17T23:24Z); I ran +`porch approve bugfix-1472 pr --a-human-explicitly-approved-this` and `porch done`. Porch +reports **PROTOCOL COMPLETE**, phase `verified`. + +`porch next` then hands out a final "Merge the pull request" task. **Deliberately not done.** +We are not cluesmith/codev maintainers on this project; the architect's standing constraint is +that a maintainer merges. GitHub agrees independently — PR #1484 is `reviewDecision: +REVIEW_REQUIRED`, `mergeStateStatus: BLOCKED`. I also did NOT run `porch done --merged 1484`, +since nothing was merged; whoever merges should record it. + +Anyone picking this up: the PR is complete and green, waiting only on maintainer review+merge.