From f169cd1cdeb208dab83b385fbaf06ae46af882bd Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Sat, 5 Sep 2026 05:35:49 -0400 Subject: [PATCH] fix(pty): require a settled writer on every PTY provider The production controller never installed `writeWithSettlement`, so every mailbox pointer write took the fire-and-forget fallback and provider settlement never controlled the pointer transition (R3-C2). Making the method required is what turns that into a compile error, so `IPtyProvider.writeWithSettlement` is now non-optional and `LocalPtyProvider` implements it synchronously: in-process node-pty is its own sole owner, so its answer is the settlement. Required-ness alone is satisfiable by a lie. `degraded-daemon-pty-provider` used to answer it with `provider.write(...) !== false`, which reproduces the bug through the fix, so its conditional is deleted outright and it delegates. `RuntimePtyController.writeWithSettlement` stays optional: 809 `setPtyController` call sites across 164 files construct partial controllers, and requiring it there would be a mechanical edit of all of them for no added fence. Instead the orchestration pointer path demands the narrow settled-writer shape and returns `refused{provider_cannot_settle}` before any effect when it is absent. Two gates, both red against the fabricated handoff: - `registerPtyHandlers` with a real controller and a provider lacking settlement refuses before any byte reaches `provider.write`. - A census pins the five production `IPtyProvider` classes, proves each instance exposes the writer, and reads every declaring body to reject a settlement synthesized from `.write(`. --- .../daemon/degraded-daemon-pty-provider.ts | 7 +- .../pty-controller-ownership-routing.test.ts | 42 +++++++-- src/main/providers/local-pty-provider.ts | 10 +++ src/main/providers/provider-dispatch.test.ts | 2 + src/main/providers/pty-provider-contract.ts | 6 +- .../settled-pty-writer-census.test.ts | 90 +++++++++++++++++++ 6 files changed, 144 insertions(+), 13 deletions(-) create mode 100644 src/main/providers/settled-pty-writer-census.test.ts diff --git a/src/main/daemon/degraded-daemon-pty-provider.ts b/src/main/daemon/degraded-daemon-pty-provider.ts index 4cd907687bd..2b50424ae83 100644 --- a/src/main/daemon/degraded-daemon-pty-provider.ts +++ b/src/main/daemon/degraded-daemon-pty-provider.ts @@ -19,7 +19,7 @@ import { } from './degraded-daemon-session-routing' import { DegradedDaemonFreshSpawnRouter } from './degraded-daemon-fresh-spawn-routing' import { DegradedDaemonOwnerRecovery } from './degraded-daemon-owner-recovery' -import { writeRefused, type WriteSettlement } from '../../shared/pty-write-settlement' +import type { WriteSettlement } from '../../shared/pty-write-settlement' export class DegradedDaemonPtyProvider implements IPtyProvider { readonly isDegraded = true @@ -114,10 +114,7 @@ export class DegradedDaemonPtyProvider implements IPtyProvider { } async writeWithSettlement(id: string, data: string): Promise { - const provider = this.providerFor(id) - return provider.writeWithSettlement - ? await provider.writeWithSettlement(id, data) - : writeRefused('provider_cannot_settle') + return await this.providerFor(id).writeWithSettlement(id, data) } resize(id: string, cols: number, rows: number): void { diff --git a/src/main/ipc/pty-controller-ownership-routing.test.ts b/src/main/ipc/pty-controller-ownership-routing.test.ts index 9fca68cedde..cf61b71c9e5 100644 --- a/src/main/ipc/pty-controller-ownership-routing.test.ts +++ b/src/main/ipc/pty-controller-ownership-routing.test.ts @@ -12,6 +12,15 @@ import { setLocalPtyProvider, unregisterSshPtyProvider } from './pty' +import { + writeRefused, + writeUnverifiable, + type WriteSettlement +} from '../../shared/pty-write-settlement' + +type SettledControllerDouble = { + writeWithSettlement: (id: string, data: string) => WriteSettlement | Promise +} vi.mock('electron', () => import('./pty-ipc-mock-registry').then((m) => m.electronModuleMock())) vi.mock('fs', () => import('./pty-ipc-mock-registry').then((m) => m.fsModuleMock())) @@ -99,17 +108,17 @@ describe('registerPtyHandlers', () => { const ptyId = `ssh:${connectionId}@@remote-pty` const provider = { ...createAgentClaimProvider({}), - writeWithSettlement: vi.fn().mockRejectedValue(new Error('connection lost after write')) + writeWithSettlement: vi + .fn() + .mockResolvedValue(writeUnverifiable('transport_settlement_lost', true)) } registerSshPtyProvider(connectionId, provider as never) setPtyOwnership(ptyId, connectionId) - const controller = registerAgentClaimController() as unknown as { - writeWithSettlement: (id: string, data: string) => Promise - } + const controller = registerAgentClaimController() as unknown as SettledControllerDouble try { expect(controller.writeWithSettlement).toBeTypeOf('function') - await expect(controller.writeWithSettlement(ptyId, 'pointer')).rejects.toThrow( - 'connection lost after write' + await expect(controller.writeWithSettlement(ptyId, 'pointer')).resolves.toEqual( + writeUnverifiable('transport_settlement_lost', true) ) expect(provider.writeWithSettlement).toHaveBeenCalledWith(ptyId, 'pointer') expect(provider.write).not.toHaveBeenCalled() @@ -119,6 +128,27 @@ describe('registerPtyHandlers', () => { } }) + it('refuses a settled write before any bytes when the routed provider cannot settle', async () => { + const connectionId = 'ssh-unsettled' + const ptyId = `ssh:${connectionId}@@remote-pty` + const provider = createAgentClaimProvider({}) as Record + // A provider predating the settled contract, reached through the production registry. + delete provider.writeWithSettlement + registerSshPtyProvider(connectionId, provider as never) + setPtyOwnership(ptyId, connectionId) + const controller = registerAgentClaimController() as unknown as SettledControllerDouble + try { + // Synchronous by construction: the refusal happens before any effect is attempted. + expect(await controller.writeWithSettlement(ptyId, 'pointer')).toEqual( + writeRefused('provider_cannot_settle') + ) + expect(provider.write).not.toHaveBeenCalled() + } finally { + unregisterSshPtyProvider(connectionId) + clearProviderPtyState(ptyId) + } + }) + it('preserves a provider write refusal for callers that gate follow-up input', () => { const provider = createAgentClaimProvider({}) provider.write.mockReturnValue(false) diff --git a/src/main/providers/local-pty-provider.ts b/src/main/providers/local-pty-provider.ts index d08e437add3..f50ad36d34c 100644 --- a/src/main/providers/local-pty-provider.ts +++ b/src/main/providers/local-pty-provider.ts @@ -1,5 +1,10 @@ import type * as pty from 'node-pty' import type { IPtyProvider, PtyProcessInfo, PtySpawnOptions, PtySpawnResult } from './types' +import { + WRITE_ACCEPTED, + writeRefused, + type WriteSettlement +} from '../../shared/pty-write-settlement' import { confirmLocalPtyForegroundProcess, confirmLocalPtyShellForeground, @@ -73,6 +78,11 @@ export class LocalPtyProvider implements IPtyProvider { write(id: string, data: string): boolean { return writeLocalPty(id, data) } + + // In-process node-pty is its own sole owner, so its synchronous answer is the settlement. + writeWithSettlement(id: string, data: string): WriteSettlement { + return writeLocalPty(id, data) ? WRITE_ACCEPTED : writeRefused('provider_refused_write') + } resize(id: string, cols: number, rows: number): void { resizeLocalPty(id, cols, rows) } diff --git a/src/main/providers/provider-dispatch.test.ts b/src/main/providers/provider-dispatch.test.ts index cf2d9979a88..9c40b3850eb 100644 --- a/src/main/providers/provider-dispatch.test.ts +++ b/src/main/providers/provider-dispatch.test.ts @@ -1,3 +1,4 @@ +import { settledWriteStub } from './settled-pty-write-stub' import { describe, expect, it, vi } from 'vitest' import { setPtyHostBindings } from '../ipc/pty-host-bindings' @@ -101,6 +102,7 @@ describe('PTY provider dispatch', () => { spawn: vi.fn().mockResolvedValue({ id }), attach: vi.fn(), write: vi.fn(), + writeWithSettlement: vi.fn(settledWriteStub()), resize: vi.fn(), shutdown: vi.fn(), sendSignal: vi.fn(), diff --git a/src/main/providers/pty-provider-contract.ts b/src/main/providers/pty-provider-contract.ts index 981ffad9a9c..9adaa580e6a 100644 --- a/src/main/providers/pty-provider-contract.ts +++ b/src/main/providers/pty-provider-contract.ts @@ -141,8 +141,10 @@ export type IPtyProvider = { /** Exact provider readback: false only when the provider answered that the PTY is absent. */ probePtyLiveness?: (id: string) => Promise write(id: string, data: string): boolean | void - /** Three-valued settlement for writes whose delivery a durable claim depends on. */ - writeWithSettlement?: (id: string, data: string) => WriteSettlement | Promise + /** Three-valued settlement for writes whose delivery a durable claim depends on. + * Required: a provider that answers this from its own fire-and-forget `write` is + * fabricating a handoff, so every provider must settle or say it cannot. */ + writeWithSettlement: (id: string, data: string) => WriteSettlement | Promise resize(id: string, cols: number, rows: number): void /** * Producer-side flow control: stop/restart reading the underlying PTY so a diff --git a/src/main/providers/settled-pty-writer-census.test.ts b/src/main/providers/settled-pty-writer-census.test.ts new file mode 100644 index 00000000000..845c4422935 --- /dev/null +++ b/src/main/providers/settled-pty-writer-census.test.ts @@ -0,0 +1,90 @@ +import { readFileSync } from 'node:fs' +import { join } from 'node:path' +import { execFileSync } from 'node:child_process' +import { describe, expect, it, vi } from 'vitest' +import { LocalPtyProvider } from './local-pty-provider' +import { SshPtyProvider } from './ssh-pty-provider' +import { createMockMux } from './ssh-pty-provider-mock-multiplexer' +import { DaemonPtyRouter } from '../daemon/daemon-pty-router' +import { DegradedDaemonPtyProvider } from '../daemon/degraded-daemon-pty-provider' +import { DaemonPtyAdapter } from '../daemon/daemon-pty-adapter' + +vi.mock('electron', () => ({ + app: { getPath: vi.fn(() => '/tmp'), isPackaged: false }, + BrowserWindow: { fromId: vi.fn(() => null) }, + ipcMain: { on: vi.fn(), removeListener: vi.fn() }, + webContents: { fromId: vi.fn(() => null) } +})) + +const REPO_ROOT = join(__dirname, '..', '..', '..') + +/** + * Requiring the method is satisfiable by a lie: the degraded daemon router used to answer it + * with `provider.write(...) !== false`, reproducing the fire-and-forget bug through the fix. + * The census pins the producers and reads their bodies, so a new provider or a revived + * fabricated handoff fails here rather than silently clearing a mailbox reservation. + */ +const SETTLED_PTY_WRITER_FILES = [ + 'src/main/providers/local-pty-provider.ts', + 'src/main/providers/ssh-pty-provider.ts', + 'src/main/daemon/daemon-pty-router.ts', + 'src/main/daemon/degraded-daemon-pty-provider.ts', + 'src/main/daemon/daemon-pty-adapter.ts' +] + +/** Where the provider-side settlement is actually decided; the adapter inherits its own. */ +const SETTLED_WRITER_DECLARATIONS = [ + 'src/main/providers/local-pty-provider.ts', + 'src/main/providers/ssh-pty-provider.ts', + 'src/main/providers/ssh-pty-provider-rpc-operations.ts', + 'src/main/daemon/daemon-pty-router.ts', + 'src/main/daemon/degraded-daemon-pty-provider.ts', + 'src/main/daemon/daemon-pty-session-input.ts' +] + +function declaredProviderFiles(): string[] { + const output = execFileSync('git', ['grep', '-l', '--', 'implements IPtyProvider', 'src/main'], { + cwd: REPO_ROOT, + encoding: 'utf8' + }) + return output.split('\n').filter(Boolean).sort() +} + +function settledWriterBody(file: string): string { + const source = readFileSync(join(REPO_ROOT, file), 'utf8') + const start = source.indexOf('writeWithSettlement') + expect(start, `${file} declares no settled writer`).toBeGreaterThan(-1) + const end = source.indexOf('\n }', start) + return source.slice(start, end === -1 ? source.length : end) +} + +describe('settled PTY writer census', () => { + it('covers every production provider class that declares IPtyProvider', () => { + expect(declaredProviderFiles()).toEqual([...SETTLED_PTY_WRITER_FILES].sort()) + }) + + it('exposes a settled writer on every production provider instance', () => { + const daemonClient = { isConnected: () => false, onEvent: vi.fn(() => vi.fn()) } + const adapter = new DaemonPtyAdapter(daemonClient as never) + const instances = [ + new LocalPtyProvider({} as never), + new SshPtyProvider('conn-census', createMockMux() as never), + new DaemonPtyRouter({ current: adapter, legacy: [] }), + new DegradedDaemonPtyProvider({ + current: adapter, + legacy: [], + fallback: new LocalPtyProvider({} as never) + }), + adapter + ] + for (const provider of instances) { + expect(typeof provider.writeWithSettlement, provider.constructor.name).toBe('function') + } + }) + + it('never synthesizes a settlement from the fire-and-forget write', () => { + for (const file of SETTLED_WRITER_DECLARATIONS) { + expect(settledWriterBody(file), file).not.toMatch(/\.write\(/) + } + }) +})