Legitimate close-handler veto is logged with console.error; the replaceTab path calls close handlers without a catch

[Vincent-chk]

TL;DR — registerCloseHandler documents the contract as “Failure preserves the tab”, and a synchronous throw is indeed the only way to preserve it (removeAfterCleanup ignores the handler's return value). But the shipped tab chrome wraps the close path in a try/catch that logs every such throw at error level: console.error("Sidebar tab close failed:", error). A plugin that uses the documented veto channel therefore emits an error-level console line on every legitimate, expected close, and cannot avoid it from the plugin side. A related call site makes it worse: place() routes tab replacement through close handlers without any catch, so one plugin's veto aborts another plugin's openTab.

Evidence — @deepseek-ai/dsh-client-ui-sidebar-right@0.2.0-rc.2, lib/client.js

1. The documented contract (~line 6338):

/** Register resource cleanup before explicit removal. Failure preserves the tab. */
registerCloseHandler(kind, handler) { … }

2. The close path — synchronous, return value ignored (~lines 6429 / 6439):

closeIn(sessionId, tabId) {
  …
  this.removeAfterCleanup(sessionId, tab, () => { actions.closeTab(sessionId, tabId); });
}
removeAfterCleanup(sessionId, tab, commit) {
  this.closeHandlers.get(tab.kind)?.(sessionId, tab);  // sync; return value ignored
  commit();                                           // skipped when the handler throws
}

3. The throw is then logged as an error (~line 9175):

closeTab: (tabId) => {
  try { controller.closeIn(sessionId, tabId); }
  catch (error) { console.error("Sidebar tab close failed:", error); }
}

4. A second path with no catch at all (~line 6488, inside place()):

this.removeAfterCleanup(sessionId, replaced, commit);   // no try/catch

Impact

• Unavoidable error-level noise for correct behaviour. Every plugin that follows the documented “throw to preserve the tab” contract pollutes the user's console and any error monitoring with a line that means “working as designed”. It also makes the console useless for answering the real question — did a close actually fail?
• Cross-plugin breakage. When another plugin opens a tab with replaceTab and the replaced tab belongs to a different plugin, that plugin's close handler runs on an uncaught path: a legitimate veto throws out of the other plugin's openTab.

Suggested fix

• Preferred: give close handlers an explicit, non-exceptional veto contract — e.g. return false / { keepOpen: true }, or await a returned Promise before commit() — and reserve exceptions for genuine failures (which should be logged).
• Minimal: keep the throw-based veto but do not report it at error level (debug/warn, or skip a recognizable veto type such as a CloseVeto marker class), and wrap the replaceTab call site in a try/catch so one plugin cannot break another.

How we worked around it

We moved to a “removal-first” protocol: the close handler never throws, the framework removes the tab in the same gesture, and cleanup (session destroy + bookkeeping) finishes in the background, with one bounded retry on failure. That removed the noise entirely. The cost is that the strict ordering “clean up before the tab is removed” can no longer be expressed through the current contract — restoring it is exactly what the preferred fix above would enable.

[argszero]

Confirmed at source — and the place() half is worse than the report says

I read this against the current upstream head (86a40220), not the published bundle, so the line numbers differ from yours (lib/client.js offsets vs. packages/client/ui-sidebar-right/src/client/service.ts); the behaviour is identical. Every claim in the post holds.

The contract, verbatim (service.ts:272-280):

/**
 * Register resource cleanup before explicit removal. Failure preserves the tab.
 * @param handler - saves any background cleanup before returning and allowing removal.
 */
registerCloseHandler(kind: string, handler: SidebarRightCloseHandler): () => void {
  if (this.closeHandlers.has(kind)) throw new Error(`sidebarRight: close handler already registered for ${kind}`)
  this.closeHandlers.set(kind, handler)
  return () => { if (this.closeHandlers.get(kind) === handler) this.closeHandlers.delete(kind) }
}

and the one place a handler runs (:379-382):

private removeAfterCleanup(sessionId: SessionId, tab: TabRecord, commit: () => void): void {
  this.closeHandlers.get(tab.kind)?.(sessionId, tab)
  commit()
}

Both call sites are as you found them: closeIn (:376) is wrapped by the seat — src/client/index.ts:226-227, try { controller.closeIn(sessionId, tabId) } catch (error) { console.error('Sidebar tab close failed:', error) } — and the replacement branch of place() (:449) is not.

Where the post understates it

On the replacement path the skipped continuation is the open. commit() at :431-443 is the call that performs actions.openContent(...) with replaceTab: placement.replaceTab. So when plugin A's tab replaces a tab owned by plugin B and B's handler vetoes, the outcome is not merely "an exception escapes A's openTab" — A's new tab is never opened and B's old tab stays, and the user sees a click that did nothing. place() is reached from the public openResource / openTab, so this is the ordinary plugin-facing path, and a veto there silently consumes another plugin's navigation.

That asymmetry is the sharpest form of your point: the same throw is contained on the seat path (degraded to a log line) and uncontained on the open path (degraded to a no-op).

One thing the post cannot see from the bundle

The error-level log is pinned by a test. tests/seat.client.spec.tsx:1001 — "keeps a resource tab and reports a synchronous cleanup failure from its close button" — clicks a tab close whose handler throws and asserts logged.mock.calls equals [['Sidebar tab close failed:', failure]], then asserts the tab is still present, then releases the handler and asserts the second click closes it. So the current behaviour is intended and enforced, and your "minimal fix" is not a one-word log-level edit: it changes an asserted contract, and the test is where the new intent has to be written. Worth knowing before the fix is scoped.

A constraint on the preferred fix

closeHandlers is a Map<kind, handler> with one slot per tab kind, and a second registration for the same kind throws rather than composing. Your preferred shape (a returned false / { keepOpen: true }) is strictly better than the throw, but it has to answer a question the registry does not currently pose: what a second handler for a kind means. Either the registry becomes a list (then "any veto keeps the tab" is a policy to state and test), or the single-slot rule stays and the veto contract is per-kind as it is today.

The minimal shape that changes the least is probably the marker one you name: keep the throw, keep the single slot, and have the two catch sites distinguish a declared veto from a genuine failure — with the seat logging a veto at debug and the place() site catching a veto into a defined outcome (skip the replacement, keep both tabs) instead of letting it escape. That is a small, testable change on top of the contract that already exists, rather than a redefinition of it.

I have not run the suite or reproduced this at runtime; this is a source read, so treat the line numbers as mine and the observations as checkable.

[Vincent-chk]

Thank you for the careful read — the place() correction in particular. "A veto silently consumes another plugin's navigation, and the user sees a click that did nothing" is a sharper and more accurate statement of the harm than our original report.

On the question you raised for the return-value shape: from a consumer's seat, we'd argue for keeping the single slot. One handler per kind maps cleanly to "the plugin that owns the tab kind owns the veto," and multi-handler composition (defining "any veto keeps the tab") is a much larger contract for a rare case. So your minimal shape — keep the throw, keep the single slot, distinguish a declared veto from a genuine failure at both catch sites, with the test rewritten to express the new intent — works for us as-is.

For context: this plugin already moved to a removal-first protocol (our handler never throws), so we're indifferent to the veto channel itself; what we care about is the place() containment, which currently makes one plugin's veto eat another plugin's openTab.