From 95c431f5c356d7545aec7f422c43514831b46ebf Mon Sep 17 00:00:00 2001 From: OrcaWin Date: Sun, 2 Aug 2026 18:00:20 -0700 Subject: [PATCH 001/239] fix(orchestration): worker-start launches the configured agent CLI, not the raw agent id (#12148) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Worker-start passed the Orca agent id straight to the shell as the worker terminal command, so `--agent cursor` ran `cursor` — which on Windows resolves to Cursor IDE's cursor.cmd and opened the desktop app, leaving a blank shell that timed out at agent_readiness. The same gap hit every agent whose CLI binary differs from its id (continue/aug/kiro/qwen-code/mistral-vibe/antigravity/trae/mimo-code/hermes/command-code/claude-agent-teams). Adds TerminalCreateOptions.startupAgent so callers name the agent outright; createTerminal then builds the launch from the TUI agent config (command, agentCmdOverrides, default args/env, preflight trust) instead of sniffing the command string. Also covers repo-less folder workspaces, which previously skipped resolution entirely, and fails loudly instead of spawning a bare shell when an explicit agent cannot resolve. Fixes #11926 --- src/main/runtime/orca-runtime.test.ts | 247 ++++++++++++++++++ src/main/runtime/orca-runtime.ts | 58 +++- ...hestration-federation-agent-launch.test.ts | 89 +++++++ .../rpc/methods/orchestration-federation.ts | 4 +- .../methods/orchestration-worker-topology.ts | 5 +- .../runtime/rpc/methods/orchestration.test.ts | 27 +- 6 files changed, 412 insertions(+), 18 deletions(-) create mode 100644 src/main/runtime/rpc/methods/orchestration-federation-agent-launch.test.ts diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index 390285e0a06..22123b7f136 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -12554,6 +12554,253 @@ describe('OrcaRuntimeService', () => { ) }) + // Why: `cursor` on PATH is the Cursor desktop launcher; only `cursor-agent` is + // the CLI Orca can host (issue #11926). + it('launches the configured agent CLI for a startupAgent id, not the raw id', async () => { + const spawn = vi.fn().mockResolvedValue({ id: 'pty-bg' }) + const runtimeStore = { + ...store, + getSettings: () => ({ + ...store.getSettings(), + disabledTuiAgents: [], + agentCmdOverrides: {}, + agentDefaultArgs: { cursor: '--force' }, + agentDefaultEnv: { cursor: { CURSOR_PROFILE: 'captured' } } + }) + } + const runtime = new OrcaRuntimeService(runtimeStore) + runtime.setPtyController({ + spawn, + write: () => true, + kill: () => true, + getForegroundProcess: async () => null + }) + + await runtime.createTerminal(`path:${TEST_WORKTREE_PATH}`, { + startupAgent: 'cursor', + title: 'worker' + }) + + const spawnCall = spawn.mock.calls[0]?.[0] as + | { command?: string; launchAgent?: string; env?: Record } + | undefined + expect(spawnCall?.command).toBe("cursor-agent '--force'") + expect(spawnCall?.launchAgent).toBe('cursor') + expect(spawnCall?.env).toMatchObject({ CURSOR_PROFILE: 'captured' }) + expect(markCursorWorkspaceTrustedMock).toHaveBeenCalledWith(TEST_WORKTREE_PATH) + }) + + it('resolves a startupAgent to the CLI binary on Windows, where `cursor` is the IDE', async () => { + setPlatform('win32') + const spawn = vi.fn().mockResolvedValue({ id: 'pty-bg' }) + const runtimeStore = { + ...store, + getSettings: () => ({ + ...store.getSettings(), + disabledTuiAgents: [], + terminalWindowsShell: 'cmd.exe', + agentCmdOverrides: {}, + // Why: pin the arg here rather than inherit the shared yolo default, so + // this test tracks Windows quoting and not an unrelated default's value. + agentDefaultArgs: { cursor: '--force' }, + agentDefaultEnv: {} + }) + } + const runtime = new OrcaRuntimeService(runtimeStore) + runtime.setPtyController({ + spawn, + write: () => true, + kill: () => true, + getForegroundProcess: async () => null + }) + + await runtime.createTerminal(`path:${TEST_WORKTREE_PATH}`, { startupAgent: 'cursor' }) + + const spawnCall = spawn.mock.calls[0]?.[0] as { command?: string } | undefined + // Why: assert the cmd.exe double quoting too — a platform-insensitive prefix + // match would pass on any OS and prove nothing about the reported platform. + expect(spawnCall?.command).toBe('cursor-agent "--force"') + }) + + // Why: claude-agent-teams is the only agent whose launcher name varies by + // platform (launchCmdByPlatform), so it is what proves resolution is + // platform-aware rather than a fixed string. + it.each([ + { platform: 'win32' as const, expected: 'orca.cmd claude-teams' }, + { platform: 'linux' as const, expected: 'orca-ide claude-teams' }, + { platform: 'darwin' as const, expected: 'orca claude-teams' } + ])( + 'resolves a startupAgent through the $platform launcher name', + async ({ platform, expected }) => { + setPlatform(platform) + const spawn = vi.fn().mockResolvedValue({ id: 'pty-bg' }) + const runtime = new OrcaRuntimeService({ + ...store, + getSettings: () => ({ + ...store.getSettings(), + disabledTuiAgents: [], + agentCmdOverrides: {}, + agentDefaultArgs: { 'claude-agent-teams': '' }, + agentDefaultEnv: {} + }) + }) + runtime.setPtyController({ + spawn, + write: () => true, + kill: () => true, + getForegroundProcess: async () => null + }) + + await runtime.createTerminal(`path:${TEST_WORKTREE_PATH}`, { + startupAgent: 'claude-agent-teams' + }) + + const spawnCall = spawn.mock.calls[0]?.[0] as { command?: string } | undefined + expect(spawnCall?.command).toBe(expected) + } + ) + + // Why: a user who worked around this bug by pointing the override at their own + // cursor-agent path must keep that override once the id resolves properly. + it('honors an agentCmdOverrides entry for a startupAgent', async () => { + const spawn = vi.fn().mockResolvedValue({ id: 'pty-bg' }) + const runtime = new OrcaRuntimeService({ + ...store, + getSettings: () => ({ + ...store.getSettings(), + disabledTuiAgents: [], + agentCmdOverrides: { cursor: 'cursor-agent --beta' }, + agentDefaultArgs: { cursor: '--force' }, + agentDefaultEnv: {} + }) + }) + runtime.setPtyController({ + spawn, + write: () => true, + kill: () => true, + getForegroundProcess: async () => null + }) + + await runtime.createTerminal(`path:${TEST_WORKTREE_PATH}`, { startupAgent: 'cursor' }) + + const spawnCall = spawn.mock.calls[0]?.[0] as { command?: string } | undefined + expect(spawnCall?.command).toBe("cursor-agent --beta '--force'") + }) + + // Why: with no selector the launch is never resolved, so a dropped startupAgent + // would reach the renderer as a bare shell — the failure this option prevents. + it('rejects a startupAgent create with no workspace selector', async () => { + const runtime = new OrcaRuntimeService({ + ...store, + getSettings: () => ({ + ...store.getSettings(), + disabledTuiAgents: [], + agentCmdOverrides: {} + }) + }) + + await expect( + runtime.createTerminal(undefined, { startupAgent: 'cursor', rendererBacked: true }) + ).rejects.toThrow(/requires a workspace selector/) + }) + + // Why: folder workspaces have no repo, so command sniffing skipped them entirely + // and spawned the bare string; an explicit agent must still resolve. + it('resolves a startupAgent in a repo-less folder workspace', async () => { + const folderPath = await mkdtemp(join(tmpdir(), 'orca-runtime-folder-startup-agent-')) + const spawn = vi.fn().mockResolvedValue({ id: 'pty-bg' }) + const folderWorkspace = makeFolderWorkspace({ folderPath }) + const projectGroup = makeFolderProjectGroup({ parentPath: folderPath }) + const runtime = new OrcaRuntimeService({ + ...createFolderWorkspaceRuntimeStore(folderWorkspace, projectGroup), + getSettings: () => ({ + ...store.getSettings(), + disabledTuiAgents: [], + agentCmdOverrides: {}, + agentDefaultArgs: { cursor: '--force' }, + agentDefaultEnv: {} + }) + } as never) + runtime.setPtyController({ + spawn, + write: () => true, + kill: () => true, + getForegroundProcess: async () => null + }) + + await runtime.createTerminal(`id:${TEST_FOLDER_WORKSPACE_KEY}`, { startupAgent: 'cursor' }) + + const spawnCall = spawn.mock.calls[0]?.[0] as + | { command?: string; launchAgent?: string } + | undefined + expect(spawnCall?.command).toBe("cursor-agent '--force'") + expect(spawnCall?.launchAgent).toBe('cursor') + }) + + // Why: silently returning the caller's opts would spawn a bare shell that can + // only time out at agent readiness — the failure startupAgent exists to stop. + it('rejects a startupAgent create that also supplies its own launch', async () => { + const spawn = vi.fn().mockResolvedValue({ id: 'pty-bg' }) + const runtime = new OrcaRuntimeService({ + ...store, + getSettings: () => ({ + ...store.getSettings(), + disabledTuiAgents: [], + agentCmdOverrides: {} + }) + }) + runtime.setPtyController({ + spawn, + write: () => true, + kill: () => true, + getForegroundProcess: async () => null + }) + + for (const conflicting of [ + { env: { SOME_VAR: 'set' } }, + // Why: a raw command would be silently overwritten by the built launch. + { command: 'cursor-agent --resume' }, + // Why: resume identity paired with a freshly built launch is incoherent. + { resumeProviderSession: { key: 'session_id', id: 'prior-session' } as never }, + { launchAgent: 'cursor' as const }, + { launchConfig: { agentArgs: '', agentEnv: {} } as never }, + { startupCommandDelivery: 'provider' as never }, + { claudeAgentTeamsSourceCommand: 'claude' } + ]) { + await expect( + runtime.createTerminal(`path:${TEST_WORKTREE_PATH}`, { + startupAgent: 'cursor', + ...conflicting + }) + ).rejects.toThrow(/cannot combine/) + } + expect(spawn).not.toHaveBeenCalled() + }) + + it('rejects a startupAgent create for a disabled agent', async () => { + const spawn = vi.fn().mockResolvedValue({ id: 'pty-bg' }) + const runtimeStore = { + ...store, + getSettings: () => ({ + ...store.getSettings(), + disabledTuiAgents: ['cursor' as const], + agentCmdOverrides: {} + }) + } + const runtime = new OrcaRuntimeService(runtimeStore) + runtime.setPtyController({ + spawn, + write: () => true, + kill: () => true, + getForegroundProcess: async () => null + }) + + await expect( + runtime.createTerminal(`path:${TEST_WORKTREE_PATH}`, { startupAgent: 'cursor' }) + ).rejects.toThrow(/disabled/) + expect(spawn).not.toHaveBeenCalled() + }) + it('quotes local Windows bare agent command defaults for cmd.exe terminal creates', async () => { setPlatform('win32') const spawn = vi.fn().mockResolvedValue({ id: 'pty-bg' }) diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 6e255216ecc..dfae2861876 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -1274,6 +1274,10 @@ type TerminalCreateOptions = { resumeProviderSession?: AgentProviderSessionMetadata launchToken?: string launchAgent?: TuiAgent + // Why: agent ids are not shell commands (`cursor` is the Cursor desktop app; its + // CLI is `cursor-agent`). Callers that know the agent name it here instead of + // guessing a command, and the runtime builds the configured launch. + startupAgent?: TuiAgent terminalColorQueryReplies?: TerminalOscColorQueryReplyColors viewMode?: 'terminal' | 'chat' startupCommandDelivery?: WorktreeStartupLaunch['startupCommandDelivery'] @@ -23884,33 +23888,49 @@ export class OrcaRuntimeService { ): Promise { // Why: raw shell commands like `codex exec` must remain user-authored shell. // Only unmanaged, repo-backed, bare agent launches get Settings defaults. - if ( - !opts.command || + const callerSuppliedLaunch = opts.env || opts.launchConfig || opts.launchAgent || opts.startupCommandDelivery || - opts.claudeAgentTeamsSourceCommand || - !workspace.repo || - !this.store - ) { + opts.claudeAgentTeamsSourceCommand + const store = this.store + if (opts.startupAgent) { + // Why: falling through unresolved would spawn a bare shell that can only time + // out waiting for an agent. A caller-supplied launch contradicts the agent: + // `command` would be overwritten, `resumeProviderSession` would pair resume + // identity with a fresh launch. + if (callerSuppliedLaunch || opts.command || opts.resumeProviderSession) { + throw new Error( + `startupAgent ${opts.startupAgent} cannot combine with a caller-supplied launch.` + ) + } + if (!store) { + throw new Error('runtime_unavailable') + } + } else if (callerSuppliedLaunch || !store || !opts.command || !workspace.repo) { return opts } - const settings = this.store.getSettings() + const settings = store.getSettings() const platform = this.getAgentLaunchPlatformForWorkspace(workspace) - const isRemote = repoIsRemote(workspace.repo) + const isRemote = workspace.repo ? repoIsRemote(workspace.repo) : Boolean(workspace.connectionId) const queuedShell = resolveLocalWindowsAgentStartupShell({ platform, isRemote, terminalWindowsShell: settings.terminalWindowsShell }) - const agent = resolveBareAgentLaunchCommand({ - command: opts.command, - settings, - platform, - isRemote - }) + if (opts.startupAgent && !isTuiAgentEnabled(opts.startupAgent, settings.disabledTuiAgents)) { + throw new Error(`Agent ${opts.startupAgent} is disabled. Choose an enabled agent.`) + } + const agent = + opts.startupAgent ?? + resolveBareAgentLaunchCommand({ + command: opts.command, + settings, + platform, + isRemote + }) if (!agent) { return opts } @@ -23927,6 +23947,11 @@ export class OrcaRuntimeService { allowEmptyPromptLaunch: true }) if (!startupPlan) { + // Why: an explicit agent that yields no plan would otherwise spawn a bare + // shell that never reaches agent readiness. + if (opts.startupAgent) { + throw new Error(`Could not build launch command for ${opts.startupAgent}.`) + } return opts } @@ -24327,6 +24352,11 @@ export class OrcaRuntimeService { worktreeSelector?: string, opts: TerminalCreateOptions = {} ): Promise { + if (opts.startupAgent && worktreeSelector === undefined) { + // Why: the launch is resolved against a workspace, so with no selector + // startupAgent is silently dropped and the terminal is a bare shell. + throw new Error(`startupAgent ${opts.startupAgent} requires a workspace selector.`) + } const presentation = resolveTerminalPresentation(opts) const requiresRendererFocus = opts.presentation === 'focused' || opts.focus === true const availableAuthoritativeWindow = this.getAvailableAuthoritativeWindow() diff --git a/src/main/runtime/rpc/methods/orchestration-federation-agent-launch.test.ts b/src/main/runtime/rpc/methods/orchestration-federation-agent-launch.test.ts new file mode 100644 index 00000000000..15b2194140a --- /dev/null +++ b/src/main/runtime/rpc/methods/orchestration-federation-agent-launch.test.ts @@ -0,0 +1,89 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { OrcaRuntimeService } from '../../orca-runtime' +import { OrchestrationDb } from '../../orchestration/db' +import { ORCHESTRATION_METHODS } from './orchestration' + +// Why: a federated worker terminal is created from an agent id. Passing that id +// as a shell command launched Cursor's desktop app instead of `cursor-agent` +// (issue #11926), so the remote path must resolve through the TUI agent config +// exactly like the local one. +describe('federated worker agent launch', () => { + let db: OrchestrationDb | undefined + + afterEach(() => { + db?.close() + vi.restoreAllMocks() + }) + + it('creates the remote worker terminal from the agent id, never as a command', async () => { + db = new OrchestrationDb(':memory:') + const runtime = new OrcaRuntimeService() + runtime.setOrchestrationDb(db) + vi.spyOn(runtime, 'validateOrchestrationAgentLauncher').mockImplementation(() => {}) + vi.spyOn(runtime, 'showManagedWorktree').mockResolvedValue({ + id: 'repo::remote-worktree' + } as never) + const createTerminal = vi.spyOn(runtime, 'createTerminal').mockResolvedValue({ + handle: 'term_remote_worker', + worktreeId: 'repo::remote-worktree', + title: 'worker' + }) + vi.spyOn(runtime, 'waitForTerminal').mockResolvedValue({ + handle: 'term_remote_worker', + condition: 'tui-idle', + satisfied: true, + status: 'running', + exitCode: null + }) + // Why: without a stable pane the handler bails at agent_readiness, so the + // assertions below would pass against a worker that never actually started. + vi.spyOn(runtime, 'getTerminalPaneKey').mockReturnValue('tab_remote:leaf_remote') + vi.spyOn(runtime, 'getTerminalProcessIncarnation').mockReturnValue( + 'runtime_test:term_remote_worker:1' + ) + vi.spyOn(runtime, 'getTerminalOrchestrationCliCommand').mockReturnValue('orca') + vi.spyOn(runtime, 'sendTerminalAgentPrompt').mockResolvedValue({ + handle: 'term_remote_worker', + accepted: true, + bytesWritten: 1 + }) + const method = ORCHESTRATION_METHODS.find( + (candidate) => candidate.name === 'orchestration.federationAttachStart' + ) + if (!method) { + throw new Error('federationAttachStart method is not registered') + } + + const result = (await method.handler( + method.params!.parse({ + dispatchId: 'ctx_remote', + taskId: 'task_remote', + taskSpec: 'remote cursor worker', + protocolVersion: 1, + worktree: 'id:repo::remote-worktree', + agent: 'cursor' + }), + { + runtime, + orchestrationMutation: { + callerFingerprint: 'home_peer', + requestId: 'request_remote', + method: 'orchestration.federationAttachStart', + payloadHash: 'remote_payload' + } + } + )) as { state: string; failedStage?: string; lastError?: string } + + // Why: assert the worker actually reached ready — a spy-only assertion would + // stay green even if every stage after terminal_create regressed. + expect(result).toMatchObject({ state: 'ready' }) + expect(createTerminal).toHaveBeenCalledWith( + 'id:repo::remote-worktree', + expect.objectContaining({ startupAgent: 'cursor' }) + ) + expect(createTerminal).toHaveBeenCalledWith( + 'id:repo::remote-worktree', + expect.not.objectContaining({ command: expect.anything() }) + ) + }) +}) diff --git a/src/main/runtime/rpc/methods/orchestration-federation.ts b/src/main/runtime/rpc/methods/orchestration-federation.ts index a0e5b877b8e..7ee1022b5a7 100644 --- a/src/main/runtime/rpc/methods/orchestration-federation.ts +++ b/src/main/runtime/rpc/methods/orchestration-federation.ts @@ -191,7 +191,9 @@ export const ORCHESTRATION_FEDERATION_ATTACH_METHODS: RpcMethod[] = [ } else { failedStage = 'terminal_create' const terminal = await runtime.createTerminal(`id:${worktree.id}`, { - command: agent, + // Why: agent ids are not shell commands (`cursor` is the desktop app, + // its CLI is `cursor-agent`); resolve through the TUI agent config. + startupAgent: agent as TuiAgent, title: `worker-${params.taskId}`, presentation: 'background' }) diff --git a/src/main/runtime/rpc/methods/orchestration-worker-topology.ts b/src/main/runtime/rpc/methods/orchestration-worker-topology.ts index 474d3cb0316..ffacf571d1b 100644 --- a/src/main/runtime/rpc/methods/orchestration-worker-topology.ts +++ b/src/main/runtime/rpc/methods/orchestration-worker-topology.ts @@ -59,7 +59,10 @@ export async function createExistingWorktreeWorkerTerminal(args: { effects: WorkerEffect[] }): Promise<{ handle: string; warning?: string }> { const terminal = await args.runtime.createTerminal(`id:${args.worktreeId}`, { - command: args.agent, + // Why: the agent id is not a shell command — `cursor` resolves to the Cursor + // desktop app while its CLI is `cursor-agent`. Let the runtime build the + // configured launcher instead of executing the raw id. + startupAgent: args.agent, title: `worker-${args.taskId}`, // Why: dispatching a worker is background work; it must not pull the sidebar // to the worker's workspace while the user is reading somewhere else. diff --git a/src/main/runtime/rpc/methods/orchestration.test.ts b/src/main/runtime/rpc/methods/orchestration.test.ts index cb79b823255..ad663269a37 100644 --- a/src/main/runtime/rpc/methods/orchestration.test.ts +++ b/src/main/runtime/rpc/methods/orchestration.test.ts @@ -2186,7 +2186,7 @@ describe('orchestration RPC methods', () => { // Why: dispatching a worker is background work — surfaceOwner:false adopts // the tab without scrolling the sidebar to the worker's workspace. expect(runtime.createTerminal).toHaveBeenCalledWith('id:repo::worktree', { - command: 'codex', + startupAgent: 'codex', title: `worker-${task.id}`, surfaceOwner: false }) @@ -2196,6 +2196,29 @@ describe('orchestration RPC methods', () => { ) }) + // Why: `cursor` on PATH is the Cursor desktop app; passing the agent id as a + // shell command opened the IDE and left a blank shell (issue #11926). + it('never passes the agent id to the worker terminal as a shell command', async () => { + setup() + mockCurrentWorkerStart() + const task = db.createTask({ spec: 'start a cursor worker' }) + + await call('orchestration.workerStart', { + task: task.id, + from: 'term_coord', + agent: 'cursor' + }) + + expect(runtime.createTerminal).toHaveBeenCalledWith( + 'id:repo::worktree', + expect.objectContaining({ startupAgent: 'cursor' }) + ) + expect(runtime.createTerminal).toHaveBeenCalledWith( + 'id:repo::worktree', + expect.not.objectContaining({ command: expect.anything() }) + ) + }) + it('commits the launched worker token with its durable authority', async () => { setup() mockCurrentWorkerStart() @@ -2289,7 +2312,7 @@ describe('orchestration RPC methods', () => { 'id:repo::other', // Why: starting a worker in an existing worktree must not pull the sidebar // away from whatever the user is looking at. - expect.objectContaining({ command: 'codex', surfaceOwner: false }) + expect.objectContaining({ startupAgent: 'codex', surfaceOwner: false }) ) expect(createWorktree).not.toHaveBeenCalled() }) From 36581651199d595adfedd9c21d1e3e019724b9f6 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sun, 2 Aug 2026 18:38:30 -0700 Subject: [PATCH 002/239] chore(audit): pin the dead-code audit to knip@5.88.1 (#12165) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pnpm dlx knip@5 downloads and runs whatever the newest 5.x is at invocation time, outside lockfile integrity review — flagged P2 by the v1.4.165-rc.0 release scan. Pin the exact version, matching the react-doctor@0.9.1 pattern one line up. Why dlx rather than a devDependency: knip 5.x peer-depends on typescript@^5, and this repo is on typescript 7.0.2 — installed as a devDependency, pnpm resolves knip against TS 7 and knip crashes at module load (verified: same knip against a TS 5 peer runs clean). The dlx sandbox auto-installs knip's own TS 5, which is the environment the original #12077 sweep actually ran in. --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index d2ccfa30283..16bf76e3794 100644 --- a/package.json +++ b/package.json @@ -16,7 +16,7 @@ "audit:code-quality:native": "oxlint --config config/oxlint-code-quality-native-plugins.json src config tests mobile --deny-warnings", "audit:code-quality:type-aware": "oxlint --type-aware --config config/oxlint-code-quality-type-aware.json src config tests --deny-warnings", "audit:react-doctor": "pnpm dlx react-doctor@0.9.1 . --yes --no-supply-chain --no-telemetry --blocking none", - "audit:dead-code": "pnpm dlx knip@5 --config config/knip.json", + "audit:dead-code": "pnpm dlx knip@5.88.1 --config config/knip.json", "check:code-quality:changed": "node config/scripts/check-changed-code-quality.mjs", "check:react-doctor:changed": "node config/scripts/check-react-doctor-changed.mjs", "check:zustand-selector-fanout": "node config/scripts/zustand-selector-fanout-benchmark.mjs --check", From a0008394651a893967d7b945af6d715504c2d26b Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sun, 2 Aug 2026 18:45:55 -0700 Subject: [PATCH 003/239] Add first prompt to agent session history rows (#12085) * Add first user prompt to AI Vault session history rows Re-parse transcripts on demand to extract and display the untruncated first user prompt for copy/reuse. List scans omit the body (payload/perf); UI loads it when session details expand. Grok sessions extract the typed ask from envelope, skipping injected bootstrap rows. Supports Claude, Codex, Grok, and OpenCode agents. * fix(ai-vault): split SessionTime out to pass max-lines lint AiVaultSessionDetails exceeded the 400-line oxlint limit after adding first-prompt UI; move SessionTime into its own module. * fix(ai-vault): handle corrupt transcripts and fix OpenCode prompt captur Corrupt transcripts now resolve null instead of rejecting the IPC call, matching behavior for other unavailable cases. OpenCode SQLite parsing now correctly captures all text parts from the earliest user message only, fixing truncation of large prompts and padding of small ones. Add stale-response guard in the UI to prevent late results from overwriting the current session when tabs switch. Consolidate text slicing via `sliceAtCodeUnitLimit` to avoid surrogate-pair splits across all callers. * test(ai-vault): add first-user-prompt UTF-16 safety tests Ensure truncation at safety limits doesn't split UTF-16 surrogate pairs, preventing corruption of astral characters in captured prompts. * fix(ai-vault): key first-prompt-card by session.id Remounting the card on session switches prevents late responses from a previous load from writing stale data into the component's refs. Also improves conversation-turn key stability. * fix(ai-vault): preserve first prompt after preview truncation * refactor(ai-vault): improve first user prompt capture robustness and per - Add 15s timeout to full-prompt load to prevent indefinite loading states - Extract seedFullFirstUserPrompt helper for reuse across parsers - Prevent AI-generated summaries from becoming the copyable first prompt - Fix truncation detection in OpenCode SQLite by probing for N+1 rows - Optimize text bounding to apply safety limit before toLowerCase - Gate synthetic OpenCode path detection on agent type, not just # presence - Add test coverage for remote execution host handling * Fix FirstPromptCard loading state stranded by stale promise reuse Clears loadPromiseRef during cleanup to prevent the dedupe handle from causing StrictMode remounts to await stale in-flight requests. Stops loading when session becomes non-loadable mid-request. Adds tests for StrictMode double-invoke resolution and main-process timeout scenarios. * refactor(ai-vault): split session parsers into modular files Split secondary-parsers into individual files per agent type (copilot, cursor, hermes, opencode) for improved modularity. Add test coverage for first-user-prompt envelope handling: unwrap user_query tags and reject bare user_info dumps. * fix(ci): clear max-lines and flaky portal readiness check Collapse an accidental multi-line regex wrap in ssh-connection-utils that pushed counted lines to 301. Harden the latched-readiness test's ready transition so CI load can re-observe attach after MutationObserver gaps. * fix(ssh): extract proxy command helpers to pass max-lines Move resolveEffectiveProxy/spawnProxyCommand out of ssh-connection-utils so oxfmt line wrapping cannot push that file over the 300-line lint cap. * capture first user prompt by ordering OpenCode messages by creation time - Add `readOpenCodeMessagesInOrder` to rebuild transcript by timestamp, handling corrupt/partial files gracefully instead of discarding sessions - Extract SSH proxy command tests to dedicated file; add backpressure handling and stderr draining to prevent proxy process stalls - On Windows, reject unsafe characters in ProxyCommand values instead of pretending to escape them; properly format cmd.exe invocation with verbatim arguments - Expand ProxyJump chains into -J plus final hop, mirroring OpenSSH behavior - Decouple portal readiness reapply budget from flip-count budget via explicit constant --- mobile/src/session/ai-vault-resume-launch.ts | 2 +- .../remote-session-scanner-sources.ts | 8 +- src/main/ai-vault/runtime-session-scanner.ts | 1 + .../session-first-user-prompt-read.test.ts | 26 ++ .../session-first-user-prompt-read.ts | 10 +- .../ai-vault/session-scanner-accumulator.ts | 60 ++-- .../ai-vault/session-scanner-agent-parser.ts | 10 +- .../session-scanner-copilot-parser.ts | 126 +++++++ .../ai-vault/session-scanner-cursor-parser.ts | 94 ++++++ .../session-scanner-first-user-prompt.test.ts | 15 + .../session-scanner-first-user-prompt.ts | 8 +- .../ai-vault/session-scanner-hermes-parser.ts | 66 ++++ .../session-scanner-injected-title.test.ts | 13 +- .../session-scanner-opencode-parser.test.ts | 93 ++++++ .../session-scanner-opencode-parser.ts | 126 +++++++ .../session-scanner-opencode-sqlite.ts | 12 +- .../ai-vault/session-scanner-parse-cache.ts | 6 +- ...-scanner-preview-window-truncation.test.ts | 45 +++ .../session-scanner-secondary-parsers.ts | 316 ------------------ src/main/ai-vault/session-scanner-types.ts | 3 + src/main/ssh/ssh-connection-utils.test.ts | 62 +--- src/main/ssh/ssh-connection-utils.ts | 120 +------ src/main/ssh/ssh-connection.test.ts | 13 +- src/main/ssh/ssh-connection.ts | 9 +- src/main/ssh/ssh-proxy-command.test.ts | 189 +++++++++++ src/main/ssh/ssh-proxy-command.ts | 178 ++++++++++ src/relay/git-handler.ts | 4 +- ...ty-portal-readiness-loop.react185.test.tsx | 23 +- .../ai-vault-first-prompt-card.test.tsx | 57 ++++ .../ai-vault-first-prompt-card.tsx | 44 ++- .../ai-vault-session-display.test.ts | 27 ++ .../src/lib/ai-vault-resume-command.ts | 6 +- src/shared/ai-vault-resume-command.test.ts | 2 +- src/shared/ai-vault-resume-command.ts | 199 +++++++++++ src/shared/ai-vault-session-display.ts | 7 + src/shared/ai-vault-types.ts | 198 +---------- 36 files changed, 1406 insertions(+), 772 deletions(-) create mode 100644 src/main/ai-vault/session-scanner-copilot-parser.ts create mode 100644 src/main/ai-vault/session-scanner-cursor-parser.ts create mode 100644 src/main/ai-vault/session-scanner-hermes-parser.ts create mode 100644 src/main/ai-vault/session-scanner-opencode-parser.test.ts create mode 100644 src/main/ai-vault/session-scanner-opencode-parser.ts create mode 100644 src/main/ai-vault/session-scanner-preview-window-truncation.test.ts delete mode 100644 src/main/ai-vault/session-scanner-secondary-parsers.ts create mode 100644 src/main/ssh/ssh-proxy-command.test.ts create mode 100644 src/main/ssh/ssh-proxy-command.ts create mode 100644 src/renderer/src/components/right-sidebar/ai-vault-first-prompt-card.test.tsx create mode 100644 src/shared/ai-vault-resume-command.ts diff --git a/mobile/src/session/ai-vault-resume-launch.ts b/mobile/src/session/ai-vault-resume-launch.ts index bb8a1e07ad9..aa8c1bf98a6 100644 --- a/mobile/src/session/ai-vault-resume-launch.ts +++ b/mobile/src/session/ai-vault-resume-launch.ts @@ -3,7 +3,7 @@ import { buildAiVaultResumeCommand, buildAiVaultResumeShellCommand, realHomeCodexResumeEnvDeletion -} from '../../../src/shared/ai-vault-types' +} from '../../../src/shared/ai-vault-resume-command' import { RESUME_RPC_TIMEOUT_MS } from './ai-vault-resume-preparation' import { isResumableTuiAgent } from '../../../src/shared/agent-session-resume' import type { SleepingAgentLaunchConfig } from '../../../src/shared/agent-session-resume' diff --git a/src/main/ai-vault/remote-session-scanner-sources.ts b/src/main/ai-vault/remote-session-scanner-sources.ts index f68ba32d6c1..5f57fa47c7b 100644 --- a/src/main/ai-vault/remote-session-scanner-sources.ts +++ b/src/main/ai-vault/remote-session-scanner-sources.ts @@ -9,11 +9,9 @@ import { parseDroidSessionContent } from './session-scanner-droid-parser' import { parseMessageGraphSessionContent } from './session-scanner-graph-parsers' import { parseClaudeSessionContent } from './session-scanner-primary-parsers' import { parseGeminiSessionContent } from './session-scanner-gemini-parsers' -import { - parseCopilotSessionContent, - parseCursorSessionContent, - parseHermesSessionContent -} from './session-scanner-secondary-parsers' +import { parseCopilotSessionContent } from './session-scanner-copilot-parser' +import { parseCursorSessionContent } from './session-scanner-cursor-parser' +import { parseHermesSessionContent } from './session-scanner-hermes-parser' import type { FileWithMtime } from './session-scanner-types' import { normalizeAgentSessionsDir } from './session-scanner-values' import { remoteCodexIndexTitles } from './remote-session-scanner-codex-index' diff --git a/src/main/ai-vault/runtime-session-scanner.ts b/src/main/ai-vault/runtime-session-scanner.ts index 014a511cea2..4ead287bb64 100644 --- a/src/main/ai-vault/runtime-session-scanner.ts +++ b/src/main/ai-vault/runtime-session-scanner.ts @@ -76,6 +76,7 @@ const aiVaultListResultSchema = z.object({ totalTokens: z.number(), previewMessages: z.array(aiVaultSessionPreviewMessageSchema), // Optional keeps paired hosts on older builds compatible. + previewMessagesTruncated: z.boolean().optional(), firstUserPrompt: z.string().nullable().optional(), lastUserPrompt: z.string().nullable().optional(), // Default keeps remote hosts running an older build (no recoverable-signal diff --git a/src/main/ai-vault/session-first-user-prompt-read.test.ts b/src/main/ai-vault/session-first-user-prompt-read.test.ts index 5213c49ab3e..b6b8ee4eee5 100644 --- a/src/main/ai-vault/session-first-user-prompt-read.test.ts +++ b/src/main/ai-vault/session-first-user-prompt-read.test.ts @@ -134,4 +134,30 @@ describe('readAiVaultFirstUserPrompt', () => { readAiVaultFirstUserPrompt({ agent: 'grok', filePath: summaryPath }) ).resolves.toEqual({ prompt: null }) }) + + // The web preload fallback and the renderer's canLoadFullFirstPrompt guard both + // rely on remote hosts never reading a local transcript body. + it('returns null for a non-local execution host', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-first-prompt-remote-')) + tempRoots.push(root) + const filePath = join(root, 'session.jsonl') + await writeFile( + filePath, + JSON.stringify({ + type: 'user', + sessionId: 'remote-session', + timestamp: '2026-05-01T10:00:00.000Z', + cwd: '/repo/app', + message: { role: 'user', content: 'Should not be returned' } + }) + ) + + await expect( + readAiVaultFirstUserPrompt({ + agent: 'claude', + filePath, + executionHostId: 'ssh:build-box' + }) + ).resolves.toEqual({ prompt: null }) + }) }) diff --git a/src/main/ai-vault/session-first-user-prompt-read.ts b/src/main/ai-vault/session-first-user-prompt-read.ts index 2023303e4e6..b656494f61c 100644 --- a/src/main/ai-vault/session-first-user-prompt-read.ts +++ b/src/main/ai-vault/session-first-user-prompt-read.ts @@ -104,7 +104,7 @@ async function parseSessionForFullFirstUserPrompt(args: { } } - const file = await fileWithMtimeForPath(args.filePath) + const file = await fileWithMtimeForPath(args.filePath, args.agent) if (!file) { return null } @@ -119,10 +119,14 @@ async function parseSessionForFullFirstUserPrompt(args: { ) } -async function fileWithMtimeForPath(filePath: string): Promise { +async function fileWithMtimeForPath( + filePath: string, + agent: AiVaultAgent +): Promise { // OpenCode SQLite candidates use a synthetic `dbPath#sessionId` path that is // not a real filesystem object; parsers that need it accept the path as-is. - if (filePath.includes('#')) { + // `#` is legal in real filenames, so gate on the agent and the synthetic shape. + if (agent === 'opencode' && splitOpenCodeSqliteCandidate(filePath)) { return { path: filePath, mtimeMs: 0, diff --git a/src/main/ai-vault/session-scanner-accumulator.ts b/src/main/ai-vault/session-scanner-accumulator.ts index 97651c57109..caf08f4259d 100644 --- a/src/main/ai-vault/session-scanner-accumulator.ts +++ b/src/main/ai-vault/session-scanner-accumulator.ts @@ -1,11 +1,11 @@ import { basename, extname } from 'node:path' import { aiVaultAgentLabel, - buildAiVaultResumeCommand, type AiVaultAgent, type AiVaultSession, type AiVaultSessionPreviewMessage } from '../../shared/ai-vault-types' +import { buildAiVaultResumeCommand } from '../../shared/ai-vault-resume-command' import { LOCAL_EXECUTION_HOST_ID, type ExecutionHostId } from '../../shared/execution-host' import type { FileWithMtime, @@ -46,6 +46,7 @@ export function createAccumulator(args: { messageCount: 0, totalTokens: 0, previewMessages: [], + previewMessagesTruncated: false, firstUserPrompt: null, lastUserPrompt: null, queuedMessageCount: 0, @@ -119,6 +120,7 @@ export function finalizeSession( messageCount: accumulator.messageCount, totalTokens: accumulator.totalTokens, previewMessages: accumulator.previewMessages, + ...(accumulator.previewMessagesTruncated ? { previewMessagesTruncated: true } : {}), ...(accumulator.firstUserPrompt ? { firstUserPrompt: accumulator.firstUserPrompt } : {}), ...(accumulator.lastUserPrompt ? { lastUserPrompt: accumulator.lastUserPrompt } : {}), queuedMessageCount: accumulator.queuedMessageCount, @@ -161,6 +163,14 @@ export function addPreviewMessage( seedFirstUserPrompt?: boolean } ): void { + // Seeded before the preview-empty return so the copy body never depends on + // preview-only normalization rules. + seedFullFirstUserPrompt( + accumulator, + args.role, + () => (args.text ? normalizeFullFirstUserPromptText(args.text) : null), + args.seedFirstUserPrompt + ) const text = normalizePreviewText(args.text ?? '') if (!text) { return @@ -172,17 +182,7 @@ export function addPreviewMessage( }) if (accumulator.previewMessages.length > SESSION_PREVIEW_MESSAGE_LIMIT) { accumulator.previewMessages.shift() - } - // Why: list scans never store firstUserPrompt (payload/perf). Only the - // on-demand full-capture path seeds the untruncated copy body. - if ( - args.role === 'user' && - args.seedFirstUserPrompt !== false && - !accumulator.firstUserPrompt && - shouldCaptureFullFirstUserPrompt() && - args.text - ) { - accumulator.firstUserPrompt = normalizeFullFirstUserPromptText(args.text) + accumulator.previewMessagesTruncated = true } } @@ -193,14 +193,12 @@ export function addPreviewContent( timestamp?: unknown, options?: { seedFirstUserPrompt?: boolean } ): void { - if ( - role === 'user' && - options?.seedFirstUserPrompt !== false && - !accumulator.firstUserPrompt && - shouldCaptureFullFirstUserPrompt() - ) { - accumulator.firstUserPrompt = extractFullFirstUserPromptText(content) - } + seedFullFirstUserPrompt( + accumulator, + role, + () => extractFullFirstUserPromptText(content), + options?.seedFirstUserPrompt + ) addPreviewMessage(accumulator, { role, text: extractPreviewContentText(content), @@ -210,6 +208,28 @@ export function addPreviewContent( }) } +/** + * Seed the copyable first prompt from the first real user turn. `fullText` is a + * thunk so list scans (capture mode `none`) never pay the extraction cost. + */ +export function seedFullFirstUserPrompt( + accumulator: SessionAccumulator, + role: AiVaultSessionPreviewMessage['role'], + fullText: () => string | null, + seedFirstUserPrompt?: boolean +): void { + // Why: list scans never store firstUserPrompt (payload/perf). Only the + // on-demand full-capture path seeds the untruncated copy body. + if ( + role === 'user' && + seedFirstUserPrompt !== false && + !accumulator.firstUserPrompt && + shouldCaptureFullFirstUserPrompt() + ) { + accumulator.firstUserPrompt = fullText() + } +} + export function timestampIso(value: unknown): string | null { const parsed = timestampMs(value) return Number.isFinite(parsed) ? new Date(parsed).toISOString() : null diff --git a/src/main/ai-vault/session-scanner-agent-parser.ts b/src/main/ai-vault/session-scanner-agent-parser.ts index bcfb8babdfc..1c2c9e84e8e 100644 --- a/src/main/ai-vault/session-scanner-agent-parser.ts +++ b/src/main/ai-vault/session-scanner-agent-parser.ts @@ -10,12 +10,10 @@ import { parseOpenCodeSqliteSessionViaWorker } from './session-scanner-opencode- import { parseClaudeSessionFile } from './session-scanner-primary-parsers' import { parseGeminiSessionFile } from './session-scanner-gemini-parsers' import { parseCodexSessionFile } from './session-scanner-codex-parser' -import { - parseCopilotSessionFile, - parseCursorSessionFile, - parseHermesSessionFile, - parseOpenCodeSessionFile -} from './session-scanner-secondary-parsers' +import { parseCopilotSessionFile } from './session-scanner-copilot-parser' +import { parseCursorSessionFile } from './session-scanner-cursor-parser' +import { parseHermesSessionFile } from './session-scanner-hermes-parser' +import { parseOpenCodeSessionFile } from './session-scanner-opencode-parser' import type { SessionFileCandidate } from './session-scanner-types' /** diff --git a/src/main/ai-vault/session-scanner-copilot-parser.ts b/src/main/ai-vault/session-scanner-copilot-parser.ts new file mode 100644 index 00000000000..3bb0b4ce1f0 --- /dev/null +++ b/src/main/ai-vault/session-scanner-copilot-parser.ts @@ -0,0 +1,126 @@ +import { createReadStream } from 'node:fs' +import { createInterface } from 'node:readline' +import type { AiVaultSession } from '../../shared/ai-vault-types' +import type { ExecutionHostId } from '../../shared/execution-host' +import type { + FileWithMtime, + ResumableSessionParseState, + SessionAccumulator +} from './session-scanner-types' +import { + accumulatorFoldResumeState, + addPreviewMessage, + createAccumulator, + sessionIdFromFileName, + updateTimeline +} from './session-scanner-accumulator' +import { + asRecord, + copilotModelMetricsTotal, + extractString, + extractTrustedFolder, + normalizeTitleText, + numberValue, + parseJsonObject +} from './session-scanner-values' + +type ParserSessionOptions = { + executionHostId?: ExecutionHostId + executionHostPlatform?: NodeJS.Platform | null +} + +export async function parseCopilotSessionFile( + file: FileWithMtime, + platform: NodeJS.Platform = process.platform +): Promise { + const lines = createInterface({ + input: createReadStream(file.path, { encoding: 'utf-8' }), + crlfDelay: Infinity + }) + return parseCopilotSessionLines({ file, lines, platform }) +} + +export async function parseCopilotSessionContent( + file: FileWithMtime, + content: string, + platform: NodeJS.Platform = process.platform, + options: ParserSessionOptions = {} +): Promise { + return parseCopilotSessionLines({ + file, + lines: content.split(/\r?\n/), + platform, + options + }) +} + +function consumeCopilotRecordLine(accumulator: SessionAccumulator, line: string): void { + const record = parseJsonObject(line) + if (!record) { + return + } + updateTimeline(accumulator, extractString(record.timestamp)) + const data = asRecord(record.data) + if (record.type === 'session.start' && data) { + const sessionId = extractString(data.sessionId) + if (sessionId) { + accumulator.sessionId = sessionId + } + updateTimeline(accumulator, extractString(data.startTime)) + return + } + if (record.type === 'session.model_change' && data) { + accumulator.model = extractString(data.newModel) ?? accumulator.model + return + } + if (record.type === 'session.info' && data) { + accumulator.cwd = extractTrustedFolder(data.message) ?? accumulator.cwd + return + } + if (record.type === 'user.message' && data) { + accumulator.messageCount++ + accumulator.title ??= normalizeTitleText( + extractString(data.transformedContent) ?? extractString(data.content) ?? '' + ) + addPreviewMessage(accumulator, { + role: 'user', + text: extractString(data.transformedContent) ?? extractString(data.content), + timestamp: record.timestamp + }) + return + } + if (record.type === 'assistant.message' && data) { + accumulator.messageCount++ + addPreviewMessage(accumulator, { + role: 'assistant', + text: extractString(data.content), + timestamp: record.timestamp + }) + return + } + if (record.type === 'session.shutdown' && data) { + accumulator.model = extractString(data.currentModel) ?? accumulator.model + accumulator.totalTokens += numberValue(data.currentTokens) + accumulator.totalTokens += copilotModelMetricsTotal(data.modelMetrics) + } +} + +export function createCopilotSessionResumeState(file: FileWithMtime): ResumableSessionParseState { + return accumulatorFoldResumeState( + createAccumulator({ agent: 'copilot', file, sessionId: sessionIdFromFileName(file.path) }), + consumeCopilotRecordLine + ) +} + +async function parseCopilotSessionLines(args: { + file: FileWithMtime + lines: AsyncIterable | Iterable + platform: NodeJS.Platform + options?: ParserSessionOptions +}): Promise { + const state = createCopilotSessionResumeState(args.file) + for await (const line of args.lines) { + state.consumeLine(line) + } + return state.finalize(args.platform, args.options) +} diff --git a/src/main/ai-vault/session-scanner-cursor-parser.ts b/src/main/ai-vault/session-scanner-cursor-parser.ts new file mode 100644 index 00000000000..44274d3b671 --- /dev/null +++ b/src/main/ai-vault/session-scanner-cursor-parser.ts @@ -0,0 +1,94 @@ +import { createReadStream } from 'node:fs' +import { createInterface } from 'node:readline' +import type { AiVaultSession } from '../../shared/ai-vault-types' +import type { ExecutionHostId } from '../../shared/execution-host' +import type { + FileWithMtime, + ResumableSessionParseState, + SessionAccumulator +} from './session-scanner-types' +import { + accumulatorFoldResumeState, + addPreviewContent, + createAccumulator, + sessionIdFromFileName, + updateTimeline +} from './session-scanner-accumulator' +import { + asRecord, + extractContentText, + extractMessageText, + extractString, + parseJsonObject +} from './session-scanner-values' + +type ParserSessionOptions = { + executionHostId?: ExecutionHostId + executionHostPlatform?: NodeJS.Platform | null +} + +export async function parseCursorSessionFile( + file: FileWithMtime, + platform: NodeJS.Platform = process.platform +): Promise { + const lines = createInterface({ + input: createReadStream(file.path, { encoding: 'utf-8' }), + crlfDelay: Infinity + }) + return parseCursorSessionLines({ file, lines, platform }) +} + +export async function parseCursorSessionContent( + file: FileWithMtime, + content: string, + platform: NodeJS.Platform = process.platform, + options: ParserSessionOptions = {} +): Promise { + return parseCursorSessionLines({ + file, + lines: content.split(/\r?\n/), + platform, + options + }) +} + +function consumeCursorRecordLine(accumulator: SessionAccumulator, line: string): void { + const record = parseJsonObject(line) + if (!record) { + return + } + updateTimeline(accumulator, extractString(record.timestamp)) + const role = extractString(record.role) + if (role === 'user' || role === 'assistant') { + accumulator.messageCount++ + if (role === 'user') { + accumulator.title ??= extractMessageText(record.message) ?? extractContentText(record.content) + } + addPreviewContent( + accumulator, + role, + asRecord(record.message)?.content ?? record.content, + record.timestamp + ) + } +} + +export function createCursorSessionResumeState(file: FileWithMtime): ResumableSessionParseState { + return accumulatorFoldResumeState( + createAccumulator({ agent: 'cursor', file, sessionId: sessionIdFromFileName(file.path) }), + consumeCursorRecordLine + ) +} + +async function parseCursorSessionLines(args: { + file: FileWithMtime + lines: AsyncIterable | Iterable + platform: NodeJS.Platform + options?: ParserSessionOptions +}): Promise { + const state = createCursorSessionResumeState(args.file) + for await (const line of args.lines) { + state.consumeLine(line) + } + return state.finalize(args.platform, args.options) +} diff --git a/src/main/ai-vault/session-scanner-first-user-prompt.test.ts b/src/main/ai-vault/session-scanner-first-user-prompt.test.ts index 47235cbce12..6400b5b6488 100644 --- a/src/main/ai-vault/session-scanner-first-user-prompt.test.ts +++ b/src/main/ai-vault/session-scanner-first-user-prompt.test.ts @@ -16,6 +16,21 @@ describe('AI Vault full first-user-prompt normalization', () => { it('keeps a prompt shorter than the safety limit intact', () => { expect(normalizeFullFirstUserPromptText('ship it 😀')).toBe('ship it 😀') }) + + // Why this matters: the bootstrap probe runs on the bounded slice. That is only + // safe because the envelope strip is unbounded, so a query past the cap is still + // unwrapped and never reaches the probe as a bare user_info dump. + it('unwraps a user_query that lands past the safety limit', () => { + const raw = `\n${'context '.repeat(SAFETY_LIMIT / 4)}\nship it` + + expect(normalizeFullFirstUserPromptText(raw)).toBe('ship it') + }) + + it('still rejects a user_info dump with no user_query anywhere', () => { + const raw = `\n${'context '.repeat(SAFETY_LIMIT / 4)}` + + expect(normalizeFullFirstUserPromptText(raw)).toBeNull() + }) }) function hasUnpairedSurrogate(value: string): boolean { diff --git a/src/main/ai-vault/session-scanner-first-user-prompt.ts b/src/main/ai-vault/session-scanner-first-user-prompt.ts index 4228abc9aaa..8751817c577 100644 --- a/src/main/ai-vault/session-scanner-first-user-prompt.ts +++ b/src/main/ai-vault/session-scanner-first-user-prompt.ts @@ -68,12 +68,16 @@ function finalizeFullFirstUserPrompt(value: string): string | null { if (isKnownHarnessInjectedUserTurnText(trimmed)) { return null } + // Bound before scanning so a multi-MB paste cannot force a full lowercase copy. + const bounded = sliceAtCodeUnitLimit(trimmed, FULL_FIRST_USER_PROMPT_SAFETY_LIMIT) // Reject pure Grok bootstrap dumps even when they arrived via a non-Grok path. - const lower = trimmed.toLowerCase() + // Safe on the bounded slice: stripGrokUserQueryEnvelope above already unwrapped + // any , wherever it sat, so a match here means there was none. + const lower = bounded.toLowerCase() if (lower.startsWith('') && !lower.includes('')) { return null } - return sliceAtCodeUnitLimit(trimmed, FULL_FIRST_USER_PROMPT_SAFETY_LIMIT) + return bounded } function isSuppressedFullFirstUserPrompt(value: string): boolean { diff --git a/src/main/ai-vault/session-scanner-hermes-parser.ts b/src/main/ai-vault/session-scanner-hermes-parser.ts new file mode 100644 index 00000000000..be844cc6627 --- /dev/null +++ b/src/main/ai-vault/session-scanner-hermes-parser.ts @@ -0,0 +1,66 @@ +import { readFile } from 'node:fs/promises' +import type { AiVaultSession } from '../../shared/ai-vault-types' +import type { ExecutionHostId } from '../../shared/execution-host' +import type { FileWithMtime } from './session-scanner-types' +import { + addPreviewContent, + createAccumulator, + finalizeSession, + sessionIdFromFileName, + updateTimeline +} from './session-scanner-accumulator' +import { + arrayValue, + asRecord, + extractContentText, + extractString, + numberValue +} from './session-scanner-values' + +type ParserSessionOptions = { + executionHostId?: ExecutionHostId + executionHostPlatform?: NodeJS.Platform | null +} + +export async function parseHermesSessionFile( + file: FileWithMtime, + platform: NodeJS.Platform = process.platform +): Promise { + return parseHermesSessionContent(file, await readFile(file.path, 'utf-8'), platform) +} + +export async function parseHermesSessionContent( + file: FileWithMtime, + content: string, + platform: NodeJS.Platform = process.platform, + options: ParserSessionOptions = {} +): Promise { + const record = asRecord(JSON.parse(content) as unknown) + if (!record) { + return null + } + const accumulator = createAccumulator({ + agent: 'hermes', + file, + sessionId: extractString(record.session_id) ?? sessionIdFromFileName(file.path) + }) + accumulator.model = extractString(record.model) + accumulator.cwd = extractString(record.cwd) + updateTimeline(accumulator, extractString(record.session_start)) + updateTimeline(accumulator, extractString(record.last_updated)) + for (const message of arrayValue(record.messages)) { + const messageRecord = asRecord(message) + const role = extractString(messageRecord?.role) + if (role === 'user' || role === 'assistant') { + accumulator.messageCount++ + if (role === 'user') { + accumulator.title ??= extractContentText(messageRecord?.content) + } + addPreviewContent(accumulator, role, messageRecord?.content) + } + } + if (accumulator.messageCount === 0) { + accumulator.messageCount = numberValue(record.message_count) + } + return finalizeSession(accumulator, platform, options) +} diff --git a/src/main/ai-vault/session-scanner-injected-title.test.ts b/src/main/ai-vault/session-scanner-injected-title.test.ts index f0f566a79f5..a1bf6f799f8 100644 --- a/src/main/ai-vault/session-scanner-injected-title.test.ts +++ b/src/main/ai-vault/session-scanner-injected-title.test.ts @@ -3,6 +3,7 @@ import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, describe, expect, it, vi } from 'vitest' import { scanAiVaultSessions } from './session-scanner' +import { withFullFirstUserPromptCapture } from './session-scanner-first-user-prompt-capture' import { isolatedScanRoots, jsonLines } from './session-scanner-test-fixtures' let tempRoots: string[] = [] @@ -127,10 +128,14 @@ describe('scanAiVaultSessions harness-injected title seeding', () => { ]) ) - const result = await scanAiVaultSessions({ - ...roots, - platform: 'darwin' - }) + // Full capture, else firstUserPrompt is never populated and the assertion + // below would pass whether or not the meta preamble is excluded. + const result = await withFullFirstUserPromptCapture(() => + scanAiVaultSessions({ + ...roots, + platform: 'darwin' + }) + ) expect(result.issues).toEqual([]) expect(result.sessions[0]?.lastUserPrompt).toBe('Fix the zoom behavior in a separate PR') diff --git a/src/main/ai-vault/session-scanner-opencode-parser.test.ts b/src/main/ai-vault/session-scanner-opencode-parser.test.ts new file mode 100644 index 00000000000..9a29927042d --- /dev/null +++ b/src/main/ai-vault/session-scanner-opencode-parser.test.ts @@ -0,0 +1,93 @@ +import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, describe, expect, it } from 'vitest' +import { parseOpenCodeSessionFile } from './session-scanner-opencode-parser' +import { withFullFirstUserPromptCapture } from './session-scanner-first-user-prompt-capture' + +let tempDirs: string[] = [] + +afterEach(async () => { + await Promise.all(tempDirs.map((dir) => rm(dir, { recursive: true, force: true }))) + tempDirs = [] +}) + +function userMessage(text: string, createdAt: number): string { + return JSON.stringify({ + role: 'user', + content: [{ type: 'text', text }], + time: { created: createdAt }, + tokens: { input: 5, output: 0 } + }) +} + +describe('parseOpenCodeSessionFile', () => { + it('skips a corrupt message file instead of discarding the whole session', async () => { + const storageRoot = await mkdtemp(join(tmpdir(), 'orca-opencode-parser-')) + tempDirs.push(storageRoot) + const sessionDir = join(storageRoot, 'session', 'project') + const messageDir = join(storageRoot, 'message', 'ses_corrupt') + await mkdir(sessionDir, { recursive: true }) + await mkdir(messageDir, { recursive: true }) + + const path = join(sessionDir, 'ses_corrupt.json') + const mtimeMs = Date.now() + await writeFile( + path, + JSON.stringify({ + id: 'ses_corrupt', + directory: '/tmp/opencode', + title: 'OpenCode title', + time: { created: 1_777_634_000_000, updated: 1_777_634_002_000 } + }) + ) + await writeFile(join(messageDir, 'msg_1.json'), userMessage('First prompt', 1_777_634_000_000)) + // Partially-written file from a live OpenCode process. + await writeFile(join(messageDir, 'msg_2.json'), '{"role":"user","content":[{"type":') + await writeFile(join(messageDir, 'msg_3.json'), userMessage('Third prompt', 1_777_634_002_000)) + + const session = await parseOpenCodeSessionFile({ + path, + mtimeMs, + modifiedAt: new Date(mtimeMs).toISOString() + }) + + expect(session).not.toBeNull() + expect(session?.messageCount).toBe(2) + expect(session?.totalTokens).toBe(10) + expect(session?.previewMessages.map((message) => message.text)).toEqual([ + 'First prompt', + 'Third prompt' + ]) + }) + + it('orders messages by created time when file order disagrees', async () => { + const storageRoot = await mkdtemp(join(tmpdir(), 'orca-opencode-parser-')) + tempDirs.push(storageRoot) + const sessionDir = join(storageRoot, 'session', 'project') + const messageDir = join(storageRoot, 'message', 'ses_order') + await mkdir(sessionDir, { recursive: true }) + await mkdir(messageDir, { recursive: true }) + + const path = join(sessionDir, 'ses_order.json') + const mtimeMs = Date.now() + await writeFile(path, JSON.stringify({ id: 'ses_order', directory: '/tmp/opencode' })) + // File names sort the opposite way from the transcript's creation order. + await writeFile(join(messageDir, 'msg_a.json'), userMessage('Later turn', 1_777_634_002_000)) + await writeFile(join(messageDir, 'msg_b.json'), userMessage('Earliest turn', 1_777_634_000_000)) + + const session = await withFullFirstUserPromptCapture(() => + parseOpenCodeSessionFile({ + path, + mtimeMs, + modifiedAt: new Date(mtimeMs).toISOString() + }) + ) + + expect(session?.firstUserPrompt).toBe('Earliest turn') + expect(session?.previewMessages.map((message) => message.text)).toEqual([ + 'Earliest turn', + 'Later turn' + ]) + }) +}) diff --git a/src/main/ai-vault/session-scanner-opencode-parser.ts b/src/main/ai-vault/session-scanner-opencode-parser.ts new file mode 100644 index 00000000000..4231f7d9de4 --- /dev/null +++ b/src/main/ai-vault/session-scanner-opencode-parser.ts @@ -0,0 +1,126 @@ +import { readFile, readdir } from 'node:fs/promises' +import { join } from 'node:path' +import type { AiVaultSession } from '../../shared/ai-vault-types' +import type { FileWithMtime, SessionAccumulator } from './session-scanner-types' +import { + addPreviewMessage, + createAccumulator, + finalizeSession, + seedFullFirstUserPrompt, + sessionIdFromFileName, + updateTimeline +} from './session-scanner-accumulator' +import { extractFullFirstUserPromptText } from './session-scanner-first-user-prompt' +import { + asRecord, + extractPreviewContentText, + extractString, + findOpenCodeStorageRoot, + normalizeTitleText, + timeObjectValue, + timestampMs, + tokenTotal +} from './session-scanner-values' + +export async function parseOpenCodeSessionFile( + file: FileWithMtime, + platform: NodeJS.Platform = process.platform +): Promise { + const record = asRecord(JSON.parse(await readFile(file.path, 'utf-8')) as unknown) + if (!record) { + return null + } + const sessionId = extractString(record.id) ?? sessionIdFromFileName(file.path) + const accumulator = createAccumulator({ agent: 'opencode', file, sessionId }) + accumulator.title = normalizeTitleText(extractString(record.title) ?? '') + accumulator.cwd = extractString(record.directory) + updateTimeline(accumulator, timeObjectValue(record.time, 'created')) + updateTimeline(accumulator, timeObjectValue(record.time, 'updated')) + await consumeOpenCodeMessages(accumulator, findOpenCodeStorageRoot(file.path), sessionId) + return finalizeSession(accumulator, platform) +} + +// Why: OpenCode stores one file per message and `readdir` order is arbitrary, +// so the transcript has to be rebuilt by creation time before anything reads +// "the first user prompt". Undated messages sort last, by file name, so a +// missing timestamp can never displace a real first turn. +async function readOpenCodeMessagesInOrder(messageDir: string): Promise[]> { + let entries + try { + entries = await readdir(messageDir, { withFileTypes: true }) + } catch { + return [] + } + const messages: { name: string; createdMs: number; message: Record }[] = [] + for (const entry of entries) { + if (!entry.isFile() || !entry.name.endsWith('.json')) { + continue + } + let message: Record | null = null + try { + message = asRecord( + JSON.parse(await readFile(join(messageDir, entry.name), 'utf-8')) as unknown + ) + } catch { + // A live OpenCode process can leave a half-written file; skip it rather + // than discard the whole session. + continue + } + if (!message) { + continue + } + const createdMs = timestampMs(timeObjectValue(message.time, 'created')) + messages.push({ + name: entry.name, + createdMs: Number.isFinite(createdMs) ? createdMs : Number.POSITIVE_INFINITY, + message + }) + } + return messages + .sort((a, b) => a.createdMs - b.createdMs || a.name.localeCompare(b.name)) + .map((entry) => entry.message) +} + +export async function consumeOpenCodeMessages( + accumulator: SessionAccumulator, + storageRoot: string | null, + sessionId: string +): Promise { + if (!storageRoot) { + return + } + for (const message of await readOpenCodeMessagesInOrder( + join(storageRoot, 'message', sessionId) + )) { + const role = extractString(message.role) + if (role === 'user' || role === 'assistant') { + accumulator.messageCount++ + updateTimeline(accumulator, timeObjectValue(message.time, 'created')) + const summary = asRecord(message.summary) + if (role === 'user') { + accumulator.title ??= extractString(summary?.title) ?? extractString(summary?.body) + } + // Why: the summary fallbacks are AI-generated. Seed the copyable first + // prompt from real typed content only, never from a summary of a + // harness-injected turn. Extraction stays inside the thunk so list scans + // (capture mode `none`) never pay for it. + seedFullFirstUserPrompt(accumulator, role, () => + extractFullFirstUserPromptText(message.content) + ) + addPreviewMessage(accumulator, { + role, + text: + extractPreviewContentText(message.content) ?? + extractString(summary?.body) ?? + extractString(summary?.title), + timestamp: timeObjectValue(message.time, 'created'), + seedFirstUserPrompt: false + }) + accumulator.model = + extractString(asRecord(message.model)?.modelID) || + extractString(message.modelID) || + accumulator.model + accumulator.totalTokens += tokenTotal(message.tokens) + } + } +} diff --git a/src/main/ai-vault/session-scanner-opencode-sqlite.ts b/src/main/ai-vault/session-scanner-opencode-sqlite.ts index d247156b8cf..c0ffdbfb894 100644 --- a/src/main/ai-vault/session-scanner-opencode-sqlite.ts +++ b/src/main/ai-vault/session-scanner-opencode-sqlite.ts @@ -294,9 +294,17 @@ export async function parseOpenCodeSqliteSession(args: { const previewSql = buildPreviewQuery(db) if (previewSql) { - const previewRows = db + // Why: SQL already dropped anything older than the newest-N window, so the + // accumulator never shifts and cannot detect the truncation itself. Ask for + // one extra row so an exactly-full window is not mistaken for a trimmed one. + const probedRows = db .prepare(previewSql) - .all(sessionId, OPENCODE_SQLITE_PREVIEW_LIMIT) as PreviewRow[] + .all(sessionId, OPENCODE_SQLITE_PREVIEW_LIMIT + 1) as PreviewRow[] + if (probedRows.length > OPENCODE_SQLITE_PREVIEW_LIMIT) { + accumulator.previewMessagesTruncated = true + } + // Newest-first, so the probe row to drop is the oldest one at the tail. + const previewRows = probedRows.slice(0, OPENCODE_SQLITE_PREVIEW_LIMIT) // Why: query returns newest-first; push in chronological order so the // accumulator's ring buffer keeps the newest OPENCODE_SQLITE_PREVIEW_LIMIT // messages. diff --git a/src/main/ai-vault/session-scanner-parse-cache.ts b/src/main/ai-vault/session-scanner-parse-cache.ts index b3e4448a410..572002197e0 100644 --- a/src/main/ai-vault/session-scanner-parse-cache.ts +++ b/src/main/ai-vault/session-scanner-parse-cache.ts @@ -8,10 +8,8 @@ import { createDroidSessionResumeState } from './session-scanner-droid-parser' import { createMessageGraphSessionResumeState } from './session-scanner-graph-parsers' import { createClaudeSessionResumeState } from './session-scanner-primary-parsers' import { createGeminiJsonlSessionResumeState } from './session-scanner-gemini-parsers' -import { - createCopilotSessionResumeState, - createCursorSessionResumeState -} from './session-scanner-secondary-parsers' +import { createCopilotSessionResumeState } from './session-scanner-copilot-parser' +import { createCursorSessionResumeState } from './session-scanner-cursor-parser' import { countSubagentTranscripts } from './session-scanner-subagent-transcripts' import type { ResumableSessionParseState, SessionFileCandidate } from './session-scanner-types' diff --git a/src/main/ai-vault/session-scanner-preview-window-truncation.test.ts b/src/main/ai-vault/session-scanner-preview-window-truncation.test.ts new file mode 100644 index 00000000000..936e97134c7 --- /dev/null +++ b/src/main/ai-vault/session-scanner-preview-window-truncation.test.ts @@ -0,0 +1,45 @@ +import { describe, expect, it } from 'vitest' +import { + addPreviewMessage, + createAccumulator, + finalizeSession +} from './session-scanner-accumulator' +import type { SessionAccumulator } from './session-scanner-types' + +// Mirrors SESSION_PREVIEW_MESSAGE_LIMIT in the module under test. +const PREVIEW_LIMIT = 5 + +function accumulatorWithUserTurns(count: number): SessionAccumulator { + const accumulator = createAccumulator({ + agent: 'claude', + file: { path: '/tmp/session-1.jsonl', mtimeMs: 0, modifiedAt: new Date(0).toISOString() }, + sessionId: 'session-1' + }) + for (let index = 0; index < count; index += 1) { + addPreviewMessage(accumulator, { role: 'user', text: `turn ${index}` }) + } + return accumulator +} + +describe('AI Vault preview-window truncation signal', () => { + it('leaves the flag off while the window still holds the whole transcript', () => { + const session = finalizeSession(accumulatorWithUserTurns(PREVIEW_LIMIT), 'darwin') + + expect(session?.previewMessages).toHaveLength(PREVIEW_LIMIT) + expect(session?.previewMessagesTruncated).toBeUndefined() + }) + + it('flags the session once an older turn falls out of the window', () => { + const session = finalizeSession(accumulatorWithUserTurns(PREVIEW_LIMIT + 1), 'darwin') + + // The opening ask is gone, so the earliest preview turn is not the first prompt. + expect(session?.previewMessages[0]?.text).toBe('turn 1') + expect(session?.previewMessagesTruncated).toBe(true) + }) + + it('carries the flag onto snapshots taken from a cloned accumulator', () => { + const accumulator = accumulatorWithUserTurns(PREVIEW_LIMIT + 1) + + expect(finalizeSession({ ...accumulator }, 'darwin')?.previewMessagesTruncated).toBe(true) + }) +}) diff --git a/src/main/ai-vault/session-scanner-secondary-parsers.ts b/src/main/ai-vault/session-scanner-secondary-parsers.ts deleted file mode 100644 index cb472aaefe9..00000000000 --- a/src/main/ai-vault/session-scanner-secondary-parsers.ts +++ /dev/null @@ -1,316 +0,0 @@ -import { createReadStream } from 'node:fs' -import { readFile, readdir } from 'node:fs/promises' -import { join } from 'node:path' -import { createInterface } from 'node:readline' -import type { AiVaultSession } from '../../shared/ai-vault-types' -import type { ExecutionHostId } from '../../shared/execution-host' -import type { - FileWithMtime, - ResumableSessionParseState, - SessionAccumulator -} from './session-scanner-types' -import { - accumulatorFoldResumeState, - addPreviewContent, - addPreviewMessage, - createAccumulator, - finalizeSession, - sessionIdFromFileName, - updateTimeline -} from './session-scanner-accumulator' -import { extractFullFirstUserPromptText } from './session-scanner-first-user-prompt' -import { - arrayValue, - asRecord, - copilotModelMetricsTotal, - extractContentText, - extractMessageText, - extractString, - extractTrustedFolder, - findOpenCodeStorageRoot, - normalizeTitleText, - numberValue, - parseJsonObject, - timeObjectValue, - tokenTotal -} from './session-scanner-values' - -type ParserSessionOptions = { - executionHostId?: ExecutionHostId - executionHostPlatform?: NodeJS.Platform | null -} - -export async function parseCopilotSessionFile( - file: FileWithMtime, - platform: NodeJS.Platform = process.platform -): Promise { - const lines = createInterface({ - input: createReadStream(file.path, { encoding: 'utf-8' }), - crlfDelay: Infinity - }) - return parseCopilotSessionLines({ file, lines, platform }) -} - -export async function parseCopilotSessionContent( - file: FileWithMtime, - content: string, - platform: NodeJS.Platform = process.platform, - options: ParserSessionOptions = {} -): Promise { - return parseCopilotSessionLines({ - file, - lines: content.split(/\r?\n/), - platform, - options - }) -} - -function consumeCopilotRecordLine(accumulator: SessionAccumulator, line: string): void { - const record = parseJsonObject(line) - if (!record) { - return - } - updateTimeline(accumulator, extractString(record.timestamp)) - const data = asRecord(record.data) - if (record.type === 'session.start' && data) { - const sessionId = extractString(data.sessionId) - if (sessionId) { - accumulator.sessionId = sessionId - } - updateTimeline(accumulator, extractString(data.startTime)) - return - } - if (record.type === 'session.model_change' && data) { - accumulator.model = extractString(data.newModel) ?? accumulator.model - return - } - if (record.type === 'session.info' && data) { - accumulator.cwd = extractTrustedFolder(data.message) ?? accumulator.cwd - return - } - if (record.type === 'user.message' && data) { - accumulator.messageCount++ - accumulator.title ??= normalizeTitleText( - extractString(data.transformedContent) ?? extractString(data.content) ?? '' - ) - addPreviewMessage(accumulator, { - role: 'user', - text: extractString(data.transformedContent) ?? extractString(data.content), - timestamp: record.timestamp - }) - return - } - if (record.type === 'assistant.message' && data) { - accumulator.messageCount++ - addPreviewMessage(accumulator, { - role: 'assistant', - text: extractString(data.content), - timestamp: record.timestamp - }) - return - } - if (record.type === 'session.shutdown' && data) { - accumulator.model = extractString(data.currentModel) ?? accumulator.model - accumulator.totalTokens += numberValue(data.currentTokens) - accumulator.totalTokens += copilotModelMetricsTotal(data.modelMetrics) - } -} - -export function createCopilotSessionResumeState(file: FileWithMtime): ResumableSessionParseState { - return accumulatorFoldResumeState( - createAccumulator({ agent: 'copilot', file, sessionId: sessionIdFromFileName(file.path) }), - consumeCopilotRecordLine - ) -} - -async function parseCopilotSessionLines(args: { - file: FileWithMtime - lines: AsyncIterable | Iterable - platform: NodeJS.Platform - options?: ParserSessionOptions -}): Promise { - const state = createCopilotSessionResumeState(args.file) - for await (const line of args.lines) { - state.consumeLine(line) - } - return state.finalize(args.platform, args.options) -} - -export async function parseCursorSessionFile( - file: FileWithMtime, - platform: NodeJS.Platform = process.platform -): Promise { - const lines = createInterface({ - input: createReadStream(file.path, { encoding: 'utf-8' }), - crlfDelay: Infinity - }) - return parseCursorSessionLines({ file, lines, platform }) -} - -export async function parseCursorSessionContent( - file: FileWithMtime, - content: string, - platform: NodeJS.Platform = process.platform, - options: ParserSessionOptions = {} -): Promise { - return parseCursorSessionLines({ - file, - lines: content.split(/\r?\n/), - platform, - options - }) -} - -function consumeCursorRecordLine(accumulator: SessionAccumulator, line: string): void { - const record = parseJsonObject(line) - if (!record) { - return - } - updateTimeline(accumulator, extractString(record.timestamp)) - const role = extractString(record.role) - if (role === 'user' || role === 'assistant') { - accumulator.messageCount++ - if (role === 'user') { - accumulator.title ??= extractMessageText(record.message) ?? extractContentText(record.content) - } - addPreviewContent( - accumulator, - role, - asRecord(record.message)?.content ?? record.content, - record.timestamp - ) - } -} - -export function createCursorSessionResumeState(file: FileWithMtime): ResumableSessionParseState { - return accumulatorFoldResumeState( - createAccumulator({ agent: 'cursor', file, sessionId: sessionIdFromFileName(file.path) }), - consumeCursorRecordLine - ) -} - -async function parseCursorSessionLines(args: { - file: FileWithMtime - lines: AsyncIterable | Iterable - platform: NodeJS.Platform - options?: ParserSessionOptions -}): Promise { - const state = createCursorSessionResumeState(args.file) - for await (const line of args.lines) { - state.consumeLine(line) - } - return state.finalize(args.platform, args.options) -} - -export async function parseOpenCodeSessionFile( - file: FileWithMtime, - platform: NodeJS.Platform = process.platform -): Promise { - const record = asRecord(JSON.parse(await readFile(file.path, 'utf-8')) as unknown) - if (!record) { - return null - } - const sessionId = extractString(record.id) ?? sessionIdFromFileName(file.path) - const accumulator = createAccumulator({ agent: 'opencode', file, sessionId }) - accumulator.title = normalizeTitleText(extractString(record.title) ?? '') - accumulator.cwd = extractString(record.directory) - updateTimeline(accumulator, timeObjectValue(record.time, 'created')) - updateTimeline(accumulator, timeObjectValue(record.time, 'updated')) - await consumeOpenCodeMessages(accumulator, findOpenCodeStorageRoot(file.path), sessionId) - return finalizeSession(accumulator, platform) -} - -export async function consumeOpenCodeMessages( - accumulator: SessionAccumulator, - storageRoot: string | null, - sessionId: string -): Promise { - if (!storageRoot) { - return - } - const messageDir = join(storageRoot, 'message', sessionId) - let entries - try { - entries = await readdir(messageDir, { withFileTypes: true }) - } catch { - return - } - for (const entry of entries) { - if (!entry.isFile() || !entry.name.endsWith('.json')) { - continue - } - const message = asRecord( - JSON.parse(await readFile(join(messageDir, entry.name), 'utf-8')) as unknown - ) - if (!message) { - continue - } - const role = extractString(message.role) - if (role === 'user' || role === 'assistant') { - accumulator.messageCount++ - updateTimeline(accumulator, timeObjectValue(message.time, 'created')) - if (role === 'user') { - accumulator.title ??= extractString(asRecord(message.summary)?.title) - accumulator.title ??= extractString(asRecord(message.summary)?.body) - } - // Why: pass raw body text so full first-prompt capture is not stuck on the - // 220-char preview fold (addPreviewMessage preview-caps for display). - addPreviewMessage(accumulator, { - role, - text: - extractFullFirstUserPromptText(message.content) ?? - extractString(asRecord(message.summary)?.body) ?? - extractString(asRecord(message.summary)?.title), - timestamp: timeObjectValue(message.time, 'created') - }) - accumulator.model = - extractString(asRecord(message.model)?.modelID) || - extractString(message.modelID) || - accumulator.model - accumulator.totalTokens += tokenTotal(message.tokens) - } - } -} - -export async function parseHermesSessionFile( - file: FileWithMtime, - platform: NodeJS.Platform = process.platform -): Promise { - return parseHermesSessionContent(file, await readFile(file.path, 'utf-8'), platform) -} - -export async function parseHermesSessionContent( - file: FileWithMtime, - content: string, - platform: NodeJS.Platform = process.platform, - options: ParserSessionOptions = {} -): Promise { - const record = asRecord(JSON.parse(content) as unknown) - if (!record) { - return null - } - const accumulator = createAccumulator({ - agent: 'hermes', - file, - sessionId: extractString(record.session_id) ?? sessionIdFromFileName(file.path) - }) - accumulator.model = extractString(record.model) - accumulator.cwd = extractString(record.cwd) - updateTimeline(accumulator, extractString(record.session_start)) - updateTimeline(accumulator, extractString(record.last_updated)) - for (const message of arrayValue(record.messages)) { - const messageRecord = asRecord(message) - const role = extractString(messageRecord?.role) - if (role === 'user' || role === 'assistant') { - accumulator.messageCount++ - if (role === 'user') { - accumulator.title ??= extractContentText(messageRecord?.content) - } - addPreviewContent(accumulator, role, messageRecord?.content) - } - } - if (accumulator.messageCount === 0) { - accumulator.messageCount = numberValue(record.message_count) - } - return finalizeSession(accumulator, platform, options) -} diff --git a/src/main/ai-vault/session-scanner-types.ts b/src/main/ai-vault/session-scanner-types.ts index cc8f4939aa2..7d4de3ef294 100644 --- a/src/main/ai-vault/session-scanner-types.ts +++ b/src/main/ai-vault/session-scanner-types.ts @@ -110,6 +110,9 @@ export type SessionAccumulator = { messageCount: number totalTokens: number previewMessages: AiVaultSessionPreviewMessage[] + // True once an older message fell out of the newest-N preview window, so the + // earliest preview turn is no longer the session's opening ask. + previewMessagesTruncated: boolean firstUserPrompt: string | null lastUserPrompt: string | null // Recoverable signal for a zero-turn transcript (see AiVaultSession). diff --git a/src/main/ssh/ssh-connection-utils.test.ts b/src/main/ssh/ssh-connection-utils.test.ts index eba0de23087..72dd7991d71 100644 --- a/src/main/ssh/ssh-connection-utils.test.ts +++ b/src/main/ssh/ssh-connection-utils.test.ts @@ -1,16 +1,7 @@ -import { EventEmitter } from 'node:events' import { afterEach, describe, expect, it, vi, beforeEach } from 'vitest' import { join } from 'node:path' import { BaseAgent, utils, type ParsedKey } from 'ssh2' -const { spawnMock } = vi.hoisted(() => ({ - spawnMock: vi.fn() -})) - -vi.mock('child_process', () => ({ - spawn: spawnMock -})) - vi.mock('os', () => ({ homedir: () => '/home/testuser' })) @@ -39,32 +30,15 @@ import { findDefaultKeyFile, buildConnectConfig, resolveAgentSocket, - resolveEffectiveProxy, CONNECT_TIMEOUT_MS, INITIAL_RETRY_ATTEMPTS, INITIAL_RETRY_DELAY_MS, - RECONNECT_BACKOFF_MS, - spawnProxyCommand + RECONNECT_BACKOFF_MS } from './ssh-connection-utils' +import { resolveEffectiveProxy } from './ssh-proxy-command' import type { SshTarget } from '../../shared/ssh-types' import type { SshResolvedConfig } from './ssh-config-parser' -type MockProxyProcess = EventEmitter & { - stdin: EventEmitter & { write: ReturnType } - stdout: EventEmitter - stderr: EventEmitter -} - -function createMockProxyProcess(): MockProxyProcess { - const proc = new EventEmitter() as MockProxyProcess - proc.stdin = Object.assign(new EventEmitter(), { - write: vi.fn((_chunk, cb?: (error?: Error | null) => void) => cb?.()) - }) - proc.stdout = new EventEmitter() - proc.stderr = new EventEmitter() - return proc -} - // ── Constants ──────────────────────────────────────────────────────── describe('SSH connection constants', () => { @@ -803,35 +777,3 @@ describe('resolveEffectiveProxy', () => { expect(resolveEffectiveProxy(makeTarget(), null)).toBeUndefined() }) }) - -// ── spawnProxyCommand ─────────────────────────────────────────────── - -describe('spawnProxyCommand', () => { - beforeEach(() => { - spawnMock.mockReset() - }) - - it('removes proxy process listeners when the socket is destroyed', () => { - const proc = createMockProxyProcess() - spawnMock.mockReturnValue(proc) - - const { sock } = spawnProxyCommand( - { kind: 'jump-host', jumpHost: 'bastion.example.com' }, - 'target.example.com', - 22, - 'deploy' - ) - - expect(proc.stdout.listenerCount('data')).toBe(1) - expect(proc.stdout.listenerCount('end')).toBe(1) - expect(proc.stdin.listenerCount('error')).toBe(1) - expect(proc.listenerCount('error')).toBe(1) - - sock.destroy() - - expect(proc.stdout.listenerCount('data')).toBe(0) - expect(proc.stdout.listenerCount('end')).toBe(0) - expect(proc.stdin.listenerCount('error')).toBe(0) - expect(proc.listenerCount('error')).toBe(0) - }) -}) diff --git a/src/main/ssh/ssh-connection-utils.ts b/src/main/ssh/ssh-connection-utils.ts index 77cb5ab07a5..6b0cac00770 100644 --- a/src/main/ssh/ssh-connection-utils.ts +++ b/src/main/ssh/ssh-connection-utils.ts @@ -1,6 +1,3 @@ -import { spawn, type ChildProcess } from 'node:child_process' -import { Duplex } from 'node:stream' -import type { Socket as NetSocket } from 'node:net' import type { ConnectConfig } from 'ssh2' import type { SshTarget, SshConnectionState } from '../../shared/ssh-types' import type { SshResolvedConfig } from './ssh-config-parser' @@ -52,7 +49,9 @@ export function isAuthError(err: Error): boolean { msg.includes('all configured authentication methods failed') || msg.includes('authentication failed') || msg.includes('too many authentication failures') || - /permission denied(?:, please try again\.?| \([^)]*(?:publickey|password|keyboard-interactive|gssapi|hostbased)[^)]*\))/.test(msg) || + /permission denied(?:, please try again\.?| \([^)]*(?:publickey|password|keyboard-interactive|gssapi|hostbased)[^)]*\))/.test( + msg + ) || (err as { level?: string }).level === 'client-authentication' ) } @@ -165,10 +164,6 @@ export function createSshOperationAbortError(): Error & { name: string } { return error } -function cmdEscape(s: string): string { - return `"${s.replace(/"/g, '""')}"` -} - type BuildConnectConfigOptions = { includeAgent?: boolean includePrivateKey?: boolean @@ -251,112 +246,3 @@ function shouldUseResolvedEndpoint(target: SshTarget, resolved: SshResolvedConfi const host = target.host.trim() return host === '' || host === target.configHost || host === target.label } - -// Why: ProxyJump and jumpHost are syntactic sugar for ProxyCommand. -// OpenSSH internally converts `ProxyJump bastion` to -// `ProxyCommand ssh -W %h:%p bastion`. We do the same so that ssh2 -// gets a single proxy spawn path regardless of how the tunnel was configured. -export type EffectiveProxy = - | { kind: 'proxy-command'; command: string } - | { kind: 'jump-host'; jumpHost: string } - -export function resolveEffectiveProxy( - target: SshTarget, - resolved: SshResolvedConfig | null -): EffectiveProxy | undefined { - if (isOpenSshConfigBackedTarget(target) && resolved) { - if (resolved.proxyCommand) { - return { kind: 'proxy-command', command: resolved.proxyCommand } - } - return resolved.proxyJump ? { kind: 'jump-host', jumpHost: resolved.proxyJump } : undefined - } - if (target.proxyCommand) { - return { kind: 'proxy-command', command: target.proxyCommand } - } - if (resolved?.proxyCommand) { - return { kind: 'proxy-command', command: resolved.proxyCommand } - } - const jump = target.jumpHost || resolved?.proxyJump - if (jump) { - return { kind: 'jump-host', jumpHost: jump } - } - return undefined -} - -// Why: ssh2 doesn't natively support ProxyCommand. When the SSH config -// specifies one (e.g. `cloudflared access ssh --hostname %h`), we spawn -// the command and bridge its stdin/stdout into a Duplex stream that ssh2 -// uses as its transport socket via `config.sock`. -function getShellSpawnConfig(command: string): { file: string; args: string[] } { - if (process.platform === 'win32') { - const comspec = process.env.ComSpec || 'cmd.exe' - return { file: comspec, args: ['/d', '/s', '/c', command] } - } - return { file: '/bin/sh', args: ['-c', command] } -} - -export function spawnProxyCommand( - proxy: EffectiveProxy, - host: string, - port: number, - user: string -): { process: ChildProcess; sock: NetSocket } { - const proc = - proxy.kind === 'jump-host' - ? // Why: ProxyJump is structured input, not a shell snippet. Spawn ssh - // directly so jump-host values cannot escape through shell parsing. - spawn('ssh', ['-W', `${host}:${port}`, '--', proxy.jumpHost], { - stdio: ['pipe', 'pipe', 'pipe'] - }) - : (() => { - const escape = process.platform === 'win32' ? cmdEscape : shellEscape - const expanded = proxy.command - .replace(/%h/g, escape(host)) - .replace(/%p/g, escape(String(port))) - .replace(/%r/g, escape(user)) - const shell = getShellSpawnConfig(expanded) - return spawn(shell.file, shell.args, { stdio: ['pipe', 'pipe', 'pipe'] }) - })() - - // Why: a single PassThrough for both directions creates a feedback loop. - // Reads come from the proxy's stdout; writes go to its stdin. - let cleanedUp = false - const cleanup = (): void => { - if (cleanedUp) { - return - } - cleanedUp = true - proc.stdout!.off('data', onStdoutData) - proc.stdout!.off('end', onStdoutEnd) - proc.stdin!.off('error', onInputError) - proc.off('error', onProcessError) - } - const onStdoutData = (data: Buffer): void => { - stream.push(data) - } - const onStdoutEnd = (): void => { - stream.push(null) - } - const onInputError = (err: Error): void => { - stream.destroy(err) - } - const onProcessError = (err: Error): void => { - stream.destroy(err) - } - const stream = new Duplex({ - read() {}, - write(chunk, _encoding, cb) { - proc.stdin!.write(chunk, cb) - }, - destroy(err, cb) { - cleanup() - cb(err) - } - }) - proc.stdout!.on('data', onStdoutData) - proc.stdout!.on('end', onStdoutEnd) - proc.stdin!.on('error', onInputError) - proc.on('error', onProcessError) - - return { process: proc, sock: stream as unknown as NetSocket } -} diff --git a/src/main/ssh/ssh-connection.test.ts b/src/main/ssh/ssh-connection.test.ts index dc8cdc42250..bdb38f83466 100644 --- a/src/main/ssh/ssh-connection.test.ts +++ b/src/main/ssh/ssh-connection.test.ts @@ -1799,9 +1799,7 @@ describe('SshConnection', () => { await expect(settled).resolves.toBeUndefined() expect(spawnSystemSshCommandMock).toHaveBeenCalledTimes(2) - expect(removeControlSocketPathMock).toHaveBeenCalledWith( - '/tmp/orca-ssh-501/stale-socket' - ) + expect(removeControlSocketPathMock).toHaveBeenCalledWith('/tmp/orca-ssh-501/stale-socket') expect(spawnSystemSshCommandMock).toHaveBeenNthCalledWith( 2, expect.anything(), @@ -1816,10 +1814,7 @@ describe('SshConnection', () => { it('skips a second probe for a definite host failure', async () => { getOrcaControlSocketPathMock.mockReturnValue('/tmp/orca-ssh-501/stale-socket') spawnSystemSshCommandMock.mockImplementation(() => - createFailingSystemCommandChannel( - 255, - 'ssh: connect to host box port 22: No route to host' - ) + createFailingSystemCommandChannel(255, 'ssh: connect to host box port 22: No route to host') ) vi.mocked(resolveWithSshG).mockResolvedValue(createResolvedConfig()) const conn = new SshConnection(createTarget({ configHost: 'fdpass-host' }), createCallbacks()) @@ -1863,9 +1858,7 @@ describe('SshConnection', () => { await expect(settled).resolves.toBeDefined() expect(spawnSystemSshMock).toHaveBeenCalledTimes(2) - expect(removeControlSocketPathMock).toHaveBeenCalledWith( - '/tmp/orca-ssh-501/stale-socket' - ) + expect(removeControlSocketPathMock).toHaveBeenCalledWith('/tmp/orca-ssh-501/stale-socket') expect(spawnSystemSshMock).toHaveBeenNthCalledWith( 2, expect.anything(), diff --git a/src/main/ssh/ssh-connection.ts b/src/main/ssh/ssh-connection.ts index 0016491e2b0..c8db921537f 100644 --- a/src/main/ssh/ssh-connection.ts +++ b/src/main/ssh/ssh-connection.ts @@ -33,13 +33,12 @@ import { isPassphraseError, sleep, buildConnectConfig, - resolveEffectiveProxy, - spawnProxyCommand, wrapRemoteCommandForPosixShell, createSshOperationAbortError, type SshExecOptions, type SshConnectionCallbacks } from './ssh-connection-utils' +import { resolveEffectiveProxy, spawnProxyCommand } from './ssh-proxy-command' import { createCancelledConnectAttemptError, isCancelledConnectAttemptError @@ -978,11 +977,7 @@ export class SshConnection { try { await this.doSystemSshProbe(connectGeneration) } catch (err) { - if ( - !controlPath || - this.disposed || - connectGeneration !== this.connectGeneration - ) { + if (!controlPath || this.disposed || connectGeneration !== this.connectGeneration) { throw err } removeControlSocketPath(controlPath) diff --git a/src/main/ssh/ssh-proxy-command.test.ts b/src/main/ssh/ssh-proxy-command.test.ts new file mode 100644 index 00000000000..aae440b4c6e --- /dev/null +++ b/src/main/ssh/ssh-proxy-command.test.ts @@ -0,0 +1,189 @@ +import { EventEmitter } from 'node:events' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const { spawnMock } = vi.hoisted(() => ({ + spawnMock: vi.fn() +})) + +vi.mock('child_process', () => ({ + spawn: spawnMock +})) + +import { spawnProxyCommand } from './ssh-proxy-command' + +type MockProxyProcess = EventEmitter & { + stdin: EventEmitter & { write: ReturnType } + stdout: EventEmitter & { pause: ReturnType; resume: ReturnType } + stderr: EventEmitter +} + +function createMockProxyProcess(): MockProxyProcess { + const proc = new EventEmitter() as MockProxyProcess + proc.stdin = Object.assign(new EventEmitter(), { + write: vi.fn((_chunk, cb?: (error?: Error | null) => void) => cb?.()) + }) + proc.stdout = Object.assign(new EventEmitter(), { pause: vi.fn(), resume: vi.fn() }) + proc.stderr = new EventEmitter() + return proc +} + +// ── spawnProxyCommand ─────────────────────────────────────────────── + +describe('spawnProxyCommand', () => { + beforeEach(() => { + spawnMock.mockReset() + }) + + it('removes proxy process listeners when the socket is destroyed', () => { + const proc = createMockProxyProcess() + spawnMock.mockReturnValue(proc) + + const { sock } = spawnProxyCommand( + { kind: 'jump-host', jumpHost: 'bastion.example.com' }, + 'target.example.com', + 22, + 'deploy' + ) + + expect(proc.stdout.listenerCount('data')).toBe(1) + expect(proc.stdout.listenerCount('end')).toBe(1) + expect(proc.stderr.listenerCount('data')).toBe(1) + expect(proc.stdin.listenerCount('error')).toBe(1) + expect(proc.listenerCount('error')).toBe(1) + + sock.destroy() + + expect(proc.stdout.listenerCount('data')).toBe(0) + expect(proc.stdout.listenerCount('end')).toBe(0) + expect(proc.stderr.listenerCount('data')).toBe(0) + expect(proc.stdin.listenerCount('error')).toBe(0) + expect(proc.listenerCount('error')).toBe(0) + }) + + it('drains proxy stderr so the pipe cannot fill and block the child', () => { + const proc = createMockProxyProcess() + spawnMock.mockReturnValue(proc) + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}) + + spawnProxyCommand( + { kind: 'jump-host', jumpHost: 'bastion.example.com' }, + 'target.example.com', + 22, + 'deploy' + ) + proc.stderr.emit('data', Buffer.from('bastion: permission denied\n')) + + expect(consoleError).toHaveBeenCalledWith('[ssh-proxy-command] bastion: permission denied') + consoleError.mockRestore() + }) + + it('expands a comma-separated ProxyJump chain into -J plus a final hop', () => { + spawnMock.mockReturnValue(createMockProxyProcess()) + + spawnProxyCommand( + { kind: 'jump-host', jumpHost: 'first.example.com, second.example.com,third.example.com' }, + 'target.example.com', + 2222, + 'deploy' + ) + + expect(spawnMock).toHaveBeenCalledWith( + 'ssh', + [ + '-W', + 'target.example.com:2222', + '-J', + 'first.example.com,second.example.com', + '--', + 'third.example.com' + ], + expect.anything() + ) + }) + + it('tunnels a single ProxyJump hop without a -J chain', () => { + spawnMock.mockReturnValue(createMockProxyProcess()) + + spawnProxyCommand( + { kind: 'jump-host', jumpHost: 'bastion.example.com' }, + 'target.example.com', + 22, + 'deploy' + ) + + expect(spawnMock).toHaveBeenCalledWith( + 'ssh', + ['-W', 'target.example.com:22', '--', 'bastion.example.com'], + expect.anything() + ) + }) + + it('pauses the proxy stdout when the transport stream applies backpressure', () => { + const proc = createMockProxyProcess() + spawnMock.mockReturnValue(proc) + + const { sock } = spawnProxyCommand( + { kind: 'jump-host', jumpHost: 'bastion.example.com' }, + 'target.example.com', + 22, + 'deploy' + ) + // Nothing reads the Duplex, so pushing past the high-water mark backs up. + for (let i = 0; i < 64; i += 1) { + proc.stdout.emit('data', Buffer.alloc(4096)) + } + + expect(proc.stdout.pause).toHaveBeenCalled() + + while (sock.read() !== null) { + // Drain below the high-water mark so the Duplex asks for more. + } + + expect(proc.stdout.resume).toHaveBeenCalled() + }) + + describe('on Windows', () => { + const realPlatform = process.platform + + const setPlatform = (platform: string): void => { + Object.defineProperty(process, 'platform', { value: platform, configurable: true }) + } + + afterEach(() => { + setPlatform(realPlatform) + }) + + it('passes the cmd.exe command line verbatim so inner quotes survive', () => { + setPlatform('win32') + spawnMock.mockReturnValue(createMockProxyProcess()) + + spawnProxyCommand( + { kind: 'proxy-command', command: 'cloudflared access ssh --hostname %h --port %p' }, + 'target.example.com', + 22, + 'deploy' + ) + + expect(spawnMock).toHaveBeenCalledWith( + process.env.ComSpec || 'cmd.exe', + ['/d', '/s', '/c', '"cloudflared access ssh --hostname "target.example.com" --port "22""'], + expect.objectContaining({ windowsVerbatimArguments: true }) + ) + }) + + it('refuses values that cmd.exe cannot quote safely', () => { + setPlatform('win32') + spawnMock.mockReturnValue(createMockProxyProcess()) + + expect(() => + spawnProxyCommand( + { kind: 'proxy-command', command: 'cloudflared access ssh --hostname %h' }, + 'host&echo injected', + 22, + 'deploy' + ) + ).toThrow(/cannot be safely expanded on Windows/) + expect(spawnMock).not.toHaveBeenCalled() + }) + }) +}) diff --git a/src/main/ssh/ssh-proxy-command.ts b/src/main/ssh/ssh-proxy-command.ts new file mode 100644 index 00000000000..c1a64ed6482 --- /dev/null +++ b/src/main/ssh/ssh-proxy-command.ts @@ -0,0 +1,178 @@ +import { spawn, type ChildProcess } from 'node:child_process' +import { Duplex } from 'node:stream' +import type { Socket as NetSocket } from 'node:net' +import type { SshTarget } from '../../shared/ssh-types' +import type { SshResolvedConfig } from './ssh-config-parser' +import { shellEscape } from './ssh-connection-utils' +import { isOpenSshConfigBackedTarget } from './system-ssh-args' + +// Why: ProxyJump and jumpHost are syntactic sugar for ProxyCommand. +// OpenSSH internally converts `ProxyJump bastion` to +// `ProxyCommand ssh -W %h:%p bastion`. We do the same so that ssh2 +// gets a single proxy spawn path regardless of how the tunnel was configured. +export type EffectiveProxy = + | { kind: 'proxy-command'; command: string } + | { kind: 'jump-host'; jumpHost: string } + +export function resolveEffectiveProxy( + target: SshTarget, + resolved: SshResolvedConfig | null +): EffectiveProxy | undefined { + if (isOpenSshConfigBackedTarget(target) && resolved) { + if (resolved.proxyCommand) { + return { kind: 'proxy-command', command: resolved.proxyCommand } + } + return resolved.proxyJump ? { kind: 'jump-host', jumpHost: resolved.proxyJump } : undefined + } + if (target.proxyCommand) { + return { kind: 'proxy-command', command: target.proxyCommand } + } + if (resolved?.proxyCommand) { + return { kind: 'proxy-command', command: resolved.proxyCommand } + } + const jump = target.jumpHost || resolved?.proxyJump + if (jump) { + return { kind: 'jump-host', jumpHost: jump } + } + return undefined +} + +// Why: cmd.exe has no quoting that survives arbitrary values — `%VAR%` expands +// inside quotes and `""` re-opens the unquoted state — so the only safe Windows +// expansion is to reject metacharacters instead of pretending to escape them. +// Hosts, ports and usernames never legitimately contain these. +const CMD_UNSAFE_PATTERN = /["%&|<>^\r\n]/ + +function cmdEscape(s: string): string { + if (CMD_UNSAFE_PATTERN.test(s)) { + throw new Error( + `ProxyCommand value cannot be safely expanded on Windows (unsupported characters): ${s}` + ) + } + return `"${s}"` +} + +type ShellSpawnConfig = { file: string; args: string[]; windowsVerbatimArguments: boolean } + +// Why: ssh2 doesn't natively support ProxyCommand. When the SSH config +// specifies one (e.g. `cloudflared access ssh --hostname %h`), we spawn +// the command and bridge its stdin/stdout into a Duplex stream that ssh2 +// uses as its transport socket via `config.sock`. +function getShellSpawnConfig(command: string): ShellSpawnConfig { + if (process.platform === 'win32') { + const comspec = process.env.ComSpec || 'cmd.exe' + // Why: mirror Node's own `shell: true` form. `/s` makes cmd.exe strip the + // outer quotes and take the rest verbatim; without verbatim arguments Node + // would backslash-escape inner quotes, which cmd.exe does not understand. + return { + file: comspec, + args: ['/d', '/s', '/c', `"${command}"`], + windowsVerbatimArguments: true + } + } + return { file: '/bin/sh', args: ['-c', command], windowsVerbatimArguments: false } +} + +// Why: ProxyJump takes a comma-separated chain, but `ssh -W host:port dest` +// only tunnels through a single final hop. Preceding hops become that hop's own +// -J chain, which is how OpenSSH expands a multi-hop ProxyJump. +function jumpHostSpawnArgs(jumpHost: string, host: string, port: number): string[] { + const hops = jumpHost + .split(',') + .map((hop) => hop.trim()) + .filter(Boolean) + const destination = hops.at(-1) ?? jumpHost + const chain = hops.slice(0, -1) + return [ + '-W', + `${host}:${port}`, + ...(chain.length > 0 ? ['-J', chain.join(',')] : []), + '--', + destination + ] +} + +export function spawnProxyCommand( + proxy: EffectiveProxy, + host: string, + port: number, + user: string +): { process: ChildProcess; sock: NetSocket } { + const proc = + proxy.kind === 'jump-host' + ? // Why: ProxyJump is structured input, not a shell snippet. Spawn ssh + // directly so jump-host values cannot escape through shell parsing. + spawn('ssh', jumpHostSpawnArgs(proxy.jumpHost, host, port), { + stdio: ['pipe', 'pipe', 'pipe'] + }) + : (() => { + const escape = process.platform === 'win32' ? cmdEscape : shellEscape + const expanded = proxy.command + .replace(/%h/g, escape(host)) + .replace(/%p/g, escape(String(port))) + .replace(/%r/g, escape(user)) + const shell = getShellSpawnConfig(expanded) + return spawn(shell.file, shell.args, { + stdio: ['pipe', 'pipe', 'pipe'], + windowsVerbatimArguments: shell.windowsVerbatimArguments + }) + })() + + // Why: a single PassThrough for both directions creates a feedback loop. + // Reads come from the proxy's stdout; writes go to its stdin. + let cleanedUp = false + const cleanup = (): void => { + if (cleanedUp) { + return + } + cleanedUp = true + proc.stdout!.off('data', onStdoutData) + proc.stdout!.off('end', onStdoutEnd) + proc.stderr!.off('data', onStderrData) + proc.stdin!.off('error', onInputError) + proc.off('error', onProcessError) + } + const onStdoutData = (data: Buffer): void => { + // Why: honour the Duplex's backpressure so a slow ssh2 consumer cannot + // buffer the proxy's output unboundedly. + if (!stream.push(data)) { + proc.stdout!.pause() + } + } + // Why: an undrained stderr pipe fills and blocks the proxy process, which + // looks like a silently hung connection. + const onStderrData = (data: Buffer): void => { + const text = data.toString('utf-8').trimEnd() + if (text) { + console.error(`[ssh-proxy-command] ${text}`) + } + } + const onStdoutEnd = (): void => { + stream.push(null) + } + const onInputError = (err: Error): void => { + stream.destroy(err) + } + const onProcessError = (err: Error): void => { + stream.destroy(err) + } + const stream = new Duplex({ + read() { + proc.stdout!.resume() + }, + write(chunk, _encoding, cb) { + proc.stdin!.write(chunk, cb) + }, + destroy(err, cb) { + cleanup() + cb(err) + } + }) + proc.stdout!.on('data', onStdoutData) + proc.stdout!.on('end', onStdoutEnd) + proc.stderr!.on('data', onStderrData) + proc.stdin!.on('error', onInputError) + proc.on('error', onProcessError) + + return { process: proc, sock: stream as unknown as NetSocket } +} diff --git a/src/relay/git-handler.ts b/src/relay/git-handler.ts index c574e7aaa62..6674500c6f9 100644 --- a/src/relay/git-handler.ts +++ b/src/relay/git-handler.ts @@ -383,7 +383,7 @@ export class GitHandler { // Why: pointer/range probes are part of the same SSH request and must not outlive its cancellation. const requestGit: GitExec = (args, cwd, options) => this.git(args, cwd, { ...options, signal: context.signal }) - // Why: moved clean gitlinks need committed changes surfaced. + // Why: moved clean gitlinks need committed changes surfaced. const { fromOid, toOid } = await resolveSubmoduleCommitRange( requestGit, worktreePath, @@ -775,7 +775,7 @@ export class GitHandler { private async branchCompare(params: Record) { const worktreePath = params.worktreePath as string const baseRef = params.baseRef as string - // Why: reject flag-like base refs to prevent rev-parse option injection. + // Why: reject flag-like base refs to prevent rev-parse option injection. if (baseRef.startsWith('-')) { throw new Error('Base ref must not start with "-"') } diff --git a/src/renderer/src/components/activity/activity-portal-readiness-loop.react185.test.tsx b/src/renderer/src/components/activity/activity-portal-readiness-loop.react185.test.tsx index 10d9fc2afa6..ab134697bb2 100644 --- a/src/renderer/src/components/activity/activity-portal-readiness-loop.react185.test.tsx +++ b/src/renderer/src/components/activity/activity-portal-readiness-loop.react185.test.tsx @@ -20,6 +20,11 @@ import { type ActivityPortalReadinessStatus } from './activity-portal-readiness-oscillation' +// Why: re-applying ready DOM never consumes the readiness flip budget (a 'ready' +// status resets the latch), so this retry budget is independent of +// ACTIVITY_PORTAL_READINESS_MAX_FLIPS and must not be derived from it. +const PORTAL_READY_REAPPLY_ATTEMPTS = 32 + const WORKTREE_ID = 'wt-1' const TAB_ID = 'tab-react185' const OTHER_TAB_ID = 'tab-react185-other' @@ -415,12 +420,18 @@ describe('Activity portal pane switching', () => { expect(sawLatchedSibling).toBe(true) expect(statuses.at(-1)).toBe('unavailable') - await act(async () => { - buildRoot('ready') - await Promise.resolve() - }) - expect(await flushPortalReadiness(frames)).toBe(true) - await flushPortalFramesUntil(frames, () => statuses.at(-1) === 'ready') + // Why: under CI load MutationObserver may miss one replaceChildren; re-apply ready DOM + // and keep draining until attach is observed. + let sawReady = false + for (let attempt = 0; attempt < PORTAL_READY_REAPPLY_ATTEMPTS && !sawReady; attempt += 1) { + await act(async () => { + buildRoot('ready') + await Promise.resolve() + }) + expect(await flushPortalReadiness(frames)).toBe(true) + await flushPortalFramesUntil(frames, () => statuses.at(-1) === 'ready') + sawReady = statuses.at(-1) === 'ready' + } expect(statuses.at(-1)).toBe('ready') }) }) diff --git a/src/renderer/src/components/right-sidebar/ai-vault-first-prompt-card.test.tsx b/src/renderer/src/components/right-sidebar/ai-vault-first-prompt-card.test.tsx new file mode 100644 index 00000000000..c272ea70094 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/ai-vault-first-prompt-card.test.tsx @@ -0,0 +1,57 @@ +// @vitest-environment happy-dom +import { StrictMode } from 'react' +import { cleanup, render, screen, waitFor } from '@testing-library/react' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { FirstPromptCard } from './ai-vault-first-prompt-card' +import type { AiVaultSession } from '../../../../shared/ai-vault-types' + +const session = { + id: 'local:claude:s1:/repo/session.jsonl', + executionHostId: 'local', + agent: 'claude', + sessionId: 's1', + filePath: '/repo/session.jsonl', + codexHome: null +} as unknown as AiVaultSession + +function stubApi(getFirstUserPrompt: unknown): void { + ;(window as unknown as { api: unknown }).api = { + aiVault: { getFirstUserPrompt }, + ui: { writeClipboardText: vi.fn().mockResolvedValue(undefined) } + } +} + +afterEach(() => { + cleanup() + vi.useRealTimers() +}) + +describe('FirstPromptCard', () => { + it('resolves loading under StrictMode double-invoke instead of stranding the card', async () => { + // StrictMode mounts, cleans up, then re-mounts. The cleanup marks the first + // request stale, so only a fresh second request can clear `loading`. + stubApi(vi.fn().mockResolvedValue({ prompt: 'Ship the copy button' })) + + render( + + + + ) + + await waitFor(() => expect(screen.getByText('Ship the copy button')).toBeTruthy()) + expect(screen.queryByText('Loading first prompt…')).toBeNull() + }) + + it('stops loading when the read never settles', async () => { + vi.useFakeTimers({ shouldAdvanceTime: true }) + // A main process that never answers must not pin the card in `loading`. + stubApi(vi.fn().mockReturnValue(new Promise(() => {}))) + + render() + + expect(screen.getByText('Loading first prompt…')).toBeTruthy() + await vi.advanceTimersByTimeAsync(15_000) + await waitFor(() => expect(screen.getByText('No first prompt available')).toBeTruthy()) + expect(screen.queryByText('Loading first prompt…')).toBeNull() + }) +}) diff --git a/src/renderer/src/components/right-sidebar/ai-vault-first-prompt-card.tsx b/src/renderer/src/components/right-sidebar/ai-vault-first-prompt-card.tsx index 819d29031d7..181a8dc0aca 100644 --- a/src/renderer/src/components/right-sidebar/ai-vault-first-prompt-card.tsx +++ b/src/renderer/src/components/right-sidebar/ai-vault-first-prompt-card.tsx @@ -7,6 +7,10 @@ import { translate } from '@/i18n/i18n' import type { AiVaultSession } from '../../../../shared/ai-vault-types' import { LOCAL_EXECUTION_HOST_ID } from '../../../../shared/execution-host' +// Why: the main-process re-parse has no deadline of its own. Without this the +// card can sit in `loading` forever on a huge or stalled transcript. +const FULL_FIRST_PROMPT_LOAD_TIMEOUT_MS = 15_000 + function canLoadFullFirstPrompt( session: Pick ): boolean { @@ -22,7 +26,11 @@ export function FirstPromptCard({ previewText }: { session: AiVaultSession - /** Short preview from list scan; replaced by the full on-demand body when available. */ + /** + * Short preview from the list scan; replaced by the full on-demand body when + * available. Empty unless the scan proved it really is the opening ask, so the + * fallback below can never show a recent turn (remote rows never re-parse). + */ previewText: string }): React.JSX.Element { // Loading starts true when an on-demand re-parse is possible so the mount effect @@ -47,24 +55,37 @@ export function FirstPromptCard({ } if (!canLoadFullFirstPrompt({ executionHostId, filePath })) { + // Deps can change to a non-loadable session after mount started `loading`. + setLoading(false) return Promise.resolve(null) } const getFirstUserPrompt = window.api.aiVault.getFirstUserPrompt const generation = generationRef.current const isStale = (): boolean => generationRef.current !== generation - const promise = getFirstUserPrompt({ - agent, - filePath, - sessionId, - executionHostId, - codexHome + let timeoutId: number | undefined + const deadline = new Promise((resolve) => { + timeoutId = window.setTimeout(() => { + resolve(null) + }, FULL_FIRST_PROMPT_LOAD_TIMEOUT_MS) }) + + const promise = Promise.race([ + getFirstUserPrompt({ + agent, + filePath, + sessionId, + executionHostId, + codexHome + }), + deadline + ]) .then((result) => { if (isStale()) { return null } - const prompt = result.prompt?.trim() || null + // A timed-out read lands here as null and falls back to the preview text. + const prompt = result?.prompt?.trim() || null fullTextRef.current = prompt setFullText(prompt) return prompt @@ -78,11 +99,13 @@ export function FirstPromptCard({ return null }) .finally(() => { + window.clearTimeout(timeoutId) // A stale settle must not clear the live request's dedupe handle. if (isStale()) { return } setLoading(false) + // Left null on timeout/failure so a later copy click can retry. loadPromiseRef.current = null }) @@ -97,6 +120,11 @@ export function FirstPromptCard({ void loadFullPrompt() return () => { generationRef.current += 1 + // Why: the bump above makes the in-flight request stale, and a stale settle + // deliberately skips setLoading(false). Drop the dedupe handle too, or the + // next pass (StrictMode's remount, or a prop change) would await that same + // stale promise and strand the card on "Loading first prompt…" forever. + loadPromiseRef.current = null } }, [loadFullPrompt]) diff --git a/src/renderer/src/components/right-sidebar/ai-vault-session-display.test.ts b/src/renderer/src/components/right-sidebar/ai-vault-session-display.test.ts index a6ef617a47c..2bd85ce29de 100644 --- a/src/renderer/src/components/right-sidebar/ai-vault-session-display.test.ts +++ b/src/renderer/src/components/right-sidebar/ai-vault-session-display.test.ts @@ -123,4 +123,31 @@ describe('ai vault session display', () => { }) ).toBeNull() }) + // Remote/SSH rows never re-parse on demand, so a truncated window would + // otherwise show a RECENT ask permanently labelled as the first prompt. + it('refuses the preview fallback once the sliding window has truncated', () => { + expect( + sessionFirstPrompt({ + ...baseSession, + previewMessagesTruncated: true, + previewMessages: [ + { role: 'user', text: 'Later user turn still in the preview window', timestamp: null }, + { role: 'assistant', text: 'Later reply', timestamp: null } + ] + }) + ).toBeNull() + }) + + it('still prefers a stored firstUserPrompt when the window has truncated', () => { + expect( + sessionFirstPrompt({ + ...baseSession, + previewMessagesTruncated: true, + firstUserPrompt: 'Original long first prompt that scrolled out of preview', + previewMessages: [ + { role: 'user', text: 'Later user turn still in the preview window', timestamp: null } + ] + }) + ).toBe('Original long first prompt that scrolled out of preview') + }) }) diff --git a/src/renderer/src/lib/ai-vault-resume-command.ts b/src/renderer/src/lib/ai-vault-resume-command.ts index 0a9bfbc20ed..8a1551b1ee6 100644 --- a/src/renderer/src/lib/ai-vault-resume-command.ts +++ b/src/renderer/src/lib/ai-vault-resume-command.ts @@ -1,9 +1,9 @@ +import type { AiVaultSession } from '../../../shared/ai-vault-types' import { buildAiVaultResumeCommand, buildAiVaultResumeShellCommand, - realHomeCodexResumeEnvDeletion, - type AiVaultSession -} from '../../../shared/ai-vault-types' + realHomeCodexResumeEnvDeletion +} from '../../../shared/ai-vault-resume-command' import { isResumableTuiAgent, type AgentProviderSessionMetadata, diff --git a/src/shared/ai-vault-resume-command.test.ts b/src/shared/ai-vault-resume-command.test.ts index 8ac4c7612fc..f1f06dabe7e 100644 --- a/src/shared/ai-vault-resume-command.test.ts +++ b/src/shared/ai-vault-resume-command.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from 'vitest' -import { buildAiVaultResumeCommand } from './ai-vault-types' +import { buildAiVaultResumeCommand } from './ai-vault-resume-command' describe('buildAiVaultResumeCommand', () => { it('uses Antigravity conversation ids instead of Gemini resume flags', () => { diff --git a/src/shared/ai-vault-resume-command.ts b/src/shared/ai-vault-resume-command.ts new file mode 100644 index 00000000000..d8406fb4ffe --- /dev/null +++ b/src/shared/ai-vault-resume-command.ts @@ -0,0 +1,199 @@ +// Resume-command construction for Agent Session History rows: turns a scanned +// session into the shell line that re-enters it, quoted for the target platform +// and (when known) the live tab's shell. +import { TUI_AGENT_CONFIG } from './tui-agent-config' +import { + commandSeparator, + quoteStartupArg, + type AgentStartupShell +} from './tui-agent-startup-shell' +import type { AiVaultAgent, AiVaultSession } from './ai-vault-types' + +export function buildAiVaultResumeCommand(args: { + agent: AiVaultAgent + sessionId: string + cwd: string | null + platform: NodeJS.Platform + commandOverride?: string | null + codexHome?: string | null + resumeFilePath?: string | null + shell?: AgentStartupShell +}): string { + const { agent, sessionId, cwd, platform, commandOverride, codexHome, resumeFilePath, shell } = + args + const baseCommand = commandOverride?.trim() || defaultAiVaultResumeCommandBase(agent) + // Why: OMP's `--resume` accepts an absolute transcript path, which resolves + // regardless of which session-dir root (custom OMP_CODING_AGENT_DIR / WSL + // home) the file was discovered under, where an id-prefix lookup scoped to + // the default store would miss it. Falls back to the id if no path is known. + const resumeTarget = agent === 'omp' && resumeFilePath?.trim() ? resumeFilePath.trim() : sessionId + const sessionArg = + shell === 'cmd' + ? quoteWindowsCmdArg(resumeTarget) + : shell + ? quoteStartupArg(resumeTarget, shell) + : quoteShellArg(resumeTarget, platform) + const resumeCommand = buildAgentResumeInvocation(agent, baseCommand, sessionArg) + + return buildAiVaultResumeShellCommand({ + resumeCommand, + cwd, + platform, + codexHome, + shell + }) +} + +export function buildAiVaultResumeShellCommand(args: { + resumeCommand: string + cwd: string | null + platform: NodeJS.Platform + codexHome?: string | null + // Why: the QUEUED resume command is typed into the live tab shell, so its + // cd/env prefix must match that shell. Shell-less persisted commands keep the + // legacy self-contained `cmd /d /s /c` wrapper. + shell?: AgentStartupShell +}): string { + const { cwd, platform, codexHome, shell } = args + + // Why: shell-aware commands are parsed by a known running shell, while + // shell-less persisted commands keep the legacy self-contained cmd wrapper. + if (platform === 'win32' && shell && shell !== 'cmd') { + return buildResumeShellCommandForShell({ + resumeCommand: args.resumeCommand, + cwd, + codexHome: codexHome?.trim() || null, + shell + }) + } + + const resumeCommand = `${codexHomeEnvPrefix(codexHome?.trim() || null, platform)}${ + args.resumeCommand + }` + if (platform === 'win32' && shell === 'cmd') { + // Why: an interactive cmd splits the doubled quotes required by a nested + // `cmd /s /c` wrapper, so queued commands must use direct cmd syntax. + return cwd ? `cd /d ${quoteWindowsCmdArg(cwd)} && ${resumeCommand}` : resumeCommand + } + if (!cwd) { + return resumeCommand + } + + if (platform === 'win32') { + const inner = `cd /d ${quoteWindowsCmdArg(cwd)} && ${resumeCommand}` + return `cmd /d /s /c ${quoteWindowsCmdArg(inner)}` + } + + return `cd ${quoteShellArg(cwd, platform)} && ${resumeCommand}` +} + +function buildResumeShellCommandForShell(args: { + resumeCommand: string + cwd: string | null + codexHome: string | null + shell: Exclude +}): string { + const { cwd, codexHome, shell } = args + if (shell === 'posix') { + // Why: git-bash on a Windows host runs a POSIX shell, so reuse the same + // inline-env + `cd ''` prefix as the non-Windows path. + const envPrefix = codexHome ? `CODEX_HOME=${quoteStartupArg(codexHome, shell)} ` : '' + const command = `${envPrefix}${args.resumeCommand}` + return cwd ? `cd ${quoteStartupArg(cwd, shell)} && ${command}` : command + } + + const separator = commandSeparator(shell) + const segments: string[] = [] + if (cwd) { + segments.push(`Set-Location -LiteralPath ${quoteStartupArg(cwd, shell)}`) + } + if (codexHome) { + segments.push(`$env:CODEX_HOME=${quoteStartupArg(codexHome, shell)}`) + } + segments.push(args.resumeCommand) + return segments.join(separator) +} + +// Why: a bare real-home resume carries no CODEX_HOME prefix, so every surface +// that spawns the pane must drop account-routed or daemon-inherited Codex +// homes from its env, not only patch a sparse env on top. +export function realHomeCodexResumeEnvDeletion( + session: Pick +): { envToDelete: string[] } | Record { + if (session.agent !== 'codex' || session.codexHome !== null) { + return {} + } + return { envToDelete: ['CODEX_HOME', 'ORCA_CODEX_HOME'] } +} + +function defaultAiVaultResumeCommandBase(agent: AiVaultAgent): string { + if (agent === 'cursor') { + return 'cursor-agent' + } + if (agent === 'hermes') { + return 'hermes' + } + if (agent === 'rovo') { + return 'acli' + } + return TUI_AGENT_CONFIG[agent].detectCmd +} + +function buildAgentResumeInvocation( + agent: AiVaultAgent, + baseCommand: string, + sessionArg: string +): string { + switch (agent) { + case 'codex': + return `${baseCommand} resume ${sessionArg}` + case 'rovo': + return `${baseCommand} rovodev run --restore ${sessionArg}` + case 'opencode': + case 'pi': + // Why: Kimi Code resumes with `kimi --session ` (alias `-S`). Sessions + // are work-dir-scoped, so the cwd prefix from buildAiVaultResumeCommand is + // required — resuming from another directory is rejected by the CLI. + // falls through + case 'kimi': + return `${baseCommand} --session ${sessionArg}` + case 'copilot': + return `${baseCommand} --resume=${sessionArg}` + case 'claude': + case 'cursor': + case 'gemini': + case 'grok': + case 'hermes': + case 'devin': + case 'openclaw': + case 'droid': + // Why: OMP resumes by absolute transcript path (see buildAiVaultResumeCommand), + // but the `--resume ` invocation form is identical to the others here. + // falls through + case 'omp': + return `${baseCommand} --resume ${sessionArg}` + case 'antigravity': + return `${baseCommand} --conversation ${sessionArg}` + } +} + +function codexHomeEnvPrefix(codexHome: string | null, platform: NodeJS.Platform): string { + if (!codexHome) { + return '' + } + if (platform === 'win32') { + return `set ${quoteWindowsCmdArg(`CODEX_HOME=${codexHome}`)} && ` + } + return `CODEX_HOME=${quoteShellArg(codexHome, platform)} ` +} + +function quoteShellArg(value: string, platform: NodeJS.Platform): string { + if (platform === 'win32') { + return quoteWindowsCmdArg(value) + } + return `'${value.replace(/'/g, `'\\''`)}'` +} + +function quoteWindowsCmdArg(value: string): string { + return `"${value.replace(/"/g, '""')}"` +} diff --git a/src/shared/ai-vault-session-display.ts b/src/shared/ai-vault-session-display.ts index c8862d90482..b45f43d2fe5 100644 --- a/src/shared/ai-vault-session-display.ts +++ b/src/shared/ai-vault-session-display.ts @@ -55,6 +55,13 @@ export function sessionFirstPrompt(session: AiVaultSession): string | null { return stored } + // Why: `previewMessages` is a newest-N sliding window. Once it has truncated, + // its earliest user turn is a RECENT ask, not the opening one — returning it + // would show (and copy) the wrong message under a "first prompt" label. + if (session.previewMessagesTruncated) { + return null + } + for (const message of session.previewMessages) { if (message.role !== 'user') { continue diff --git a/src/shared/ai-vault-types.ts b/src/shared/ai-vault-types.ts index a8adf9ddfd9..307352c5ddd 100644 --- a/src/shared/ai-vault-types.ts +++ b/src/shared/ai-vault-types.ts @@ -1,9 +1,3 @@ -import { TUI_AGENT_CONFIG } from './tui-agent-config' -import { - commandSeparator, - quoteStartupArg, - type AgentStartupShell -} from './tui-agent-startup-shell' import type { TuiAgent } from './types' import type { ExecutionHostId, ExecutionHostScope } from './execution-host' @@ -91,6 +85,9 @@ export type AiVaultSession = { messageCount: number totalTokens: number previewMessages: AiVaultSessionPreviewMessage[] + /** Older messages fell out of the newest-N window: the earliest preview turn + * is NOT the opening ask, so first-prompt consumers must not scan it. */ + previewMessagesTruncated?: boolean /** * Full first non-injected user prompt. List scans omit this (payload/perf); * populated only by on-demand `aiVault.getFirstUserPrompt` re-parses for copy. @@ -194,195 +191,6 @@ export type AiVaultListResult = { scannedAt: string } -export function buildAiVaultResumeCommand(args: { - agent: AiVaultAgent - sessionId: string - cwd: string | null - platform: NodeJS.Platform - commandOverride?: string | null - codexHome?: string | null - resumeFilePath?: string | null - shell?: AgentStartupShell -}): string { - const { agent, sessionId, cwd, platform, commandOverride, codexHome, resumeFilePath, shell } = - args - const baseCommand = commandOverride?.trim() || defaultAiVaultResumeCommandBase(agent) - // Why: OMP's `--resume` accepts an absolute transcript path, which resolves - // regardless of which session-dir root (custom OMP_CODING_AGENT_DIR / WSL - // home) the file was discovered under, where an id-prefix lookup scoped to - // the default store would miss it. Falls back to the id if no path is known. - const resumeTarget = agent === 'omp' && resumeFilePath?.trim() ? resumeFilePath.trim() : sessionId - const sessionArg = - shell === 'cmd' - ? quoteWindowsCmdArg(resumeTarget) - : shell - ? quoteStartupArg(resumeTarget, shell) - : quoteShellArg(resumeTarget, platform) - const resumeCommand = buildAgentResumeInvocation(agent, baseCommand, sessionArg) - - return buildAiVaultResumeShellCommand({ - resumeCommand, - cwd, - platform, - codexHome, - shell - }) -} - -export function buildAiVaultResumeShellCommand(args: { - resumeCommand: string - cwd: string | null - platform: NodeJS.Platform - codexHome?: string | null - // Why: the QUEUED resume command is typed into the live tab shell, so its - // cd/env prefix must match that shell. Shell-less persisted commands keep the - // legacy self-contained `cmd /d /s /c` wrapper. - shell?: AgentStartupShell -}): string { - const { cwd, platform, codexHome, shell } = args - - // Why: shell-aware commands are parsed by a known running shell, while - // shell-less persisted commands keep the legacy self-contained cmd wrapper. - if (platform === 'win32' && shell && shell !== 'cmd') { - return buildResumeShellCommandForShell({ - resumeCommand: args.resumeCommand, - cwd, - codexHome: codexHome?.trim() || null, - shell - }) - } - - const resumeCommand = `${codexHomeEnvPrefix(codexHome?.trim() || null, platform)}${ - args.resumeCommand - }` - if (platform === 'win32' && shell === 'cmd') { - // Why: an interactive cmd splits the doubled quotes required by a nested - // `cmd /s /c` wrapper, so queued commands must use direct cmd syntax. - return cwd ? `cd /d ${quoteWindowsCmdArg(cwd)} && ${resumeCommand}` : resumeCommand - } - if (!cwd) { - return resumeCommand - } - - if (platform === 'win32') { - const inner = `cd /d ${quoteWindowsCmdArg(cwd)} && ${resumeCommand}` - return `cmd /d /s /c ${quoteWindowsCmdArg(inner)}` - } - - return `cd ${quoteShellArg(cwd, platform)} && ${resumeCommand}` -} - -function buildResumeShellCommandForShell(args: { - resumeCommand: string - cwd: string | null - codexHome: string | null - shell: Exclude -}): string { - const { cwd, codexHome, shell } = args - if (shell === 'posix') { - // Why: git-bash on a Windows host runs a POSIX shell, so reuse the same - // inline-env + `cd ''` prefix as the non-Windows path. - const envPrefix = codexHome ? `CODEX_HOME=${quoteStartupArg(codexHome, shell)} ` : '' - const command = `${envPrefix}${args.resumeCommand}` - return cwd ? `cd ${quoteStartupArg(cwd, shell)} && ${command}` : command - } - - const separator = commandSeparator(shell) - const segments: string[] = [] - if (cwd) { - segments.push(`Set-Location -LiteralPath ${quoteStartupArg(cwd, shell)}`) - } - if (codexHome) { - segments.push(`$env:CODEX_HOME=${quoteStartupArg(codexHome, shell)}`) - } - segments.push(args.resumeCommand) - return segments.join(separator) -} - -// Why: a bare real-home resume carries no CODEX_HOME prefix, so every surface -// that spawns the pane must drop account-routed or daemon-inherited Codex -// homes from its env, not only patch a sparse env on top. -export function realHomeCodexResumeEnvDeletion( - session: Pick -): { envToDelete: string[] } | Record { - if (session.agent !== 'codex' || session.codexHome !== null) { - return {} - } - return { envToDelete: ['CODEX_HOME', 'ORCA_CODEX_HOME'] } -} - export function aiVaultAgentLabel(agent: AiVaultAgent): string { return AI_VAULT_AGENT_LABELS[agent] } - -function defaultAiVaultResumeCommandBase(agent: AiVaultAgent): string { - if (agent === 'cursor') { - return 'cursor-agent' - } - if (agent === 'hermes') { - return 'hermes' - } - if (agent === 'rovo') { - return 'acli' - } - return TUI_AGENT_CONFIG[agent].detectCmd -} - -function buildAgentResumeInvocation( - agent: AiVaultAgent, - baseCommand: string, - sessionArg: string -): string { - switch (agent) { - case 'codex': - return `${baseCommand} resume ${sessionArg}` - case 'rovo': - return `${baseCommand} rovodev run --restore ${sessionArg}` - case 'opencode': - case 'pi': - // Why: Kimi Code resumes with `kimi --session ` (alias `-S`). Sessions - // are work-dir-scoped, so the cwd prefix from buildAiVaultResumeCommand is - // required — resuming from another directory is rejected by the CLI. - // falls through - case 'kimi': - return `${baseCommand} --session ${sessionArg}` - case 'copilot': - return `${baseCommand} --resume=${sessionArg}` - case 'claude': - case 'cursor': - case 'gemini': - case 'grok': - case 'hermes': - case 'devin': - case 'openclaw': - case 'droid': - // Why: OMP resumes by absolute transcript path (see buildAiVaultResumeCommand), - // but the `--resume ` invocation form is identical to the others here. - // falls through - case 'omp': - return `${baseCommand} --resume ${sessionArg}` - case 'antigravity': - return `${baseCommand} --conversation ${sessionArg}` - } -} - -function codexHomeEnvPrefix(codexHome: string | null, platform: NodeJS.Platform): string { - if (!codexHome) { - return '' - } - if (platform === 'win32') { - return `set ${quoteWindowsCmdArg(`CODEX_HOME=${codexHome}`)} && ` - } - return `CODEX_HOME=${quoteShellArg(codexHome, platform)} ` -} - -function quoteShellArg(value: string, platform: NodeJS.Platform): string { - if (platform === 'win32') { - return quoteWindowsCmdArg(value) - } - return `'${value.replace(/'/g, `'\\''`)}'` -} - -function quoteWindowsCmdArg(value: string): string { - return `"${value.replace(/"/g, '""')}"` -} From 45a78e13da6320aa3b983ad39f29695607c30842 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sun, 2 Aug 2026 18:48:39 -0700 Subject: [PATCH 004/239] feat(editor): preserve PDF scroll position across tab switches (#12163) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(editor): preserve PDF scroll position across tab switches PDFs snapped back to page 1 on every tab switch — the one scrollable editor viewer never wired into the shared scroll cache. A raw scrollTop is the wrong unit here: PdfViewer resets zoom to page-width on each mount, and page-width resolves to a different absolute scale for a different container width, so a cached pixel offset restores to the wrong place. Store the pdf.js location ({ pageNumber, top, left }) instead — PDF user space, scale independent — and restore via scrollPageIntoView with the same XYZ destination pdf.js uses for its own scale-change restore. Scoped to the single-pane edit path: diff and conflict-review mount several viewers on one path, so they pass no key and keep no memory. Closes #12117 * fix(editor): defer PDF scroll recorder arm until the restore settles pdf.js dispatches the init `updateviewarea` synchronously from `update()` right after the `pagesinit` handler, so arming there recorded the restore's own provisional landing. On a mixed-page-size document that landing uses page-1 geometry and is wrong, and a tab switch before `pagesloaded` flushed it over the good cached position. Arm at the first of `pagesloaded` or first user input instead; arm immediately only when nothing was cached. If neither ever happens the recorder never arms and the cached position survives untouched, which is correct because the reader never moved. Co-authored-by: Orca * fix(editor): keep pdf.js location live after the PDF scroll re-apply Two defects in the restore half, both from calling scrollPageIntoView outside pdf.js's own scale-update path. `#scrollIntoView` nulls `_location` when `currentScaleValue` is unset, and it stays unset because 'page-width' resolves against a page list that is still empty at load. At `pagesinit` that is harmless (`_location` is already null and the init `update()` repopulates it), but the `pagesloaded` re-apply runs with a live `_location` and nothing recomputes it — on a uniform-page document the re-apply moves nothing, so no scroll event fires. The next zoom then found no location, fell back to the page top, and the recorder persisted that page-top position over the reader's offset. Call `update()` after the re-apply to recompute it. The user-moved guard also missed find: `PDFFindController` scrolls to a match programmatically, and the find bar is not inside the watched container, so neither the input listeners nor the arm gate saw a search. Searching during the pagesinit-to-pagesloaded window yanked the reader back to the cached page. Treat a 'find' dispatch as reader movement. Co-authored-by: Orca * test(editor): pin the PDF scroll cache wiring and drop a dead seam The key's journey from EditorContent through ImageViewer to PdfViewer had no coverage, so dropping the prop anywhere along it would have shipped as silently amnesiac scrolling with a green suite. Assert both directions: forwarded when supplied, null when omitted. Verified the tests fail when the forwarding is removed. Also pin the trailing-from-first-record debounce, which every existing test advanced past and so could be mutated into a restarting debounce undetected, and delete the `timers` injection seam — it had no callers, so only its two fallbacks ever ran. Co-authored-by: Orca * fix(editor): do not arm the PDF recorder in a hidden worktree pane Editors in background worktrees stay mounted under display:none so their layouts survive worktree switches. There the container has no layout box, so pdf.js bails out of scrollPageIntoView on a null offsetParent and update() early-returns with zero visible pages, leaving its location null. Arming anyway meant the reader's first zoom after switching to that worktree scrolled to the page top and persisted it over the cached position — turning a pane that merely failed to restore into one that destroys the saved position. Stay disarmed and keep the input watcher attached while the container has no height, so the cached entry survives until the reader is actually looking at the pane. A pane that gains layout between pagesinit and pagesloaded still restores normally. Co-authored-by: Orca * defer PDF scroll restore until hidden pane becomes visible Why: a display:none pane can't scroll or update pdf.js's internal location, so the reader's initial zoom persists over the cached position. Use ResizeObserver to detect when the pane regains a layout box, then re-apply the saved scroll. --------- Co-authored-by: Orca --- .../src/components/editor/EditorContent.tsx | 13 +- .../components/editor/ImageViewer.test.tsx | 45 ++++ .../src/components/editor/ImageViewer.tsx | 10 +- .../src/components/editor/PdfViewer.tsx | 142 ++++++++++- .../closed-editor-tab-cache-sweep.test.ts | 48 ++++ .../editor/closed-editor-tab-cache-sweep.ts | 23 ++ .../editor/pdf-view-position.test.ts | 224 ++++++++++++++++++ .../components/editor/pdf-view-position.ts | 146 ++++++++++++ .../editor/useClosedEditorTabCleanup.ts | 10 +- src/renderer/src/lib/scroll-cache.test.ts | 51 +++- src/renderer/src/lib/scroll-cache.ts | 6 + 11 files changed, 711 insertions(+), 7 deletions(-) create mode 100644 src/renderer/src/components/editor/closed-editor-tab-cache-sweep.test.ts create mode 100644 src/renderer/src/components/editor/closed-editor-tab-cache-sweep.ts create mode 100644 src/renderer/src/components/editor/pdf-view-position.test.ts create mode 100644 src/renderer/src/components/editor/pdf-view-position.ts diff --git a/src/renderer/src/components/editor/EditorContent.tsx b/src/renderer/src/components/editor/EditorContent.tsx index 52b079dc526..a9da4e54065 100644 --- a/src/renderer/src/components/editor/EditorContent.tsx +++ b/src/renderer/src/components/editor/EditorContent.tsx @@ -180,6 +180,12 @@ export function EditorContent({ viewStateScopeId === activeFile.id ? `${activeFile.id}:preview` : `${activeFile.id}::${viewStateScopeId}:preview` + // Why: only the single-pane edit path gets PDF scroll memory — the diff and + // conflict-review paths mount several viewers on one path (see PdfViewer). + const pdfViewStateKey = + viewStateScopeId === activeFile.id + ? `${activeFile.filePath}:pdf` + : `${activeFile.filePath}::${viewStateScopeId}:pdf` const monacoLanguage = resolvedLanguage === 'notebook' ? 'json' : resolvedLanguage const openConflictReviewFile = useAppStore((s) => s.openConflictReviewFile) @@ -740,7 +746,12 @@ export function EditorContent({ if (fc.isBinary) { if (fc.isImage) { return ( - + ) } return ( diff --git a/src/renderer/src/components/editor/ImageViewer.test.tsx b/src/renderer/src/components/editor/ImageViewer.test.tsx index 222d26701eb..cdd067ae0e3 100644 --- a/src/renderer/src/components/editor/ImageViewer.test.tsx +++ b/src/renderer/src/components/editor/ImageViewer.test.tsx @@ -154,6 +154,51 @@ function pngBase64(width: number): string { return bytes.toString('base64') } +async function renderPdfViewerProps( + scrollCacheKey?: string | null +): Promise> { + reactHookRuntime.index = 0 + const module = await import('./ImageViewer') + const rendered = expandNode( + module.default({ + content: 'JVBERi0xLjQK', + filePath: '/repo/report.pdf', + mimeType: 'application/pdf', + ...(scrollCacheKey === undefined ? {} : { scrollCacheKey }) + }) + ) + const [pdf] = findElementsByType(rendered, 'PdfViewer') + if (!pdf) { + throw new Error('PdfViewer not rendered') + } + return pdf.props +} + +describe('ImageViewer PDF scroll cache key', () => { + beforeEach(() => { + reactHookRuntime.states = [] + reactHookRuntime.index = 0 + vi.clearAllMocks() + }) + + // Why: nothing else pins the wiring, so a dropped prop anywhere between + // EditorContent and PdfViewer would ship as silently amnesiac scrolling. + it('forwards the scroll cache key to PdfViewer', async () => { + expect(await renderPdfViewerProps('/repo/report.pdf::tab-2:pdf')).toMatchObject({ + filePath: '/repo/report.pdf', + scrollCacheKey: '/repo/report.pdf::tab-2:pdf' + }) + }) + + it('passes null when no key is supplied, so unkeyed callers stay opted out', async () => { + expect((await renderPdfViewerProps()).scrollCacheKey).toBeNull() + }) + + it('passes an explicit null through unchanged', async () => { + expect((await renderPdfViewerProps(null)).scrollCacheKey).toBeNull() + }) +}) + describe('ImageViewer preview source retry', () => { beforeEach(() => { reactHookRuntime.states = [] diff --git a/src/renderer/src/components/editor/ImageViewer.tsx b/src/renderer/src/components/editor/ImageViewer.tsx index 07f49750b2d..81917eed2bb 100644 --- a/src/renderer/src/components/editor/ImageViewer.tsx +++ b/src/renderer/src/components/editor/ImageViewer.tsx @@ -29,13 +29,17 @@ type ImageViewerProps = { filePath: string mimeType?: string layout?: 'fill' | 'intrinsic' + // Why: absent means "no PDF scroll memory" — diff and conflict-review callers + // mount several viewers on one path, so they deliberately pass nothing. + scrollCacheKey?: string | null } export default function ImageViewer({ content, filePath, mimeType = FALLBACK_IMAGE_MIME_TYPE, - layout = 'fill' + layout = 'fill', + scrollCacheKey = null }: ImageViewerProps): JSX.Element { const [isPopupOpen, setIsPopupOpen] = useState(false) const [inlineZoom, setInlineZoom] = useState(1) @@ -210,7 +214,9 @@ export default function ImageViewer({ }, [isPopupOpen]) if (isPdf) { - return + return ( + + ) } if (imageError) { diff --git a/src/renderer/src/components/editor/PdfViewer.tsx b/src/renderer/src/components/editor/PdfViewer.tsx index f2d460d9faf..5009eaeb663 100644 --- a/src/renderer/src/components/editor/PdfViewer.tsx +++ b/src/renderer/src/components/editor/PdfViewer.tsx @@ -22,6 +22,12 @@ import { stepPdfScalePreference, type PdfScalePreference } from './pdf-scale-preference' +import { pdfViewPositionCache, setWithLRU } from '@/lib/scroll-cache' +import { + buildPdfScrollDestination, + clampPdfViewPosition, + createPdfViewPositionRecorder +} from './pdf-view-position' pdfjsLib.GlobalWorkerOptions.workerSrc = workerUrl @@ -30,12 +36,23 @@ const MAX_SCALE = 5 const SCALE_STEP = 1.25 const SCALE_BOUNDS = { min: MIN_SCALE, max: MAX_SCALE, step: SCALE_STEP } +// Why: these are the inputs that actually move this container's scroll; a +// window-level keydown would also fire for typing in an unrelated pane. +const USER_SCROLL_INPUT_EVENTS = ['wheel', 'touchstart', 'keydown', 'pointerdown'] as const + type PdfViewerProps = { content: string filePath: string + // Why: absent means "no scroll memory" — the diff and conflict-review callers + // mount several viewers on one path, so a shared key would cross-write. + scrollCacheKey?: string | null } -export default function PdfViewer({ content, filePath }: PdfViewerProps): JSX.Element { +export default function PdfViewer({ + content, + filePath, + scrollCacheKey = null +}: PdfViewerProps): JSX.Element { const containerRef = useRef(null) const viewerDivRef = useRef(null) const [pdfError, setPdfError] = useState(null) @@ -115,6 +132,115 @@ export default function PdfViewer({ content, filePath }: PdfViewerProps): JSX.El } eventBus.on('scalechanging', handleScaleChanging) + // Why: read the key from this effect's own closure, never a ref — the ref + // would already hold the next file's key by the time this setup runs. + const recorder = scrollCacheKey + ? createPdfViewPositionRecorder({ + key: scrollCacheKey, + write: (key, position) => setWithLRU(pdfViewPositionCache, key, position) + }) + : null + + const handleUpdateViewArea = (evt: { location?: unknown }): void => { + recorder?.record(evt?.location) + } + + let restored: ReturnType | null = null + let userMoved = false + let detachInputWatcher: (() => void) | null = null + const markUserMoved = (): void => { + userMoved = true + detachInputWatcher?.() + recorder?.arm() + } + + const handlePagesInit = (): void => { + const cached = scrollCacheKey ? pdfViewPositionCache.get(scrollCacheKey) : undefined + const clamped = cached ? clampPdfViewPosition(cached, viewer.pagesCount) : null + if (!clamped) { + recorder?.arm() + return + } + restored = buildPdfScrollDestination(clamped) + viewer.scrollPageIntoView(restored) + // Why: stays disarmed until the restore settles (below). pdf.js dispatches + // updateviewarea synchronously after this handler, so arming here would + // record the restore's own scroll — which on a mixed-page-size document is + // the provisional, wrong position, and a tab switch before pagesloaded + // would then flush it over the good cached one. + for (const type of USER_SCROLL_INPUT_EVENTS) { + container.addEventListener(type, markUserMoved, { passive: true }) + } + detachInputWatcher = (): void => { + detachInputWatcher = null + for (const type of USER_SCROLL_INPUT_EVENTS) { + container.removeEventListener(type, markUserMoved) + } + } + } + + let visibilityObserver: ResizeObserver | null = null + const disconnectVisibilityObserver = (): void => { + visibilityObserver?.disconnect() + visibilityObserver = null + } + // Why: a display:none pane regaining a layout box fires no scroll or resize + // event — ResizeObserver's no-box/box transition is the only signal for it. + const observeVisibility = (): void => { + if (visibilityObserver) { + return + } + visibilityObserver = new ResizeObserver(() => { + if (container.clientHeight > 0) { + handlePagesLoaded() + } + }) + visibilityObserver.observe(container) + } + + // Why: pagesinit lays every page out with page 1's dimensions, so on a + // mixed-page-size document the restore above lands short. pagesloaded is the + // first point with real per-page heights — but it can arrive seconds later, + // so re-apply only if the reader has not touched the scroller since. + const handlePagesLoaded = (): void => { + // Why: an editor in a background worktree stays mounted under display:none, + // where pdf.js can neither scroll nor recompute its location. Arming there + // would let the reader's first zoom persist a page-1 position over the + // cached one, so stay disarmed — and hold `restored` and the input watcher + // — until the observer above sees this pane land on screen. + if (container.clientHeight === 0) { + observeVisibility() + return + } + disconnectVisibilityObserver() + const destination = restored + restored = null + detachInputWatcher?.() + if (cancelled) { + return + } + if (destination && !userMoved) { + viewer.scrollPageIntoView(destination) + // Why: scrollPageIntoView nulls pdf.js's own `_location` whenever + // `currentScaleValue` is unset, and it stays unset here because + // page-width resolves against a not-yet-built page list. A null + // `_location` makes the next zoom fall back to scrolling the page top, + // losing the intra-page offset. update() recomputes it; on a + // uniform-page document the re-apply moves nothing, so no scroll event + // would otherwise fire to do it for us. + viewer.update() + } + recorder?.arm() + } + + eventBus.on('pagesinit', handlePagesInit) + eventBus.on('pagesloaded', handlePagesLoaded) + eventBus.on('updateviewarea', handleUpdateViewArea) + // Why: the find controller scrolls to a match programmatically, and the find + // bar sits outside this container — so a search is reader movement that no + // input listener above can see. + eventBus.on('find', markUserMoved) + const loadingTask = pdfjsLib.getDocument({ data: bytes }) loadingTask.promise @@ -142,6 +268,15 @@ export default function PdfViewer({ content, filePath }: PdfViewerProps): JSX.El return () => { cancelled = true + // Why: flush before setDocument(null) below, so nothing dispatched during + // pdf.js teardown can overwrite the position we just persisted. + detachInputWatcher?.() + disconnectVisibilityObserver() + recorder?.dispose() + eventBus.off('pagesinit', handlePagesInit) + eventBus.off('pagesloaded', handlePagesLoaded) + eventBus.off('updateviewarea', handleUpdateViewArea) + eventBus.off('find', markUserMoved) setFindOpen(false) loadingTask.destroy().catch(() => {}) if (pdfDocument) { @@ -158,7 +293,10 @@ export default function PdfViewer({ content, filePath }: PdfViewerProps): JSX.El findControllerRef.current = null pdfViewerRef.current = null } - }, [cleanedContent]) + // Why: scrollCacheKey is a dependency because two distinct paths can hold + // identical bytes — without it this effect would not re-run on the switch, + // and the second file would restore to the first file's position. + }, [cleanedContent, scrollCacheKey]) const closeFindBar = useCallback(() => { const eventBus = eventBusRef.current diff --git a/src/renderer/src/components/editor/closed-editor-tab-cache-sweep.test.ts b/src/renderer/src/components/editor/closed-editor-tab-cache-sweep.test.ts new file mode 100644 index 00000000000..1b975d81339 --- /dev/null +++ b/src/renderer/src/components/editor/closed-editor-tab-cache-sweep.test.ts @@ -0,0 +1,48 @@ +import { describe, expect, it } from 'vitest' +import type { PdfViewPosition } from '@/lib/scroll-cache' +import { sweepClosedPdfViewPositions } from './closed-editor-tab-cache-sweep' + +const position = (pageNumber: number): PdfViewPosition => ({ pageNumber, top: 0, left: 0 }) + +describe('sweepClosedPdfViewPositions', () => { + it('deletes the unscoped :pdf entry', () => { + const cache = new Map([['/a.pdf:pdf', position(4)]]) + sweepClosedPdfViewPositions(cache, '/a.pdf') + expect(cache.size).toBe(0) + }) + + it('deletes pane-scoped entries for the same file', () => { + const cache = new Map([ + ['/a.pdf:pdf', position(4)], + ['/a.pdf::tab-2:pdf', position(9)], + ['/a.pdf::tab-3:pdf', position(11)] + ]) + sweepClosedPdfViewPositions(cache, '/a.pdf') + expect(cache.size).toBe(0) + }) + + it('leaves other files untouched', () => { + const cache = new Map([ + ['/a.pdf:pdf', position(4)], + ['/b.pdf:pdf', position(7)], + ['/b.pdf::tab-2:pdf', position(8)] + ]) + sweepClosedPdfViewPositions(cache, '/a.pdf') + expect([...cache.keys()]).toEqual(['/b.pdf:pdf', '/b.pdf::tab-2:pdf']) + }) + + it('does not delete a different file that shares a path prefix', () => { + const cache = new Map([ + ['/report.pdf:pdf', position(2)], + ['/report.pdf.bak:pdf', position(3)] + ]) + sweepClosedPdfViewPositions(cache, '/report.pdf') + expect([...cache.keys()]).toEqual(['/report.pdf.bak:pdf']) + }) + + it('is a no-op when the file has no cached position', () => { + const cache = new Map([['/b.pdf:pdf', position(7)]]) + sweepClosedPdfViewPositions(cache, '/a.pdf') + expect(cache.size).toBe(1) + }) +}) diff --git a/src/renderer/src/components/editor/closed-editor-tab-cache-sweep.ts b/src/renderer/src/components/editor/closed-editor-tab-cache-sweep.ts new file mode 100644 index 00000000000..d53f0212723 --- /dev/null +++ b/src/renderer/src/components/editor/closed-editor-tab-cache-sweep.ts @@ -0,0 +1,23 @@ +import type { PdfViewPosition } from '@/lib/scroll-cache' + +function deleteCacheEntriesByPrefix(cache: Map, prefix: string): void { + for (const key of cache.keys()) { + if (key.startsWith(prefix)) { + cache.delete(key) + } + } +} + +/** + * Release the PDF positions a closed edit tab owns. Split out from the cleanup + * hook so it is testable without pulling in the hook's `monaco-editor` import. + */ +export function sweepClosedPdfViewPositions( + cache: Map, + filePath: string +): void { + // Why: the `::`-scoped sweep does not cover the single-colon suffix, so the + // unscoped key needs its own delete (same shape as :rich / :preview). + cache.delete(`${filePath}:pdf`) + deleteCacheEntriesByPrefix(cache, `${filePath}::`) +} diff --git a/src/renderer/src/components/editor/pdf-view-position.test.ts b/src/renderer/src/components/editor/pdf-view-position.test.ts new file mode 100644 index 00000000000..b4ea4b614d6 --- /dev/null +++ b/src/renderer/src/components/editor/pdf-view-position.test.ts @@ -0,0 +1,224 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { PdfViewPosition } from '@/lib/scroll-cache' +import { + buildPdfScrollDestination, + clampPdfViewPosition, + createPdfViewPositionRecorder, + PDF_POSITION_FLUSH_MS, + readPdfViewPosition +} from './pdf-view-position' + +describe('readPdfViewPosition', () => { + it('normalizes a well-formed updateviewarea location', () => { + expect( + readPdfViewPosition({ pageNumber: 12, top: 431, left: 62, scale: 1.5, rotation: 0 }) + ).toEqual({ pageNumber: 12, top: 431, left: 62 }) + }) + + it('accepts negative offsets (inter-page gap positions)', () => { + expect(readPdfViewPosition({ pageNumber: 3, top: -14, left: -2 })).toEqual({ + pageNumber: 3, + top: -14, + left: -2 + }) + }) + + it.each([ + ['null', null], + ['undefined', undefined], + ['a non-object', 'page 12'], + ['a number', 7] + ])('returns null for %s', (_label, input) => { + expect(readPdfViewPosition(input)).toBeNull() + }) + + it('returns null when top is missing', () => { + expect(readPdfViewPosition({ pageNumber: 2, left: 0 })).toBeNull() + }) + + it('returns null when left is missing', () => { + expect(readPdfViewPosition({ pageNumber: 2, top: 0 })).toBeNull() + }) + + it.each([ + ['NaN top', { pageNumber: 2, top: Number.NaN, left: 0 }], + ['Infinity left', { pageNumber: 2, top: 0, left: Number.POSITIVE_INFINITY }], + ['NaN pageNumber', { pageNumber: Number.NaN, top: 0, left: 0 }] + ])('returns null for %s', (_label, input) => { + expect(readPdfViewPosition(input)).toBeNull() + }) + + it('returns null for pageNumber 0', () => { + expect(readPdfViewPosition({ pageNumber: 0, top: 0, left: 0 })).toBeNull() + }) + + it('returns null for a fractional pageNumber', () => { + expect(readPdfViewPosition({ pageNumber: 2.5, top: 0, left: 0 })).toBeNull() + }) + + it('preserves a top-of-page-1 position verbatim', () => { + // Why: `top` is a bottom-origin PDF coordinate, so the top of a Letter page + // is ~792, not 0. Nothing may treat this as a "document start" sentinel. + expect(readPdfViewPosition({ pageNumber: 1, top: 792, left: 0 })).toEqual({ + pageNumber: 1, + top: 792, + left: 0 + }) + }) +}) + +describe('buildPdfScrollDestination', () => { + it('emits the 5-element XYZ destArray with left and top in slots 2 and 3', () => { + expect(buildPdfScrollDestination({ pageNumber: 12, top: 431, left: 62 })).toEqual({ + pageNumber: 12, + destArray: [null, { name: 'XYZ' }, 62, 431, null], + ignoreDestinationZoom: true, + allowNegativeOffset: true + }) + }) + + it('always sets both flags', () => { + const destination = buildPdfScrollDestination({ pageNumber: 1, top: 0, left: 0 }) + expect(destination.ignoreDestinationZoom).toBe(true) + expect(destination.allowNegativeOffset).toBe(true) + }) +}) + +describe('clampPdfViewPosition', () => { + const position = { pageNumber: 40, top: 100, left: 10 } + + it('clamps a page beyond the document to the last page', () => { + expect(clampPdfViewPosition(position, 12)).toEqual({ pageNumber: 12, top: 100, left: 10 }) + }) + + it('returns an in-range position untouched', () => { + const inRange = { pageNumber: 5, top: 100, left: 10 } + expect(clampPdfViewPosition(inRange, 12)).toBe(inRange) + }) + + it('returns null when the document reports no pages', () => { + expect(clampPdfViewPosition(position, 0)).toBeNull() + }) + + it('returns null for a non-finite page count', () => { + expect(clampPdfViewPosition(position, Number.NaN)).toBeNull() + }) + + it('preserves top and left while clamping the page', () => { + const clamped = clampPdfViewPosition(position, 3) + expect(clamped).toEqual({ pageNumber: 3, top: 100, left: 10 }) + }) +}) + +describe('createPdfViewPositionRecorder', () => { + let writes: [string, PdfViewPosition][] + + const makeRecorder = (key = 'a.pdf:pdf') => + createPdfViewPositionRecorder({ + key, + write: (writtenKey, position) => writes.push([writtenKey, position]) + }) + + const location = (pageNumber: number) => ({ pageNumber, top: 400, left: 20, scale: 1 }) + + beforeEach(() => { + writes = [] + vi.useFakeTimers() + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it('ignores records before arm, so a pre-restore scroll cannot overwrite the cache', () => { + const recorder = makeRecorder() + recorder.record(location(1)) + vi.advanceTimersByTime(PDF_POSITION_FLUSH_MS * 2) + recorder.dispose() + expect(writes).toEqual([]) + }) + + it('disposing without any record is a strict no-op (StrictMode double-mount)', () => { + const recorder = makeRecorder() + recorder.arm() + recorder.dispose() + expect(writes).toEqual([]) + }) + + it('flushes the last recorded position on dispose', () => { + const recorder = makeRecorder() + recorder.arm() + recorder.record(location(7)) + recorder.dispose() + expect(writes).toEqual([['a.pdf:pdf', { pageNumber: 7, top: 400, left: 20 }]]) + }) + + it('fires the debounced write once and reflects the latest of several rapid records', () => { + const recorder = makeRecorder() + recorder.arm() + recorder.record(location(2)) + recorder.record(location(3)) + recorder.record(location(4)) + expect(writes).toEqual([]) + vi.advanceTimersByTime(PDF_POSITION_FLUSH_MS) + expect(writes).toEqual([['a.pdf:pdf', { pageNumber: 4, top: 400, left: 20 }]]) + }) + + // Why: the timer is deliberately trailing-from-first-record, not a restarting + // debounce — a restarting one would starve the LRU refresh under continuous + // scrolling, which is exactly when the entry most needs to stay warm. + it('does not restart the pending timer when a later record arrives', () => { + const recorder = makeRecorder() + recorder.arm() + recorder.record(location(2)) + vi.advanceTimersByTime(PDF_POSITION_FLUSH_MS - 50) + recorder.record(location(3)) + vi.advanceTimersByTime(50) + expect(writes).toEqual([['a.pdf:pdf', { pageNumber: 3, top: 400, left: 20 }]]) + }) + + it('ignores a malformed location without clearing the last good position', () => { + const recorder = makeRecorder() + recorder.arm() + recorder.record(location(5)) + recorder.record({ pageNumber: Number.NaN, top: 1, left: 1 }) + recorder.dispose() + expect(writes.at(-1)).toEqual(['a.pdf:pdf', { pageNumber: 5, top: 400, left: 20 }]) + }) + + it('only ever writes under the key it was constructed with', () => { + const recorder = makeRecorder('b.pdf::tab-2:pdf') + recorder.arm() + recorder.record(location(9)) + recorder.dispose() + expect(writes).toEqual([['b.pdf::tab-2:pdf', { pageNumber: 9, top: 400, left: 20 }]]) + }) + + it('cancels the pending timer on dispose so teardown writes exactly once', () => { + const recorder = makeRecorder() + recorder.arm() + recorder.record(location(6)) + recorder.dispose() + vi.advanceTimersByTime(PDF_POSITION_FLUSH_MS * 2) + expect(writes).toHaveLength(1) + }) + + it('record after dispose schedules no timer and writes nothing', () => { + const recorder = makeRecorder() + recorder.arm() + recorder.record(location(6)) + recorder.dispose() + recorder.record(location(99)) + vi.advanceTimersByTime(PDF_POSITION_FLUSH_MS * 2) + expect(writes).toEqual([['a.pdf:pdf', { pageNumber: 6, top: 400, left: 20 }]]) + }) + + it('disposing twice does not write twice', () => { + const recorder = makeRecorder() + recorder.arm() + recorder.record(location(6)) + recorder.dispose() + recorder.dispose() + expect(writes).toHaveLength(1) + }) +}) diff --git a/src/renderer/src/components/editor/pdf-view-position.ts b/src/renderer/src/components/editor/pdf-view-position.ts new file mode 100644 index 00000000000..d4cc107cbd1 --- /dev/null +++ b/src/renderer/src/components/editor/pdf-view-position.ts @@ -0,0 +1,146 @@ +import type { PdfViewPosition } from '@/lib/scroll-cache' + +/** Argument object for pdf.js `PDFViewer.scrollPageIntoView`. */ +export type PdfScrollDestination = { + pageNumber: number + destArray: unknown[] + ignoreDestinationZoom: true + allowNegativeOffset: true +} + +function isFiniteNumber(value: unknown): value is number { + return typeof value === 'number' && Number.isFinite(value) +} + +/** + * Normalize an `updateviewarea` payload's `location`. pdf.js does not type + * EventBus payloads, so this is the validation boundary — a pdf.js upgrade that + * reshapes the payload degrades to "no restore" rather than throwing. + */ +export function readPdfViewPosition(location: unknown): PdfViewPosition | null { + if (typeof location !== 'object' || location === null) { + return null + } + const { pageNumber, top, left } = location as Record + // Why: scrollPageIntoView itself guards on Number.isInteger and silently + // logs-and-returns, so a fractional page would fail the restore invisibly. + if (!isFiniteNumber(pageNumber) || !Number.isInteger(pageNumber) || pageNumber < 1) { + return null + } + if (!isFiniteNumber(top) || !isFiniteNumber(left)) { + return null + } + return { pageNumber, top, left } +} + +/** + * Build the XYZ destination pdf.js uses for its own scale-change position + * restore (`pdf_viewer.mjs:8843-8853`). + */ +export function buildPdfScrollDestination(position: PdfViewPosition): PdfScrollDestination { + return { + pageNumber: position.pageNumber, + // Why: slot 0 (page ref) is never read on this path; the trailing null zoom + // pairs with ignoreDestinationZoom so the destination cannot overwrite scale. + destArray: [null, { name: 'XYZ' }, position.left, position.top, null], + ignoreDestinationZoom: true, + // Why: a position in the inter-page gap transforms to a negative offset; + // clamping it would land at the page top instead, off by the gap height. + allowNegativeOffset: true + } +} + +/** + * Clamp a cached page against a document that shrank on disk. Not a safety + * guard — pdf.js already ignores an out-of-range page — this just lands the + * reader on the last page instead of nowhere. + */ +export function clampPdfViewPosition( + position: PdfViewPosition, + pagesCount: number +): PdfViewPosition | null { + if (!isFiniteNumber(pagesCount) || pagesCount < 1) { + return null + } + const pageNumber = Math.min(Math.max(position.pageNumber, 1), Math.floor(pagesCount)) + return pageNumber === position.pageNumber ? position : { ...position, pageNumber } +} + +export const PDF_POSITION_FLUSH_MS = 150 + +type TimerHandle = ReturnType + +export type PdfViewPositionRecorder = { + arm: () => void + record: (location: unknown) => void + dispose: () => void +} + +/** + * Owns the record half of PDF scroll preservation: the arm gate and the + * trailing debounce. Extracted from `PdfViewer` because the effect itself is + * untestable (node vitest env, and ImageViewer.test mocks useEffect), so every + * race guard below would otherwise ship uncovered. + * + * `dispose` is terminal: afterwards `record` is inert, so a late timer can + * never resurrect an entry `useClosedEditorTabCleanup` just swept. + */ +export function createPdfViewPositionRecorder({ + key, + write +}: { + key: string + write: (key: string, position: PdfViewPosition) => void +}): PdfViewPositionRecorder { + let armed = false + let disposed = false + let pending: PdfViewPosition | null = null + let timer: TimerHandle | null = null + + // Why: a strict no-op when nothing was recorded — writing here would clobber a + // good cached position on a StrictMode double-mount, whose cleanup runs before + // the reader has scrolled at all. + const flush = (): void => { + if (pending) { + write(key, pending) + } + } + + return { + arm: () => { + armed = true + }, + + record: (location) => { + if (!armed || disposed) { + return + } + const position = readPdfViewPosition(location) + if (!position) { + return + } + pending = position + if (timer !== null) { + return + } + // Why: the debounced write only refreshes LRU recency — teardown is what + // actually persists the position — so a pending timer need not restart. + timer = setTimeout(() => { + timer = null + flush() + }, PDF_POSITION_FLUSH_MS) + }, + + dispose: () => { + if (disposed) { + return + } + disposed = true + if (timer !== null) { + clearTimeout(timer) + timer = null + } + flush() + } + } +} diff --git a/src/renderer/src/components/editor/useClosedEditorTabCleanup.ts b/src/renderer/src/components/editor/useClosedEditorTabCleanup.ts index f220230325d..cdf0d7f7cd2 100644 --- a/src/renderer/src/components/editor/useClosedEditorTabCleanup.ts +++ b/src/renderer/src/components/editor/useClosedEditorTabCleanup.ts @@ -1,11 +1,17 @@ import { useEffect, useRef } from 'react' import * as monaco from 'monaco-editor' import type { OpenFile } from '@/store/slices/editor' -import { cursorPositionCache, diffViewStateCache, scrollTopCache } from '@/lib/scroll-cache' +import { + cursorPositionCache, + diffViewStateCache, + pdfViewPositionCache, + scrollTopCache +} from '@/lib/scroll-cache' import { disposeUnattachedMonacoModelsByPathPrefix, getDiffViewerMonacoModelPathPrefixes } from './diff-monaco-model-disposal' +import { sweepClosedPdfViewPositions } from './closed-editor-tab-cache-sweep' function deleteCacheEntriesByPrefix(cache: Map, prefix: string): void { for (const key of cache.keys()) { @@ -43,6 +49,8 @@ function disposeClosedEditorTab(prevId: string, prevFile: OpenFile): void { scrollTopCache.delete(`${prevFile.filePath}:mermaid-diagram`) cursorPositionCache.delete(prevFile.filePath) deleteCacheEntriesByPrefix(cursorPositionCache, `${prevFile.filePath}::`) + // Why: only 'edit' tabs ever get a PDF scroll key (see EditorContent). + sweepClosedPdfViewPositions(pdfViewPositionCache, prevFile.filePath) break case 'markdown-preview': // Why: preview tabs own pane-scoped preview scroll cache entries even diff --git a/src/renderer/src/lib/scroll-cache.test.ts b/src/renderer/src/lib/scroll-cache.test.ts index 8e09f8bdf77..98f5021541a 100644 --- a/src/renderer/src/lib/scroll-cache.test.ts +++ b/src/renderer/src/lib/scroll-cache.test.ts @@ -1,10 +1,17 @@ import { describe, expect, it, beforeEach } from 'vitest' -import { cursorPositionCache, diffViewStateCache, setWithLRU, scrollTopCache } from './scroll-cache' +import { + cursorPositionCache, + diffViewStateCache, + pdfViewPositionCache, + setWithLRU, + scrollTopCache +} from './scroll-cache' beforeEach(() => { scrollTopCache.clear() cursorPositionCache.clear() diffViewStateCache.clear() + pdfViewPositionCache.clear() }) describe('setWithLRU', () => { @@ -113,6 +120,48 @@ describe('scrollTopCache', () => { }) }) +describe('pdfViewPositionCache', () => { + it('is an empty Map on import', () => { + expect(pdfViewPositionCache).toBeInstanceOf(Map) + expect(pdfViewPositionCache.size).toBe(0) + }) + + it('round-trips pageNumber, top, and left intact', () => { + setWithLRU(pdfViewPositionCache, '/doc.pdf:pdf', { pageNumber: 12, top: 431.5, left: 62 }) + expect(pdfViewPositionCache.get('/doc.pdf:pdf')).toEqual({ + pageNumber: 12, + top: 431.5, + left: 62 + }) + }) + + it('keeps pane-scoped keys for the same file independent', () => { + setWithLRU(pdfViewPositionCache, '/doc.pdf:pdf', { pageNumber: 3, top: 700, left: 0 }) + setWithLRU(pdfViewPositionCache, '/doc.pdf::tab-2:pdf', { pageNumber: 40, top: 120, left: 0 }) + expect(pdfViewPositionCache.get('/doc.pdf:pdf')?.pageNumber).toBe(3) + expect(pdfViewPositionCache.get('/doc.pdf::tab-2:pdf')?.pageNumber).toBe(40) + }) + + it('evicts the oldest entry at the default cap of 20', () => { + for (let i = 0; i <= 20; i++) { + setWithLRU(pdfViewPositionCache, `/doc-${i}.pdf:pdf`, { pageNumber: i, top: 0, left: 0 }) + } + expect(pdfViewPositionCache.size).toBe(20) + expect(pdfViewPositionCache.has('/doc-0.pdf:pdf')).toBe(false) + expect(pdfViewPositionCache.has('/doc-20.pdf:pdf')).toBe(true) + }) + + it('refreshes recency when an existing PDF key is re-set', () => { + for (let i = 0; i < 20; i++) { + setWithLRU(pdfViewPositionCache, `/doc-${i}.pdf:pdf`, { pageNumber: i, top: 0, left: 0 }) + } + setWithLRU(pdfViewPositionCache, '/doc-0.pdf:pdf', { pageNumber: 99, top: 0, left: 0 }) + setWithLRU(pdfViewPositionCache, '/doc-new.pdf:pdf', { pageNumber: 1, top: 0, left: 0 }) + expect(pdfViewPositionCache.get('/doc-0.pdf:pdf')?.pageNumber).toBe(99) + expect(pdfViewPositionCache.has('/doc-1.pdf:pdf')).toBe(false) + }) +}) + describe('diffViewStateCache', () => { it('is an empty Map on import', () => { expect(diffViewStateCache).toBeInstanceOf(Map) diff --git a/src/renderer/src/lib/scroll-cache.ts b/src/renderer/src/lib/scroll-cache.ts index a00df35ce06..fba5eec01f7 100644 --- a/src/renderer/src/lib/scroll-cache.ts +++ b/src/renderer/src/lib/scroll-cache.ts @@ -40,6 +40,12 @@ export const scrollTopCache = new Map() // re-renders on every cursor move. export const cursorPositionCache = new Map() +// Why: PDFs store a pdf.js location in PDF user space, not a scrollTop — page +// layout is rebuilt at a scale that depends on container width, so a pixel +// offset restores to the wrong place at a different zoom or pane width. +export type PdfViewPosition = { pageNumber: number; top: number; left: number } +export const pdfViewPositionCache = new Map() + // Why: Diff editors need more than a numeric scroll offset to restore the same // working context. Monaco's diff view state also carries cursor/selection state // for both sides plus diff model state, which matches VS Code's restore path From a20165a43db63046d2aedd126ac768a14289d635 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sun, 2 Aug 2026 18:53:22 -0700 Subject: [PATCH 005/239] fix(browser): import __Host- cookies host-only so Chromium keeps them (#12166) * fix(browser): import __Host- cookies host-only so Chromium keeps them The cookie file/JSON import path passed a Domain attribute for every cookie. Chromium rejects any __Host--prefixed cookie that carries a Domain (the prefix requires host-only, path=/, Secure), so file import silently dropped session cookies like GitHub's __Host-user_session_same_site and users stayed logged out after importing. Mirror the browser-native import path, which already shapes __Host- cookies host-only: omit domain and force path=/ when the name is __Host--prefixed. Add a regression test covering both a __Host- cookie and a normal domain cookie in one import. * test(browser): cover __Host- cookie payload constraints * test(browser): preserve ordinary cookie paths --- .../browser/browser-cookie-import.test.ts | 29 +++++++++++++++++++ src/main/browser/browser-cookie-import.ts | 6 ++-- 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/src/main/browser/browser-cookie-import.test.ts b/src/main/browser/browser-cookie-import.test.ts index 55fae27c93e..657215a6fbc 100644 --- a/src/main/browser/browser-cookie-import.test.ts +++ b/src/main/browser/browser-cookie-import.test.ts @@ -223,6 +223,35 @@ describe('importCookiesFromFile', () => { expect(firstCall.sameSite).toBe('lax') }) + it('sets __Host- cookies host-only so Chromium does not reject them', async () => { + const filePath = writeCookieFile([ + { + domain: 'github.com', + name: '__Host-user_session_same_site', + value: 'sess', + path: '/account', + secure: true, + httpOnly: true, + sameSite: 'lax' + }, + { domain: '.github.com', name: '_gh_sess', value: 'abc', path: '/settings', secure: true } + ]) + + const result = await importCookiesFromFile(filePath, 'persist:test') + expect(result.ok).toBe(true) + + const hostCall = cookiesSetMock.mock.calls + .map((c) => c[0]) + .find((c) => c.name === '__Host-user_session_same_site') + // __Host- prefix requires no Domain attribute and path=/, or Chromium drops it. + expect(hostCall).not.toHaveProperty('domain') + expect(hostCall.path).toBe('/') + + const normalCall = cookiesSetMock.mock.calls.map((c) => c[0]).find((c) => c.name === '_gh_sess') + expect(normalCall.domain).toBe('.github.com') + expect(normalCall.path).toBe('/settings') + }) + it('rejects non-JSON files', async () => { const filePath = join(tmpDir, 'bad.json') writeFileSync(filePath, 'not json at all') diff --git a/src/main/browser/browser-cookie-import.ts b/src/main/browser/browser-cookie-import.ts index 22b891cd2a0..23bdd248777 100644 --- a/src/main/browser/browser-cookie-import.ts +++ b/src/main/browser/browser-cookie-import.ts @@ -567,12 +567,14 @@ async function importValidatedCookies( for (const cookie of cookies) { try { + // Why: Chromium rejects __Host- cookies unless they omit domain and use path=/. + const isHostPrefixed = cookie.name.startsWith('__Host-') await targetSession.cookies.set({ url: cookie.url, name: cookie.name, value: stripNonPrintable(cookie.value), - domain: cookie.domain, - path: cookie.path, + ...(isHostPrefixed ? {} : { domain: cookie.domain }), + path: isHostPrefixed ? '/' : cookie.path, secure: cookie.secure, httpOnly: cookie.httpOnly, sameSite: cookie.sameSite, From bcf7d1df1b1da308aef2b8b5c0fc600b3100d0ca Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sun, 2 Aug 2026 18:54:31 -0700 Subject: [PATCH 006/239] fix(codex): execute accepted account-switch restarts for unmounted panes (#12146) * fix(codex): execute accepted account-switch restarts for unmounted panes Accepting the Codex account-switch restart prompt queued every awaiting pane but only mounted TerminalPane instances executed the queue, so background-tab and parked panes stayed input-blocked on the old account with no prompt. - Add a detached store-level driver (codex-detached-pane-restart) that kill-and-respawns any queued pane no mounted transport claims, rebinding tab/layout state so a later mount reattaches to the replacement PTY. - Re-offer the prompt when a detached execution fails, and clear the notice when the pane is gone, so input is never silently blocked. - Sweep restored PTY ids at startup so stale panes in never-mounted tabs are re-offered after an app restart. - Carry launchAgent codex on the restart respawn so it waits for managed-auth readiness and records the pane's launch account. * fix(codex): fence detached restart ownership races * fix(codex): unblock detached restart handoffs * fix(codex): contain detached restart cleanup * fix(codex): detach restart progress from cleanup * fix(codex): avoid detached restart size wait * fix(terminal): scope Codex restart prompt to pane * fix(codex): contain detached restart sweep failures --- src/renderer/src/App.tsx | 7 + .../src/components/CodexRestartChip.tsx | 127 +---- src/renderer/src/components/Terminal.tsx | 3 - .../codex-restart-chip-inputs.test.ts | 83 --- .../components/codex-restart-chip-inputs.ts | 41 -- .../src/components/codex-restart-chip.test.ts | 486 ------------------ .../components/codex-restart-chip.test.tsx | 295 +++++++++++ .../components/terminal-pane/TerminalPane.tsx | 22 + .../codex-detached-pane-restart-scheduler.ts | 94 ++++ .../codex-detached-pane-restart.test.ts | 441 ++++++++++++++++ .../codex-detached-pane-restart.ts | 342 ++++++++++++ .../src/lib/codex-session-restart.test.ts | 7 +- src/renderer/src/lib/codex-session-restart.ts | 7 +- .../src/lib/codex-stale-pane-sweep.test.ts | 25 +- .../src/lib/codex-stale-pane-sweep.ts | 19 + .../codex-restart-notice-lifecycle.test.ts | 59 +++ src/renderer/src/store/slices/terminals.ts | 39 ++ 17 files changed, 1377 insertions(+), 720 deletions(-) delete mode 100644 src/renderer/src/components/codex-restart-chip-inputs.test.ts delete mode 100644 src/renderer/src/components/codex-restart-chip-inputs.ts delete mode 100644 src/renderer/src/components/codex-restart-chip.test.ts create mode 100644 src/renderer/src/components/codex-restart-chip.test.tsx create mode 100644 src/renderer/src/components/terminal-pane/codex-detached-pane-restart-scheduler.ts create mode 100644 src/renderer/src/components/terminal-pane/codex-detached-pane-restart.test.ts create mode 100644 src/renderer/src/components/terminal-pane/codex-detached-pane-restart.ts diff --git a/src/renderer/src/App.tsx b/src/renderer/src/App.tsx index 179108b194b..a57e7c90f53 100644 --- a/src/renderer/src/App.tsx +++ b/src/renderer/src/App.tsx @@ -123,6 +123,8 @@ import { shouldPersistWorkspaceSession } from './lib/workspace-session' import { createSessionWriteSubscriber } from './lib/session-write-subscriber' +import { sweepRestoredCodexPanesForStaleAccounts } from './lib/codex-stale-pane-sweep' +import { installCodexDetachedPaneRestartExecutor } from '@/components/terminal-pane/codex-detached-pane-restart-scheduler' import { buildActiveViewUnloadPatch } from './lib/active-view-persist' import { buildWorkspaceSessionHostSnapshots, @@ -1090,6 +1092,9 @@ function App(): React.JSX.Element { await timeRendererStartupStep('recover-legacy-worker-terminals-post-reconnect', () => window.api.app.recoverLegacyWorkerTerminalsForRendererStartup() ) + // Why here: reconnect just published restored PTY ids; sweeping them now + // re-offers stale Codex panes whose tabs never mount this session. + sweepRestoredCodexPanesForStaleAccounts(useAppStore.getState()) syncZoomCSSVar() // Why (issue #1158): unlock the session writer only after hydration and all dependent steps succeeded, so a mid-startup throw can't serialize partially-mutated state to disk. actions.setHydrationSucceeded(true) @@ -1210,6 +1215,8 @@ function App(): React.JSX.Element { } }, []) + useEffect(() => installCodexDetachedPaneRestartExecutor(), []) + useEffect(() => { let previousKey = getRuntimeMobileSessionSyncKey(useAppStore.getState()) return useAppStore.subscribe((state, previousState) => { diff --git a/src/renderer/src/components/CodexRestartChip.tsx b/src/renderer/src/components/CodexRestartChip.tsx index 4df9c54a9ed..09835b71302 100644 --- a/src/renderer/src/components/CodexRestartChip.tsx +++ b/src/renderer/src/components/CodexRestartChip.tsx @@ -1,70 +1,11 @@ -import { useEffect, useId, useMemo, useRef } from 'react' -import { useShallow } from 'zustand/react/shallow' +import { useEffect, useId, useRef } from 'react' import { RefreshCw } from 'lucide-react' import { Button } from '@/components/ui/button' import { useAppStore } from '../store' -import { selectCodexRestartInputs } from './codex-restart-chip-inputs' import { translate } from '@/i18n/i18n' import { shouldFocusMobileDriverAction } from './terminal-pane/mobile-driver-overlay-focus' import { buildCodexRestartNoticeKey } from './codex-restart-notice-key' import { awaitsCodexRestartAnswer } from './codex-restart-notice-state' -import type { CodexRestartNotice } from '../store/slices/terminals' - -const EMPTY_TABS: { id: string }[] = [] - -export function collectStalePtyIdsForTabs({ - tabs, - ptyIdsByTabId, - codexRestartNoticeByPtyId -}: { - tabs: { id: string }[] - ptyIdsByTabId: Record - codexRestartNoticeByPtyId: Record -}): string[] { - // Why: an already-requested restart runs when its pane next mounts. Keeping it - // out of the prompt is what stops the panel from sticking on a worktree whose - // stale pane is parked or deferred and cannot answer the request yet. A - // dismissed notice is likewise answered — it only survives as launch-account - // memory. - return tabs.flatMap((tab) => - (ptyIdsByTabId[tab.id] ?? []).filter((ptyId) => - awaitsCodexRestartAnswer(codexRestartNoticeByPtyId[ptyId]) - ) - ) -} - -export function collectStaleWorktreePtyIds({ - tabsByWorktree, - ptyIdsByTabId, - codexRestartNoticeByPtyId, - worktreeId -}: { - tabsByWorktree: Record - ptyIdsByTabId: Record - codexRestartNoticeByPtyId: Record - worktreeId: string -}): string[] { - return collectStalePtyIdsForTabs({ - tabs: tabsByWorktree[worktreeId] ?? EMPTY_TABS, - ptyIdsByTabId, - codexRestartNoticeByPtyId - }) -} - -export function dismissStaleWorktreePtyIds( - staleWorktreePtyIds: string[], - dismissCodexRestartNotices: (ptyIds: string[]) => void, - forgetLaunchAccounts: (ptyIds: string[]) => void -): void { - // Why: restart notices are stored per PTY, but the workspace host presents - // one shared prompt. Dismissing all matching PTY notices keeps every pane in - // that worktree consistent with the dismissal. - dismissCodexRestartNotices(staleWorktreePtyIds) - // Why: notices are renderer-only, so without dropping the on-disk launch - // record the startup sweep re-raises this exact prompt — and re-blocks the - // pane's input — after every app restart the user already answered. - forgetLaunchAccounts(staleWorktreePtyIds) -} function isInsideHiddenTree(element: HTMLElement): boolean { return element.closest('[aria-hidden="true"], [hidden], [inert]') !== null @@ -78,58 +19,39 @@ type RestartNotice = { export default function CodexRestartChip({ isVisible = true, - worktreeId + ptyId, + shouldFocus = false }: { isVisible?: boolean - worktreeId: string + ptyId: string + shouldFocus?: boolean }): React.JSX.Element | null { - const tabs = useAppStore((s) => s.tabsByWorktree[worktreeId] ?? EMPTY_TABS) - // Why: both of these maps churn on unrelated pty lifecycle events (ptyIdsByTabId - // on attach/detach; codexRestartNoticeByPtyId is re-spread even when empty on - // pty teardown), so subscribe to them only while a restart notice actually - // exists. Otherwise this per-worktree chip re-rendered on every pty event to - // compute "no notice → render nothing". See codex-restart-chip-inputs. - const { ptyIdsByTabId, codexRestartNoticeByPtyId } = useAppStore( - useShallow(selectCodexRestartInputs) - ) - const staleWorktreePtyIds = useMemo( - () => - collectStalePtyIdsForTabs({ - tabs, - ptyIdsByTabId, - codexRestartNoticeByPtyId - }), - [codexRestartNoticeByPtyId, ptyIdsByTabId, tabs] - ) - const restartNotice = staleWorktreePtyIds[0] - ? codexRestartNoticeByPtyId[staleWorktreePtyIds[0]] - : undefined - const queueCodexPaneRestarts = useAppStore((s) => s.queueCodexPaneRestarts) - const dismissCodexRestartNotices = useAppStore((s) => s.dismissCodexRestartNotices) - - const noticeKey = restartNotice ? buildCodexRestartNoticeKey(restartNotice) : null - - if (staleWorktreePtyIds.length === 0 || !restartNotice) { + // Why: one O(1) selector per mounted pane stays idle when unrelated PTY maps + // churn and prevents a worktree-wide scan for every split pane. + const restartNotice = useAppStore((state) => state.codexRestartNoticeByPtyId[ptyId]) + if (!restartNotice || !awaitsCodexRestartAnswer(restartNotice)) { return null } const handleRestart = (): void => { - queueCodexPaneRestarts(staleWorktreePtyIds) + useAppStore.getState().queueCodexPaneRestarts([ptyId]) } const handleDismiss = (): void => { - dismissStaleWorktreePtyIds(staleWorktreePtyIds, dismissCodexRestartNotices, (ptyIds) => { - void window.api.codexAccounts.forgetStalePanes({ ptyIds }).catch((err: unknown) => { - console.warn('Failed to forget dismissed Codex pane accounts:', err) - }) + useAppStore.getState().dismissCodexRestartNotices([ptyId]) + // Why: notices are renderer-only, so the persisted launch record must be + // cleared for this pane or the startup sweep re-raises its answered prompt. + void window.api.codexAccounts.forgetStalePanes({ ptyIds: [ptyId] }).catch((err: unknown) => { + console.warn('Failed to forget dismissed Codex pane account:', err) }) } return ( @@ -140,12 +62,14 @@ function LoudRestartOverlay({ isVisible, noticeKey, restartNotice, + shouldFocus, onDismiss, onRestart }: { isVisible: boolean noticeKey: string | null restartNotice: RestartNotice + shouldFocus: boolean onDismiss: () => void onRestart: () => void }): React.JSX.Element { @@ -156,15 +80,10 @@ function LoudRestartOverlay({ // Why: move focus to the card only when the user isn't typing elsewhere; // unconditional autoFocus would steal keys from an active composer. // - // The target is the dialog itself, never Restart. This card is mounted per - // WORKTREE (a sibling of the split layout), so its focus scope is every - // terminal in the worktree — a notice for one pane lands while the user may - // be typing in a different, perfectly healthy pane. With Restart focused, the - // next Space/Enter of their prose queued a restart of every stale pane here - // and destroyed those sessions. Focus must not land on a destructive action - // the user never aimed at. See #10863. + // The target is the dialog itself, never Restart, so the next Space/Enter + // cannot destroy a session the user did not explicitly choose. See #10863. useEffect(() => { - if (!isVisible) { + if (!isVisible || !shouldFocus) { return } const root = rootRef.current @@ -175,7 +94,7 @@ function LoudRestartOverlay({ if (shouldFocusMobileDriverAction(document.activeElement, document.body, paneScope)) { root.focus() } - }, [isVisible, noticeKey]) + }, [isVisible, noticeKey, shouldFocus]) return (
- {(tabsByWorktree[workspace.id] ?? []) .filter((tab) => shouldMountBackgroundWorktreeTab( @@ -2656,7 +2654,6 @@ const WorktreeSplitSurface = React.memo(function WorktreeSplitSurface({ inert={!isVisible} aria-hidden={!isVisible} > - { - it('returns the frozen empty bundle while no restart notice exists', () => { - const state: CodexRestartInputsState = { - ptyIdsByTabId: { 'tab-1': ['pty-1'] }, - codexRestartNoticeByPtyId: {} - } - expect(selectCodexRestartInputs(state)).toBe(EMPTY_CODEX_RESTART_INPUTS) - - // Churning EITHER map while no notice exists must NOT change the selected - // reference, so a useShallow subscription skips the re-render. This covers - // the pty-teardown path that re-spreads codexRestartNoticeByPtyId even empty. - const churnedPty: CodexRestartInputsState = { - ...state, - ptyIdsByTabId: { 'tab-1': ['pty-2'], 'tab-2': ['pty-3'] } - } - const churnedNotice: CodexRestartInputsState = { - ptyIdsByTabId: state.ptyIdsByTabId, - codexRestartNoticeByPtyId: {} // fresh empty object, same as a teardown re-spread - } - expect(selectCodexRestartInputs(churnedPty)).toBe(EMPTY_CODEX_RESTART_INPUTS) - expect(selectCodexRestartInputs(churnedNotice)).toBe(EMPTY_CODEX_RESTART_INPUTS) - expect(shallow(selectCodexRestartInputs(state), selectCodexRestartInputs(churnedNotice))).toBe( - true - ) - }) - - it('re-idles once every surviving notice is answered', () => { - // Why: answered notices linger as launch-account memory for the pty's whole - // life, so existence alone would keep every chip subscribed to pty churn. - const dismissed: CodexRestartInputsState = { - ptyIdsByTabId: { 'tab-1': ['pty-1'] }, - codexRestartNoticeByPtyId: { - 'pty-1': { previousAccountLabel: 'a', nextAccountLabel: 'b', dismissed: true } - } - } - expect(selectCodexRestartInputs(dismissed)).toBe(EMPTY_CODEX_RESTART_INPUTS) - - const alsoUnanswered: CodexRestartInputsState = { - ...dismissed, - codexRestartNoticeByPtyId: { - ...dismissed.codexRestartNoticeByPtyId, - 'pty-2': { previousAccountLabel: 'a', nextAccountLabel: 'b' } - } - } - expect(selectCodexRestartInputs(alsoUnanswered)).not.toBe(EMPTY_CODEX_RESTART_INPUTS) - }) - - it('exposes both live maps the instant a restart notice exists', () => { - const ptyIdsByTabId = { 'tab-1': ['pty-1'] } - const codexRestartNoticeByPtyId = { - 'pty-1': { previousAccountLabel: 'a', nextAccountLabel: 'b' } - } - const state: CodexRestartInputsState = { ptyIdsByTabId, codexRestartNoticeByPtyId } - const selected = selectCodexRestartInputs(state) - // Live references pass straight through so the stale-pty memo + notice lookup derive fully. - expect(selected.ptyIdsByTabId).toBe(ptyIdsByTabId) - expect(selected.codexRestartNoticeByPtyId).toBe(codexRestartNoticeByPtyId) - expect(selected).not.toBe(EMPTY_CODEX_RESTART_INPUTS) - }) - - it('shallow-changes only when a live map reference changes while a notice exists', () => { - const ptyIdsByTabId = { 'tab-1': ['pty-1'] } - const s1: CodexRestartInputsState = { - ptyIdsByTabId, - codexRestartNoticeByPtyId: { - 'pty-1': { previousAccountLabel: 'a', nextAccountLabel: 'b' } - } - } - const r1 = selectCodexRestartInputs(s1) - expect(shallow(r1, selectCodexRestartInputs(s1))).toBe(true) - - const s2: CodexRestartInputsState = { ...s1, ptyIdsByTabId: { 'tab-1': ['pty-9'] } } - expect(shallow(r1, selectCodexRestartInputs(s2))).toBe(false) - }) -}) diff --git a/src/renderer/src/components/codex-restart-chip-inputs.ts b/src/renderer/src/components/codex-restart-chip-inputs.ts deleted file mode 100644 index 52adc9b9e26..00000000000 --- a/src/renderer/src/components/codex-restart-chip-inputs.ts +++ /dev/null @@ -1,41 +0,0 @@ -import { awaitsCodexRestartAnswer } from './codex-restart-notice-state' -import type { AppState } from '@/store/types' - -export type CodexRestartInputsState = Pick - -export type CodexRestartInputs = CodexRestartInputsState - -// Why: shared frozen bundle returned while no Codex restart notice exists — the -// overwhelmingly common case. CodexRestartChip is mounted once per worktree -// Terminal (visible AND hidden-measurable) and once per split group, and reads -// these two maps only to find panes carrying a restart notice, which appear only -// on a Codex account switch. BOTH churn on unrelated pty lifecycle: -// ptyIdsByTabId gets a fresh identity on every pty register/attach/detach, and -// codexRestartNoticeByPtyId is re-spread into a new object even when empty on the -// pty exit/teardown path (terminals.ts clearTabPtyId). Subscribing to either -// re-rendered every mounted chip on that churn. Gate both behind notice-existence -// so idle chips keep the same reference and stop reacting; behavior is identical -// (no notice -> [] stale ids -> null render). Frozen so the singleton can't be -// mutated. -export const EMPTY_CODEX_RESTART_INPUTS: CodexRestartInputs = Object.freeze({ - ptyIdsByTabId: {}, - codexRestartNoticeByPtyId: {} -}) - -/** - * Expose the pty and restart-notice maps to the chip only while at least one - * Codex restart notice is live; otherwise return a stable frozen bundle so a - * `useShallow` subscription skips re-renders on unrelated pty-lifecycle churn. - */ -export function selectCodexRestartInputs(s: CodexRestartInputsState): CodexRestartInputs { - // Why: answered notices linger as launch-account memory for the life of the - // pty, so gating on mere existence would leave every chip permanently - // subscribed to that churn after a single dismissal. - if (!Object.values(s.codexRestartNoticeByPtyId).some(awaitsCodexRestartAnswer)) { - return EMPTY_CODEX_RESTART_INPUTS - } - return { - ptyIdsByTabId: s.ptyIdsByTabId, - codexRestartNoticeByPtyId: s.codexRestartNoticeByPtyId - } -} diff --git a/src/renderer/src/components/codex-restart-chip.test.ts b/src/renderer/src/components/codex-restart-chip.test.ts deleted file mode 100644 index d5bdd44e124..00000000000 --- a/src/renderer/src/components/codex-restart-chip.test.ts +++ /dev/null @@ -1,486 +0,0 @@ -// @vitest-environment happy-dom - -import React, { act } from 'react' -import { createRoot, type Root } from 'react-dom/client' -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -import { useAppStore } from '../store' -import CodexRestartChip, { - collectStalePtyIdsForTabs, - collectStaleWorktreePtyIds, - dismissStaleWorktreePtyIds -} from './CodexRestartChip' - -let container: HTMLDivElement -let root: Root - -beforeEach(() => { - useAppStore.setState(useAppStore.getInitialState(), true) - container = document.createElement('div') - document.body.appendChild(container) - root = createRoot(container) -}) - -afterEach(() => { - act(() => { - root.unmount() - }) - container.remove() - useAppStore.setState(useAppStore.getInitialState(), true) -}) - -describe('CodexRestartChip helpers', () => { - it('collects all stale PTY ids for tabs in a worktree', () => { - expect( - collectStaleWorktreePtyIds({ - tabsByWorktree: { - wt1: [{ id: 'tab-1' }, { id: 'tab-2' }], - wt2: [{ id: 'tab-3' }] - }, - ptyIdsByTabId: { - 'tab-1': ['pty-1', 'pty-2'], - 'tab-2': ['pty-3'], - 'tab-3': ['pty-4'] - }, - codexRestartNoticeByPtyId: { - 'pty-1': { previousAccountLabel: 'a', nextAccountLabel: 'b' }, - 'pty-3': { previousAccountLabel: 'a', nextAccountLabel: 'b' }, - 'pty-4': { previousAccountLabel: 'a', nextAccountLabel: 'b' } - }, - worktreeId: 'wt1' - }) - ).toEqual(['pty-1', 'pty-3']) - }) - - it('returns an empty list when a worktree has no stale PTYs', () => { - expect( - collectStaleWorktreePtyIds({ - tabsByWorktree: { - wt1: [{ id: 'tab-1' }] - }, - ptyIdsByTabId: { - 'tab-1': ['pty-1'] - }, - codexRestartNoticeByPtyId: {}, - worktreeId: 'wt1' - }) - ).toEqual([]) - }) - - it('collects from one worktree tab slice without scanning the whole tab map', () => { - expect( - collectStalePtyIdsForTabs({ - tabs: [{ id: 'tab-1' }], - ptyIdsByTabId: { - 'tab-1': ['pty-1'], - 'tab-2': ['pty-2'] - }, - codexRestartNoticeByPtyId: { - 'pty-1': { previousAccountLabel: 'a', nextAccountLabel: 'b' }, - 'pty-2': { previousAccountLabel: 'a', nextAccountLabel: 'b' } - } - }) - ).toEqual(['pty-1']) - }) - - it('drops PTYs whose restart is already requested', () => { - expect( - collectStalePtyIdsForTabs({ - tabs: [{ id: 'tab-1' }], - ptyIdsByTabId: { - 'tab-1': ['pty-1', 'pty-2'] - }, - codexRestartNoticeByPtyId: { - 'pty-1': { previousAccountLabel: 'a', nextAccountLabel: 'b', restartRequested: true }, - 'pty-2': { previousAccountLabel: 'a', nextAccountLabel: 'b' } - } - }) - ).toEqual(['pty-2']) - }) - - it('dismisses every stale PTY notice in the worktree prompt', () => { - const dismissCodexRestartNotices = vi.fn() - const forgetLaunchAccounts = vi.fn() - - dismissStaleWorktreePtyIds(['pty-1', 'pty-3'], dismissCodexRestartNotices, forgetLaunchAccounts) - - expect(dismissCodexRestartNotices).toHaveBeenCalledWith(['pty-1', 'pty-3']) - expect(forgetLaunchAccounts).toHaveBeenCalledWith(['pty-1', 'pty-3']) - }) - - it('drops dismissed PTYs from the prompt while keeping their launch account', () => { - expect( - collectStalePtyIdsForTabs({ - tabs: [{ id: 'tab-1' }], - ptyIdsByTabId: { - 'tab-1': ['pty-1', 'pty-2'] - }, - codexRestartNoticeByPtyId: { - 'pty-1': { previousAccountLabel: 'a', nextAccountLabel: 'b', dismissed: true }, - 'pty-2': { previousAccountLabel: 'a', nextAccountLabel: 'b' } - } - }) - ).toEqual(['pty-2']) - }) - - it('renders only account-resolution actions without an external-store update loop', async () => { - useAppStore.setState({ - tabsByWorktree: { - 'worktree-1': [ - { - id: 'tab-1', - worktreeId: 'worktree-1', - title: 'Terminal', - customTitle: null, - color: null, - sortOrder: 0, - createdAt: 1, - ptyId: null - } - ] - }, - ptyIdsByTabId: { - 'tab-1': ['pty-1'] - }, - codexRestartNoticeByPtyId: { - 'pty-1': { - previousAccountLabel: 'old@example.com', - nextAccountLabel: 'new@example.com' - } - } - }) - - await act(async () => { - root.render(React.createElement(CodexRestartChip, { worktreeId: 'worktree-1' })) - }) - - expect(container.textContent).toContain('Codex is still signed in as old@example.com') - expect( - Array.from(container.querySelectorAll('button'), (button) => button.textContent?.trim()) - ).toEqual(['Keep old account', 'Restart']) - }) - - it('uses configuration wording for a home-route restart', async () => { - useAppStore.setState({ - tabsByWorktree: { - 'worktree-1': [ - { - id: 'tab-1', - worktreeId: 'worktree-1', - title: 'Terminal', - customTitle: null, - color: null, - sortOrder: 0, - createdAt: 1, - ptyId: null - } - ] - }, - ptyIdsByTabId: { 'tab-1': ['pty-1'] }, - codexRestartNoticeByPtyId: { - 'pty-1': { - previousAccountLabel: 'System default', - nextAccountLabel: 'System default', - previousAccountId: null, - nextAccountId: null, - homeRouteChanged: true - } - } - }) - - await act(async () => { - root.render(React.createElement(CodexRestartChip, { worktreeId: 'worktree-1' })) - }) - - expect(container.textContent).toContain('Codex setup changed') - expect(container.textContent).toContain('This Codex session is using an outdated configuration') - expect(container.textContent).toContain( - 'Restart this session to load your current Codex configuration.' - ) - expect( - Array.from(container.querySelectorAll('button'), (button) => button.textContent?.trim()) - ).toEqual(['Keep current session', 'Restart']) - }) - - it('forgets the launch record when the user keeps the old account', async () => { - const forgetStalePanes = vi.fn(() => Promise.resolve()) - Object.defineProperty(window, 'api', { - configurable: true, - value: { codexAccounts: { forgetStalePanes } } - }) - useAppStore.setState({ - tabsByWorktree: { - 'worktree-1': [ - { - id: 'tab-1', - worktreeId: 'worktree-1', - title: 'Terminal', - customTitle: null, - color: null, - sortOrder: 0, - createdAt: 1, - ptyId: null - } - ] - }, - ptyIdsByTabId: { 'tab-1': ['pty-1'] }, - codexRestartNoticeByPtyId: { - 'pty-1': { - previousAccountLabel: 'old@example.com', - nextAccountLabel: 'new@example.com' - } - } - }) - await act(async () => { - root.render(React.createElement(CodexRestartChip, { worktreeId: 'worktree-1' })) - }) - - const dismissButton = Array.from(container.querySelectorAll('button')).find( - (button) => button.textContent?.trim() === 'Keep old account' - ) - await act(async () => { - dismissButton?.click() - }) - - // Why: without this the startup sweep re-raises the prompt the user just answered. - expect(forgetStalePanes).toHaveBeenCalledWith({ ptyIds: ['pty-1'] }) - expect(container.textContent).toBe('') - // Why: the record survives as the pane's launch-account memory, but marked - // answered so it neither prompts again nor blocks the pane's keyboard. - expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ - previousAccountLabel: 'old@example.com', - nextAccountLabel: 'new@example.com', - dismissed: true - }) - }) - - it('stays closed when the user re-selects the account the pane launched under', async () => { - const forgetStalePanes = vi.fn(() => Promise.resolve()) - Object.defineProperty(window, 'api', { - configurable: true, - value: { codexAccounts: { forgetStalePanes } } - }) - useAppStore.setState({ - tabsByWorktree: { - 'worktree-1': [ - { - id: 'tab-1', - worktreeId: 'worktree-1', - title: 'Terminal', - customTitle: null, - color: null, - sortOrder: 0, - createdAt: 1, - ptyId: null - } - ] - }, - ptyIdsByTabId: { 'tab-1': ['pty-1'] } - }) - useAppStore.getState().markCodexRestartNotices([ - { - ptyId: 'pty-1', - previousAccountLabel: 'old@example.com', - nextAccountLabel: 'new@example.com' - } - ]) - await act(async () => { - root.render(React.createElement(CodexRestartChip, { worktreeId: 'worktree-1' })) - }) - const dismissButton = Array.from(container.querySelectorAll('button')).find( - (button) => button.textContent?.trim() === 'Keep old account' - ) - await act(async () => { - dismissButton?.click() - }) - - // Re-select the pane's original account: it never left it, so there is - // nothing to restart and nothing to prompt about. - await act(async () => { - useAppStore.getState().markCodexRestartNotices([ - { - ptyId: 'pty-1', - previousAccountLabel: 'new@example.com', - nextAccountLabel: 'old@example.com' - } - ]) - }) - - expect(container.textContent).toBe('') - expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toBeUndefined() - }) - - it('reopens the prompt when a dismissed pane is out of date against a third account', async () => { - const forgetStalePanes = vi.fn(() => Promise.resolve()) - Object.defineProperty(window, 'api', { - configurable: true, - value: { codexAccounts: { forgetStalePanes } } - }) - useAppStore.setState({ - tabsByWorktree: { - 'worktree-1': [ - { - id: 'tab-1', - worktreeId: 'worktree-1', - title: 'Terminal', - customTitle: null, - color: null, - sortOrder: 0, - createdAt: 1, - ptyId: null - } - ] - }, - ptyIdsByTabId: { 'tab-1': ['pty-1'] } - }) - useAppStore.getState().markCodexRestartNotices([ - { - ptyId: 'pty-1', - previousAccountLabel: 'old@example.com', - nextAccountLabel: 'new@example.com' - } - ]) - await act(async () => { - root.render(React.createElement(CodexRestartChip, { worktreeId: 'worktree-1' })) - }) - const dismissButton = Array.from(container.querySelectorAll('button')).find( - (button) => button.textContent?.trim() === 'Keep old account' - ) - await act(async () => { - dismissButton?.click() - }) - expect(container.textContent).toBe('') - - await act(async () => { - useAppStore.getState().markCodexRestartNotices([ - { - ptyId: 'pty-1', - previousAccountLabel: 'new@example.com', - nextAccountLabel: 'third@example.com' - } - ]) - }) - - // Why: the dismissal answered "keep old instead of new", not "never prompt - // again"; the pane really is out of date against third@example.com. - expect(container.textContent).toContain('Codex is still signed in as old@example.com') - expect(container.textContent).toContain('Restart this session to use third@example.com') - }) - - it('closes the prompt after Restart even when no mounted pane can run it yet', async () => { - useAppStore.setState({ - tabsByWorktree: { - 'worktree-1': [ - { - id: 'tab-1', - worktreeId: 'worktree-1', - title: 'Terminal', - customTitle: null, - color: null, - sortOrder: 0, - createdAt: 1, - ptyId: null - } - ] - }, - ptyIdsByTabId: { 'tab-1': ['pty-1'] }, - codexRestartNoticeByPtyId: { - 'pty-1': { - previousAccountLabel: 'old@example.com', - nextAccountLabel: 'new@example.com' - } - } - }) - await act(async () => { - root.render(React.createElement(CodexRestartChip, { worktreeId: 'worktree-1' })) - }) - - const restartButton = Array.from(container.querySelectorAll('button')).find( - (button) => button.textContent?.trim() === 'Restart' - ) - await act(async () => { - restartButton?.click() - }) - - expect(container.textContent).toBe('') - // Why: the pane still has to restart; only the prompt is answered. - expect(useAppStore.getState().pendingCodexPaneRestartIds).toEqual({ 'pty-1': true }) - expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']?.restartRequested).toBe(true) - }) -}) - -describe('CodexRestartChip focus target', () => { - function renderWithFocusedSiblingPane(): HTMLTextAreaElement { - // Mirrors Terminal.tsx, which mounts the chip as a SIBLING of the split - // layout — so the chip's parentElement is the whole worktree surface and - // its focus scope covers every pane in it, not just the stale one. - const splitLayout = document.createElement('div') - const healthyPaneInput = document.createElement('textarea') - healthyPaneInput.className = 'xterm-helper-textarea' - splitLayout.appendChild(healthyPaneInput) - container.appendChild(splitLayout) - healthyPaneInput.focus() - - useAppStore.setState({ - tabsByWorktree: { - 'worktree-1': [ - { - id: 'tab-1', - worktreeId: 'worktree-1', - title: 'Terminal', - customTitle: null, - color: null, - sortOrder: 0, - createdAt: 1, - ptyId: null - } - ] - }, - ptyIdsByTabId: { 'tab-1': ['pty-1'] } - }) - useAppStore.getState().markCodexRestartNotices([ - { - ptyId: 'pty-1', - previousAccountLabel: 'old@example.com', - nextAccountLabel: 'new@example.com' - } - ]) - return healthyPaneInput - } - - it('never parks focus on Restart, so a stray keystroke cannot kill the session', async () => { - renderWithFocusedSiblingPane() - - await act(async () => { - root.render(React.createElement(CodexRestartChip, { worktreeId: 'worktree-1' })) - }) - - // Regression (#10863): focus used to land on Restart. The card is worktree - // scoped, so it fires while the user is typing in a DIFFERENT, healthy pane - // — and the next Space/Enter of their prose restarted every stale pane here. - const restartButton = Array.from(container.querySelectorAll('button')).find( - (button) => button.textContent?.trim() === 'Restart' - ) - expect(restartButton).toBeDefined() - expect(document.activeElement).not.toBe(restartButton) - expect((document.activeElement as HTMLElement | null)?.getAttribute('role')).toBe('dialog') - - // The keystroke that used to destroy the session now does nothing. - await act(async () => { - ;(document.activeElement as HTMLElement | null)?.click() - }) - expect(useAppStore.getState().pendingCodexPaneRestartIds).toEqual({}) - }) - - it('still moves focus off the terminal so the card is reachable by keyboard', async () => { - const healthyPaneInput = renderWithFocusedSiblingPane() - - await act(async () => { - root.render(React.createElement(CodexRestartChip, { worktreeId: 'worktree-1' })) - }) - - // Why: the fix must not turn into "never take focus" — assistive tech and - // keyboard users still need to land on the dialog and Tab to its actions. - expect(document.activeElement).not.toBe(healthyPaneInput) - expect(document.activeElement).toBe(container.querySelector('[role="dialog"]')) - }) -}) diff --git a/src/renderer/src/components/codex-restart-chip.test.tsx b/src/renderer/src/components/codex-restart-chip.test.tsx new file mode 100644 index 00000000000..d3767b2dc92 --- /dev/null +++ b/src/renderer/src/components/codex-restart-chip.test.tsx @@ -0,0 +1,295 @@ +// @vitest-environment happy-dom + +import React, { act, Profiler } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { useAppStore } from '../store' +import CodexRestartChip from './CodexRestartChip' + +globalThis.IS_REACT_ACT_ENVIRONMENT = true + +const PTY_ONE = 'worktree-1@@pty-1' +const PTY_TWO = 'worktree-1@@pty-2' + +let container: HTMLDivElement +let forgetStalePanes: ReturnType +let root: Root + +function notice(previousAccountLabel: string, nextAccountLabel: string) { + return { previousAccountLabel, nextAccountLabel } +} + +function button(scope: ParentNode, label: string): HTMLButtonElement { + const match = Array.from(scope.querySelectorAll('button')).find( + (candidate) => candidate.textContent?.trim() === label + ) + if (!match) { + throw new Error(`missing ${label} button`) + } + return match +} + +beforeEach(() => { + useAppStore.setState(useAppStore.getInitialState(), true) + container = document.createElement('div') + document.body.appendChild(container) + root = createRoot(container) + forgetStalePanes = vi.fn(() => Promise.resolve()) + Object.defineProperty(window, 'api', { + configurable: true, + value: { codexAccounts: { forgetStalePanes } } + }) +}) + +afterEach(() => { + act(() => { + root.unmount() + }) + container.remove() + useAppStore.setState(useAppStore.getInitialState(), true) +}) + +describe('CodexRestartChip pane ownership', () => { + it('renders the notice owned by its exact PTY', async () => { + useAppStore.setState({ + codexRestartNoticeByPtyId: { + [PTY_ONE]: notice('old-one@example.com', 'new-one@example.com'), + [PTY_TWO]: notice('old-two@example.com', 'new-two@example.com') + } + }) + + await act(async () => { + root.render() + }) + + expect(container.textContent).toContain('Codex is still signed in as old-two@example.com') + expect(container.textContent).toContain('Restart this session to use new-two@example.com') + expect(container.textContent).not.toContain('old-one@example.com') + }) + + it('uses configuration wording for a home-route restart', async () => { + useAppStore.setState({ + codexRestartNoticeByPtyId: { + [PTY_ONE]: { + previousAccountLabel: 'System default', + nextAccountLabel: 'System default', + previousAccountId: null, + nextAccountId: null, + homeRouteChanged: true + } + } + }) + + await act(async () => { + root.render() + }) + + expect(container.textContent).toContain('Codex setup changed') + expect(container.textContent).toContain('This Codex session is using an outdated configuration') + expect(container.textContent).toContain( + 'Restart this session to load your current Codex configuration.' + ) + }) + + it('restarts only the pane whose action was clicked', async () => { + useAppStore.setState({ + codexRestartNoticeByPtyId: { + [PTY_ONE]: notice('old-one@example.com', 'new-one@example.com'), + [PTY_TWO]: notice('old-two@example.com', 'new-two@example.com') + } + }) + + await act(async () => { + root.render( + <> +
+ +
+
+ +
+ + ) + }) + const firstPane = container.querySelector('[data-pane="one"]')! + const secondPane = container.querySelector('[data-pane="two"]')! + + await act(async () => { + button(firstPane, 'Restart').click() + }) + + const state = useAppStore.getState() + expect(state.pendingCodexPaneRestartIds).toEqual({ [PTY_ONE]: true }) + expect(state.codexRestartNoticeByPtyId[PTY_ONE]?.restartRequested).toBe(true) + expect(state.codexRestartNoticeByPtyId[PTY_TWO]).toEqual( + notice('old-two@example.com', 'new-two@example.com') + ) + expect(firstPane.textContent).toBe('') + expect(secondPane.textContent).toContain('old-two@example.com') + }) + + it('dismisses and forgets only the pane whose action was clicked', async () => { + useAppStore.setState({ + codexRestartNoticeByPtyId: { + [PTY_ONE]: notice('old-one@example.com', 'new-one@example.com'), + [PTY_TWO]: notice('old-two@example.com', 'new-two@example.com') + } + }) + + await act(async () => { + root.render( + <> +
+ +
+
+ +
+ + ) + }) + const firstPane = container.querySelector('[data-pane="one"]')! + const secondPane = container.querySelector('[data-pane="two"]')! + + await act(async () => { + button(firstPane, 'Keep old account').click() + }) + + expect(forgetStalePanes).toHaveBeenCalledExactlyOnceWith({ ptyIds: [PTY_ONE] }) + expect(useAppStore.getState().codexRestartNoticeByPtyId[PTY_ONE]?.dismissed).toBe(true) + expect(useAppStore.getState().codexRestartNoticeByPtyId[PTY_TWO]?.dismissed).toBeUndefined() + expect(firstPane.textContent).toBe('') + expect(secondPane.textContent).toContain('old-two@example.com') + }) + + it('follows a pane replacement id without acting on the retired PTY', async () => { + useAppStore.setState({ + codexRestartNoticeByPtyId: { + [PTY_ONE]: notice('retired@example.com', 'target@example.com') + } + }) + await act(async () => { + root.render() + }) + + await act(async () => { + useAppStore.setState({ + codexRestartNoticeByPtyId: { + [PTY_TWO]: notice('replacement@example.com', 'target@example.com') + } + }) + root.render() + }) + await act(async () => { + button(container, 'Restart').click() + }) + + expect(useAppStore.getState().pendingCodexPaneRestartIds).toEqual({ [PTY_TWO]: true }) + expect(useAppStore.getState().pendingCodexPaneRestartIds[PTY_ONE]).toBeUndefined() + }) + + it('stays hidden after this pane answers while a sibling remains unanswered', async () => { + useAppStore.setState({ + codexRestartNoticeByPtyId: { + [PTY_ONE]: { ...notice('a', 'b'), restartRequested: true }, + [PTY_TWO]: notice('c', 'd') + } + }) + + await act(async () => { + root.render() + }) + + expect(container.textContent).toBe('') + }) + + it('does not re-render for unrelated PTY and notice churn', async () => { + const onRender = vi.fn() + const ownedNotice = notice('a', 'b') + useAppStore.setState({ codexRestartNoticeByPtyId: { [PTY_ONE]: ownedNotice } }) + await act(async () => { + root.render( + + + + ) + }) + const commits = onRender.mock.calls.length + + await act(async () => { + useAppStore.setState({ + ptyIdsByTabId: { unrelated: [PTY_TWO] }, + codexRestartNoticeByPtyId: { + [PTY_ONE]: ownedNotice, + [PTY_TWO]: notice('c', 'd') + } + }) + }) + + expect(onRender).toHaveBeenCalledTimes(commits) + }) + + it('does not mutate restart state during a StrictMode replay', async () => { + useAppStore.setState({ + codexRestartNoticeByPtyId: { [PTY_ONE]: notice('old@example.com', 'new@example.com') } + }) + + await act(async () => { + root.render( + + + + ) + }) + + expect(useAppStore.getState().pendingCodexPaneRestartIds).toEqual({}) + expect(useAppStore.getState().codexRestartNoticeByPtyId[PTY_ONE]).toEqual( + notice('old@example.com', 'new@example.com') + ) + }) +}) + +describe('CodexRestartChip pane focus', () => { + async function renderPane(shouldFocus: boolean, isVisible = true): Promise { + await act(async () => { + root.render( +
+