From 1b99b14c89a4762bc88a8ef39f0edab20a68b7d7 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Fri, 25 Sep 2026 01:51:17 -0700 Subject: [PATCH 1/5] docs(plans): screen IPC by interest (#762) Co-Authored-By: Claude Opus 5.5 --- docs/plans/2026-09-25-screen-ipc-interest.md | 42 ++++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 docs/plans/2026-09-25-screen-ipc-interest.md diff --git a/docs/plans/2026-09-25-screen-ipc-interest.md b/docs/plans/2026-09-25-screen-ipc-interest.md new file mode 100644 index 000000000..62db0df41 --- /dev/null +++ b/docs/plans/2026-09-25-screen-ipc-interest.md @@ -0,0 +1,42 @@ +# Screen IPC by interest (#762) + +## Problem +`session:screen` is the largest IPC stream by far: +- **Share of traffic:** 93% of recorded IPC bytes in recording g00000190 (16,728 frames of about 8.8 KB over 123 minutes). The committed fixture `testing/fixtures/worktree-live-attribution/codex-live-channel-gap.json` shows the same shape: 2,358 of about 12,600 recorded events. +- **Rate:** 6–10 frames a second for each busy session. Every frame is structured-cloned, dispatched, and committed into the renderer's runtime map. + +## Evidence that nothing live needs it +All consumers were mapped (file:line in PR #1236's description): +- **Main never needs the renderer's copy.** Prompt delivery, paste confirmation and readiness read the screen in main through `session.snapshotScreen()`. The raw snapshot cache (`SessionManager.lastScreenSnapshot`) and the spinner gate are untouched. +- **The composer picker is owned by conditions.** It comes from the `claude.slash-picker` condition via `applyConditionSnapshot`. The screen handler's picker write was a second writer (split authority). +- **Dead code:** `latestScreenRef` (the Enter baseline) and the renderer `claudePaste` helpers have no callers. +- **ReaderView** has no screen fallback since #855. +- **What remains is debug-only:** DebugPanel, three dev modules, debug bundles (including the 60 s autosave), and the screen-tail trace. + +## Change +1. **Main, forwarder gate.** `session:screen` is forwarded only for a session that some renderer holds a lease on (`sessions/screenInterest.ts`). Every frame still feeds a bounded screen-tail history in main (`shared/debug/screenTail.ts`), because debug bundles must not depend on a panel having been open. +2. **Lease contract:** + - `session:screen-lease` acquires a lease and seeds the current screen down the ordinary `session:screen` path, the same seed `session:recover` sends. An idle backend therefore still shows a correct panel. + - `session:screen-release` releases a lease. + - Leases are owned by the calling webContents. A main-frame navigation (reload) or destruction drops all of that owner's leases, because a dying renderer never runs its cleanup. A release the owner does not hold is ignored. +3. **Debug bundles** read `session:get-screen-debug`: main's latest raw snapshot plus the tail history. +4. **Renderer:** + - The screen handler applies only the screen strings. + - DebugPanel and the dev modules that read the screen hold a lease while open (`useScreenLease`). + - Dead code is deleted: `latestScreenRef`, `claudePaste.ts`, and `ComposerSubmitIo.getScreen`. + +## Unchanged, deliberately +- Main's raw screen processing: the headless snapshot, condition parsers, prompt gate, the `lastScreenSnapshot` cache, and `ScreenFrameGate`. +- The phone feed (it taps main directly). +- Recover and resync seeds. + +## Tests (fail-first where the behaviour changes) +- **Forwarder:** the recorded Claude 2.1.278 screen frame is not forwarded without a lease, is forwarded with one, and is recorded in the tail history either way. The first assertion is red on origin/main. +- **Leases:** the IPC seed on acquire; dropping on a main-frame reload and on destroy, but not on a sub-frame navigation; counting across owners. +- **Picker:** a screen frame carrying a picker no longer sets `runtime.picker`. +- **Mutations:** removing the gate, recording only leased frames, dropping on sub-frames, removing the seed, and restoring the picker write each fail a test. + +## Risks +- **A missed release** costs bytes until that renderer reloads, never correctness. +- **Debug surfaces** show nothing new until their lease is acquired; the acquire seed covers the first frame. +- **The DevDebug copy payload** reads the renderer copy, which can be stale unless a leasing module is open. This is accepted for a copy-to-clipboard debug aid. From e26e0c2dfbdebcea235298696bf64af6deec9704 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Fri, 25 Sep 2026 01:51:52 -0700 Subject: [PATCH 2/5] perf(ipc): forward session:screen only to a renderer that leased it #762. session:screen was 93% of recorded IPC bytes and nothing live reads it: main does its own prompt delivery, paste confirmation and readiness from the screen, the composer picker is owned by the conditions channel, and the Enter-baseline ref and renderer paste helpers were dead. See docs/plans/2026-09-25-screen-ipc-interest.md. - Main: the forwarder sends a frame only while some renderer holds a lease (sessions/screenInterest.ts), and records every frame in a bounded screen-tail history (shared/debug/screenTail.ts). Main's raw snapshot processing and gate are unchanged. - Leases: session:screen-lease seeds the current screen down the ordinary path; leases belong to the calling webContents and drop on a main-frame reload or destroy. - Debug bundles read session:get-screen-debug (latest raw screen and history). DebugPanel and the three screen-reading dev modules hold a lease while open (useScreenLease). - Renderer: the screen handler applies only the screen strings (no picker). latestScreenRef, claudePaste.ts and ComposerSubmitIo.getScreen are deleted as dead code. Fail-first: the recorded Claude 2.1.278 frame is forwarded with no lease on origin/main. Mutations killed: gate removed, history recorded only for leased frames, lease dropped on a sub-frame navigation, seed removed, picker write restored. Fixes #762 Co-Authored-By: Claude Opus 5.5 --- src/main/ipc/session.test.ts | 31 +++ src/main/ipc/session.ts | 44 ++++ src/main/sessions/forwarder.test.ts | 50 ++++ src/main/sessions/forwarder.ts | 23 +- src/main/sessions/screenInterest.test.ts | 45 ++++ src/main/sessions/screenInterest.ts | 77 ++++++ .../window/sessionRoutingComposition.test.ts | 8 + src/preload/api/session.ts | 16 +- .../opencode/renderer/composerSubmit.test.ts | 2 - .../registry.renderer.capabilities.ts | 1 - ...rminalDimensionOwnership.renderer.test.tsx | 7 + .../ClaudePasteDetection/module.tsx | 3 + .../debug/devModules/Conditions/module.tsx | 3 + .../HeadlessSnapshotProbe/module.tsx | 3 + .../src/features/debug/renderTrace.ts | 118 +++------- .../src/features/debug/saveDebugBundle.ts | 27 ++- .../src/features/debug/ui/DebugPanel.tsx | 4 + .../debug/useScreenLease.renderer.test.tsx | 27 +++ .../src/features/debug/useScreenLease.ts | 22 ++ .../agentIndexNavigation.renderer.test.tsx | 1 - .../src/workspace/hook/actions/pane.ts | 1 - .../paneRecoveryOwnership.renderer.test.tsx | 2 - .../src/workspace/hook/actions/session.ts | 9 +- .../actions/sessionRecovery.renderer.test.tsx | 1 - ...essionReplacementHandoff.renderer.test.tsx | 1 - .../actions/testing/paneActionsHarness.tsx | 1 - src/renderer/src/workspace/hook/index.ts | 1 - .../hook/ipc/testing/workspaceRefsForTest.ts | 1 - .../ipc/useIpcSubscriptions.renderer.test.tsx | 20 +- .../workspace/hook/ipc/useIpcSubscriptions.ts | 41 ++-- .../codexLiveContinuity.renderer.test.tsx | 1 - src/renderer/src/workspace/hook/refs.ts | 3 - .../tile-tree/TileLeaf/claudePaste.ts | 219 ------------------ .../tile-tree/TileLeaf/useComposerKeybinds.ts | 3 +- src/shared/debug/screenTail.test.ts | 32 +++ src/shared/debug/screenTail.ts | 97 ++++++++ 36 files changed, 563 insertions(+), 382 deletions(-) create mode 100644 src/main/sessions/screenInterest.test.ts create mode 100644 src/main/sessions/screenInterest.ts create mode 100644 src/renderer/src/features/debug/useScreenLease.renderer.test.tsx create mode 100644 src/renderer/src/features/debug/useScreenLease.ts delete mode 100644 src/renderer/src/workspace/tile-tree/TileLeaf/claudePaste.ts create mode 100644 src/shared/debug/screenTail.test.ts create mode 100644 src/shared/debug/screenTail.ts diff --git a/src/main/ipc/session.test.ts b/src/main/ipc/session.test.ts index 22b69fe6b..e248da39c 100644 --- a/src/main/ipc/session.test.ts +++ b/src/main/ipc/session.test.ts @@ -149,3 +149,34 @@ describe('session input transcript observations', () => { expect(append).toHaveBeenCalledTimes(3) }) }) + +describe('screen leases (#762)', () => { + it('seeds the current screen on acquire, and drops a renderer\'s leases when it reloads or dies', async () => { + const { screenInterest } = await import('@main/sessions/screenInterest.js') + const manager = new EventEmitter() + Object.assign(manager, { + getScreenSnapshot: () => ({ plain: 'now', markdown: 'now', recent: 'now', recentMarkdown: 'now' }), + }) + registerSessionIpc(manager as never, {} as never, new SessionFeedTap(manager as never)) + harness.routed.mockClear() + const sender = Object.assign(new EventEmitter(), { id: 4242 }) + const lease = harness.handlers.get('session:screen-lease')! + + // An opening debug panel is right at once, even for an idle backend: + // the current screen goes down the ordinary session:screen path. + lease({ sender }, 'pane') + expect(screenInterest.wants('pane')).toBe(true) + expect(harness.routed).toHaveBeenCalledWith('pane', 'session:screen', expect.objectContaining({ sessionId: 'pane', plain: 'now' })) + + // A reload never runs the renderer's cleanup. A sub-frame navigation is + // not a reload and must not drop it. + sender.emit('did-start-navigation', {}, 'about:blank', false, false) + expect(screenInterest.wants('pane')).toBe(true) + sender.emit('did-start-navigation', {}, 'app://index.html', false, true) + expect(screenInterest.wants('pane')).toBe(false) + + lease({ sender }, 'pane') + sender.emit('destroyed') + expect(screenInterest.wants('pane')).toBe(false) + }) +}) diff --git a/src/main/ipc/session.ts b/src/main/ipc/session.ts index 8e63dd3dc..a95a1b218 100644 --- a/src/main/ipc/session.ts +++ b/src/main/ipc/session.ts @@ -32,6 +32,9 @@ import { windowIdFor, } from '@main/window/windowRegistry.js' import type { SessionWindowLease } from '@main/window/sessionWindowRouter.js' +import { screenInterest, screenTailHistory } from '@main/sessions/screenInterest.js' +import type { AgentScreenSnapshot } from '@shared/types/session.js' +import type { ScreenTailSample } from '@shared/debug/screenTail.js' // One secret per app process is enough for correlation inside that run's // incident journal, and unlike a bare SHA-256 it prevents an exported bundle @@ -249,6 +252,47 @@ export function registerSessionIpc( manager.detachAgentPty(sessionId) }) + // #762. Live `session:screen` frames are forwarded only while a renderer + // holds a lease for that session (see sessions/screenInterest.ts for why + // nothing else needs them). The acquire also pushes the current screen down + // the ordinary `session:screen` path, the same seed `session:recover` sends, + // so an opening debug panel is correct at once even for an idle backend that + // emits no further frame, and the renderer needs no second way to apply a + // screen. Leases are owned by the calling webContents and + // dropped when it navigates (reload) or is destroyed, because a renderer + // that dies never runs its cleanup. + const leaseOwnersWatched = new Set() + ipcMain.handle('session:screen-lease', (evt, sessionId: string): void => { + const sender = evt.sender + const owner = sender.id + if (!leaseOwnersWatched.has(owner)) { + leaseOwnersWatched.add(owner) + const drop = () => screenInterest.dropOwner(owner) + sender.on('did-start-navigation', (_event, _url, _inPlace, isMainFrame) => { + if (isMainFrame) drop() + }) + sender.once('destroyed', () => { + drop() + leaseOwnersWatched.delete(owner) + }) + } + screenInterest.acquire(owner, sessionId) + const screen = manager.getScreenSnapshot(sessionId) + if (screen) sendToSessionWindow(sessionId, 'session:screen', aliasScreenSnapshotForWire({ sessionId, ...screen })) + }) + ipcMain.handle('session:screen-release', (evt, sessionId: string) => { + screenInterest.release(evt.sender.id, sessionId) + }) + // Debug bundles read the screen on demand instead of holding a lease: the + // latest raw snapshot plus the tail history main records from every frame. + ipcMain.handle('session:get-screen-debug', (_evt, sessionId: string): { + screen: AgentScreenSnapshot | null + samples: ScreenTailSample[] + } => ({ + screen: manager.getScreenSnapshot(sessionId), + samples: screenTailHistory.samples(sessionId), + })) + ipcMain.handle( 'session:input', (_evt, sessionId: string, data: string, pasteId?: string) => { diff --git a/src/main/sessions/forwarder.test.ts b/src/main/sessions/forwarder.test.ts index dd2a65c4f..330ae8014 100644 --- a/src/main/sessions/forwarder.test.ts +++ b/src/main/sessions/forwarder.test.ts @@ -1,4 +1,5 @@ import { EventEmitter } from 'node:events' +import { readFileSync } from 'node:fs' import { expect, it, vi } from 'vitest' import type { SessionManager } from '@main/sessionManager.js' import type { LspManager } from '@main/lspManager.js' @@ -11,6 +12,7 @@ vi.mock('@main/window/windowRegistry.js', () => ({ vi.mock('@main/subagents/index.js', () => ({ SubAgentWatcherManager: class { observeParentEntry() {} stop() {} stopAll() {} } })) import { wireSessionForwarder } from './forwarder.js' +import { screenInterest, screenTailHistory } from './screenInterest.js' import { SessionFeedSource } from '@main/remote/SessionFeedSource.js' import { SessionFeedTap } from './sessionFeedTap.js' @@ -101,3 +103,51 @@ it('broadcasts a managed-skill warning to every window, carrying domain names on manager.removeAllListeners() } }) + +it('forwards screen frames only to a leased session, and records every frame for debug bundles (#762)', () => { + // session:screen was 93% of recorded IPC bytes and nothing live reads it. + // An unleased session's frames stay in main; the tail history still sees + // them, because a debug bundle must not depend on a panel having been open. + const manager = new EventEmitter() as SessionManager & EventEmitter + manager.list = () => [] + const sent: string[] = [] + wire.receive = (channel, payload) => { + if (channel === 'session:screen') sent.push((payload as { plain: string }).plain) + } + const forwarder = wireSessionForwarder(manager, new EventEmitter() as LspManager) + // A real frame: the recorded Claude Code 2.1.278 screen from the #1113 + // capture (idle, then with a composer draft). What the gate forwards is + // exactly what the provider painted. + const recorded = JSON.parse(readFileSync( + new URL('../../../testing/fixtures/image-absorption/wrapped-image-pill-2026-09-21.json', import.meta.url), 'utf8', + )) as { deliveries: { wrapped: { baseline: { screen: string }; after: { screen: string } } } } + const idle = recorded.deliveries.wrapped.baseline.screen + const drafted = recorded.deliveries.wrapped.after.screen + const frame = (screen: string) => ({ sessionId: 'pane', plain: screen, markdown: screen, recent: screen, recentMarkdown: screen }) + try { + manager.emit('screen', frame(idle)) + forwarder.flush() + expect(sent).toEqual([]) + + screenInterest.acquire(7, 'pane') + manager.emit('screen', frame(drafted)) + forwarder.flush() + expect(sent).toEqual([drafted]) + + screenInterest.release(7, 'pane') + manager.emit('screen', frame(idle)) + forwarder.flush() + expect(sent).toEqual([drafted]) + + // Every frame reached the tail history, forwarded or not (idle, drafted, + // idle again: the dedupe only collapses consecutive repeats). + expect(screenTailHistory.samples('pane')).toHaveLength(3) + manager.emit('removed', { sessionId: 'pane' }) + forwarder.flush() + expect(screenTailHistory.samples('pane')).toEqual([]) + } finally { + screenInterest.dropOwner(7) + wire.receive = () => {} + manager.removeAllListeners() + } +}) diff --git a/src/main/sessions/forwarder.ts b/src/main/sessions/forwarder.ts index 6afc7267a..4d6424c4c 100644 --- a/src/main/sessions/forwarder.ts +++ b/src/main/sessions/forwarder.ts @@ -14,6 +14,7 @@ import { sendToSessionWindow, } from '@main/window/windowRegistry.js' import { SessionFeedTap } from '@main/sessions/sessionFeedTap.js' +import { screenInterest, screenTailHistory } from '@main/sessions/screenInterest.js' // Session event forwarder — the desktop WINDOW SINK over the SessionFeedTap. // @@ -32,7 +33,10 @@ import { SessionFeedTap } from '@main/sessions/sessionFeedTap.js' // `emptyRuntime()` for an unrecognized id, so a misrouted event grows a // ghost runtime rather than being ignored. // 2. The IPC channel names (`session:`). -// 3. The screen alias (#746), an IPC-edge byte optimisation. +// 3. The screen alias (#746), an IPC-edge byte optimisation, and the +// screen-interest gate (#762): frames go only to a renderer that holds a +// lease (screenInterest.ts), while every frame still feeds the debug +// screen-tail history. // 4. Broadcast-only events that are machine-wide rather than per session. // // terminal-data and agent-pty-data are intentionally separate channels from @@ -63,8 +67,17 @@ export function wireSessionForwarder( // `removed` is a cleanup signal the tap needed for its own buffers; the // desktop learns removal from workspace state, and never had an IPC // channel for it. - if (channel === 'removed') return + if (channel === 'removed') { + screenTailHistory.forget(payload.sessionId) + return + } if (channel === 'screen') { + const frame = payload as { sessionId: string } & AgentScreenSnapshot + // Recorded for EVERY frame, forwarded or not: debug bundles read this + // history, and it must not depend on whether a panel happened to be + // open (#762, screenInterest.ts). + screenTailHistory.record(frame.sessionId, frame.recent) + if (!screenInterest.wants(frame.sessionId)) return // WHY the alias happens here and not in the tap or the manager (#746): // the remote server and the recorder-independent readers take the full // payload; only the renderer IPC edge pays structured-clone bytes for @@ -72,11 +85,7 @@ export function wireSessionForwarder( // back. (The session recorder taps this send, so recordings carry the // wire form; replay treats screen frames as no-op ticks and never reads // the fields.) - sendToSessionWindow( - payload.sessionId, - 'session:screen', - aliasScreenSnapshotForWire(payload as { sessionId: string } & AgentScreenSnapshot), - ) + sendToSessionWindow(frame.sessionId, 'session:screen', aliasScreenSnapshotForWire(frame)) return } sendToSessionWindow(payload.sessionId, `session:${channel}`, payload) diff --git a/src/main/sessions/screenInterest.test.ts b/src/main/sessions/screenInterest.test.ts new file mode 100644 index 000000000..2c3dc397b --- /dev/null +++ b/src/main/sessions/screenInterest.test.ts @@ -0,0 +1,45 @@ +import { describe, expect, it } from 'vitest' + +import { ScreenInterest } from './screenInterest.js' + +// #762: screen frames leave main only while some renderer holds a lease. The +// failure this guards is a count that drifts: too low stops a debug panel's +// frames, too high forwards the 93%-of-IPC-bytes stream again forever. +describe('ScreenInterest', () => { + it('wants a session while any lease on it is held, across owners', () => { + const interest = new ScreenInterest() + interest.acquire(1, 's') + interest.acquire(2, 's') + interest.release(1, 's') + expect(interest.wants('s')).toBe(true) + interest.release(2, 's') + expect(interest.wants('s')).toBe(false) + }) + + it('ignores a release its owner does not hold, so it cannot cancel another owner', () => { + const interest = new ScreenInterest() + interest.acquire(1, 's') + interest.release(2, 's') + interest.release(1, 's') + interest.release(1, 's') + expect(interest.wants('s')).toBe(false) + interest.acquire(2, 's') + interest.release(1, 's') + expect(interest.wants('s')).toBe(true) + }) + + it('drops everything an owner held when it goes away, and only that', () => { + // A renderer that reloads or crashes never releases; main drops its + // leases instead (ipc/session.ts), and another window's must survive. + const interest = new ScreenInterest() + interest.acquire(1, 'a') + interest.acquire(1, 'a') + interest.acquire(1, 'b') + interest.acquire(2, 'b') + interest.dropOwner(1) + expect(interest.wants('a')).toBe(false) + expect(interest.wants('b')).toBe(true) + interest.dropOwner(1) + expect(interest.wants('b')).toBe(true) + }) +}) diff --git a/src/main/sessions/screenInterest.ts b/src/main/sessions/screenInterest.ts new file mode 100644 index 000000000..3ccdd7f7c --- /dev/null +++ b/src/main/sessions/screenInterest.ts @@ -0,0 +1,77 @@ +import { ScreenTailHistory } from '@shared/debug/screenTail.js' + +// Which sessions a renderer currently wants live `session:screen` frames for. +// +// WHY frames are opt-in (#762): `session:screen` carried 93% of recorded IPC +// bytes (16,728 frames x 8.8 KB in one 123-minute recording) and arrived 6-10 +// times a second per busy session, every one structured-cloned, dispatched and +// committed into the renderer's runtime map. A map of every consumer found that +// nothing live needs them: +// - main already reads the screen itself for prompt delivery, paste +// confirmation and readiness (`session.snapshotScreen()`); +// - the composer's slash picker is owned by the conditions channel +// (`claude.slash-picker`, applyConditionSnapshot); +// - the Enter-baseline `latestScreenRef` and the renderer paste helpers +// were dead code; +// - ReaderView no longer has a screen fallback (#855). +// What remains are debug surfaces (DebugPanel, the dev modules), which take a +// lease while they are open, and debug bundles, which ask main directly +// (`session:get-screen-debug`) and read the screen-tail history recorded here. +// +// WHY leases are owned by a webContents and not just counted: a renderer that +// reloads or crashes never runs its cleanup, and a bare counter would then +// forward that session's frames forever. Dropping an owner's leases when its +// webContents navigates or is destroyed keeps the count honest. (A leaked +// lease would only cost bytes, never correctness, but the whole point here is +// the bytes.) + +export class ScreenInterest { + private readonly byOwner = new Map>() + private readonly totals = new Map() + + acquire(owner: number, sessionId: string): void { + let leases = this.byOwner.get(owner) + if (!leases) { + leases = new Map() + this.byOwner.set(owner, leases) + } + leases.set(sessionId, (leases.get(sessionId) ?? 0) + 1) + this.totals.set(sessionId, (this.totals.get(sessionId) ?? 0) + 1) + } + + release(owner: number, sessionId: string): void { + const leases = this.byOwner.get(owner) + const held = leases?.get(sessionId) ?? 0 + // A release the owner does not hold (double release, or one that raced + // dropOwner) must not decrement someone else's lease. + if (!leases || held <= 0) return + if (held === 1) leases.delete(sessionId) + else leases.set(sessionId, held - 1) + if (leases.size === 0) this.byOwner.delete(owner) + this.decrementTotal(sessionId, 1) + } + + dropOwner(owner: number): void { + const leases = this.byOwner.get(owner) + if (!leases) return + this.byOwner.delete(owner) + for (const [sessionId, held] of leases) this.decrementTotal(sessionId, held) + } + + wants(sessionId: string): boolean { + return (this.totals.get(sessionId) ?? 0) > 0 + } + + private decrementTotal(sessionId: string, by: number): void { + const next = (this.totals.get(sessionId) ?? 0) - by + if (next > 0) this.totals.set(sessionId, next) + else this.totals.delete(sessionId) + } +} + +/** The process-wide instances: the forwarder consults and feeds them, the + * session IPC handlers mutate and read them. Module singletons for the same + * reason windowRegistry is one: both sides are wired in different files and + * there is exactly one main process. */ +export const screenInterest = new ScreenInterest() +export const screenTailHistory = new ScreenTailHistory() diff --git a/src/main/window/sessionRoutingComposition.test.ts b/src/main/window/sessionRoutingComposition.test.ts index f3b04c53c..9ba7a1600 100644 --- a/src/main/window/sessionRoutingComposition.test.ts +++ b/src/main/window/sessionRoutingComposition.test.ts @@ -38,13 +38,20 @@ vi.mock('@main/subagents/index.js', () => ({ import * as registry from './windowRegistry.js' import { wireSessionForwarder } from '@main/sessions/forwarder.js' +import { screenInterest } from '@main/sessions/screenInterest.js' import { registerSessionIpc } from '@main/ipc/session.js' import { registerSessionRoutingIpc } from '@main/ipc/sessionRouting.js' import { abandonPendingBequest, recordPendingBequest, registerWindowIpc } from '@main/ipc/window.js' let manager: SessionManager & EventEmitter let forwarder: ReturnType +// These cases are about ROUTING (which window may see a session's frames), so +// every session here holds a screen lease: since #762 an unleased session's +// frames never leave main, which would make "nothing was sent" true for the +// wrong reason. The lease owner id is not a real webContents. +const TEST_SCREEN_OWNER = -1 beforeEach(() => { + for (const sessionId of ['pane', 'refused-close', 'admission-pane']) screenInterest.acquire(TEST_SCREEN_OWNER, sessionId) registry.resetWindowRegistryForTests() harness.handlers.clear() harness.built.length = 0 @@ -73,6 +80,7 @@ beforeEach(() => { registerSessionRoutingIpc(manager, forwarder) }) afterEach(() => { + screenInterest.dropOwner(TEST_SCREEN_OWNER) forwarder.flush() manager.removeAllListeners() registry.resetWindowRegistryForTests() diff --git a/src/preload/api/session.ts b/src/preload/api/session.ts index 0659b4b86..41476290c 100644 --- a/src/preload/api/session.ts +++ b/src/preload/api/session.ts @@ -5,7 +5,8 @@ import type { PromptDeliveryOptions, PromptDeliveryResult } from '@shared/types/ import { subscribe } from '@preload/api/ipc.js' import { expandScreenSnapshotFromWire } from '@shared/types/session.js' -import type { AgentScreenSnapshotWire } from '@shared/types/session.js' +import type { AgentScreenSnapshot, AgentScreenSnapshotWire } from '@shared/types/session.js' +import type { ScreenTailSample } from '@shared/debug/screenTail.js' import type { TerminalForegroundEvent, TerminalForegroundState } from '@shared/types/terminalForeground.js' import type { SessionExitEvent, @@ -129,6 +130,19 @@ export const sessionApi = { detachAgentPty: (sessionId: string): Promise => ipcRenderer.invoke('session:agent-pty-detach', sessionId), + // #762: live `session:screen` frames are forwarded only while a lease is + // held (debug surfaces). Acquire also sends the current screen as one + // ordinary session:screen event. + acquireScreenLease: (sessionId: string): Promise => + ipcRenderer.invoke('session:screen-lease', sessionId), + + releaseScreenLease: (sessionId: string): Promise => + ipcRenderer.invoke('session:screen-release', sessionId), + + /** The latest screen and main's screen-tail history, for debug bundles. */ + getScreenDebug: (sessionId: string): Promise<{ screen: AgentScreenSnapshot | null; samples: ScreenTailSample[] }> => + ipcRenderer.invoke('session:get-screen-debug', sessionId), + // --- Per-session I/O --- // // Optional `pasteId` correlates this write against the per-paste diff --git a/src/providers/opencode/renderer/composerSubmit.test.ts b/src/providers/opencode/renderer/composerSubmit.test.ts index bc5a24ab4..2cd0c4e07 100644 --- a/src/providers/opencode/renderer/composerSubmit.test.ts +++ b/src/providers/opencode/renderer/composerSubmit.test.ts @@ -22,7 +22,6 @@ describe('opencodeComposerSubmit', () => { send: vi.fn(), deliverPrompt: vi.fn(async () => delivery), pasteId: 'paste-1', - getScreen: () => undefined, } await expect(opencodeComposerSubmit(io)).rejects.toMatchObject({ @@ -44,7 +43,6 @@ describe('opencodeComposerSubmit acceptance', () => { send: vi.fn(), deliverPrompt: vi.fn(async () => ({ ok: true as const, acceptance })), pasteId: 'paste-1', - getScreen: () => undefined, } await expect(opencodeComposerSubmit(io)).resolves.toEqual(acceptance) diff --git a/src/providers/registry.renderer.capabilities.ts b/src/providers/registry.renderer.capabilities.ts index 8b860ba61..3980b7df1 100644 --- a/src/providers/registry.renderer.capabilities.ts +++ b/src/providers/registry.renderer.capabilities.ts @@ -326,7 +326,6 @@ export type ComposerSubmitIo = { send: (data: string, pasteId?: string) => Promise deliverPrompt: (prompt: string, imagePaths?: string[]) => Promise pasteId: string - getScreen: () => string | undefined } // Does a tool name identify Agent Code's OWN orchestration spawn verb? The diff --git a/src/renderer/src/app/shell/TerminalDimensionOwnership.renderer.test.tsx b/src/renderer/src/app/shell/TerminalDimensionOwnership.renderer.test.tsx index 5b5074133..6015463e6 100644 --- a/src/renderer/src/app/shell/TerminalDimensionOwnership.renderer.test.tsx +++ b/src/renderer/src/app/shell/TerminalDimensionOwnership.renderer.test.tsx @@ -127,6 +127,12 @@ vi.mock('@renderer/features/debug/devDebugConfig', () => ({ describe('terminal dimension ownership across main-surface takeovers', () => { beforeEach(() => { + // The mounted DebugPanel takes a screen lease (#762); the bridge is the + // only window.api this suite's surfaces touch. + Object.defineProperty(window, 'api', { + configurable: true, + value: { acquireScreenLease: vi.fn(async () => {}), releaseScreenLease: vi.fn(async () => {}) }, + }) harness.paneMounts = {} harness.paneUnmounts = {} const runtime = emptyRuntime() @@ -184,6 +190,7 @@ describe('terminal dimension ownership across main-surface takeovers', () => { afterEach(() => { cleanup() + Reflect.deleteProperty(window, 'api') }) it('hands dimension ownership to the inline terminal while Settings hides the retained pane terminal', async () => { diff --git a/src/renderer/src/features/debug/devModules/ClaudePasteDetection/module.tsx b/src/renderer/src/features/debug/devModules/ClaudePasteDetection/module.tsx index a8e478dac..031038a66 100644 --- a/src/renderer/src/features/debug/devModules/ClaudePasteDetection/module.tsx +++ b/src/renderer/src/features/debug/devModules/ClaudePasteDetection/module.tsx @@ -19,6 +19,7 @@ import type { DevDebugModule, DevDebugModuleProps } from '@renderer/features/deb import type { PasteDebugSession } from '@preload/api/types' import { buildLifecycle, buildStats } from './timeline' +import { useScreenLease } from '@renderer/features/debug/useScreenLease' // The real submit-detection rules, mirrored from claude-code-headless so we can // watch them fire against the same screen text the parser sees: @@ -53,6 +54,8 @@ function matches(value: string, pattern: string, flags: string): boolean { } function ClaudePasteDetection({ sessionId, runtime, kind }: DevDebugModuleProps) { + // Screen frames arrive only while leased (#762, useScreenLease). + useScreenLease(sessionId) const plain = runtime.screen ?? '' const markdown = runtime.screenMarkdown ?? '' const [sessions, setSessions] = useState([]) diff --git a/src/renderer/src/features/debug/devModules/Conditions/module.tsx b/src/renderer/src/features/debug/devModules/Conditions/module.tsx index 1d8476081..1aadd7498 100644 --- a/src/renderer/src/features/debug/devModules/Conditions/module.tsx +++ b/src/renderer/src/features/debug/devModules/Conditions/module.tsx @@ -33,6 +33,7 @@ import type { ClaudeAskUserQuestionState, ProviderConditionSnapshot, } from '@shared/types/providerConditions' +import { useScreenLease } from '@renderer/features/debug/useScreenLease' export const conditionsDebugModule: DevDebugModule = { id: 'conditions', @@ -135,6 +136,8 @@ const AUQ_SCREEN_PROBES = [ ] function ConditionsDebug({ sessionId, runtime, kind }: DevDebugModuleProps) { + // Screen frames arrive only while leased (#762, useScreenLease). + useScreenLease(sessionId) const snapshot = runtime.conditions const conditionKeys = snapshot ? Object.keys(snapshot.conditions) : [] const attention = dispatchAttentionLabelFromConditions(snapshot) diff --git a/src/renderer/src/features/debug/devModules/HeadlessSnapshotProbe/module.tsx b/src/renderer/src/features/debug/devModules/HeadlessSnapshotProbe/module.tsx index fd6853709..9b4eece50 100644 --- a/src/renderer/src/features/debug/devModules/HeadlessSnapshotProbe/module.tsx +++ b/src/renderer/src/features/debug/devModules/HeadlessSnapshotProbe/module.tsx @@ -1,6 +1,7 @@ import { useMemo, useState } from 'react' import type { DevDebugModule, DevDebugModuleProps } from '@renderer/features/debug/devModules/types' +import { useScreenLease } from '@renderer/features/debug/useScreenLease' export const headlessSnapshotProbeModule: DevDebugModule = { id: 'headless-snapshot-probe', @@ -10,6 +11,8 @@ export const headlessSnapshotProbeModule: DevDebugModule = { } function HeadlessSnapshotProbe({ sessionId, runtime, kind }: DevDebugModuleProps) { + // Screen frames arrive only while leased (#762, useScreenLease). + useScreenLease(sessionId) // Wrap-tolerant by default (#1113), so the probe agrees with what // `placeholderCount` counts in production rather than missing every pill the // composer happened to wrap. diff --git a/src/renderer/src/features/debug/renderTrace.ts b/src/renderer/src/features/debug/renderTrace.ts index cb72ffa21..39ccda267 100644 --- a/src/renderer/src/features/debug/renderTrace.ts +++ b/src/renderer/src/features/debug/renderTrace.ts @@ -1,4 +1,10 @@ import { sanitizeHtml } from '@renderer/lib/sanitizeHtml' +import { + hashText, + SCREEN_MAX_SAMPLES, + SCREEN_TAIL_LINES, + type ScreenTailSample, +} from '@shared/debug/screenTail' type TraceReason = 'initial' | 'mutation' | 'screen' | 'manual' @@ -28,28 +34,15 @@ type HtmlCheckpoint = { content: string } -type ScreenTailSample = { - id: string - seq: number - ts: number - tsIso: string - hash: string - lineCount: number - content: string -} - type SessionTrace = { sessionId: string startedAt: number htmlCommits: HtmlCommit[] htmlCheckpoints: HtmlCheckpoint[] - screenSamples: ScreenTailSample[] nextHtmlSeq: number - nextScreenSeq: number lastHtml: string lastHtmlHash: string | null lastHtmlCommitId: string | null - lastScreenHash: string | null } type BundleFile = { @@ -61,8 +54,6 @@ const HTML_TRACE_DIR = 'trace/html' const SCREEN_TRACE_DIR = 'trace/screen' const HTML_CHECKPOINT_EVERY = 20 const HTML_MAX_COMMITS = 200 -const SCREEN_MAX_SAMPLES = 300 -const SCREEN_TAIL_LINES = 50 const traces = new Map() @@ -74,32 +65,16 @@ function getTrace(sessionId: string): SessionTrace { startedAt: Date.now(), htmlCommits: [], htmlCheckpoints: [], - screenSamples: [], nextHtmlSeq: 0, - nextScreenSeq: 0, lastHtml: '', lastHtmlHash: null, lastHtmlCommitId: null, - lastScreenHash: null, } traces.set(sessionId, trace) } return trace } -function hashText(text: string): string { - let h1 = 0xdeadbeef ^ text.length - let h2 = 0x41c6ce57 ^ text.length - for (let i = 0; i < text.length; i++) { - const ch = text.charCodeAt(i) - h1 = Math.imul(h1 ^ ch, 2654435761) - h2 = Math.imul(h2 ^ ch, 1597334677) - } - h1 = Math.imul(h1 ^ (h1 >>> 16), 2246822507) ^ Math.imul(h2 ^ (h2 >>> 13), 3266489909) - h2 = Math.imul(h2 ^ (h2 >>> 16), 2246822507) ^ Math.imul(h1 ^ (h1 >>> 13), 3266489909) - const n = 4294967296 * (2097151 & h2) + (h1 >>> 0) - return n.toString(16).padStart(13, '0') -} function nowIso(ts: number): string { return new Date(ts).toISOString() @@ -129,20 +104,6 @@ function buildDelta(base: string, next: string, baseHash: string): TextDelta { } } -function sanitizeScreenText(text: string): string { - return text - .replace(/\x1B(?:[@-Z\\-_]|\[[0-?]*[ -/]*[@-~])/g, '') - .replace(/\r\n?/g, '\n') - .replace(/[^\S\n\t]+$/gm, '') - .replace(/[\x00-\x08\x0B\x0C\x0E-\x1F\x7F]/g, '') - .trimEnd() -} - -function tailLines(text: string, count: number): string { - const lines = text.split('\n') - return lines.length <= count ? text : lines.slice(lines.length - count).join('\n') -} - function pruneHtmlTrace(trace: SessionTrace): void { if (trace.htmlCommits.length <= HTML_MAX_COMMITS) return const overflow = trace.htmlCommits.length - HTML_MAX_COMMITS @@ -216,57 +177,38 @@ export function recordHtmlTraceSnapshot( pruneHtmlTrace(trace) } -export function recordScreenTailSnapshot(sessionId: string, screenText: string): void { - const trace = getTrace(sessionId) - const content = tailLines(sanitizeScreenText(screenText), SCREEN_TAIL_LINES) - if (!content) return - - const hash = hashText(content) - if (hash === trace.lastScreenHash) return - - const ts = Date.now() - const seq = trace.nextScreenSeq++ - trace.screenSamples.push({ - id: `${seq.toString(36)}-${hash}`, - seq, - ts, - tsIso: nowIso(ts), - hash, - lineCount: content.split('\n').length, - content, - }) - trace.lastScreenHash = hash - - if (trace.screenSamples.length > SCREEN_MAX_SAMPLES) { - trace.screenSamples.splice(0, trace.screenSamples.length - SCREEN_MAX_SAMPLES) - } -} - -export function exportDebugTraceFiles(sessionId: string): BundleFile[] { +/** + * @param screenSamples main's screen-tail history for this session (#762: + * recorded in main from every frame, since the renderer no longer receives + * them; fetched with window.api.getScreenDebug). + */ +export function exportDebugTraceFiles(sessionId: string, screenSamples: readonly ScreenTailSample[] = []): BundleFile[] { const trace = traces.get(sessionId) - if (!trace) return [] + if (!trace && screenSamples.length === 0) return [] + const htmlCommits = trace?.htmlCommits ?? [] + const htmlCheckpoints = trace?.htmlCheckpoints ?? [] const capturedAt = Date.now() const manifest = { schemaVersion: 1, sessionId, - startedAt: trace.startedAt, - startedAtIso: nowIso(trace.startedAt), + startedAt: trace?.startedAt ?? null, + startedAtIso: trace ? nowIso(trace.startedAt) : null, capturedAt, capturedAtIso: nowIso(capturedAt), html: { - commits: trace.htmlCommits.length, - checkpoints: trace.htmlCheckpoints.length, + commits: htmlCommits.length, + checkpoints: htmlCheckpoints.length, checkpointEvery: HTML_CHECKPOINT_EVERY, maxCommits: HTML_MAX_COMMITS, - latestHash: trace.lastHtmlHash, + latestHash: trace?.lastHtmlHash ?? null, }, screen: { - samples: trace.screenSamples.length, + samples: screenSamples.length, tailLines: SCREEN_TAIL_LINES, maxSamples: SCREEN_MAX_SAMPLES, - latestHash: trace.lastScreenHash, - mode: 'deduped snapshots; no commit chain', + latestHash: screenSamples.at(-1)?.hash ?? null, + mode: 'deduped snapshots; no commit chain; recorded in main', }, } @@ -277,22 +219,22 @@ export function exportDebugTraceFiles(sessionId: string): BundleFile[] { }, { name: `${HTML_TRACE_DIR}/commits.jsonl`, - content: trace.htmlCommits.map(commit => JSON.stringify(commit)).join('\n') + - (trace.htmlCommits.length ? '\n' : ''), + content: htmlCommits.map(commit => JSON.stringify(commit)).join('\n') + + (htmlCommits.length ? '\n' : ''), }, { name: `${HTML_TRACE_DIR}/checkpoints.jsonl`, - content: trace.htmlCheckpoints.map(checkpoint => JSON.stringify(checkpoint)).join('\n') + - (trace.htmlCheckpoints.length ? '\n' : ''), + content: htmlCheckpoints.map(checkpoint => JSON.stringify(checkpoint)).join('\n') + + (htmlCheckpoints.length ? '\n' : ''), }, { name: `${SCREEN_TRACE_DIR}/tail-samples.jsonl`, - content: trace.screenSamples.map(sample => JSON.stringify(sample)).join('\n') + - (trace.screenSamples.length ? '\n' : ''), + content: screenSamples.map(sample => JSON.stringify(sample)).join('\n') + + (screenSamples.length ? '\n' : ''), }, ] - const latestScreen = trace.screenSamples[trace.screenSamples.length - 1] + const latestScreen = screenSamples[screenSamples.length - 1] if (latestScreen) { files.push({ name: `${SCREEN_TRACE_DIR}/latest-tail.txt`, diff --git a/src/renderer/src/features/debug/saveDebugBundle.ts b/src/renderer/src/features/debug/saveDebugBundle.ts index 99fb82a98..5426fa76e 100644 --- a/src/renderer/src/features/debug/saveDebugBundle.ts +++ b/src/renderer/src/features/debug/saveDebugBundle.ts @@ -7,8 +7,9 @@ import { commandTargetSessionId } from '@renderer/workspace/hook/selectors/comma import { exportDebugTraceFiles, recordHtmlTraceSnapshot, - recordScreenTailSnapshot, } from '@renderer/features/debug/renderTrace' +import type { AgentScreenSnapshot } from '@shared/types/session' +import type { ScreenTailSample } from '@shared/debug/screenTail' import { buildCommittedAssistantText, buildSemanticRenderUnits, @@ -270,8 +271,10 @@ function buildRenderDiagnostics(runtime: SessionRuntime, kind: string): Record { +// Tail-truncated to SCREEN_TAIL_LINES. Taken from MAIN's latest snapshot +// when available (#762): the renderer's copy only moves while a debug +// surface holds a screen lease, so it is usually stale or empty. +function buildStateSnapshot(runtime: SessionRuntime, mainScreen: AgentScreenSnapshot | null): Record { const { entries: _entries, toolUseIndex: _toolUseIndex, @@ -296,10 +299,10 @@ function buildStateSnapshot(runtime: SessionRuntime): Record { return { ...rest, - screen: tailLines(screen, SCREEN_TAIL_LINES), - screenMarkdown: tailLines(screenMarkdown, SCREEN_TAIL_LINES), - recentScreen: tailLines(recentScreen, SCREEN_TAIL_LINES), - recentScreenMarkdown: tailLines(recentScreenMarkdown, SCREEN_TAIL_LINES), + screen: tailLines(mainScreen?.plain ?? screen, SCREEN_TAIL_LINES), + screenMarkdown: tailLines(mainScreen?.markdown ?? screenMarkdown, SCREEN_TAIL_LINES), + recentScreen: tailLines(mainScreen?.recent ?? recentScreen, SCREEN_TAIL_LINES), + recentScreenMarkdown: tailLines(mainScreen?.recentMarkdown ?? recentScreenMarkdown, SCREEN_TAIL_LINES), // Counts are preserved so you can see "there were 2145 entries // in the feed" without having to open the JSONL. _counts: { @@ -488,7 +491,11 @@ export async function assembleAndSaveDebugBundle(params: { const html = capturePaneHtml(sessionId) recordHtmlTraceSnapshot(sessionId, html.raw, 'manual') - recordScreenTailSnapshot(sessionId, runtime.recentScreen) + // Main owns the screen now (#762): its latest snapshot and the tail history + // it records from every frame. A failed read degrades to the renderer's copy + // and no samples, never to a failed bundle. + const screenDebug: { screen: AgentScreenSnapshot | null; samples: ScreenTailSample[] } = + await window.api.getScreenDebug(sessionId).catch(() => ({ screen: null, samples: [] })) // Manual bundles include a bounded proxy tail because a human just // asked for a forensic snapshot. Autosave bundles intentionally skip @@ -511,7 +518,7 @@ export async function assembleAndSaveDebugBundle(params: { }, { name: FILE_NAMES.state, - content: JSON.stringify(buildStateSnapshot(runtime), null, 2), + content: JSON.stringify(buildStateSnapshot(runtime, screenDebug.screen), null, 2), }, { name: FILE_NAMES.feedDebug, @@ -542,7 +549,7 @@ export async function assembleAndSaveDebugBundle(params: { ...(proxySection?.sessionMeta ? [{ name: FILE_NAMES.proxySessionMeta, content: proxySection.sessionMeta }] : []), - ...exportDebugTraceFiles(sessionId), + ...exportDebugTraceFiles(sessionId, screenDebug.samples), ...(performanceSnapshot?.files ?? []), ] diff --git a/src/renderer/src/features/debug/ui/DebugPanel.tsx b/src/renderer/src/features/debug/ui/DebugPanel.tsx index 04390e8ce..3570477dd 100644 --- a/src/renderer/src/features/debug/ui/DebugPanel.tsx +++ b/src/renderer/src/features/debug/ui/DebugPanel.tsx @@ -4,6 +4,7 @@ import { useEffect, useMemo, useState } from 'react' import type { SessionRuntime } from '@renderer/workspace/workspaceStore' import type { Entry } from '@shared/types/transcript' import { AgentInlineTerminal } from '@renderer/features/debug/ui/AgentInlineTerminal' +import { useScreenLease } from '@renderer/features/debug/useScreenLease' // DebugPanel — inline diagnostic overlay showing the raw state of the // focused pane. Toggled via "Toggle Debug Panel" in the command palette. @@ -35,6 +36,9 @@ export function DebugPanel({ onClose, }: Props) { const [rawTerminalOpen, setRawTerminalOpen] = useState(false) + // The raw and markdown screen below need live frames, which main forwards + // only while a debug surface holds a lease (#762). + useScreenLease(sessionId) // WHY disable the inline xterm while the pane itself is terminal: // AgentInlineTerminal is not a passive transcript viewer; it attaches to and // resizes the same provider PTY as AgentTerminalLeaf. If both are mounted, diff --git a/src/renderer/src/features/debug/useScreenLease.renderer.test.tsx b/src/renderer/src/features/debug/useScreenLease.renderer.test.tsx new file mode 100644 index 000000000..f4311906c --- /dev/null +++ b/src/renderer/src/features/debug/useScreenLease.renderer.test.tsx @@ -0,0 +1,27 @@ +import { renderHook } from '@testing-library/react' +import { afterEach, expect, it, vi } from 'vitest' + +import { useScreenLease } from './useScreenLease' + +// #762: a debug surface receives screen frames only while it holds a lease. +// A lease that is never released keeps the 93%-of-IPC-bytes stream flowing +// for that session; one released early blanks the panel. +afterEach(() => { Reflect.deleteProperty(window, 'api') }) + +it('holds one lease per mounted session and moves it when the session changes', () => { + const acquireScreenLease = vi.fn(async () => {}) + const releaseScreenLease = vi.fn(async () => {}) + Object.defineProperty(window, 'api', { configurable: true, value: { acquireScreenLease, releaseScreenLease } }) + + const { rerender, unmount } = renderHook(({ id }: { id: string | null }) => useScreenLease(id), { initialProps: { id: 'a' as string | null } }) + expect(acquireScreenLease.mock.calls).toEqual([['a']]) + + rerender({ id: 'b' }) + expect(releaseScreenLease.mock.calls).toEqual([['a']]) + expect(acquireScreenLease.mock.calls).toEqual([['a'], ['b']]) + + rerender({ id: null }) + unmount() + expect(releaseScreenLease.mock.calls).toEqual([['a'], ['b']]) + expect(acquireScreenLease).toHaveBeenCalledTimes(2) +}) diff --git a/src/renderer/src/features/debug/useScreenLease.ts b/src/renderer/src/features/debug/useScreenLease.ts new file mode 100644 index 000000000..a3ecd0049 --- /dev/null +++ b/src/renderer/src/features/debug/useScreenLease.ts @@ -0,0 +1,22 @@ +import { useEffect } from 'react' + +/** + * Receive live `session:screen` frames for this session while mounted. + * + * WHY a lease (#762): main forwards screen frames only to a renderer that + * asked for them, because nothing live reads them and they were 93% of IPC + * bytes (src/main/sessions/screenInterest.ts). Debug surfaces that show the + * raw screen take a lease for as long as they are open; main seeds the + * current screen on acquire, so the surface is right even when the backend + * is idle. Main also drops a renderer's leases on reload, so a missed + * release can only cost bytes until then, never correctness. + */ +export function useScreenLease(sessionId: string | null | undefined): void { + useEffect(() => { + if (!sessionId) return + void window.api.acquireScreenLease(sessionId) + return () => { + void window.api.releaseScreenLease(sessionId) + } + }, [sessionId]) +} diff --git a/src/renderer/src/workspace/hook/actions/agentIndexNavigation.renderer.test.tsx b/src/renderer/src/workspace/hook/actions/agentIndexNavigation.renderer.test.tsx index cbed3dfd3..e97bac952 100644 --- a/src/renderer/src/workspace/hook/actions/agentIndexNavigation.renderer.test.tsx +++ b/src/renderer/src/workspace/hook/actions/agentIndexNavigation.renderer.test.tsx @@ -44,7 +44,6 @@ function makeRefs(state: WorkspaceState): WorkspaceRefs { seenUuidsRef: ref({}), historyWindowsRef: { current: {} } as never, historyAwaitingTurnStartRef: { current: new Set() } as never, - latestScreenRef: ref({}), undoStackRef: ref(new UndoCloseStack()), bootstrapTimersRef: ref(new Map()), persistedFeedDebugIdRef: ref({}), diff --git a/src/renderer/src/workspace/hook/actions/pane.ts b/src/renderer/src/workspace/hook/actions/pane.ts index f94fcdcd3..477308994 100644 --- a/src/renderer/src/workspace/hook/actions/pane.ts +++ b/src/renderer/src/workspace/hook/actions/pane.ts @@ -80,7 +80,6 @@ function forgetClosedSessionDebugState(refs: WorkspaceRefs, sessionId: SessionId // liveEntryWindow.ts). Every site that deletes/resets a session's seen // set must clear it too. clearLiveEntryWindowSession(sessionId) - delete refs.latestScreenRef.current[sessionId] // Render traces are intentionally not stored in SessionRuntime: they are // large, debug-only forensic buffers populated by DOM/screen capture paths // that do not need to re-render the app. That module-level map must still diff --git a/src/renderer/src/workspace/hook/actions/paneRecoveryOwnership.renderer.test.tsx b/src/renderer/src/workspace/hook/actions/paneRecoveryOwnership.renderer.test.tsx index 764c8ea17..7047c6fc3 100644 --- a/src/renderer/src/workspace/hook/actions/paneRecoveryOwnership.renderer.test.tsx +++ b/src/renderer/src/workspace/hook/actions/paneRecoveryOwnership.renderer.test.tsx @@ -44,7 +44,6 @@ function renderPaneActionsHarness( latestStateRef: ref(state), latestRuntimesRef: ref(runtimes), seenUuidsRef: ref>>({}), - latestScreenRef: ref>({}), undoStackRef: ref(new UndoCloseStack()), // Close operations release a pending bootstrap debounce for each session // they end, as sessionActions.killSession always did (#886 round 2). @@ -124,7 +123,6 @@ describe('pane recovery ownership', () => { latestStateRef: ref(state), latestRuntimesRef: ref(runtimes), seenUuidsRef: ref>>({}), - latestScreenRef: ref>({}), undoStackRef: ref(new UndoCloseStack()), bootstrapTimersRef: ref(new Map()), } as unknown as WorkspaceRefs diff --git a/src/renderer/src/workspace/hook/actions/session.ts b/src/renderer/src/workspace/hook/actions/session.ts index 148885c5d..742095c4a 100644 --- a/src/renderer/src/workspace/hook/actions/session.ts +++ b/src/renderer/src/workspace/hook/actions/session.ts @@ -1209,7 +1209,6 @@ export function useSessionActions( // Live-window bookkeeping follows the seen-uuid lifecycle (see // liveEntryWindow.ts: trimmed ⊆ ever-seen must hold). clearLiveEntryWindowSession(sessionId) - delete refs.latestScreenRef.current[sessionId] // If a bootstrap debounce was in flight for this session, // cancel it — the session is gone; firing the deferred // bootstrapping→false flip later would be a no-op against a @@ -1221,7 +1220,7 @@ export function useSessionActions( refs.bootstrapTimersRef.current.delete(sessionId) } }, - [refs.bootstrapTimersRef, refs.latestScreenRef, refs.seenUuidsRef, setRuntimes, setState], + [refs.bootstrapTimersRef, refs.seenUuidsRef, setRuntimes, setState], ) // Kills the current session in the focused leaf and spawns a new @@ -1532,7 +1531,6 @@ export function useSessionActions( }) delete refs.seenUuidsRef.current[oldId] clearLiveEntryWindowSession(oldId) - delete refs.latestScreenRef.current[oldId] return newId } finally { @@ -1541,7 +1539,6 @@ export function useSessionActions( }, [ refs.latestRuntimesRef, - refs.latestScreenRef, refs.seenUuidsRef, refs.stateRef, setRuntimes, @@ -1613,7 +1610,6 @@ export function useSessionActions( delete refs.seenUuidsRef.current[oldId] clearLiveEntryWindowSession(oldId) - delete refs.latestScreenRef.current[oldId] try { const kind: SessionKind = meta.kind ?? DEFAULT_PROVIDER @@ -1743,7 +1739,6 @@ export function useSessionActions( [ refs.dangerousAgentsRef, refs.latestRuntimesRef, - refs.latestScreenRef, refs.seenUuidsRef, refs.stateRef, refs.useProxyStreamingRef, @@ -1806,7 +1801,6 @@ export function useSessionActions( // on the live path while older-history kept releasing them — the // trimmed ⊆ ever-seen invariant (liveEntryWindow.ts) would be gone. clearLiveEntryWindowSession(sessionId) - delete refs.latestScreenRef.current[sessionId] setRuntimes(prev => { const current = prev[sessionId] ?? emptyRuntime() @@ -1829,7 +1823,6 @@ export function useSessionActions( }, [ refs.bootstrapTimersRef, - refs.latestScreenRef, refs.seenUuidsRef, refs.stateRef, setRuntimes, diff --git a/src/renderer/src/workspace/hook/actions/sessionRecovery.renderer.test.tsx b/src/renderer/src/workspace/hook/actions/sessionRecovery.renderer.test.tsx index 6ee8ef847..a1e65bed3 100644 --- a/src/renderer/src/workspace/hook/actions/sessionRecovery.renderer.test.tsx +++ b/src/renderer/src/workspace/hook/actions/sessionRecovery.renderer.test.tsx @@ -427,7 +427,6 @@ describe('useSessionActions recovery retry', () => { useProxyStreamingRef: ref(false), defaultBuiltInMcpDomainsRef: ref([]), seenUuidsRef: ref({}), - latestScreenRef: ref({}), } as unknown as WorkspaceRefs const setState = (next: WorkspaceState | ((prev: WorkspaceState) => WorkspaceState)) => { state = typeof next === 'function' ? next(state) : next diff --git a/src/renderer/src/workspace/hook/actions/sessionReplacementHandoff.renderer.test.tsx b/src/renderer/src/workspace/hook/actions/sessionReplacementHandoff.renderer.test.tsx index 7a029e730..005652943 100644 --- a/src/renderer/src/workspace/hook/actions/sessionReplacementHandoff.renderer.test.tsx +++ b/src/renderer/src/workspace/hook/actions/sessionReplacementHandoff.renderer.test.tsx @@ -71,7 +71,6 @@ describe('renderer session replacement handoff', () => { seenUuidsRef: ref({}), historyWindowsRef: { current: {} } as never, historyAwaitingTurnStartRef: { current: new Set() } as never, - latestScreenRef: ref({}), undoStackRef: ref(new UndoCloseStack()), bootstrapTimersRef: ref(new Map()), persistedFeedDebugIdRef: ref({}), diff --git a/src/renderer/src/workspace/hook/actions/testing/paneActionsHarness.tsx b/src/renderer/src/workspace/hook/actions/testing/paneActionsHarness.tsx index 9bc60ab9c..ae67475c5 100644 --- a/src/renderer/src/workspace/hook/actions/testing/paneActionsHarness.tsx +++ b/src/renderer/src/workspace/hook/actions/testing/paneActionsHarness.tsx @@ -43,7 +43,6 @@ export function makeRefs(state: WorkspaceState): WorkspaceRefs { seenUuidsRef: ref({}), historyWindowsRef: { current: {} } as never, historyAwaitingTurnStartRef: { current: new Set() } as never, - latestScreenRef: ref({}), undoStackRef: ref(new UndoCloseStack()), bootstrapTimersRef: ref(new Map()), persistedFeedDebugIdRef: ref({}), diff --git a/src/renderer/src/workspace/hook/index.ts b/src/renderer/src/workspace/hook/index.ts index 61fe731ec..16ddb621d 100644 --- a/src/renderer/src/workspace/hook/index.ts +++ b/src/renderer/src/workspace/hook/index.ts @@ -1051,7 +1051,6 @@ export function useWorkspace( setReaderModeTarget, toggleReaderMode, setReaderModeSession, - latestScreenRef: refs.latestScreenRef, getRuntime, // Patch a session's runtime (sessionId, partial). Used by TileLeaf for // pane toasts and the prompt-suggestion chip's apply/dismiss handlers. diff --git a/src/renderer/src/workspace/hook/ipc/testing/workspaceRefsForTest.ts b/src/renderer/src/workspace/hook/ipc/testing/workspaceRefsForTest.ts index 947f28186..981814b13 100644 --- a/src/renderer/src/workspace/hook/ipc/testing/workspaceRefsForTest.ts +++ b/src/renderer/src/workspace/hook/ipc/testing/workspaceRefsForTest.ts @@ -22,7 +22,6 @@ export function makeWorkspaceRefsForTest(state: WorkspaceState): WorkspaceRefs { seenUuidsRef: ref({}), historyWindowsRef: { current: {} } as never, historyAwaitingTurnStartRef: { current: new Set() } as never, - latestScreenRef: ref({}), undoStackRef: ref(new UndoCloseStack()), bootstrapTimersRef: ref(new Map()), persistedFeedDebugIdRef: ref({}), diff --git a/src/renderer/src/workspace/hook/ipc/useIpcSubscriptions.renderer.test.tsx b/src/renderer/src/workspace/hook/ipc/useIpcSubscriptions.renderer.test.tsx index a5ff4dd31..4f832adbd 100644 --- a/src/renderer/src/workspace/hook/ipc/useIpcSubscriptions.renderer.test.tsx +++ b/src/renderer/src/workspace/hook/ipc/useIpcSubscriptions.renderer.test.tsx @@ -890,6 +890,23 @@ describe('useIpcSubscriptions with an injected SessionFeed', () => { expect(runtimes.s1?.screen).toBe('hello world') expect(runtimes.s1?.recentScreen).toBe('hello world') + + // #762: the picker a screen frame carries is NOT applied. The conditions + // channel owns runtime.picker (see the first case in this file); a second + // writer could re-show a picker the conditions had just cleared, and + // frames now arrive only while a debug surface holds a lease. + act(() => { + fake.emitScreen({ + sessionId: 's1', + plain: 'hello again', + markdown: 'hello again', + recent: 'hello again', + recentMarkdown: 'hello again', + picker: { visible: true, items: [{ name: '/help', description: 'stale' }] }, + } as never) + }) + expect(runtimes.s1?.screen).toBe('hello again') + expect(runtimes.s1?.picker.visible).toBe(false) }) it('treats provider readiness as versioned state, never as process activity', () => { @@ -978,14 +995,12 @@ describe('useIpcSubscriptions with an injected SessionFeed', () => { draftInput: 'renderer-local draft survives', } let runtimes: Record = { s1: conflictedRuntime } - let refsForAssertion: WorkspaceRefs | null = null function Harness(): React.JSX.Element { const refs = useRef(null) if (refs.current === null) { refs.current = makeRefs(state) refs.current.latestRuntimesRef.current = runtimes - refsForAssertion = refs.current } useIpcSubscriptions( fake, @@ -1033,7 +1048,6 @@ describe('useIpcSubscriptions with an injected SessionFeed', () => { // workspace's text and mutate hidden semantic/debug state. Quarantine must // return before every state/ref write until matching ownership is proven. expect(runtimes.s1).toBe(conflictedRuntime) - expect(refsForAssertion!.latestScreenRef.current.s1).toBeUndefined() }) it('retains the retired run across exit and replaces it only on the next started event', () => { diff --git a/src/renderer/src/workspace/hook/ipc/useIpcSubscriptions.ts b/src/renderer/src/workspace/hook/ipc/useIpcSubscriptions.ts index 99caf5e40..e52e34694 100644 --- a/src/renderer/src/workspace/hook/ipc/useIpcSubscriptions.ts +++ b/src/renderer/src/workspace/hook/ipc/useIpcSubscriptions.ts @@ -37,7 +37,6 @@ import { import { applyPromptSuggestionToRuntime } from '@renderer/workspace/hook/ipc/applyPromptSuggestionToRuntime' import { summarizeSemanticEventForDebug } from '@renderer/session-runtime/semantic/summarize' import { isSemanticRawCaptureEnabled } from '@renderer/session-runtime/semantic/rawCapture' -import { recordScreenTailSnapshot } from '@renderer/features/debug/renderTrace' import { isCodexRolloutEntry, isOptimisticCodexUserEntry, @@ -63,7 +62,6 @@ import { type CommittedSeenLedger, } from '@renderer/session-runtime/ingest/committedRecords' import { emitRendererMemoryGauges } from '@renderer/performance/memoryInstrumentation' -import { pickerEqual } from '@renderer/workspace/layout/helpers' import { ghostsFromSemanticTurn, ghostsToPersist, @@ -410,7 +408,7 @@ const WALL_CLOCK_MS_FLOOR = 1_000_000_000_000 // // Handlers intentionally stay inline here rather than broken into // one-per-event files because they share refs, setters, and -// bookkeeping state (seenUuidsRef, latestScreenRef, bootstrapTimersRef) +// bookkeeping state (seenUuidsRef, bootstrapTimersRef) // — breaking them apart would mean passing the whole ctx into each, // gaining nothing but file count. If a handler ever grows to >200 // lines AND has no cross-handler state, it's a candidate for @@ -705,18 +703,18 @@ export function useIpcSubscriptions( }) const offScreen = feed.onSessionScreen( - ({ sessionId, plain, markdown, recent, recentMarkdown, picker }) => { + ({ sessionId, plain, markdown, recent, recentMarkdown }) => { if (quarantinesSessionFeed(sessionId)) return const startedAt = performance.now() - // latestScreenRef is the synchronous source of truth for - // the Enter-baseline capture in TileLeaf — always update - // it, even when we bail on React state below. - // Use `recent` (wider window) so the baseline includes any - // assistant text that may have already scrolled out of the - // viewport. The baseline comparison is the basis for "is - // the streaming card stale?", which depends on seeing the - // same marker the streaming extractor will see. - refs.latestScreenRef.current[sessionId] = recent + // WHY only the screen strings are applied here (#762): frames now + // arrive only while a debug surface holds a screen lease, so this + // handler feeds DebugPanel and the dev modules, nothing live. The + // frame's `picker` is NOT written: the composer's slash picker is + // owned by the conditions channel (applyConditionSnapshot), and a + // second writer here was split authority that could re-show a + // picker the conditions had just cleared. The Enter-baseline ref and + // the screen-tail trace this used to feed are gone: the baseline was + // dead code, and main records the trace from every frame. setRuntimes(prev => { const current = prev[sessionId] ?? emptyRuntime() @@ -733,11 +731,7 @@ export function useIpcSubscriptions( // every no-op frame. This is the difference between // "scheduled work on every frame" and "scheduled work // only when the screen actually changed". - if ( - current.screen === plain && - current.recentScreen === recent && - pickerEqual(current.picker, picker) - ) { + if (current.screen === plain && current.recentScreen === recent) { const durationMs = performance.now() - startedAt if (durationMs > 8) { perf.metric('workspace.ipc.screen.noop.slow', durationMs, 'sample', { @@ -750,7 +744,6 @@ export function useIpcSubscriptions( const changed: string[] = [] if (current.screen !== plain) changed.push('screen') if (current.recentScreen !== recent) changed.push('recent') - if (!pickerEqual(current.picker, picker)) changed.push('picker') // Screen frames can differ only by transient TUI chrome // (cursor blink, spinner tick, timestamp) while the // visible transcript is unchanged. We still commit the @@ -770,8 +763,7 @@ export function useIpcSubscriptions( const chromeTickOnly = changed.every(k => k === 'screen' || k === 'recent') && changed.length > 0 && - recent.length === current.recentScreen.length && - pickerEqual(current.picker, picker) + recent.length === current.recentScreen.length const nextBody = { ...current, @@ -779,7 +771,6 @@ export function useIpcSubscriptions( screenMarkdown: markdown, recentScreen: recent, recentScreenMarkdown: recentMarkdown, - picker, // activityStatus is owned by the process-state IPC // handler below — it carries the provider-correct verb // (Claude's spinner verb, Codex's bottom-row text). @@ -798,22 +789,16 @@ export function useIpcSubscriptions( summary: `screen update · ${changed.join(', ')}`, data: { changed, - pickerVisible: picker.visible, - pickerCount: picker.items.length, recentLength: recent.length, }, }, ) - if (!chromeTickOnly) { - recordScreenTailSnapshot(sessionId, recent) - } const durationMs = performance.now() - startedAt if (durationMs > 8) { perf.metric('workspace.ipc.screen.apply.slow', durationMs, 'sample', { sessionId, changed, recentLength: recent.length, - pickerVisible: picker.visible, }) } return { diff --git a/src/renderer/src/workspace/hook/persistence/codexLiveContinuity.renderer.test.tsx b/src/renderer/src/workspace/hook/persistence/codexLiveContinuity.renderer.test.tsx index baaf759fd..dd18bdbd0 100644 --- a/src/renderer/src/workspace/hook/persistence/codexLiveContinuity.renderer.test.tsx +++ b/src/renderer/src/workspace/hook/persistence/codexLiveContinuity.renderer.test.tsx @@ -211,7 +211,6 @@ function makeRefs(state: WorkspaceState, runtimes: Record> - latestScreenRef: MutableRefObject> undoStackRef: MutableRefObject bootstrapTimersRef: MutableRefObject>> persistedFeedDebugIdRef: MutableRefObject> @@ -105,7 +104,6 @@ export function useWorkspaceRefs( const seenUuidsRef = useRef>>({}) const historyWindowsRef = useRef>({}) const historyAwaitingTurnStartRef = useRef>(new Set()) - const latestScreenRef = useRef>({}) const undoStackRef = useRef(new UndoCloseStack()) const bootstrapTimersRef = useRef>>(new Map()) const pendingAdoptionWindowIdsRef = useRef([]) @@ -147,7 +145,6 @@ export function useWorkspaceRefs( // Latest screen per session — mirrored from state into a ref so // the Enter handler in TileLeaf can capture a baseline // synchronously. - latestScreenRef, // Undo-close stack — mutable ref because the stack is imperative // (push/pop) and we don't want React re-renders on every close. diff --git a/src/renderer/src/workspace/tile-tree/TileLeaf/claudePaste.ts b/src/renderer/src/workspace/tile-tree/TileLeaf/claudePaste.ts deleted file mode 100644 index 05e6ffc9e..000000000 --- a/src/renderer/src/workspace/tile-tree/TileLeaf/claudePaste.ts +++ /dev/null @@ -1,219 +0,0 @@ -// Claude's TUI has its own paste-state machine. Large input and -// bracketed paste do NOT go straight from "bytes arrived" to "submit -// immediately" — Claude buffers every `isPasted` chunk and flushes on a -// ~100ms debounce (`PASTE_COMPLETION_TIMEOUT_MS` in claude-code-src). -// INSIDE that debounce window a `pastePendingRef` guard swallows every -// subsequent keystroke — INCLUDING our submit `\r` — as more paste -// content. So if the Enter lands before the debounce fires, it becomes -// literal text, no turn starts, and the user has to press Enter again. -// That is the exact user-facing bug: -// -// 1. the pasted prompt appears in Claude's composer, -// 2. Claude flips into a "working" looking state, -// 3. but no real turn starts, -// 4. and the NEXT Enter finally submits the already-buffered prompt. -// -// We fix this at the shell boundary instead of forking Claude's paste -// logic: deliver the paste in TWO phases — first the bracketed payload, -// then a separate submit `\r` only AFTER we can see Claude's composer -// has visibly absorbed the paste (the debounce has provably fired). -// -// ---------------------------------------------------------------------------- -// THE SIGNAL IS A CONTENT MATCH, NOT A CLOCK. -// ---------------------------------------------------------------------------- -// -// We compare what the user submitted against what is actually in the -// headless composer, via the live screen snapshot (`latestScreenRef`), -// and send `\r` the instant the composer reflects the paste. Two -// screen-truth manifestations, because Claude renders pastes two ways: -// -// * COLLAPSE: a large paste becomes a `[Pasted text #N]` placeholder. -// Signal: a NEW placeholder appeared vs. the pre-paste baseline. -// * INLINE: a medium paste (≈100 chars up to Claude's collapse -// threshold) is inserted as raw text. NO placeholder ever renders — -// this is the band that issue #279 / #90 kept failing on, because -// the old detector only watched for the placeholder, timed out, and -// blind-fired `\r` straight into the debounce. Signal: the paste's -// tail newly appears in the composer. -// -// Both are load-independent: 10ms or 10s, we wait for the *condition*. -// -// The wall-clock wait survives ONLY as a rare safety floor — if neither -// signal materializes within a short bound (a future Claude UI rename, -// or a screen snapshot we can't read), we send `\r` anyway so submit -// degrades rather than hangs. It must never again be the primary path: -// real production traces show the timer races even at 1000ms under load, -// and "there's no value of T that's correct under all load conditions." -// -// Why these numbers: -// - 100 chars: empirical lower bound where Claude's paste ACCUMULATOR -// engages, independent of the inline-vs-collapse cutoff. Below this we -// send raw `text + \r` in one write (the accumulator doesn't engage, -// so the `\r` is safe). -// - 125 ms floor: slightly above Claude's 100 ms debounce. Consulted -// ONLY when the content match can't be made (the rare safety net). -// - 500 ms detection bound: well past the 100 ms accumulator window, so -// a real absorption always wins first; short enough that the rare -// safety-net path still feels responsive. - -import { sha8Web } from '@shared/code/sha8' -import { - CLAUDE_PASTE_THRESHOLD, - placeholderCount, - pasteTailNeedle, - pollPasteAbsorbed, -} from '@shared/claude/pasteConfirm' -import type { PasteAbsorbedOutcome } from '@shared/claude/pasteConfirm' - -// The paste-commit DETECTION (placeholder-or-inline) now lives in the shared -// module that BOTH this desktop composer and the runtime delivery path consume -// — killing the drift that had left the runtime on a placeholder-only probe. -// Re-exported here so existing importers keep their `@renderer/.../claudePaste` -// import path. The battle-scarred TIMING constants below are unchanged. -export { CLAUDE_PASTE_THRESHOLD, placeholderCount, pasteTailNeedle } -export type { PasteAbsorbedOutcome } - -export const CLAUDE_PASTE_SUBMIT_DELAY_MS = 125 -export const CLAUDE_IMAGE_PATH_SUBMIT_DELAY_MS = 750 -export const CLAUDE_PASTE_EVENT_DRIVEN_TIMEOUT_MS = 500 -export const CLAUDE_PASTE_POLL_INTERVAL_MS = 10 - -function wait(ms: number): Promise { - return new Promise(resolve => setTimeout(resolve, ms)) -} - -// SHA-256 prefix used as a fingerprint to correlate paste payloads -// across the renderer→IPC→main→PTY chain in the per-paste debug -// journal. crypto.subtle is the standard renderer-side digest API; -// 4 bytes is more than enough for cross-process pairing (collision -// probability is negligible for a single paste's chunk set). -const sha8 = sha8Web - -// Content-match detection (placeholderCount / pasteTailNeedle / -// pollPasteAbsorbed) moved verbatim to @shared/claude/pasteConfirm so the -// runtime delivery path shares the exact same logic. Imported + re-exported at -// the top of this file. - -export type ClaudePasteSendFn = (data: string, pasteId?: string) => Promise - -export type ClaudePasteOptions = { - /** Renderer-minted UUID for the per-paste debug journal. When set, - * every PTY write in this paste emits an IPC:write event to the - * journal. */ - pasteId?: string - /** Event-driven submit for Claude paste-like text. `getScreen` returns the - * live headless TUI snapshot so we can confirm the composer absorbed the - * paste before sending Enter. Optional in the type only because lower-level - * helpers are also used by Codex and image-path flows that do not need it. */ - eventDriven?: { enabled: boolean; getScreen: () => string | undefined } -} - -export async function sendBracketedPaste( - send: ClaudePasteSendFn, - payload: string, - opts?: ClaudePasteOptions, -): Promise { - const pasteId = opts?.pasteId - if (pasteId) { - window.api.recordPasteDebugEvent(pasteId, { - layer: 'IPC', - event: 'write:paste-payload', - data: { bytes: payload.length, sha8: await sha8(payload) }, - }) - } - await send(`\x1b[200~${payload}\x1b[201~`, pasteId) -} - -export async function sendBracketedPasteThenSubmit( - send: ClaudePasteSendFn, - payload: string, - delayMs = 0, - opts?: ClaudePasteOptions, -): Promise { - const pasteId = opts?.pasteId - const eventDriven = opts?.eventDriven - - // Single-write fast path. Used by callers that don't care about the - // accumulator race (e.g. Codex, which has its own paste-state machine that - // does NOT exhibit this bug). On Claude this branch is only taken when - // `delayMs <= 0` AND event-driven is off — the composer call site always - // sets event-driven, so in production this is codex-only. - if (delayMs <= 0 && !eventDriven?.enabled) { - if (pasteId) { - window.api.recordPasteDebugEvent(pasteId, { - layer: 'IPC', - event: 'write:paste-and-submit-single', - data: { bytes: payload.length, sha8: await sha8(payload) }, - }) - } - await send(`\x1b[200~${payload}\x1b[201~\r`, pasteId) - return - } - - // Capture the composer baseline BEFORE writing the paste so the detector - // keys on the placeholder/inline-text *transition*, not a stale match. - const baselineScreen = eventDriven?.enabled ? (eventDriven.getScreen() ?? '') : '' - - await sendBracketedPaste(send, payload, opts) - - if (eventDriven?.enabled) { - const outcome = await pollPasteAbsorbed(eventDriven.getScreen, baselineScreen, payload, { - timeoutMs: CLAUDE_PASTE_EVENT_DRIVEN_TIMEOUT_MS, - pollIntervalMs: CLAUDE_PASTE_POLL_INTERVAL_MS, - }) - if (pasteId) { - window.api.recordPasteDebugEvent(pasteId, { - layer: 'SCREEN', - event: outcome.kind === 'absorbed' ? 'paste:absorbed' : 'paste:absorb-timeout', - data: - outcome.kind === 'absorbed' - ? { waitedMs: outcome.waitedMs, via: outcome.via } - : {}, - }) - } - if (outcome.kind === 'absorbed') { - if (pasteId) { - window.api.recordPasteDebugEvent(pasteId, { - layer: 'IPC', - event: 'write:submit-cr', - data: { strategy: `event-driven:${outcome.via}`, waitedMs: outcome.waitedMs }, - }) - } - await send('\r', pasteId) - return - } - // Absorption not detected within the safety bound. Fall through to the - // wall-clock floor — the rare safety net, NOT the primary path. - } - - if (delayMs > 0) await wait(delayMs) - if (pasteId) { - window.api.recordPasteDebugEvent(pasteId, { - layer: 'IPC', - event: 'write:submit-cr', - data: { strategy: 'wall-clock-floor', delayMs }, - }) - } - await send('\r', pasteId) -} - -export async function sendClaudeDraftText( - send: ClaudePasteSendFn, - text: string, - opts?: ClaudePasteOptions, -): Promise { - if (text.length === 0) return - const isPasteLike = text.includes('\n') || text.length > CLAUDE_PASTE_THRESHOLD - if (isPasteLike) { - await sendBracketedPaste(send, text, opts) - await wait(CLAUDE_PASTE_SUBMIT_DELAY_MS) - return - } - await send(text, opts?.pasteId) -} - -export function buildClaudeImagePastePayload(text: string, imagePaths: string[]): string { - if (imagePaths.length === 0) return text - if (text.trim().length === 0) return imagePaths.join('\n') - return `${imagePaths.join('\n')}\n${text}` -} diff --git a/src/renderer/src/workspace/tile-tree/TileLeaf/useComposerKeybinds.ts b/src/renderer/src/workspace/tile-tree/TileLeaf/useComposerKeybinds.ts index 6159282fe..ecf523622 100644 --- a/src/renderer/src/workspace/tile-tree/TileLeaf/useComposerKeybinds.ts +++ b/src/renderer/src/workspace/tile-tree/TileLeaf/useComposerKeybinds.ts @@ -9,7 +9,7 @@ import { isSessionExited } from '@renderer/workspace/providerSessionIdentity' // stable across providers so historical dumps stay comparable. The // actual submit routing moved to the provider composerSubmit // capability (#394 phase 2c-4). -import { CLAUDE_PASTE_THRESHOLD } from '@renderer/workspace/tile-tree/TileLeaf/claudePaste' +import { CLAUDE_PASTE_THRESHOLD } from '@shared/claude/pasteConfirm' import { getRendererProviderCapabilities } from '@providers/registry.renderer.capabilities' import { DEFAULT_PROVIDER, @@ -357,7 +357,6 @@ export function useComposerKeybinds({ () => workspace.ensureSessionLive(sessionId, 'tile-leaf.deliver-retry'), ), pasteId, - getScreen: () => workspace.latestScreenRef.current[sessionId], }) // Accepted: the sent prompt leaves the draft, keeping anything another // writer added during the send (draftAfterAcceptance). diff --git a/src/shared/debug/screenTail.test.ts b/src/shared/debug/screenTail.test.ts new file mode 100644 index 000000000..b00c3844f --- /dev/null +++ b/src/shared/debug/screenTail.test.ts @@ -0,0 +1,32 @@ +import { describe, expect, it } from 'vitest' + +import { SCREEN_MAX_SAMPLES, SCREEN_TAIL_LINES, ScreenTailHistory } from './screenTail.js' + +// The debug bundle's trace/screen/tail-samples.jsonl, now recorded in main +// (#762). Same contract as the renderer version it replaced: tail only, +// deduplicated, bounded, forgettable. +describe('ScreenTailHistory', () => { + it('keeps the last lines of each distinct screen, once', () => { + let now = 1_000 + const history = new ScreenTailHistory(() => now) + const long = Array.from({ length: SCREEN_TAIL_LINES + 10 }, (_, i) => `line ${i}`).join('\n') + history.record('s', `\u001b[31m${long}\u001b[0m `) + now += 1 + history.record('s', long) + const [sample, ...rest] = history.samples('s') + expect(rest).toEqual([]) + expect(sample!.lineCount).toBe(SCREEN_TAIL_LINES) + expect(sample!.content.startsWith('line 10')).toBe(true) + expect(sample!.content).not.toContain('\u001b') + }) + + it('caps the samples per session and forgets a closed session', () => { + const history = new ScreenTailHistory(() => 0) + for (let i = 0; i < SCREEN_MAX_SAMPLES + 5; i += 1) history.record('s', `frame ${i}`) + const samples = history.samples('s') + expect(samples).toHaveLength(SCREEN_MAX_SAMPLES) + expect(samples[0]!.content).toBe('frame 5') + history.forget('s') + expect(history.samples('s')).toEqual([]) + }) +}) diff --git a/src/shared/debug/screenTail.ts b/src/shared/debug/screenTail.ts new file mode 100644 index 000000000..83d94b617 --- /dev/null +++ b/src/shared/debug/screenTail.ts @@ -0,0 +1,97 @@ +// Screen-tail history for debug bundles: a bounded, deduplicated sample of +// the last SCREEN_TAIL_LINES of each session's terminal screen over time. +// +// WHY this lives in shared code and is recorded in MAIN (#762): it used to be +// recorded in the renderer from every `session:screen` frame. That IPC was +// 93% of recorded IPC bytes and is now only forwarded to a renderer that +// asked for it (see src/main/sessions/screenInterest.ts), so the renderer no +// longer sees every frame. Main does, in the forwarder sink, so the history +// is recorded there and handed to the debug bundle on request. The format is +// unchanged: bundles still carry trace/screen/tail-samples.jsonl. + +export const SCREEN_TAIL_LINES = 50 +export const SCREEN_MAX_SAMPLES = 300 + +export type ScreenTailSample = { + id: string + seq: number + ts: number + tsIso: string + hash: string + lineCount: number + content: string +} + +/** A fast, non-cryptographic 53-bit string hash (cyrb53), hex. Used to + * deduplicate identical samples and to label trace content. */ +export function hashText(text: string): string { + let h1 = 0xdeadbeef ^ text.length + let h2 = 0x41c6ce57 ^ text.length + for (let i = 0; i < text.length; i++) { + const ch = text.charCodeAt(i) + h1 = Math.imul(h1 ^ ch, 2654435761) + h2 = Math.imul(h2 ^ ch, 1597334677) + } + h1 = Math.imul(h1 ^ (h1 >>> 16), 2246822507) ^ Math.imul(h2 ^ (h2 >>> 13), 3266489909) + h2 = Math.imul(h2 ^ (h2 >>> 16), 2246822507) ^ Math.imul(h1 ^ (h1 >>> 13), 3266489909) + const n = 4294967296 * (2097151 & h2) + (h1 >>> 0) + return n.toString(16).padStart(13, '0') +} + +export function sanitizeScreenText(text: string): string { + return text + .replace(/\x1B(?:[@-Z\\-_]|\[[0-?]*[ -/]*[@-~])/g, '') + .replace(/\r\n?/g, '\n') + .replace(/[^\S\n\t]+$/gm, '') + .replace(/[\x00-\x08\x0B\x0C\x0E-\x1F\x7F]/g, '') + .trimEnd() +} + +export function tailLines(text: string, count: number): string { + const lines = text.split('\n') + return lines.length <= count ? text : lines.slice(lines.length - count).join('\n') +} + +type SessionTail = { samples: ScreenTailSample[]; nextSeq: number; lastHash: string | null } + +export class ScreenTailHistory { + private readonly sessions = new Map() + + constructor(private readonly now: () => number = Date.now) {} + + record(sessionId: string, screenText: string): void { + const content = tailLines(sanitizeScreenText(screenText), SCREEN_TAIL_LINES) + if (!content) return + const hash = hashText(content) + let tail = this.sessions.get(sessionId) + if (!tail) { + tail = { samples: [], nextSeq: 0, lastHash: null } + this.sessions.set(sessionId, tail) + } + if (hash === tail.lastHash) return + const ts = this.now() + const seq = tail.nextSeq++ + tail.samples.push({ + id: `${seq.toString(36)}-${hash}`, + seq, + ts, + tsIso: new Date(ts).toISOString(), + hash, + lineCount: content.split('\n').length, + content, + }) + tail.lastHash = hash + if (tail.samples.length > SCREEN_MAX_SAMPLES) { + tail.samples.splice(0, tail.samples.length - SCREEN_MAX_SAMPLES) + } + } + + samples(sessionId: string): ScreenTailSample[] { + return [...(this.sessions.get(sessionId)?.samples ?? [])] + } + + /** Per-session arrays are bounded, but a map of closed sessions is not. */ + forget(sessionId: string): void { + this.sessions.delete(sessionId) + } +} From de8e69cf0740436e6b45728eb956c7b90fdc71d0 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Fri, 25 Sep 2026 03:05:49 -0700 Subject: [PATCH 3/5] =?UTF-8?q?fix(ipc):=20#762=20review=20=E2=80=94=20sam?= =?UTF-8?q?e-document=20guard,=20bundle=20seam=20tests,=20dead=20paste=20c?= =?UTF-8?q?hain?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #1236: - C2: a same-document (hash/history) main-frame navigation no longer drops a renderer's screen leases, matching workflows.ts's identical per-renderer lease contract. - C1: the debug-bundle consumer seam is tested end to end: the real assembleAndSaveDebugBundle with window.api as the only mock saves main's tail history and screen; buildStateSnapshot prefers main's screen; the get-screen-debug handler answers snapshot plus history. M1, M2 and M20 now fail. - C3/C4/C5: the history records `recent` (not `plain`); a recent-only frame applies; the lease seed carries the exact aliased payload. M18, M15 and M19 now fail. - C6 plus cleanup: the whole awaitClaudePastePlaceholder chain (preload API, IPC handler, manager and session methods, AgentSession optional) had no caller, the same dead cluster as the deleted claudePaste.ts; removed, and the comments that pointed at claudePaste.ts corrected. Co-Authored-By: Claude Opus 5.5 --- src/main/ipc/session.test.ts | 36 +++++++++++++-- src/main/ipc/session.ts | 31 ++++--------- src/main/sessionManager.ts | 42 +---------------- src/main/sessions/forwarder.test.ts | 9 ++-- src/preload/api/session.ts | 17 ------- src/preload/api/types.ts | 2 +- src/providers/claude/runtime/claudeSession.ts | 20 -------- .../debug/renderTrace.renderer.test.ts | 15 ++++++ .../debug/saveDebugBundle.renderer.test.ts | 46 +++++++++++++++++++ .../src/features/debug/saveDebugBundle.ts | 2 +- .../ipc/useIpcSubscriptions.renderer.test.tsx | 12 +++++ .../src/workspace/tile-tree/TileLeaf.tsx | 5 +- .../tile-tree/TileLeaf/useComposerKeybinds.ts | 3 +- src/shared/claude/pasteConfirm.ts | 8 ++-- src/shared/types/providerConfig.ts | 8 ++-- src/shared/types/session.ts | 12 +---- 16 files changed, 135 insertions(+), 133 deletions(-) create mode 100644 src/renderer/src/features/debug/saveDebugBundle.renderer.test.ts diff --git a/src/main/ipc/session.test.ts b/src/main/ipc/session.test.ts index e248da39c..845e4ca46 100644 --- a/src/main/ipc/session.test.ts +++ b/src/main/ipc/session.test.ts @@ -166,13 +166,17 @@ describe('screen leases (#762)', () => { // the current screen goes down the ordinary session:screen path. lease({ sender }, 'pane') expect(screenInterest.wants('pane')).toBe(true) - expect(harness.routed).toHaveBeenCalledWith('pane', 'session:screen', expect.objectContaining({ sessionId: 'pane', plain: 'now' })) + // The same aliased wire payload the recover seed sends (recent/markdown + // equal to plain/markdown are dropped on the wire, #746). + expect(harness.routed).toHaveBeenCalledWith('pane', 'session:screen', { sessionId: 'pane', plain: 'now', markdown: 'now' }) - // A reload never runs the renderer's cleanup. A sub-frame navigation is - // not a reload and must not drop it. - sender.emit('did-start-navigation', {}, 'about:blank', false, false) + // A reload never runs the renderer's cleanup. A sub-frame navigation and a + // same-document (hash/history) navigation are not reloads and keep it. + sender.emit('did-start-navigation', { isMainFrame: false, isSameDocument: false }) expect(screenInterest.wants('pane')).toBe(true) - sender.emit('did-start-navigation', {}, 'app://index.html', false, true) + sender.emit('did-start-navigation', { isMainFrame: true, isSameDocument: true }) + expect(screenInterest.wants('pane')).toBe(true) + sender.emit('did-start-navigation', { isMainFrame: true, isSameDocument: false }) expect(screenInterest.wants('pane')).toBe(false) lease({ sender }, 'pane') @@ -180,3 +184,25 @@ describe('screen leases (#762)', () => { expect(screenInterest.wants('pane')).toBe(false) }) }) + +describe('session:get-screen-debug (#762)', () => { + it('answers with main\'s latest raw screen and the recorded tail history', async () => { + const { screenTailHistory } = await import('@main/sessions/screenInterest.js') + const manager = new EventEmitter() + Object.assign(manager, { + getScreenSnapshot: () => ({ plain: 'latest', markdown: 'latest', recent: 'latest\nmore', recentMarkdown: 'latest\nmore' }), + }) + registerSessionIpc(manager as never, {} as never, new SessionFeedTap(manager as never)) + screenTailHistory.record('debug-pane', 'first frame') + screenTailHistory.record('debug-pane', 'second frame') + try { + const answer = await harness.handlers.get('session:get-screen-debug')!({}, 'debug-pane') as { + screen: { recent: string } | null; samples: Array<{ content: string }> + } + expect(answer.screen?.recent).toBe('latest\nmore') + expect(answer.samples.map(sample => sample.content)).toEqual(['first frame', 'second frame']) + } finally { + screenTailHistory.forget('debug-pane') + } + }) +}) diff --git a/src/main/ipc/session.ts b/src/main/ipc/session.ts index a95a1b218..876f48370 100644 --- a/src/main/ipc/session.ts +++ b/src/main/ipc/session.ts @@ -268,8 +268,12 @@ export function registerSessionIpc( if (!leaseOwnersWatched.has(owner)) { leaseOwnersWatched.add(owner) const drop = () => screenInterest.dropOwner(owner) - sender.on('did-start-navigation', (_event, _url, _inPlace, isMainFrame) => { - if (isMainFrame) drop() + // A reload is a main-frame navigation to a new document. Same-document + // hash/history navigation keeps the JavaScript context and its live + // leases, as workflows.ts's identical per-renderer lease contract + // already rules (#1236 review C). + sender.on('did-start-navigation', details => { + if (details.isMainFrame && !details.isSameDocument) drop() }) sender.once('destroyed', () => { drop() @@ -297,7 +301,7 @@ export function registerSessionIpc( 'session:input', (_evt, sessionId: string, data: string, pasteId?: string) => { // Optional pasteId journals THIS write into the per-paste debug - // dump. Only set by the Agent Code paste flow (claudePaste.ts) — + // dump. Only set by the composer's paste flow (useComposerKeybinds) — // never set on keystrokes, agent-pty bridging, or other normal // I/O. Pairs against the renderer's IPC:write:* events by sha8 // + byte count, same way dictation pairs renderer-produced @@ -321,7 +325,7 @@ export function registerSessionIpc( // between and reports the wrong cause — the same misdiagnosis this // replaces, just narrower. const deliveryInFlight = manager.isDeliveryInFlight(sessionId) - // `pasteId` is set only by the Agent Code paste flow (claudePaste.ts) and + // `pasteId` is set only by the composer's paste flow (useComposerKeybinds) and // never by keystrokes, so it is also the exact renderer-side attribution // signal required by SessionManager's prompt-delivery ownership fence. const attributedPasteId = typeof pasteId === 'string' && pasteId.length > 0 @@ -442,25 +446,6 @@ export function registerSessionIpc( }, ) - // Event-driven paste-submit (Track C of the paste-submit harness PR). - // Renderer's claudePaste.ts invokes this AFTER writing the bracketed - // paste payload but BEFORE writing `\r`. We resolve as soon as - // Claude's TUI renders `[Pasted text #N]`, falling back to a 2 s - // timeout if the placeholder never appears (future Claude UI rename - // insurance). See `claudePaste.ts` and - // `packages/claude-code-headless/src/ClaudeCodeHeadless.ts:awaitPastePlaceholder` - // for the full rationale chain. - ipcMain.handle( - 'claude:await-paste-placeholder', - async ( - _evt, - sessionId: string, - opts?: { timeoutMs?: number; pollIntervalMs?: number }, - ) => { - return manager.awaitClaudePastePlaceholder(sessionId, opts) - }, - ) - ipcMain.handle( 'session:load-older-history', async ( diff --git a/src/main/sessionManager.ts b/src/main/sessionManager.ts index 05ba4c854..65a02d0d8 100644 --- a/src/main/sessionManager.ts +++ b/src/main/sessionManager.ts @@ -287,8 +287,7 @@ export interface SessionManager { // for the migration rationale. // // Local aliases used below just narrow AgentSession for the -// duck-typed-optional-method call sites (awaitPastePlaceholder, -// awaitReadyForPrompt), so the callers keep reading naturally. +// duck-typed-optional-method call sites (awaitReadyForPrompt), so the callers keep reading naturally. type AgentSessionLike = AgentSession type RegistryLifecycle = { @@ -4390,45 +4389,6 @@ export class SessionManager extends EventEmitter { } } - /** - * Claude-specific accessor for the paste-submit event-driven path - * in `src/renderer/.../claudePaste.ts`. Returns the live ClaudeSession - * cast through `unknown` because AgentSessionLike doesn't (and - * shouldn't) expose `awaitPastePlaceholder` — that's a Claude-only - * affordance and adding it to the cross-provider interface would - * force every other runtime to ship a no-op stub. - * - * Returns `null` for missing sessions or non-Claude kinds. Callers - * MUST treat null as a benign "couldn't reach this session" and - * fall through to whatever non-event-driven path they were using - * before; the absence of a Claude session is not an error worth - * crashing over. - */ - async awaitClaudePastePlaceholder( - sessionId: string, - opts?: { timeoutMs?: number; pollIntervalMs?: number }, - ): Promise< - | { kind: 'appeared'; waitedMs: number } - | { kind: 'timeout' } - | { kind: 'no-headless' } - | { kind: 'no-session' } - > { - const entry = this.sessions.get(sessionId) - if (!entry || entry.kind !== 'claude') return { kind: 'no-session' } - // The cross-provider AgentSessionLike interface doesn't carry - // `awaitPastePlaceholder`; ClaudeSession does. We assert via a - // structural duck-type so misconfigured Claude provider builds - // (a future ClaudeSession that loses the method) surface as - // 'no-session' rather than a TypeError. - // No cast: awaitPastePlaceholder is now a typed optional on - // AgentSession (see @shared/types/session.ts). - const session = entry.session - if (typeof session.awaitPastePlaceholder !== 'function') { - return { kind: 'no-session' } - } - return session.awaitPastePlaceholder(opts) - } - /** * Ask the provider to scroll its own transcript view to the newest message * (#843). Only providers whose TUI pages its transcript on the alternate diff --git a/src/main/sessions/forwarder.test.ts b/src/main/sessions/forwarder.test.ts index 330ae8014..413a3b27c 100644 --- a/src/main/sessions/forwarder.test.ts +++ b/src/main/sessions/forwarder.test.ts @@ -123,7 +123,9 @@ it('forwards screen frames only to a leased session, and records every frame for )) as { deliveries: { wrapped: { baseline: { screen: string }; after: { screen: string } } } } const idle = recorded.deliveries.wrapped.baseline.screen const drafted = recorded.deliveries.wrapped.after.screen - const frame = (screen: string) => ({ sessionId: 'pane', plain: screen, markdown: screen, recent: screen, recentMarkdown: screen }) + // `recent` is the wider window (Codex scrollback) and is what the history + // must record, so it differs from `plain` here to pin that (#1236 review C). + const frame = (screen: string) => ({ sessionId: 'pane', plain: screen.slice(-200), markdown: screen, recent: screen, recentMarkdown: screen }) try { manager.emit('screen', frame(idle)) forwarder.flush() @@ -132,16 +134,17 @@ it('forwards screen frames only to a leased session, and records every frame for screenInterest.acquire(7, 'pane') manager.emit('screen', frame(drafted)) forwarder.flush() - expect(sent).toEqual([drafted]) + expect(sent).toEqual([drafted.slice(-200)]) screenInterest.release(7, 'pane') manager.emit('screen', frame(idle)) forwarder.flush() - expect(sent).toEqual([drafted]) + expect(sent).toEqual([drafted.slice(-200)]) // Every frame reached the tail history, forwarded or not (idle, drafted, // idle again: the dedupe only collapses consecutive repeats). expect(screenTailHistory.samples('pane')).toHaveLength(3) + expect(screenTailHistory.samples('pane')[1]!.content.length).toBeGreaterThan(drafted.slice(-200).trimEnd().length) manager.emit('removed', { sessionId: 'pane' }) forwarder.flush() expect(screenTailHistory.samples('pane')).toEqual([]) diff --git a/src/preload/api/session.ts b/src/preload/api/session.ts index 41476290c..dcfb56ad4 100644 --- a/src/preload/api/session.ts +++ b/src/preload/api/session.ts @@ -181,23 +181,6 @@ export const sessionApi = { resize: (sessionId: string, cols: number, rows: number): Promise => ipcRenderer.invoke('session:resize', sessionId, cols, rows), - // Event-driven paste-submit primitive. See - // src/renderer/.../claudePaste.ts and - // packages/claude-code-headless/src/ClaudeCodeHeadless.ts. - // Resolves when Claude's TUI renders `[Pasted text #N]`, or after - // the configured timeout. Renderer treats every non-'appeared' - // outcome as "fall through to the wall-clock submit path." - awaitClaudePastePlaceholder: ( - sessionId: string, - opts?: { timeoutMs?: number; pollIntervalMs?: number }, - ): Promise< - | { kind: 'appeared'; waitedMs: number } - | { kind: 'timeout' } - | { kind: 'no-headless' } - | { kind: 'no-session' } - > => - ipcRenderer.invoke('claude:await-paste-placeholder', sessionId, opts), - loadOlderHistory: (params: { kind: AgentProviderKind cwd: string diff --git a/src/preload/api/types.ts b/src/preload/api/types.ts index c49c0aaf6..3959f1cf6 100644 --- a/src/preload/api/types.ts +++ b/src/preload/api/types.ts @@ -382,7 +382,7 @@ export type DictationDebugEvent = DictationDebugEventInput & { // because the file is local 0o600 and the whole point is to see // what the user actually pasted vs. what reached Claude. export type PasteDebugLayer = - | 'RENDER' // composer keydown, state snapshot, call into claudePaste fn + | 'RENDER' // composer keydown, state snapshot, composer submit | 'IPC' // renderer-side IPC write (paste payload, submit \r) | 'PTY' // main-side PTY write (sha8 + byte count) — pairs with IPC | 'SCREEN' // [Pasted text #N] placeholder observed to appear / clear diff --git a/src/providers/claude/runtime/claudeSession.ts b/src/providers/claude/runtime/claudeSession.ts index 431364113..ec30cd5ea 100644 --- a/src/providers/claude/runtime/claudeSession.ts +++ b/src/providers/claude/runtime/claudeSession.ts @@ -737,26 +737,6 @@ export class ClaudeSession extends EventEmitter { return this.headless?.getScreenMarkdown() ?? '' } - /** - * Poll the live Claude TUI screen for `[Pasted text #N]` and resolve - * the instant it appears. Delegates to ClaudeCodeHeadless; see the - * method comment there for why this is load-independent and why a - * timeout fallback is mandatory. Used by claudePaste.ts to send `\r` - * exactly when Claude has visibly committed the paste — the - * primary path of the paste-submit fix. - * - * Returns `{ kind: 'no-headless' }` when the headless instance has - * not been constructed yet (start() not called or session torn down) - * so the renderer can fall through to the wall-clock path without - * waiting for the full 2 s timeout. - */ - async awaitPastePlaceholder( - opts?: { timeoutMs?: number; pollIntervalMs?: number }, - ): Promise<{ kind: 'appeared'; waitedMs: number } | { kind: 'timeout' } | { kind: 'no-headless' }> { - if (!this.headless) return { kind: 'no-headless' } - return this.headless.awaitPastePlaceholder(opts ?? {}) - } - async resolveCondition( action: ConditionCustomAction, ): Promise { diff --git a/src/renderer/src/features/debug/renderTrace.renderer.test.ts b/src/renderer/src/features/debug/renderTrace.renderer.test.ts index 61bff63a2..b13b69eb8 100644 --- a/src/renderer/src/features/debug/renderTrace.renderer.test.ts +++ b/src/renderer/src/features/debug/renderTrace.renderer.test.ts @@ -70,4 +70,19 @@ describe('renderTrace', () => { expect(exportDebugTraceFiles(sessionId)).toEqual([]) }) + + it('writes the screen samples main recorded into the bundle trace (#762)', () => { + // The samples now come from main (window.api.getScreenDebug); dropping + // the argument at the call site shipped empty screen traces while every + // test stayed green (#1236 review C). + const sessionId = `trace-screen-${crypto.randomUUID()}` + const samples = [ + { id: '0-a', seq: 0, ts: 1, tsIso: new Date(1).toISOString(), hash: 'a', lineCount: 1, content: 'first' }, + { id: '1-b', seq: 1, ts: 2, tsIso: new Date(2).toISOString(), hash: 'b', lineCount: 1, content: 'latest' }, + ] + const files = exportDebugTraceFiles(sessionId, samples) + expect(readJsonl<{ content: string }>(files, 'trace/screen/tail-samples.jsonl').map(sample => sample.content)) + .toEqual(['first', 'latest']) + expect(files.find(file => file.name === 'trace/screen/latest-tail.txt')?.content).toBe('latest') + }) }) diff --git a/src/renderer/src/features/debug/saveDebugBundle.renderer.test.ts b/src/renderer/src/features/debug/saveDebugBundle.renderer.test.ts new file mode 100644 index 000000000..5d6124223 --- /dev/null +++ b/src/renderer/src/features/debug/saveDebugBundle.renderer.test.ts @@ -0,0 +1,46 @@ +import { expect, it } from 'vitest' + +import { emptyRuntime } from '@renderer/session-runtime/state' +import { assembleAndSaveDebugBundle, buildStateSnapshot } from './saveDebugBundle' + +// #762: the renderer's screen copy only moves while a debug surface holds a +// lease, so the bundle's state snapshot takes the screen from MAIN when it +// has one, and falls back to the renderer copy only when main's read failed +// (#1236 review C: an unconditional fallback survived every test). +it('takes the screen fields from main, falling back to the renderer copy only without one', () => { + const runtime = { ...emptyRuntime(), screen: 'stale', screenMarkdown: 'stale', recentScreen: 'stale', recentScreenMarkdown: 'stale' } + const fromMain = buildStateSnapshot(runtime, { plain: 'live', markdown: 'live md', recent: 'live recent', recentMarkdown: 'live recent md' }) + expect(fromMain).toMatchObject({ screen: 'live', screenMarkdown: 'live md', recentScreen: 'live recent', recentScreenMarkdown: 'live recent md' }) + expect(buildStateSnapshot(runtime, null)).toMatchObject({ screen: 'stale', recentScreen: 'stale' }) +}) + +// The whole consumer seam, through the real bundle assembly: main's screen +// and tail history must land in the saved files (#1236 review C: dropping +// the samples at this call site survived every test). window.api is the only +// mocked edge. +it('saves the screen and tail history main recorded into the bundle', async () => { + const saved: Array<{ name: string; content: string }> = [] + const originalApi = window.api + window.api = { + ...originalApi, + flushPerformance: async () => {}, + getPerformanceSnapshot: async () => null, + readProxyEvents: async () => null, + getScreenDebug: async () => ({ + screen: { plain: 'main screen', markdown: 'main screen', recent: 'main recent', recentMarkdown: 'main recent' }, + samples: [{ id: '0-a', seq: 0, ts: 1, tsIso: new Date(1).toISOString(), hash: 'a', lineCount: 1, content: 'recorded in main' }], + }), + saveDebugBundle: async (input: { files: Array<{ name: string; content: string }> }) => { + saved.push(...input.files) + return { bundlePath: '/tmp/bundle' } + }, + } as never + try { + await assembleAndSaveDebugBundle({ sessionId: 'bundle-pane', runtime: emptyRuntime(), kind: 'claude' }) + } finally { + window.api = originalApi + } + expect(saved.find(file => file.name === 'trace/screen/latest-tail.txt')?.content).toBe('recorded in main') + const state = JSON.parse(saved.find(file => file.name.endsWith('state-snapshot.json'))!.content) as { recentScreen: string } + expect(state.recentScreen).toBe('main recent') +}) diff --git a/src/renderer/src/features/debug/saveDebugBundle.ts b/src/renderer/src/features/debug/saveDebugBundle.ts index 5426fa76e..21b69fd79 100644 --- a/src/renderer/src/features/debug/saveDebugBundle.ts +++ b/src/renderer/src/features/debug/saveDebugBundle.ts @@ -274,7 +274,7 @@ function buildRenderDiagnostics(runtime: SessionRuntime, kind: string): Record { +export function buildStateSnapshot(runtime: SessionRuntime, mainScreen: AgentScreenSnapshot | null): Record { const { entries: _entries, toolUseIndex: _toolUseIndex, diff --git a/src/renderer/src/workspace/hook/ipc/useIpcSubscriptions.renderer.test.tsx b/src/renderer/src/workspace/hook/ipc/useIpcSubscriptions.renderer.test.tsx index cc0acd3f4..7abb65b0f 100644 --- a/src/renderer/src/workspace/hook/ipc/useIpcSubscriptions.renderer.test.tsx +++ b/src/renderer/src/workspace/hook/ipc/useIpcSubscriptions.renderer.test.tsx @@ -906,6 +906,18 @@ describe('useIpcSubscriptions with an injected SessionFeed', () => { }) expect(runtimes.s1?.screen).toBe('hello again') expect(runtimes.s1?.picker.visible).toBe(false) + + // A frame whose visible screen is unchanged but whose scrollback grew + // must still apply (#1236 review C: the no-op bail on `screen` alone + // survived). + act(() => { + fake.emitScreen({ + sessionId: 's1', plain: 'hello again', markdown: 'hello again', + recent: 'earlier line\nhello again', recentMarkdown: 'earlier line\nhello again', + picker: { visible: false, items: [] }, + }) + }) + expect(runtimes.s1?.recentScreen).toBe('earlier line\nhello again') }) it('treats provider readiness as versioned state, never as process activity', () => { diff --git a/src/renderer/src/workspace/tile-tree/TileLeaf.tsx b/src/renderer/src/workspace/tile-tree/TileLeaf.tsx index 8e64c95a6..77e6622ca 100644 --- a/src/renderer/src/workspace/tile-tree/TileLeaf.tsx +++ b/src/renderer/src/workspace/tile-tree/TileLeaf.tsx @@ -59,8 +59,9 @@ import { const MAX_TRACKED_VISIBLE_SUBMIT_SURFACES = 2_048 -// Claude paste-state-machine constants + helpers moved to -// ./TileLeaf/claudePaste.ts. Image helpers moved to +// Claude paste detection lives in @shared/claude/pasteConfirm and runs in +// main's prompt delivery (the renderer paste helpers were removed as dead +// code in #762). Image helpers moved to // ./TileLeaf/claudeImages.ts. Label helpers moved to // ./TileLeaf/labels.ts. See those files for the full rationale on // the paste debounce, the image size/format gates, and the pane diff --git a/src/renderer/src/workspace/tile-tree/TileLeaf/useComposerKeybinds.ts b/src/renderer/src/workspace/tile-tree/TileLeaf/useComposerKeybinds.ts index ecf523622..9d3dec7b1 100644 --- a/src/renderer/src/workspace/tile-tree/TileLeaf/useComposerKeybinds.ts +++ b/src/renderer/src/workspace/tile-tree/TileLeaf/useComposerKeybinds.ts @@ -35,7 +35,8 @@ import { reportLifecycle } from '@renderer/lifecycle/report' // Escape forwards, Shift+Enter inserts a literal newline. // The submit path forks by provider because Claude and // Codex have DIFFERENT paste + submit timing requirements -// (see claudePaste.ts for the debounce story). +// (see @shared/claude/pasteConfirm and main's +// providers/claude/runtime/promptDelivery.ts for the paste story). // 3. Bash-style prompt history — Up/Down cycle through previous // prompts when the composer is empty. Modifier combos fall // through to the PTY-forward path so OS line navigation diff --git a/src/shared/claude/pasteConfirm.ts b/src/shared/claude/pasteConfirm.ts index ce3db288e..e60ef4f6a 100644 --- a/src/shared/claude/pasteConfirm.ts +++ b/src/shared/claude/pasteConfirm.ts @@ -1,6 +1,6 @@ -// Claude paste-commit detection — THE ONE implementation, shared by both the -// desktop composer (renderer: claudePaste.ts) and the remote/runtime delivery -// path (main: providers/claude/runtime/promptDelivery.ts). +// Claude paste-commit detection — THE ONE implementation, used by prompt +// delivery in main (providers/claude/runtime/promptDelivery.ts), which every +// composer, remote and runtime prompt goes through. // // WHY this module exists (the bug it kills): // Claude's TUI buffers bracketed-paste bytes on a ~100ms accumulator; an @@ -32,7 +32,7 @@ * engages, so a plain `text + \r` in one write is safe and needs no * confirmation. Above it, OR with any newline, the paste route + this * detection is mandatory. Empirical lower bound; single source for every - * `isPasteLike` check (previously duplicated in claudePaste.ts, + * `isPasteLike` check (previously duplicated in the renderer's paste helpers, * composerSubmit.ts, useComposerKeybinds.ts, and promptDelivery.ts). */ export const CLAUDE_PASTE_THRESHOLD = 100 diff --git a/src/shared/types/providerConfig.ts b/src/shared/types/providerConfig.ts index dd7847c9c..b2c95e540 100644 --- a/src/shared/types/providerConfig.ts +++ b/src/shared/types/providerConfig.ts @@ -362,8 +362,8 @@ export type MainProviderConfig = { * The io bag deliberately passes the AgentSession plus a bound * write-with-liveness function rather than the SessionManager: * providers must not depend on the manager (dependency arrow), and - * the typed optionals they need (awaitReadyForPrompt / - * awaitPastePlaceholder) live on AgentSession since phase 2a. + * the typed optional they need (awaitReadyForPrompt) lives on + * AgentSession since phase 2a. */ /** * Optional provider-owned history source, for providers whose durable @@ -409,8 +409,8 @@ export type MainProviderConfig = { * The io bag deliberately passes the AgentSession plus a bound * write-with-liveness function rather than the SessionManager: * providers must not depend on the manager (dependency arrow), and - * the typed optionals they need (awaitReadyForPrompt / - * awaitPastePlaceholder) live on AgentSession since phase 2a. + * the typed optional they need (awaitReadyForPrompt) lives on + * AgentSession since phase 2a. */ deliverPrompt: (io: PromptDeliveryIo) => Promise } diff --git a/src/shared/types/session.ts b/src/shared/types/session.ts index d975f44be..262678035 100644 --- a/src/shared/types/session.ts +++ b/src/shared/types/session.ts @@ -510,7 +510,7 @@ export interface AgentSessionEmitter { * * Optional methods encode capabilities that are legitimately * provider-specific today. The manager already duck-types them - * (sessionManager awaitClaudePastePlaceholder / resolveCondition / + * (sessionManager resolveCondition / * awaitCodexReadyForPrompt) — declaring them optional here just makes * that duck-typing type-checked. Phase 2c generalizes these into a * `promptDelivery` capability shape; the individual optional methods @@ -573,16 +573,6 @@ export interface AgentSession extends AgentSessionEmitter { * not one. Never throws. */ jumpToLatest?(): Promise<{ ok: true } | { ok: false; reason: string }> - /** Optional (Claude today): wait for the bracketed-paste placeholder - * to appear before firing Enter. See sessionManager.ts:952. */ - awaitPastePlaceholder?( - opts?: { timeoutMs?: number; pollIntervalMs?: number }, - ): Promise< - | { kind: 'appeared'; waitedMs: number } - | { kind: 'timeout' } - | { kind: 'no-headless' } - > - /** Optional (Claude today): a synchronous read of the live plain-text TUI * screen (headless snapshotPlain). The prompt-delivery path polls this to * detect the paste-commit *transition* — a NEW `[Pasted text #N]` From 379d998ceed0386e39ef0971010640cc0ca1b120 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Fri, 25 Sep 2026 03:35:45 -0700 Subject: [PATCH 4/5] =?UTF-8?q?fix(ipc):=20#762=20review=20round=202=20?= =?UTF-8?q?=E2=80=94=20key=20screen=20leases=20to=20the=20document,=20pin?= =?UTF-8?q?=20release?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Leases no longer drop on did-start-navigation. Chromium reports it before the throttle where the app window blocks every will-navigate, so a blocked stray link click dropped every lease of a page that never changed, and an open debug panel went stale (review B's suspicion). The preload mints one id per loaded document; a lease from a new id drops the old document's leases, order-independently. Only destroy drops otherwise. - Pins round-1 survivors: a release for a session the owner does not hold cannot cancel another owner's lease (J), the release handler acts for its caller (A), and a lease on a session with no screen sends no empty frame (E). Co-Authored-By: Claude Opus 5.5 --- docs/plans/2026-09-25-screen-ipc-interest.md | 4 +- src/main/ipc/session.test.ts | 30 +++++++--- src/main/ipc/session.ts | 25 +++------ src/main/sessions/forwarder.test.ts | 4 +- src/main/sessions/screenInterest.test.ts | 56 ++++++++++++++----- src/main/sessions/screenInterest.ts | 29 ++++++++-- .../window/sessionRoutingComposition.test.ts | 2 +- src/preload/api/session.ts | 10 +++- 8 files changed, 110 insertions(+), 50 deletions(-) diff --git a/docs/plans/2026-09-25-screen-ipc-interest.md b/docs/plans/2026-09-25-screen-ipc-interest.md index 62db0df41..9a7d1b1a7 100644 --- a/docs/plans/2026-09-25-screen-ipc-interest.md +++ b/docs/plans/2026-09-25-screen-ipc-interest.md @@ -18,7 +18,7 @@ All consumers were mapped (file:line in PR #1236's description): 2. **Lease contract:** - `session:screen-lease` acquires a lease and seeds the current screen down the ordinary `session:screen` path, the same seed `session:recover` sends. An idle backend therefore still shows a correct panel. - `session:screen-release` releases a lease. - - Leases are owned by the calling webContents. A main-frame navigation (reload) or destruction drops all of that owner's leases, because a dying renderer never runs its cleanup. A release the owner does not hold is ignored. + - Leases are owned by the calling webContents and the document that took them (a per-load id minted in preload). A lease from a new document (a reload), or destruction, drops all of that owner's earlier leases, because a dying renderer never runs its cleanup. Review amendment: this replaced dropping on `did-start-navigation`, which Chromium fires before the throttle where this app blocks every `will-navigate`, so a blocked link click used to drop live leases. A release the owner does not hold is ignored. 3. **Debug bundles** read `session:get-screen-debug`: main's latest raw snapshot plus the tail history. 4. **Renderer:** - The screen handler applies only the screen strings. @@ -32,7 +32,7 @@ All consumers were mapped (file:line in PR #1236's description): ## Tests (fail-first where the behaviour changes) - **Forwarder:** the recorded Claude 2.1.278 screen frame is not forwarded without a lease, is forwarded with one, and is recorded in the tail history either way. The first assertion is red on origin/main. -- **Leases:** the IPC seed on acquire; dropping on a main-frame reload and on destroy, but not on a sub-frame navigation; counting across owners. +- **Leases:** the IPC seed on acquire; dropping when a new document leases and on destroy, but never on a navigation event; counting across owners. - **Picker:** a screen frame carrying a picker no longer sets `runtime.picker`. - **Mutations:** removing the gate, recording only leased frames, dropping on sub-frames, removing the seed, and restoring the picker write each fail a test. diff --git a/src/main/ipc/session.test.ts b/src/main/ipc/session.test.ts index 845e4ca46..502861259 100644 --- a/src/main/ipc/session.test.ts +++ b/src/main/ipc/session.test.ts @@ -151,7 +151,7 @@ describe('session input transcript observations', () => { }) describe('screen leases (#762)', () => { - it('seeds the current screen on acquire, and drops a renderer\'s leases when it reloads or dies', async () => { + it('seeds the current screen on acquire, and drops a renderer\'s leases only when its document is replaced or it dies', async () => { const { screenInterest } = await import('@main/sessions/screenInterest.js') const manager = new EventEmitter() Object.assign(manager, { @@ -164,25 +164,39 @@ describe('screen leases (#762)', () => { // An opening debug panel is right at once, even for an idle backend: // the current screen goes down the ordinary session:screen path. - lease({ sender }, 'pane') + lease({ sender }, 'pane', 'doc-1') expect(screenInterest.wants('pane')).toBe(true) // The same aliased wire payload the recover seed sends (recent/markdown // equal to plain/markdown are dropped on the wire, #746). expect(harness.routed).toHaveBeenCalledWith('pane', 'session:screen', { sessionId: 'pane', plain: 'now', markdown: 'now' }) - // A reload never runs the renderer's cleanup. A sub-frame navigation and a - // same-document (hash/history) navigation are not reloads and keep it. - sender.emit('did-start-navigation', { isMainFrame: false, isSameDocument: false }) + // Any navigation, including one the window blocks, leaves the page and + // its leases alone: only a new document's lease or death drops them. + sender.emit('did-start-navigation', { isMainFrame: true, isSameDocument: false }) expect(screenInterest.wants('pane')).toBe(true) - sender.emit('did-start-navigation', { isMainFrame: true, isSameDocument: true }) + // The release is the caller's own (another webContents cannot end it). + const release = harness.handlers.get('session:screen-release')! + release({ sender: { id: 1 } }, 'pane', 'doc-1') expect(screenInterest.wants('pane')).toBe(true) - sender.emit('did-start-navigation', { isMainFrame: true, isSameDocument: false }) + release({ sender }, 'pane', 'doc-1') expect(screenInterest.wants('pane')).toBe(false) - lease({ sender }, 'pane') + // A reload: the new document's first lease drops what the old one held. + lease({ sender }, 'old-page', 'doc-1') + lease({ sender }, 'pane', 'doc-2') + expect(screenInterest.wants('old-page')).toBe(false) sender.emit('destroyed') expect(screenInterest.wants('pane')).toBe(false) }) + + it('leases a session with no screen yet without sending an empty frame', async () => { + const manager = new EventEmitter() + Object.assign(manager, { getScreenSnapshot: () => null }) + registerSessionIpc(manager as never, {} as never, new SessionFeedTap(manager as never)) + harness.routed.mockClear() + harness.handlers.get('session:screen-lease')!({ sender: Object.assign(new EventEmitter(), { id: 77 }) }, 'fresh', 'doc') + expect(harness.routed).not.toHaveBeenCalledWith('fresh', 'session:screen', expect.anything()) + }) }) describe('session:get-screen-debug (#762)', () => { diff --git a/src/main/ipc/session.ts b/src/main/ipc/session.ts index 876f48370..541b636e6 100644 --- a/src/main/ipc/session.ts +++ b/src/main/ipc/session.ts @@ -258,34 +258,27 @@ export function registerSessionIpc( // the ordinary `session:screen` path, the same seed `session:recover` sends, // so an opening debug panel is correct at once even for an idle backend that // emits no further frame, and the renderer needs no second way to apply a - // screen. Leases are owned by the calling webContents and - // dropped when it navigates (reload) or is destroyed, because a renderer - // that dies never runs its cleanup. + // screen. Leases are owned by the calling webContents and document (see + // sessions/screenInterest.ts for why not a navigation event), and dropped + // when the webContents is destroyed, because a renderer that dies never + // runs its cleanup. const leaseOwnersWatched = new Set() - ipcMain.handle('session:screen-lease', (evt, sessionId: string): void => { + ipcMain.handle('session:screen-lease', (evt, sessionId: string, document: string): void => { const sender = evt.sender const owner = sender.id if (!leaseOwnersWatched.has(owner)) { leaseOwnersWatched.add(owner) - const drop = () => screenInterest.dropOwner(owner) - // A reload is a main-frame navigation to a new document. Same-document - // hash/history navigation keeps the JavaScript context and its live - // leases, as workflows.ts's identical per-renderer lease contract - // already rules (#1236 review C). - sender.on('did-start-navigation', details => { - if (details.isMainFrame && !details.isSameDocument) drop() - }) sender.once('destroyed', () => { - drop() + screenInterest.dropOwner(owner) leaseOwnersWatched.delete(owner) }) } - screenInterest.acquire(owner, sessionId) + screenInterest.acquire(owner, sessionId, document) const screen = manager.getScreenSnapshot(sessionId) if (screen) sendToSessionWindow(sessionId, 'session:screen', aliasScreenSnapshotForWire({ sessionId, ...screen })) }) - ipcMain.handle('session:screen-release', (evt, sessionId: string) => { - screenInterest.release(evt.sender.id, sessionId) + ipcMain.handle('session:screen-release', (evt, sessionId: string, document: string) => { + screenInterest.release(evt.sender.id, sessionId, document) }) // Debug bundles read the screen on demand instead of holding a lease: the // latest raw snapshot plus the tail history main records from every frame. diff --git a/src/main/sessions/forwarder.test.ts b/src/main/sessions/forwarder.test.ts index 413a3b27c..6b2134f48 100644 --- a/src/main/sessions/forwarder.test.ts +++ b/src/main/sessions/forwarder.test.ts @@ -131,12 +131,12 @@ it('forwards screen frames only to a leased session, and records every frame for forwarder.flush() expect(sent).toEqual([]) - screenInterest.acquire(7, 'pane') + screenInterest.acquire(7, 'pane', 'doc') manager.emit('screen', frame(drafted)) forwarder.flush() expect(sent).toEqual([drafted.slice(-200)]) - screenInterest.release(7, 'pane') + screenInterest.release(7, 'pane', 'doc') manager.emit('screen', frame(idle)) forwarder.flush() expect(sent).toEqual([drafted.slice(-200)]) diff --git a/src/main/sessions/screenInterest.test.ts b/src/main/sessions/screenInterest.test.ts index 2c3dc397b..cf1b27732 100644 --- a/src/main/sessions/screenInterest.test.ts +++ b/src/main/sessions/screenInterest.test.ts @@ -8,34 +8,62 @@ import { ScreenInterest } from './screenInterest.js' describe('ScreenInterest', () => { it('wants a session while any lease on it is held, across owners', () => { const interest = new ScreenInterest() - interest.acquire(1, 's') - interest.acquire(2, 's') - interest.release(1, 's') + interest.acquire(1, 's', 'doc') + interest.acquire(2, 's', 'doc') + interest.release(1, 's', 'doc') expect(interest.wants('s')).toBe(true) - interest.release(2, 's') + interest.release(2, 's', 'doc') expect(interest.wants('s')).toBe(false) }) it('ignores a release its owner does not hold, so it cannot cancel another owner', () => { const interest = new ScreenInterest() - interest.acquire(1, 's') - interest.release(2, 's') - interest.release(1, 's') - interest.release(1, 's') + interest.acquire(1, 's', 'doc') + interest.release(2, 's', 'doc') + interest.release(1, 's', 'doc') + interest.release(1, 's', 'doc') expect(interest.wants('s')).toBe(false) - interest.acquire(2, 's') - interest.release(1, 's') + interest.acquire(2, 's', 'doc') + interest.release(1, 's', 'doc') expect(interest.wants('s')).toBe(true) }) + it('never lets a release for a session the owner does not hold cancel another owner', () => { + // Owner 1 holds 'a' only. Its release of 'b' must not go negative on its + // own books and take owner 2's 'b' lease with it. + const interest = new ScreenInterest() + interest.acquire(1, 'a', 'doc') + interest.acquire(2, 'b', 'doc') + interest.release(1, 'b', 'doc') + expect(interest.wants('b')).toBe(true) + expect(interest.wants('a')).toBe(true) + }) + + it('drops a webContents\' leases when a new document takes one, and only then', () => { + // A reload never runs the old page's cleanup; the new page's first lease + // proves the old document is gone. A blocked navigation keeps the page and + // its document id, so nothing is dropped (#1236 review). + const interest = new ScreenInterest() + interest.acquire(1, 'old-only', 'first-load') + interest.acquire(1, 'shared', 'first-load') + interest.acquire(1, 'shared', 'reloaded') + expect(interest.wants('old-only')).toBe(false) + expect(interest.wants('shared')).toBe(true) + // The dead document's late release cannot touch the new one's lease. + interest.release(1, 'shared', 'first-load') + expect(interest.wants('shared')).toBe(true) + interest.release(1, 'shared', 'reloaded') + expect(interest.wants('shared')).toBe(false) + }) + it('drops everything an owner held when it goes away, and only that', () => { // A renderer that reloads or crashes never releases; main drops its // leases instead (ipc/session.ts), and another window's must survive. const interest = new ScreenInterest() - interest.acquire(1, 'a') - interest.acquire(1, 'a') - interest.acquire(1, 'b') - interest.acquire(2, 'b') + interest.acquire(1, 'a', 'doc') + interest.acquire(1, 'a', 'doc') + interest.acquire(1, 'b', 'doc') + interest.acquire(2, 'b', 'doc') interest.dropOwner(1) expect(interest.wants('a')).toBe(false) expect(interest.wants('b')).toBe(true) diff --git a/src/main/sessions/screenInterest.ts b/src/main/sessions/screenInterest.ts index 3ccdd7f7c..b369f15d0 100644 --- a/src/main/sessions/screenInterest.ts +++ b/src/main/sessions/screenInterest.ts @@ -21,15 +21,30 @@ import { ScreenTailHistory } from '@shared/debug/screenTail.js' // WHY leases are owned by a webContents and not just counted: a renderer that // reloads or crashes never runs its cleanup, and a bare counter would then // forward that session's frames forever. Dropping an owner's leases when its -// webContents navigates or is destroyed keeps the count honest. (A leaked -// lease would only cost bytes, never correctness, but the whole point here is -// the bytes.) +// webContents is destroyed, or when a NEW document in it starts taking leases, +// keeps the count honest. (A leaked lease would only cost bytes, never +// correctness, but the whole point here is the bytes.) +// +// WHY a document id and not a navigation event (#1236 review): the app window +// blocks every main-frame `will-navigate` (appWindow.ts), and Chromium reports +// `did-start-navigation` BEFORE the throttle that emits and blocks +// `will-navigate`. Dropping on navigation start therefore also dropped every +// lease when a stray link click was blocked and the page never changed, so an +// open debug panel silently went stale. The preload mints one id per loaded +// document and sends it with every lease; a lease from a different id proves +// the old document is gone, whatever event order Electron uses. The cost: after +// a reload, the dead document's leases survive until the new one takes its +// first lease or the window closes, which is debug-surface bytes only. export class ScreenInterest { private readonly byOwner = new Map>() private readonly totals = new Map() + private readonly documents = new Map() - acquire(owner: number, sessionId: string): void { + acquire(owner: number, sessionId: string, document: string): void { + const current = this.documents.get(owner) + if (current !== undefined && current !== document) this.dropOwner(owner) + this.documents.set(owner, document) let leases = this.byOwner.get(owner) if (!leases) { leases = new Map() @@ -39,7 +54,10 @@ export class ScreenInterest { this.totals.set(sessionId, (this.totals.get(sessionId) ?? 0) + 1) } - release(owner: number, sessionId: string): void { + release(owner: number, sessionId: string, document: string): void { + // A release from a document that no longer owns the leases has nothing + // of its own to release. + if (this.documents.get(owner) !== document) return const leases = this.byOwner.get(owner) const held = leases?.get(sessionId) ?? 0 // A release the owner does not hold (double release, or one that raced @@ -52,6 +70,7 @@ export class ScreenInterest { } dropOwner(owner: number): void { + this.documents.delete(owner) const leases = this.byOwner.get(owner) if (!leases) return this.byOwner.delete(owner) diff --git a/src/main/window/sessionRoutingComposition.test.ts b/src/main/window/sessionRoutingComposition.test.ts index 9ba7a1600..c4347dd67 100644 --- a/src/main/window/sessionRoutingComposition.test.ts +++ b/src/main/window/sessionRoutingComposition.test.ts @@ -51,7 +51,7 @@ let forwarder: ReturnType // wrong reason. The lease owner id is not a real webContents. const TEST_SCREEN_OWNER = -1 beforeEach(() => { - for (const sessionId of ['pane', 'refused-close', 'admission-pane']) screenInterest.acquire(TEST_SCREEN_OWNER, sessionId) + for (const sessionId of ['pane', 'refused-close', 'admission-pane']) screenInterest.acquire(TEST_SCREEN_OWNER, sessionId, 'test-document') registry.resetWindowRegistryForTests() harness.handlers.clear() harness.built.length = 0 diff --git a/src/preload/api/session.ts b/src/preload/api/session.ts index dcfb56ad4..7c5f9da8e 100644 --- a/src/preload/api/session.ts +++ b/src/preload/api/session.ts @@ -9,6 +9,7 @@ import type { AgentScreenSnapshot, AgentScreenSnapshotWire } from '@shared/types import type { ScreenTailSample } from '@shared/debug/screenTail.js' import type { TerminalForegroundEvent, TerminalForegroundState } from '@shared/types/terminalForeground.js' import type { + SessionExitEvent, SessionHistoryChunk, SessionKind, @@ -39,6 +40,11 @@ import type { SessionProviderSessionChangedEvent, } from '@preload/api/types.js' +// One id per loaded document: preload runs again on every reload, so a new id +// tells main the previous document's screen leases are dead (#762, see +// main/sessions/screenInterest.ts for why this replaced a navigation event). +const screenLeaseDocument = globalThis.crypto.randomUUID() + type SessionScreenWireEvent = Omit & AgentScreenSnapshotWire // Session lifecycle + I/O bridge methods. @@ -134,10 +140,10 @@ export const sessionApi = { // held (debug surfaces). Acquire also sends the current screen as one // ordinary session:screen event. acquireScreenLease: (sessionId: string): Promise => - ipcRenderer.invoke('session:screen-lease', sessionId), + ipcRenderer.invoke('session:screen-lease', sessionId, screenLeaseDocument), releaseScreenLease: (sessionId: string): Promise => - ipcRenderer.invoke('session:screen-release', sessionId), + ipcRenderer.invoke('session:screen-release', sessionId, screenLeaseDocument), /** The latest screen and main's screen-tail history, for debug bundles. */ getScreenDebug: (sessionId: string): Promise<{ screen: AgentScreenSnapshot | null; samples: ScreenTailSample[] }> => From 21801af438b982af87894d3b14f4d27af802fd9f Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Fri, 25 Sep 2026 03:38:47 -0700 Subject: [PATCH 5/5] fix(ipc): retire a reloaded page's screen leases at load, not at next lease Steering q15: with leases keyed to the document, a reloaded page that never opened a debug panel took no lease, so the old page's leases kept the heaviest IPC stream forwarding until the window closed. The preload now announces its document id on every load (session:screen-document), which retires an earlier document's leases at once. Blocked navigations still drop nothing, and a dead page's late release still cannot touch the new page's lease. Messages from one renderer arrive in order, so the announcement lands after the old page's last message and before the new page's first lease. Co-Authored-By: Claude Opus 5.5 --- docs/plans/2026-09-25-screen-ipc-interest.md | 2 +- src/main/ipc/session.test.ts | 17 ++++++++++++++--- src/main/ipc/session.ts | 13 ++++++++++--- src/main/sessions/screenInterest.ts | 18 +++++++++++++----- src/preload/api/session.ts | 3 +++ 5 files changed, 41 insertions(+), 12 deletions(-) diff --git a/docs/plans/2026-09-25-screen-ipc-interest.md b/docs/plans/2026-09-25-screen-ipc-interest.md index 9a7d1b1a7..84290d600 100644 --- a/docs/plans/2026-09-25-screen-ipc-interest.md +++ b/docs/plans/2026-09-25-screen-ipc-interest.md @@ -18,7 +18,7 @@ All consumers were mapped (file:line in PR #1236's description): 2. **Lease contract:** - `session:screen-lease` acquires a lease and seeds the current screen down the ordinary `session:screen` path, the same seed `session:recover` sends. An idle backend therefore still shows a correct panel. - `session:screen-release` releases a lease. - - Leases are owned by the calling webContents and the document that took them (a per-load id minted in preload). A lease from a new document (a reload), or destruction, drops all of that owner's earlier leases, because a dying renderer never runs its cleanup. Review amendment: this replaced dropping on `did-start-navigation`, which Chromium fires before the throttle where this app blocks every `will-navigate`, so a blocked link click used to drop live leases. A release the owner does not hold is ignored. + - Leases are owned by the calling webContents and the document that took them (a per-load id minted in preload). The preload announces its document on every load, leasing or not; a new document (a reload), or destruction, drops all of that owner's earlier leases, because a dying renderer never runs its cleanup. Review amendment: this replaced dropping on `did-start-navigation`, which Chromium fires before the throttle where this app blocks every `will-navigate`, so a blocked link click used to drop live leases. A release the owner does not hold is ignored. 3. **Debug bundles** read `session:get-screen-debug`: main's latest raw snapshot plus the tail history. 4. **Renderer:** - The screen handler applies only the screen strings. diff --git a/src/main/ipc/session.test.ts b/src/main/ipc/session.test.ts index 502861259..9d55e219b 100644 --- a/src/main/ipc/session.test.ts +++ b/src/main/ipc/session.test.ts @@ -12,7 +12,9 @@ vi.mock('electron', () => ({ }, }, ipcRenderer: { - invoke: (channel: string, ...args: unknown[]) => harness.handlers.get(channel)!({}, ...args), + // Asynchronous like the real one: the preload's load-time document + // announcement runs at import, before any handler is registered. + invoke: async (channel: string, ...args: unknown[]) => harness.handlers.get(channel)?.({}, ...args), }, })) @@ -181,10 +183,19 @@ describe('screen leases (#762)', () => { release({ sender }, 'pane', 'doc-1') expect(screenInterest.wants('pane')).toBe(false) - // A reload: the new document's first lease drops what the old one held. + // A real reload whose new page never opens a debug panel: its preload's + // load-time announcement alone retires the old page's leases, or the + // heaviest IPC stream would forward forever (steering q15). + const announce = harness.handlers.get('session:screen-document')! lease({ sender }, 'old-page', 'doc-1') - lease({ sender }, 'pane', 'doc-2') + announce({ sender }, 'doc-2') expect(screenInterest.wants('old-page')).toBe(false) + // Re-announcing the live document (nothing reloaded) keeps its leases, + // and the dead page's late release cannot touch them. + lease({ sender }, 'pane', 'doc-2') + announce({ sender }, 'doc-2') + release({ sender }, 'pane', 'doc-1') + expect(screenInterest.wants('pane')).toBe(true) sender.emit('destroyed') expect(screenInterest.wants('pane')).toBe(false) }) diff --git a/src/main/ipc/session.ts b/src/main/ipc/session.ts index 541b636e6..6a514dabc 100644 --- a/src/main/ipc/session.ts +++ b/src/main/ipc/session.ts @@ -263,8 +263,7 @@ export function registerSessionIpc( // when the webContents is destroyed, because a renderer that dies never // runs its cleanup. const leaseOwnersWatched = new Set() - ipcMain.handle('session:screen-lease', (evt, sessionId: string, document: string): void => { - const sender = evt.sender + const watchLeaseOwner = (sender: Electron.WebContents): number => { const owner = sender.id if (!leaseOwnersWatched.has(owner)) { leaseOwnersWatched.add(owner) @@ -273,7 +272,15 @@ export function registerSessionIpc( leaseOwnersWatched.delete(owner) }) } - screenInterest.acquire(owner, sessionId, document) + return owner + } + // Sent by the preload on every page load, whether or not the page ever + // leases: it is what retires a reloaded page's leases (steering q15). + ipcMain.handle('session:screen-document', (evt, document: string): void => { + screenInterest.enterDocument(watchLeaseOwner(evt.sender), document) + }) + ipcMain.handle('session:screen-lease', (evt, sessionId: string, document: string): void => { + screenInterest.acquire(watchLeaseOwner(evt.sender), sessionId, document) const screen = manager.getScreenSnapshot(sessionId) if (screen) sendToSessionWindow(sessionId, 'session:screen', aliasScreenSnapshotForWire({ sessionId, ...screen })) }) diff --git a/src/main/sessions/screenInterest.ts b/src/main/sessions/screenInterest.ts index b369f15d0..c62abaa48 100644 --- a/src/main/sessions/screenInterest.ts +++ b/src/main/sessions/screenInterest.ts @@ -31,20 +31,28 @@ import { ScreenTailHistory } from '@shared/debug/screenTail.js' // `will-navigate`. Dropping on navigation start therefore also dropped every // lease when a stray link click was blocked and the page never changed, so an // open debug panel silently went stale. The preload mints one id per loaded -// document and sends it with every lease; a lease from a different id proves -// the old document is gone, whatever event order Electron uses. The cost: after -// a reload, the dead document's leases survive until the new one takes its -// first lease or the window closes, which is debug-surface bytes only. +// document, ANNOUNCES it as soon as it loads (enterDocument), and sends it +// with every lease. A different id proves the old document is gone, whatever +// event order Electron uses; messages from one renderer reach main in order, +// so the announcement lands after the old page's last message and before the +// new page's first lease. The announcement matters as much as the lease id: a +// reloaded page that never opens a debug panel would otherwise leave the old +// page's leases forwarding the heaviest IPC stream forever (steering q15). export class ScreenInterest { private readonly byOwner = new Map>() private readonly totals = new Map() private readonly documents = new Map() - acquire(owner: number, sessionId: string, document: string): void { + /** A document loaded in `owner`; everything an earlier document held is dead. */ + enterDocument(owner: number, document: string): void { const current = this.documents.get(owner) if (current !== undefined && current !== document) this.dropOwner(owner) this.documents.set(owner, document) + } + + acquire(owner: number, sessionId: string, document: string): void { + this.enterDocument(owner, document) let leases = this.byOwner.get(owner) if (!leases) { leases = new Map() diff --git a/src/preload/api/session.ts b/src/preload/api/session.ts index 7c5f9da8e..cdee73c95 100644 --- a/src/preload/api/session.ts +++ b/src/preload/api/session.ts @@ -44,6 +44,9 @@ import type { // tells main the previous document's screen leases are dead (#762, see // main/sessions/screenInterest.ts for why this replaced a navigation event). const screenLeaseDocument = globalThis.crypto.randomUUID() +// Announce the document at load, not at first lease: a reloaded page that +// never leases must still retire the previous page's leases (steering q15). +void ipcRenderer.invoke('session:screen-document', screenLeaseDocument).catch(() => undefined) type SessionScreenWireEvent = Omit & AgentScreenSnapshotWire