diff --git a/docs/remote-web-paste-activity-fixes.md b/docs/remote-web-paste-activity-fixes.md new file mode 100644 index 00000000000..dca8ee5bed6 --- /dev/null +++ b/docs/remote-web-paste-activity-fixes.md @@ -0,0 +1,108 @@ +# Remote Web Paste And Activity Fixes + +## Problem + +- Paired browser image paste reads a clipboard image in `src/renderer/src/web/web-preload-api.ts:120`, converts it to PNG base64, and sends the whole string through `clipboard.saveImageAsTempFile` at `src/renderer/src/web/web-preload-api.ts:1613`. `src/main/runtime/rpc/ws-transport.ts` sets `maxPayload` to 1 MiB, and the browser RPC client encrypts JSON requests into base64 text frames. A screenshot well below the existing 24 MiB clipboard schema limit can still exceed the encrypted WebSocket frame cap and close the socket as `Remote Orca runtime connection interrupted`. +- Paired browser workspace activity is inflated. `src/renderer/src/lib/worktree-status.ts` and `src/renderer/src/lib/worktree-activity-state.ts` currently treat any mirrored web terminal surface ID as active, even when `src/renderer/src/runtime/web-session-tabs-sync.ts` only has a pending host terminal with no ready PTY handle. + +## Root Cause + +- The clipboard RPC accepts up to 24 MiB of base64 in `src/main/runtime/rpc/methods/clipboard.ts`, but the web transport limit applies before the server can validate RPC params. Because the web client encrypts JSON and base64-encodes the encrypted bytes, the safe plaintext chunk is materially smaller than 1 MiB. The existing 512 KiB file-upload chunk size in `src/renderer/src/runtime/runtime-file-client.ts` is safe after JSON plus encryption expansion; a chunk close to 1 MiB is not. +- The file-upload RPCs cannot be reused directly. They write worktree-relative files and then commit a rename; clipboard paste must call `saveClipboardImageBufferAsTempFile`, return a local or SSH temp path, and preserve the terminal repo's `connectionId`. +- Web session-tab sync intentionally mirrors pending host terminal surfaces so the tab model stays in parity, but it writes `ptyIdsByTabId[mirroredTabId]` only from `status: "ready"` surfaces. The status/filter helpers then bypass that liveness map with `isWebTerminalSurfaceTabId`, so pending mirrors look active. + +## Non-Goals + +- Do not raise the global WebSocket `maxPayload`; it protects all runtime traffic, including pre-auth and mobile sockets. +- Do not change sidebar grouping, labels, counts, pairing flow, terminal UI, or clipboard permission UI. +- Do not change local Electron clipboard behavior or the existing one-shot RPC contract. + +## Design + +1. Add bounded chunked clipboard-image RPCs. + - Keep `clipboard.saveImageAsTempFile` for small/old clients. + - Add `clipboard.startImageUpload`, `clipboard.appendImageUploadChunk`, `clipboard.commitImageUpload`, and `clipboard.abortImageUpload`. + - `start` takes `expectedBase64Length` and `connectionId`, rejects values over the existing 24 MiB base64 limit, records the target connection, and returns an unguessable upload ID. + - `append` takes `uploadId`, `offset`, and `contentBase64`; reject unknown IDs, out-of-order offsets, chunks above 512 KiB, invalid base64 characters, and cumulative length beyond `expectedBase64Length`. + - `commit` verifies the received length equals `expectedBase64Length`, validates the full base64 payload with the same rules as the one-shot RPC, calls `saveClipboardImageBufferAsTempFile` with the recorded `connectionId`, and deletes the upload state in `finally`. + - `abort` deletes the upload state and is idempotent. + - Bound server memory with a small max concurrent upload count and a TTL cleanup for abandoned uploads. Upload state is process-local; reconnects restart the paste instead of resuming. + +2. Switch paired browser image paste to the chunked path. + - After `navigator.clipboard.read()` and PNG conversion, return `null` for no image as today. + - Preflight `contentBase64.length` against the 24 MiB limit before starting an upload. + - Send 512 KiB base64 slices. This is below the 1 MiB encrypted frame cap after JSON and E2EE base64 expansion. + - Abort best-effort on append or commit failure. If `start` returns `method_not_found`, fall back to `clipboard.saveImageAsTempFile` only when the payload is below a conservative single-frame threshold; never send a large fallback frame. + +3. Tighten workspace activity liveness. + - Treat terminal workspaces as active only when `tabHasLivePty(ptyIdsByTabId, tab.id)` is true. + - Remove the blanket `isWebTerminalSurfaceTabId` active shortcut from `getWorktreeStatus` and `hasActiveWorkspaceActivity`. + - Keep browser tabs active without terminals. + - Keep fresh/retained explicit agent rows able to promote status to `permission`, `working`, or `done`; this is separate from terminal liveness. + +## Data Flow + +- Paste image: + - Browser paste command -> `navigator.clipboard.read()` -> PNG base64 in web preload memory. + - `clipboard.startImageUpload({ expectedBase64Length, connectionId })` -> upload ID. + - Repeated `clipboard.appendImageUploadChunk({ uploadId, offset, contentBase64 })`. + - `clipboard.commitImageUpload({ uploadId })` -> runtime saves temp image locally or on the SSH target -> terminal receives the temp path. + +- Workspace activity: + - Host `session.tabs.listAll` or subscription snapshot includes terminal surfaces. + - Web sync mirrors tabs but writes live PTY handles only for ready surfaces. + - Sidebar status/filter reads `ptyIdsByTabId` and browser tabs. + - Pending mirrored terminals with no PTY remain visible but do not count as active. + +## Edge Cases + +- Clipboard has no image or browser lacks `navigator.clipboard.read`: return `null`, no upload session. +- Clipboard read/permission/conversion fails: existing terminal paste error path reports the failure; the runtime socket should stay open. +- Non-PNG clipboard images may grow during PNG conversion; validate the post-conversion base64 length. +- Image exceeds 24 MiB base64: reject before upload and enforce again on the server. +- Chunk boundaries must preserve base64 validity; the 512 KiB chunk size is divisible by 4, and the final full payload validation catches padding errors. +- Append retry or concurrent append with the same upload ID: offset validation rejects duplicate, skipped, or out-of-order data. +- Multiple paired browser clients or windows paste at once: upload IDs isolate sessions and the concurrent-upload cap bounds memory. +- Append or commit fails: browser aborts best-effort; server TTL cleans abandoned state. +- SSH connection missing or drops during commit: commit fails, upload state is still deleted, and the paste path reports the error. +- Runtime restarts during upload: pending RPC fails; no resume is attempted. +- Pending host terminal surface later becomes ready: the next snapshot writes PTY handles and the workspace becomes active. +- Host closes a terminal or browser tab: the next snapshot removes the mirrored tab and stale PTY/browser handles before activity is recomputed. + +## Test Plan + +- Unit: `src/main/runtime/rpc/methods/clipboard.test.ts` for start/append/commit/abort, offset validation, invalid base64, size limit, TTL cleanup, commit cleanup on save failure, SSH `connectionId` forwarding, and concurrent upload isolation. +- Unit: `src/renderer/src/web/web-preload-api.test.ts` for chunk sequencing, 512 KiB max chunks, `method_not_found` small-payload fallback, no large fallback frame, and abort on append/commit failure. +- Unit: `src/renderer/src/lib/worktree-status.test.ts` and `src/renderer/src/lib/worktree-activity-state.test.ts` for pending mirrored terminal inactivity, ready mirrored terminal activity, browser-only activity, and explicit agent-row promotion. +- Focused run: `pnpm exec vitest run --config config/vitest.config.ts src/main/runtime/rpc/methods/clipboard.test.ts src/renderer/src/web/web-preload-api.test.ts src/renderer/src/lib/worktree-status.test.ts src/renderer/src/lib/worktree-activity-state.test.ts`. +- Type/lint: `pnpm typecheck`, `pnpm lint`. +- Paired-client validation: restart the host app, pair a real browser client, paste a screenshot large enough that the old one-shot RPC exceeded the 1 MiB encrypted frame cap into a local terminal and an SSH-backed terminal, confirm the runtime socket stays connected, then compare browser and host sidebar activity. + +## UI Quality Bar + +No layout changes. The paired browser sidebar should show active dots only for workspaces with live terminal PTYs, browser tabs, or explicit agent rows. Image paste should insert the generated temp image path into the terminal without connection-error toasts or DevTools runtime disconnect errors. + +## Review Screenshots + +1. Paired browser sidebar after hydration with inactive Done/Todo workspaces visible but not marked active. +2. Paired browser terminal after image paste, showing the generated temp image path inserted. +3. Host sidebar for the same session, showing matching active workspace state. + +## Rollout + +1. Add chunked clipboard RPC implementation and tests. +2. Switch web preload image paste to chunked upload and tests. +3. Tighten activity/status liveness helpers and tests. +4. Run focused tests, typecheck, and lint. +5. Validate in a paired Electron/browser session, including SSH paste, and capture the review screenshots. + +## Lightweight Eng Review + +- Scope: narrow to web clipboard transport and activity liveness. No global WebSocket limit, sidebar redesign, or local Electron clipboard changes. +- Architecture/data flow: web preload owns browser clipboard read and chunk sequencing; main runtime clipboard RPC owns upload session state and final temp-file save; sidebar helpers stay pure and consume the existing live-PTY map. +- Failure modes: oversized images must fail before a large frame is sent; abandoned uploads expire; failed append/commit paths clean up; pending host terminal mirrors do not inflate activity; SSH commit failures do not leak upload state. +- Tests: cover RPC lifecycle and bounds, web preload sequencing/fallback/abort, status/filter liveness, and real paired-client paste/sidebar parity. +- Performance/blast radius: chunking is not free. A max-size image is dozens of serialized runtime RPC calls and duplicates base64 in browser and main memory. The impact is limited to image paste by TTL, size, and concurrency caps. Activity changes affect sidebar filters, jump palette activity, and status dots. +- UI quality: no new chrome. Judge only sidebar parity, paste result, and absence of runtime disconnect/toast regressions. +- Screenshots: browser sidebar, browser terminal after paste, and matching host sidebar. +- Residual risk: clipboard permission and image conversion behavior are browser-dependent, so paired browser validation is required in addition to unit tests. diff --git a/package.json b/package.json index d5b2f6dcff2..08f85453ded 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "orca", - "version": "1.4.36-rc.3", + "version": "1.4.36-rc.4", "description": "Next-gen IDE for parallel agentic development", "homepage": "https://github.com/stablyai/orca", "author": "stablyai", diff --git a/src/main/agent-hooks/first-work-branch-rename.ts b/src/main/agent-hooks/first-work-branch-rename.ts index ac427172753..20a73d753ec 100644 --- a/src/main/agent-hooks/first-work-branch-rename.ts +++ b/src/main/agent-hooks/first-work-branch-rename.ts @@ -250,7 +250,8 @@ async function resolveGenerationTarget( return { kind: 'remote', cwd: worktreePath, - execute: (plan, cwd, timeoutMs) => provider.executeCommitMessagePlan(plan, cwd, timeoutMs), + execute: (plan, cwd, timeoutMs, operation) => + provider.executeCommitMessagePlan(plan, cwd, timeoutMs, operation), missingBinaryLocation: 'remote PATH' } } diff --git a/src/main/git/status-discard-symlink.test.ts b/src/main/git/status-discard-symlink.test.ts index 0f0bfaad801..88a22d06009 100644 --- a/src/main/git/status-discard-symlink.test.ts +++ b/src/main/git/status-discard-symlink.test.ts @@ -91,6 +91,53 @@ describe('discardChanges symlink safety', () => { await expect(access(path.join(repo, 'ignored'))).rejects.toThrow() }) + it('treats untracked discard paths with Git glob characters as literal paths', async () => { + const { repo } = await createRepoWithOutsideDirectory() + await writeFile(path.join(repo, '.gitignore'), 'ignored.log\n') + execFileSync('git', ['add', '.gitignore'], { cwd: repo }) + execFileSync('git', ['commit', '-q', '-m', 'ignore log fixture'], { cwd: repo }) + await writeFile(path.join(repo, '*.log'), 'selected') + await writeFile(path.join(repo, 'keep.log'), 'unrelated') + await writeFile(path.join(repo, 'ignored.log'), 'ignored') + + await discardChanges(repo, '*.log') + + await expect(access(path.join(repo, '*.log'))).rejects.toThrow() + await expect(access(path.join(repo, 'keep.log'))).resolves.toBeUndefined() + await expect(access(path.join(repo, 'ignored.log'))).resolves.toBeUndefined() + }) + + it('treats tracked discard paths with Git glob characters as literal paths', async () => { + const { repo } = await createRepoWithOutsideDirectory() + await writeFile(path.join(repo, '*.log'), 'selected') + await writeFile(path.join(repo, 'keep.log'), 'keep') + execFileSync('git', ['add', '*.log', 'keep.log'], { cwd: repo }) + execFileSync('git', ['commit', '-q', '-m', 'track log fixtures'], { cwd: repo }) + await writeFile(path.join(repo, '*.log'), 'selected modified') + await writeFile(path.join(repo, 'keep.log'), 'keep modified') + + await discardChanges(repo, '*.log') + + await expect(readFile(path.join(repo, '*.log'), 'utf8')).resolves.toBe('selected') + await expect(readFile(path.join(repo, 'keep.log'), 'utf8')).resolves.toBe('keep modified') + }) + + it('treats bulk untracked discard paths with Git glob characters as literal paths', async () => { + const { repo } = await createRepoWithOutsideDirectory() + await writeFile(path.join(repo, '.gitignore'), 'ignored.log\n') + execFileSync('git', ['add', '.gitignore'], { cwd: repo }) + execFileSync('git', ['commit', '-q', '-m', 'ignore log fixture'], { cwd: repo }) + await writeFile(path.join(repo, '*.log'), 'selected') + await writeFile(path.join(repo, 'keep.log'), 'unrelated') + await writeFile(path.join(repo, 'ignored.log'), 'ignored') + + await bulkDiscardChanges(repo, ['*.log']) + + await expect(access(path.join(repo, '*.log'))).rejects.toThrow() + await expect(access(path.join(repo, 'keep.log'))).resolves.toBeUndefined() + await expect(access(path.join(repo, 'ignored.log'))).resolves.toBeUndefined() + }) + it('removes untracked nested git repos selected for discard', async () => { const { repo } = await createRepoWithOutsideDirectory() const nestedRepo = path.join(repo, 'nested') diff --git a/src/main/git/status.test.ts b/src/main/git/status.test.ts index 2547f60b7b3..252b23ff608 100644 --- a/src/main/git/status.test.ts +++ b/src/main/git/status.test.ts @@ -75,14 +75,14 @@ describe('discardChanges', () => { expect(gitExecFileAsyncMock).toHaveBeenNthCalledWith( 1, - ['ls-files', '--error-unmatch', '--', 'src/file.ts'], + ['ls-files', '--error-unmatch', '--', ':(literal)src/file.ts'], { cwd: '/repo' } ) expect(gitExecFileAsyncMock).toHaveBeenNthCalledWith( 2, - ['restore', '--worktree', '--source=HEAD', '--', 'src/file.ts'], + ['restore', '--worktree', '--source=HEAD', '--', ':(literal)src/file.ts'], { cwd: '/repo' } @@ -98,7 +98,7 @@ describe('discardChanges', () => { expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(2) expect(gitExecFileAsyncMock).toHaveBeenNthCalledWith( 2, - ['clean', '-ffdx', '--', 'src/new-file.ts'], + ['clean', '-ffdx', '--', ':(literal)src/new-file.ts'], { cwd: '/repo' } @@ -178,7 +178,15 @@ describe('bulk git helpers', () => { expect(gitExecFileAsyncMock).toHaveBeenNthCalledWith( 1, - ['ls-files', '-z', '--', 'src/file.ts', 'src/new-file.ts', 'docs', 'scratch'], + [ + 'ls-files', + '-z', + '--', + ':(literal)src/file.ts', + ':(literal)src/new-file.ts', + ':(literal)docs', + ':(literal)scratch' + ], { cwd: '/repo' } @@ -187,14 +195,14 @@ describe('bulk git helpers', () => { // tracked descendant, which keeps directory pathspecs on the restore path. expect(gitExecFileAsyncMock).toHaveBeenNthCalledWith( 2, - ['restore', '--worktree', '--source=HEAD', '--', 'src/file.ts', 'docs'], + ['restore', '--worktree', '--source=HEAD', '--', ':(literal)src/file.ts', ':(literal)docs'], { cwd: '/repo' } ) expect(gitExecFileAsyncMock).toHaveBeenNthCalledWith( 3, - ['clean', '-ffdx', '--', 'src/new-file.ts', 'scratch'], + ['clean', '-ffdx', '--', ':(literal)src/new-file.ts', ':(literal)scratch'], { cwd: '/repo' } diff --git a/src/main/git/status.ts b/src/main/git/status.ts index ed34cd56bdf..5ab2c757d83 100644 --- a/src/main/git/status.ts +++ b/src/main/git/status.ts @@ -1105,7 +1105,7 @@ export async function discardChanges(worktreePath: string, filePath: string): Pr let tracked = false try { - await gitExecFileAsync(['ls-files', '--error-unmatch', '--', filePath], { + await gitExecFileAsync(['ls-files', '--error-unmatch', '--', literalPathspec(filePath)], { cwd: worktreePath }) tracked = true @@ -1114,9 +1114,12 @@ export async function discardChanges(worktreePath: string, filePath: string): Pr } if (tracked) { - await gitExecFileAsync(['restore', '--worktree', '--source=HEAD', '--', filePath], { - cwd: worktreePath - }) + await gitExecFileAsync( + ['restore', '--worktree', '--source=HEAD', '--', literalPathspec(filePath)], + { + cwd: worktreePath + } + ) return } @@ -1129,6 +1132,11 @@ function normalizeGitPathForCompare(filePath: string): string { return filePath.replace(/\\/g, '/').replace(/\/+$/, '') } +function literalPathspec(filePath: string): string { + // Why: source-control selections are concrete paths, not user-authored Git globs. + return `:(literal)${filePath}` +} + function isTrackedPathSpec(filePath: string, trackedPaths: readonly string[]): boolean { const normalized = normalizeGitPathForCompare(filePath) return trackedPaths.some((trackedPath) => { @@ -1144,9 +1152,12 @@ async function listTrackedPathSpecs( const trackedPaths: string[] = [] for (let i = 0; i < filePaths.length; i += BULK_CHUNK_SIZE) { const chunk = filePaths.slice(i, i + BULK_CHUNK_SIZE) - const { stdout } = await gitExecFileAsync(['ls-files', '-z', '--', ...chunk], { - cwd: worktreePath - }) + const { stdout } = await gitExecFileAsync( + ['ls-files', '-z', '--', ...chunk.map(literalPathspec)], + { + cwd: worktreePath + } + ) trackedPaths.push(...stdout.split('\0').filter(Boolean)) } return trackedPaths @@ -1160,7 +1171,9 @@ async function cleanUntrackedPaths( const chunk = filePaths.slice(i, i + BULK_CHUNK_SIZE) if (chunk.length > 0) { // Why: Git pathspec cleanup avoids raw recursive deletion through symlinked parents. - await gitExecFileAsync(['clean', '-ffdx', '--', ...chunk], { cwd: worktreePath }) + await gitExecFileAsync(['clean', '-ffdx', '--', ...chunk.map(literalPathspec)], { + cwd: worktreePath + }) } } } @@ -1193,9 +1206,12 @@ export async function bulkDiscardChanges(worktreePath: string, filePaths: string async () => { for (let i = 0; i < trackedPaths.length; i += BULK_CHUNK_SIZE) { const chunk = trackedPaths.slice(i, i + BULK_CHUNK_SIZE) - await gitExecFileAsync(['restore', '--worktree', '--source=HEAD', '--', ...chunk], { - cwd: worktreePath - }) + await gitExecFileAsync( + ['restore', '--worktree', '--source=HEAD', '--', ...chunk.map(literalPathspec)], + { + cwd: worktreePath + } + ) } } ) diff --git a/src/main/index.ts b/src/main/index.ts index 8ba2c81c749..d1d9c5ff7ad 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -55,7 +55,10 @@ import { import { startFirstWindowStartupServices } from './startup/first-window-startup-services' import { getDevInstanceIdentity } from './startup/dev-instance-identity' import { hydrateShellPath, mergePathSegments } from './startup/hydrate-shell-path' -import { acquireSingleInstanceLock } from './startup/single-instance-lock' +import { + acquireSingleInstanceLock, + logSingleInstanceLockFailure +} from './startup/single-instance-lock' import { RateLimitService } from './rate-limits/service' import { getInitialClaudeRateLimitTarget } from './rate-limits/claude-rate-limit-target' import { getInitialCodexRateLimitTarget } from './rate-limits/codex-rate-limit-target' @@ -273,14 +276,9 @@ function getExpectedTeardownScope(webContentsId?: number): ExpectedTeardownScope const hasSingleInstanceLock = is.dev && !isServeMode ? true : acquireSingleInstanceLock(app, focusExistingWindow) if (!hasSingleInstanceLock) { - if (is.dev) { - // Why: packaged runs have no attached console, but dev runs do. Emit a - // single line so a `pnpm dev` operator does not mistake a silent exit - // for a broken launcher. - console.log( - '[single-instance] Another Orca instance is already running against this userData path — focusing existing window.' - ) - } + // Why: if Electron returns a false negative here, packaged macOS launches + // otherwise look like silent crashes. `open --stderr` can capture this line. + logSingleInstanceLockFailure() app.quit() } diff --git a/src/main/ipc/filesystem.test.ts b/src/main/ipc/filesystem.test.ts index a6bfe4c5213..3722855261f 100644 --- a/src/main/ipc/filesystem.test.ts +++ b/src/main/ipc/filesystem.test.ts @@ -31,9 +31,11 @@ const { listWorktreesMock, resolveCommitMessageSettingsMock, generateCommitMessageFromContextMock, + generatePullRequestFieldsFromContextMock, discoverCommitMessageModelsLocalMock, discoverCommitMessageModelsRemoteMock, cancelGenerateCommitMessageLocalMock, + cancelGeneratePullRequestFieldsLocalMock, getSshFilesystemProviderMock, getSshGitProviderMock } = vi.hoisted(() => ({ @@ -64,9 +66,11 @@ const { listWorktreesMock: vi.fn(), resolveCommitMessageSettingsMock: vi.fn(), generateCommitMessageFromContextMock: vi.fn(), + generatePullRequestFieldsFromContextMock: vi.fn(), discoverCommitMessageModelsLocalMock: vi.fn(), discoverCommitMessageModelsRemoteMock: vi.fn(), cancelGenerateCommitMessageLocalMock: vi.fn(), + cancelGeneratePullRequestFieldsLocalMock: vi.fn(), getSshFilesystemProviderMock: vi.fn(), getSshGitProviderMock: vi.fn() })) @@ -139,9 +143,11 @@ vi.mock('../providers/ssh-git-dispatch', () => ({ vi.mock('../text-generation/commit-message-text-generation', () => ({ resolveCommitMessageSettings: resolveCommitMessageSettingsMock, generateCommitMessageFromContext: generateCommitMessageFromContextMock, + generatePullRequestFieldsFromContext: generatePullRequestFieldsFromContextMock, discoverCommitMessageModelsLocal: discoverCommitMessageModelsLocalMock, discoverCommitMessageModelsRemote: discoverCommitMessageModelsRemoteMock, - cancelGenerateCommitMessageLocal: cancelGenerateCommitMessageLocalMock + cancelGenerateCommitMessageLocal: cancelGenerateCommitMessageLocalMock, + cancelGeneratePullRequestFieldsLocal: cancelGeneratePullRequestFieldsLocalMock })) import { registerFilesystemHandlers } from './filesystem' @@ -219,9 +225,11 @@ describe('registerFilesystemHandlers', () => { listWorktreesMock, resolveCommitMessageSettingsMock, generateCommitMessageFromContextMock, + generatePullRequestFieldsFromContextMock, discoverCommitMessageModelsLocalMock, discoverCommitMessageModelsRemoteMock, cancelGenerateCommitMessageLocalMock, + cancelGeneratePullRequestFieldsLocalMock, getSshFilesystemProviderMock, getSshGitProviderMock ]) { @@ -1192,17 +1200,44 @@ describe('registerFilesystemHandlers', () => { await target.execute( { binary: 'agent', args: [], stdinPayload: null, label: 'agent' }, '/cwd', - 1 + 1, + 'commit-message' ) expect(executeCommitMessagePlan).toHaveBeenCalledWith( { binary: 'agent', args: [], stdinPayload: null, label: 'agent' }, '/cwd', - 1 + 1, + 'commit-message' ) expect(prepareForCodexLaunch).not.toHaveBeenCalled() expect(prepareForClaudeLaunch).not.toHaveBeenCalled() }) + it('routes SSH generation cancellations to separate provider operations', async () => { + const cancelGenerateCommitMessage = vi.fn().mockResolvedValue(undefined) + getSshGitProviderMock.mockReturnValue({ cancelGenerateCommitMessage }) + + registerFilesystemHandlers(store as never) + + await handlers.get('git:cancelGenerateCommitMessage')!(null, { + worktreePath: '/remote/repo', + connectionId: 'conn-1' + }) + await handlers.get('git:cancelGeneratePullRequestFields')!(null, { + worktreePath: '/remote/repo', + connectionId: 'conn-1' + }) + + expect(cancelGenerateCommitMessage).toHaveBeenNthCalledWith(1, '/remote/repo', 'commit-message') + expect(cancelGenerateCommitMessage).toHaveBeenNthCalledWith( + 2, + '/remote/repo', + 'pull-request-fields' + ) + expect(cancelGenerateCommitMessageLocalMock).not.toHaveBeenCalled() + expect(cancelGeneratePullRequestFieldsLocalMock).not.toHaveBeenCalled() + }) + it('does not call the generator when no staged changes exist', async () => { resolveCommitMessageSettingsMock.mockReturnValue({ ok: true, diff --git a/src/main/ipc/filesystem.ts b/src/main/ipc/filesystem.ts index 8d57e5a15bf..5fb43bd5c1f 100644 --- a/src/main/ipc/filesystem.ts +++ b/src/main/ipc/filesystem.ts @@ -823,8 +823,8 @@ export function registerFilesystemHandlers( return generateCommitMessageFromContext(context, resolvedSettings.params, { kind: 'remote', cwd: args.worktreePath, - execute: (plan, cwd, timeoutMs) => - provider.executeCommitMessagePlan(plan, cwd, timeoutMs), + execute: (plan, cwd, timeoutMs, operation) => + provider.executeCommitMessagePlan(plan, cwd, timeoutMs, operation), missingBinaryLocation: 'remote PATH' }) } @@ -865,7 +865,7 @@ export function registerFilesystemHandlers( if (!provider) { return } - await provider.cancelGenerateCommitMessage(args.worktreePath) + await provider.cancelGenerateCommitMessage(args.worktreePath, 'commit-message') return } const worktreePath = await resolveRegisteredWorktreePath(args.worktreePath, store) @@ -967,8 +967,8 @@ export function registerFilesystemHandlers( return generatePullRequestFieldsFromContext(context, resolvedSettings.params, { kind: 'remote', cwd: args.worktreePath, - execute: (plan, cwd, timeoutMs) => - provider.executeCommitMessagePlan(plan, cwd, timeoutMs), + execute: (plan, cwd, timeoutMs, operation) => + provider.executeCommitMessagePlan(plan, cwd, timeoutMs, operation), missingBinaryLocation: 'remote PATH' }) } @@ -1017,7 +1017,7 @@ export function registerFilesystemHandlers( if (!provider) { return } - await provider.cancelGenerateCommitMessage(args.worktreePath) + await provider.cancelGenerateCommitMessage(args.worktreePath, 'pull-request-fields') return } const worktreePath = await resolveRegisteredWorktreePath(args.worktreePath, store) diff --git a/src/main/ipc/workspace-ports.test.ts b/src/main/ipc/workspace-ports.test.ts index c688cdb156f..ce66b79b8b6 100644 --- a/src/main/ipc/workspace-ports.test.ts +++ b/src/main/ipc/workspace-ports.test.ts @@ -258,6 +258,26 @@ describe('registerWorkspacePortHandlers', () => { port: 3002 }) }) + + it('unsubscribes the previous advertised URL listener when handlers are registered again', () => { + const store = makeStore() + const firstUnsubscribe = vi.fn() + const secondUnsubscribe = vi.fn() + const onDidChange = vi + .fn() + .mockReturnValueOnce(firstUnsubscribe) + .mockReturnValueOnce(secondUnsubscribe) + + registerWorkspacePortHandlers(store as never, { + advertisedUrlEvents: { onDidChange } + }) + registerWorkspacePortHandlers(store as never, { + advertisedUrlEvents: { onDidChange } + }) + + expect(firstUnsubscribe).toHaveBeenCalledTimes(1) + expect(secondUnsubscribe).not.toHaveBeenCalled() + }) }) function workspacePort({ pid, port }: { pid: number; port: number }): WorkspacePort { diff --git a/src/main/ipc/workspace-ports.ts b/src/main/ipc/workspace-ports.ts index b2041fa4e09..27d1df254d7 100644 --- a/src/main/ipc/workspace-ports.ts +++ b/src/main/ipc/workspace-ports.ts @@ -19,6 +19,8 @@ type WorkspacePortHandlersOptions = { getWindows?: () => BrowserWindow[] } +let unsubscribeAdvertisedUrlChanges: (() => void) | null = null + export function registerWorkspacePortHandlers( store: Store, options: WorkspacePortHandlersOptions = {} @@ -27,7 +29,8 @@ export function registerWorkspacePortHandlers( const advertisedUrlEvents = options.advertisedUrlEvents ?? advertisedUrlWatcher const getWindows = options.getWindows ?? (() => BrowserWindow.getAllWindows()) - advertisedUrlEvents.onDidChange((event) => { + unsubscribeAdvertisedUrlChanges?.() + unsubscribeAdvertisedUrlChanges = advertisedUrlEvents.onDidChange((event) => { const localWorktrees = getStoreWorkspacePortProbes(store) if (!localWorktrees.some((worktree) => worktree.id === event.worktreeId)) { return diff --git a/src/main/providers/ssh-git-provider.test.ts b/src/main/providers/ssh-git-provider.test.ts index f3cf469a1c6..0d0c334342b 100644 --- a/src/main/providers/ssh-git-provider.test.ts +++ b/src/main/providers/ssh-git-provider.test.ts @@ -241,12 +241,80 @@ describe('SshGitProvider', () => { args: ['exec', 'PROMPT'], cwd: '/home/user/repo', stdin: null, - timeoutMs: 60_000 + timeoutMs: 60_000, + operation: 'commit-message' }) expect(result).toEqual(execResult) }) - it('serializes non-interactive relay execs for the same cwd', async () => { + it('keeps SSH commit-message and pull-request execution lanes separate', async () => { + const completeRequests: (() => void)[] = [] + mux.request.mockImplementation((method) => { + if (method === 'agent.cancelExec') { + return Promise.resolve({ canceled: true }) + } + return new Promise((resolve) => { + completeRequests.push(() => + resolve({ + stdout: '', + stderr: '', + exitCode: 0, + timedOut: false + }) + ) + }) + }) + const plan = { + binary: 'codex', + args: ['exec', 'PROMPT'], + stdinPayload: null, + label: 'Codex' + } + + const commit = provider.executeCommitMessagePlan(plan, '/home/user/repo', 60_000) + const pullRequest = provider.executeCommitMessagePlan( + plan, + '/home/user/repo', + 60_000, + 'pull-request-fields' + ) + + await waitForRequestCount(mux.request, 2) + expect(mux.request).toHaveBeenNthCalledWith(1, 'agent.execNonInteractive', { + binary: 'codex', + args: ['exec', 'PROMPT'], + cwd: '/home/user/repo', + stdin: null, + timeoutMs: 60_000, + operation: 'commit-message' + }) + expect(mux.request).toHaveBeenNthCalledWith(2, 'agent.execNonInteractive', { + binary: 'codex', + args: ['exec', 'PROMPT'], + cwd: '/home/user/repo', + stdin: null, + timeoutMs: 60_000, + operation: 'pull-request-fields' + }) + + await provider.cancelGenerateCommitMessage('/home/user/repo') + await provider.cancelGenerateCommitMessage('/home/user/repo', 'pull-request-fields') + + expect(mux.request).toHaveBeenNthCalledWith(3, 'agent.cancelExec', { + cwd: '/home/user/repo', + operation: 'commit-message' + }) + expect(mux.request).toHaveBeenNthCalledWith(4, 'agent.cancelExec', { + cwd: '/home/user/repo', + operation: 'pull-request-fields' + }) + + completeRequests.shift()?.() + completeRequests.shift()?.() + await Promise.all([commit, pullRequest]) + }) + + it('serializes non-interactive relay execs for the same cwd and operation', async () => { const completeRequests: (() => void)[] = [] mux.request.mockImplementation( () => @@ -263,16 +331,7 @@ describe('SshGitProvider', () => { ) const first = provider.execNonInteractive('pnpm', ['store', 'prune'], '/home/user/repo', 8000) - const second = provider.executeCommitMessagePlan( - { - binary: 'codex', - args: ['exec', 'PROMPT'], - stdinPayload: null, - label: 'Codex' - }, - '/home/user/repo', - 60_000 - ) + const second = provider.execNonInteractive('pnpm', ['install'], '/home/user/repo', 8000) await waitForRequestCount(mux.request, 1) expect(mux.request).toHaveBeenCalledTimes(1) @@ -282,11 +341,11 @@ describe('SshGitProvider', () => { await waitForRequestCount(mux.request, 2) expect(mux.request).toHaveBeenNthCalledWith(2, 'agent.execNonInteractive', { - binary: 'codex', - args: ['exec', 'PROMPT'], + binary: 'pnpm', + args: ['install'], cwd: '/home/user/repo', stdin: null, - timeoutMs: 60_000 + timeoutMs: 8000 }) completeRequests.shift()?.() await second @@ -309,16 +368,7 @@ describe('SshGitProvider', () => { ) const first = provider.execNonInteractive('pnpm', ['store', 'prune'], '/home/user/repo', 8000) - const second = provider.executeCommitMessagePlan( - { - binary: 'codex', - args: ['exec', 'PROMPT'], - stdinPayload: null, - label: 'Codex' - }, - '/home/user/repo', - 60_000 - ) + const second = provider.execNonInteractive('pnpm', ['install'], '/home/user/repo', 8000) await waitForRequestCount(mux.request, 1) await provider.cancelNonInteractiveExec('/home/user/repo') @@ -360,16 +410,7 @@ describe('SshGitProvider', () => { 8000, controller.signal ) - const second = provider.executeCommitMessagePlan( - { - binary: 'codex', - args: ['exec', 'PROMPT'], - stdinPayload: null, - label: 'Codex' - }, - '/home/user/repo', - 60_000 - ) + const second = provider.execNonInteractive('pnpm', ['install'], '/home/user/repo', 8000) await waitForRequestCount(mux.request, 1) controller.abort() @@ -381,11 +422,11 @@ describe('SshGitProvider', () => { await first await waitForRequestCount(mux.request, 3) expect(mux.request).toHaveBeenNthCalledWith(3, 'agent.execNonInteractive', { - binary: 'codex', - args: ['exec', 'PROMPT'], + binary: 'pnpm', + args: ['install'], cwd: '/home/user/repo', stdin: null, - timeoutMs: 60_000 + timeoutMs: 8000 }) completeRequests.shift()?.() await second @@ -394,7 +435,10 @@ describe('SshGitProvider', () => { it('cancelGenerateCommitMessage sends best-effort relay cancellation', async () => { await provider.cancelGenerateCommitMessage('/home/user/repo') - expect(mux.request).toHaveBeenCalledWith('agent.cancelExec', { cwd: '/home/user/repo' }) + expect(mux.request).toHaveBeenCalledWith('agent.cancelExec', { + cwd: '/home/user/repo', + operation: 'commit-message' + }) }) it('getDiff sends git.diff request', async () => { diff --git a/src/main/providers/ssh-git-provider.ts b/src/main/providers/ssh-git-provider.ts index 2cbce98b597..b96361e527e 100644 --- a/src/main/providers/ssh-git-provider.ts +++ b/src/main/providers/ssh-git-provider.ts @@ -105,15 +105,22 @@ export class SshGitProvider implements IGitProvider { async executeCommitMessagePlan( plan: CommitMessagePlan, cwd: string, - timeoutMs: number + timeoutMs: number, + operation = 'commit-message' ): Promise { - return this.runQueuedNonInteractiveExec(cwd, { - binary: plan.binary, - args: plan.args, + return this.runQueuedNonInteractiveExec( cwd, - stdin: plan.stdinPayload, - timeoutMs - }) + { + binary: plan.binary, + args: plan.args, + cwd, + stdin: plan.stdinPayload, + timeoutMs, + operation + }, + undefined, + operation + ) } async execNonInteractive( @@ -138,28 +145,38 @@ export class SshGitProvider implements IGitProvider { ) } - async cancelNonInteractiveExec(cwd: string): Promise { - const queue = this.nonInteractiveExecQueues.get(cwd) + async cancelNonInteractiveExec(cwd: string, operation?: string): Promise { + const queue = this.nonInteractiveExecQueues.get(this.nonInteractiveLaneKey(cwd, operation)) const queuedEntry = queue?.find((entry) => !entry.started && !entry.canceled) if (queuedEntry) { queuedEntry.canceled = true return } - await this.cancelActiveNonInteractiveExec(cwd) + await this.cancelActiveNonInteractiveExec(cwd, operation) } - private async cancelActiveNonInteractiveExec(cwd: string): Promise { + private async cancelActiveNonInteractiveExec(cwd: string, operation?: string): Promise { try { - await this.mux.request('agent.cancelExec', { cwd }) + await this.mux.request('agent.cancelExec', { + cwd, + ...(operation ? { operation } : {}) + }) } catch { // Best-effort: callers are already unwinding after cancellation. } } - async cancelGenerateCommitMessage(worktreePath: string): Promise { + async cancelGenerateCommitMessage( + worktreePath: string, + operation = 'commit-message' + ): Promise { // Why: best-effort — the relay returns `{canceled: false}` when there is // nothing in flight. Callers should not block UI updates on this. - await this.cancelNonInteractiveExec(worktreePath) + await this.cancelNonInteractiveExec(worktreePath, operation) + } + + private nonInteractiveLaneKey(cwd: string, operation?: string): string { + return JSON.stringify([operation || 'default', cwd]) } private async runQueuedNonInteractiveExec( @@ -171,10 +188,13 @@ export class SshGitProvider implements IGitProvider { stdin: string | null timeoutMs: number env?: Record + operation?: string }, - signal?: AbortSignal + signal?: AbortSignal, + operation?: string ): Promise { - const queue = this.nonInteractiveExecQueues.get(cwd) ?? [] + const laneKey = this.nonInteractiveLaneKey(cwd, operation) + const queue = this.nonInteractiveExecQueues.get(laneKey) ?? [] const previous = queue.at(-1)?.done ?? Promise.resolve() let releaseEntry!: () => void const entry: NonInteractiveExecQueueEntry = { @@ -191,13 +211,13 @@ export class SshGitProvider implements IGitProvider { release: () => releaseEntry() } queue.push(entry) - this.nonInteractiveExecQueues.set(cwd, queue) + this.nonInteractiveExecQueues.set(laneKey, queue) const abortEntry = (): void => { if (!entry.started) { entry.canceled = true return } - void this.cancelActiveNonInteractiveExec(cwd) + void this.cancelActiveNonInteractiveExec(cwd, operation) } if (signal?.aborted) { entry.canceled = true @@ -205,8 +225,8 @@ export class SshGitProvider implements IGitProvider { signal?.addEventListener('abort', abortEntry, { once: true }) } - // Why: the SSH relay tracks one non-interactive child per cwd; serializing - // here keeps cache cleanup and commit-message generation from overwriting it. + // Why: the SSH relay tracks children per operation; serialize only matching + // lanes so commit-message and PR-field generation can coexist. await previous.catch(() => {}) try { if (entry.canceled) { @@ -226,13 +246,13 @@ export class SshGitProvider implements IGitProvider { } finally { signal?.removeEventListener('abort', abortEntry) entry.release() - const currentQueue = this.nonInteractiveExecQueues.get(cwd) + const currentQueue = this.nonInteractiveExecQueues.get(laneKey) const entryIndex = currentQueue?.indexOf(entry) ?? -1 if (entryIndex >= 0) { currentQueue?.splice(entryIndex, 1) } if (currentQueue?.length === 0) { - this.nonInteractiveExecQueues.delete(cwd) + this.nonInteractiveExecQueues.delete(laneKey) } } } diff --git a/src/main/rate-limits/codex-fetcher-auth-errors.test.ts b/src/main/rate-limits/codex-fetcher-auth-errors.test.ts new file mode 100644 index 00000000000..08674c9d98a --- /dev/null +++ b/src/main/rate-limits/codex-fetcher-auth-errors.test.ts @@ -0,0 +1,130 @@ +import { EventEmitter } from 'node:events' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { childSpawnMock, resolveCodexCommandMock, ptySpawnMock } = vi.hoisted(() => ({ + childSpawnMock: vi.fn(), + resolveCodexCommandMock: vi.fn(), + ptySpawnMock: vi.fn() +})) + +vi.mock('node:child_process', () => ({ + spawn: childSpawnMock +})) + +vi.mock('../codex-cli/command', () => ({ + resolveCodexCommand: resolveCodexCommandMock +})) + +vi.mock('node-pty', () => ({ + spawn: ptySpawnMock +})) + +import { fetchCodexRateLimits } from './codex-fetcher' + +function makeDisposable() { + return { dispose: vi.fn() } +} + +function makeRpcChild() { + const child = new EventEmitter() as EventEmitter & { + stdout: EventEmitter + stderr: EventEmitter + stdin: { write: ReturnType } + kill: ReturnType + } + child.stdout = new EventEmitter() + child.stderr = new EventEmitter() + child.stdin = { write: vi.fn() } + child.kill = vi.fn() + return child +} + +describe('fetchCodexRateLimits auth errors', () => { + beforeEach(() => { + vi.useFakeTimers() + vi.clearAllMocks() + resolveCodexCommandMock.mockReturnValue('codex') + }) + + it('returns Codex RPC auth refresh errors without masking them behind PTY fallback', async () => { + const rpcChild = makeRpcChild() + const authError = + 'Your access token could not be refreshed because your refresh token was already used. Please log out and sign in again.' + + childSpawnMock.mockReturnValue(rpcChild) + rpcChild.stdin.write.mockImplementation((line: string) => { + const msg = JSON.parse(line) as { id?: number; method?: string } + if (msg.method === 'initialize') { + setTimeout(() => { + rpcChild.stdout.emit( + 'data', + Buffer.from(`${JSON.stringify({ jsonrpc: '2.0', id: msg.id, result: {} })}\n`) + ) + }, 0) + } + if (msg.method === 'account/rateLimits/read') { + setTimeout(() => { + rpcChild.stdout.emit( + 'data', + Buffer.from( + `${JSON.stringify({ + jsonrpc: '2.0', + id: msg.id, + error: { code: -32000, message: authError } + })}\n` + ) + ) + }, 0) + } + }) + + const resultPromise = fetchCodexRateLimits() + await vi.advanceTimersByTimeAsync(1) + await vi.advanceTimersByTimeAsync(1) + + await expect(resultPromise).resolves.toMatchObject({ + provider: 'codex', + session: null, + weekly: null, + status: 'error', + error: authError + }) + expect(ptySpawnMock).not.toHaveBeenCalled() + }) + + it('preserves Codex PTY auth errors when the CLI exits before status is available', async () => { + const ptyHandlers: { onData?: (data: string) => void; onExit?: () => void } = {} + const authError = + 'Error loading configuration: Your authentication session could not be refreshed automatically.' + + childSpawnMock.mockImplementation(() => { + throw new Error('rpc unavailable') + }) + ptySpawnMock.mockReturnValue({ + onData: vi.fn((callback) => { + ptyHandlers.onData = callback + return makeDisposable() + }), + onExit: vi.fn((callback) => { + ptyHandlers.onExit = callback + return makeDisposable() + }), + write: vi.fn(), + kill: vi.fn() + }) + + const resultPromise = fetchCodexRateLimits() + await vi.advanceTimersByTimeAsync(0) + + ptyHandlers.onData?.(`${authError}\n`) + ptyHandlers.onExit?.() + + await expect(resultPromise).resolves.toMatchObject({ + provider: 'codex', + session: null, + weekly: null, + status: 'error', + error: authError + }) + }) +}) diff --git a/src/main/rate-limits/codex-fetcher.ts b/src/main/rate-limits/codex-fetcher.ts index feec38d98f7..d46da8c40a7 100644 --- a/src/main/rate-limits/codex-fetcher.ts +++ b/src/main/rate-limits/codex-fetcher.ts @@ -8,6 +8,7 @@ import { withMacTailscaleDnsHint } from '../network/macos-tailscale-dns-diagnost import { getCmdExePath, getSpawnArgsForWindows } from '../win32-utils' import { cleanupHiddenRateLimitPty } from './hidden-pty-cleanup' import { parseWslUncPath } from '../../shared/wsl-paths' +import { extractCodexAuthError, isCodexAuthError } from '../../shared/codex-auth-errors' const RPC_TIMEOUT_MS = 10_000 const WSL_RPC_TIMEOUT_MS = 25_000 @@ -395,7 +396,7 @@ async function fetchViaPty(options?: FetchCodexRateLimitsOptions): Promise provider.executeCommitMessagePlan(plan, cwd, timeoutMs), + execute: (plan, cwd, timeoutMs, operation) => + provider.executeCommitMessagePlan(plan, cwd, timeoutMs, operation), missingBinaryLocation: 'remote PATH' }) } @@ -515,7 +516,7 @@ export class RuntimeGitCommands { const target = await this.host.resolveRuntimeGitTarget(worktreeSelector) const provider = target.connectionId ? getSshGitProvider(target.connectionId) : null if (target.connectionId) { - await provider?.cancelGenerateCommitMessage(target.worktree.path) + await provider?.cancelGenerateCommitMessage(target.worktree.path, 'commit-message') return { ok: true } } cancelGenerateCommitMessageLocal(target.worktree.path) @@ -584,7 +585,8 @@ export class RuntimeGitCommands { return generatePullRequestFieldsFromContext(context, resolvedSettings.params, { kind: 'remote', cwd: target.worktree.path, - execute: (plan, cwd, timeoutMs) => provider!.executeCommitMessagePlan(plan, cwd, timeoutMs), + execute: (plan, cwd, timeoutMs, operation) => + provider!.executeCommitMessagePlan(plan, cwd, timeoutMs, operation), missingBinaryLocation: 'remote PATH' }) } @@ -607,7 +609,7 @@ export class RuntimeGitCommands { const target = await this.host.resolveRuntimeGitTarget(worktreeSelector) const provider = target.connectionId ? getSshGitProvider(target.connectionId) : null if (target.connectionId) { - await provider?.cancelGenerateCommitMessage(target.worktree.path) + await provider?.cancelGenerateCommitMessage(target.worktree.path, 'pull-request-fields') return { ok: true } } cancelGeneratePullRequestFieldsLocal(target.worktree.path) diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index 3e22639615c..6867cca9e93 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -837,6 +837,60 @@ describe('OrcaRuntimeService', () => { expect(shown.ptyId).toBe('pty-1') }) + it('routes PTY output through the PTY leaf index in large terminal graphs', () => { + const runtime = new OrcaRuntimeService(store) + const liveLeafCount = 2773 + const targetIndex = liveLeafCount - 17 + const tabs = Array.from({ length: liveLeafCount }, (_, index) => ({ + tabId: `tab-${index}`, + worktreeId: `repo-1::/tmp/worktree-${index}`, + title: `Terminal ${index}`, + activeLeafId: 'pane:1', + layout: null + })) + const leaves = Array.from({ length: liveLeafCount }, (_, index) => ({ + tabId: `tab-${index}`, + worktreeId: `repo-1::/tmp/worktree-${index}`, + leafId: 'pane:1', + paneRuntimeId: 1, + ptyId: `pty-${index}` + })) + + runtime.attachWindow(1) + runtime.syncWindowGraph(1, { tabs, leaves }) + + const runtimePrivate = runtime as unknown as { + leaves: Map + leavesByPtyId: Map + } + const originalLeaves = runtimePrivate.leaves + runtimePrivate.leaves = new Proxy(originalLeaves, { + get(target, prop) { + if ( + prop === 'values' || + prop === 'entries' || + prop === 'keys' || + prop === Symbol.iterator + ) { + return () => { + throw new Error('onPtyData should use the PTY leaf index') + } + } + const value = Reflect.get(target, prop, target) + return typeof value === 'function' ? value.bind(target) : value + } + }) as Map + + runtime.onPtyData(`pty-${targetIndex}`, 'hello indexed\n', 123) + + const [targetLeaf] = runtimePrivate.leavesByPtyId.get(`pty-${targetIndex}`) ?? [] + expect(targetLeaf).toMatchObject({ + preview: 'hello indexed', + lastOutputAt: 123 + }) + expect(runtime.getStatus().liveLeafCount).toBe(liveLeafCount) + }) + it('resolves branch selectors when worktrees store refs/heads-prefixed branches', async () => { vi.mocked(listWorktrees).mockResolvedValueOnce([ { diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index b3c3f42262c..6b38055a34e 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -1054,6 +1054,9 @@ export class OrcaRuntimeService { private mobileSessionTabsByWorktree = new Map() private mobileSessionTabListeners = new Set<(snapshot: RuntimeMobileSessionTabsResult) => void>() private leaves = new Map() + // Why: PTY output is a per-keystroke hot path. Looking up affected leaves by + // ptyId keeps active TUI redraws independent of the total open terminal count. + private leavesByPtyId = new Map() private handles = new Map() private handleByLeafKey = new Map() private handleByPtyId = new Map() @@ -1772,6 +1775,7 @@ export class OrcaRuntimeService { } this.leaves = nextLeaves + this.rebuildLeafPtyIndex() this.notifyMobileSessionTabSnapshots() this.graphStatus = 'ready' this.refreshWritableFlags() @@ -2215,10 +2219,8 @@ export class OrcaRuntimeService { registerPreAllocatedHandleForPty(ptyId: string, handle: string): void { this.handleByPtyId.set(ptyId, handle) - for (const leaf of this.leaves.values()) { - if (leaf.ptyId === ptyId) { - this.adoptPreAllocatedHandle(leaf) - } + for (const leaf of this.getLeavesForPty(ptyId)) { + this.adoptPreAllocatedHandle(leaf) } } @@ -2227,12 +2229,10 @@ export class OrcaRuntimeService { if (pty) { pty.connected = true } - for (const leaf of this.leaves.values()) { - if (leaf.ptyId === ptyId) { - leaf.connected = true - leaf.writable = this.graphStatus === 'ready' - this.adoptPreAllocatedHandle(leaf) - } + for (const leaf of this.getLeavesForPty(ptyId)) { + leaf.connected = true + leaf.writable = this.graphStatus === 'ready' + this.adoptPreAllocatedHandle(leaf) } } @@ -2310,10 +2310,7 @@ export class OrcaRuntimeService { } } - for (const leaf of this.leaves.values()) { - if (leaf.ptyId !== ptyId) { - continue - } + for (const leaf of this.getLeavesForPty(ptyId)) { this.recordPtyWorktree(ptyId, leaf.worktreeId, { connected: true, lastOutputAt: pty?.lastOutputAt ?? at, @@ -2608,15 +2605,13 @@ export class OrcaRuntimeService { return } const status = detectAgentStatusFromTitle(title) - for (const leaf of this.leaves.values()) { - if (leaf.ptyId === ptyId) { - // Why: seed lastOscTitle even when the seeded title doesn't classify - // as an agent state, so worktree.ps recomputes status from the live - // title rather than treating the leaf as agentless. - leaf.lastOscTitle = title - if (status !== null) { - leaf.lastAgentStatus = status - } + for (const leaf of this.getLeavesForPty(ptyId)) { + // Why: seed lastOscTitle even when the seeded title doesn't classify + // as an agent state, so worktree.ps recomputes status from the live + // title rather than treating the leaf as agentless. + leaf.lastOscTitle = title + if (status !== null) { + leaf.lastAgentStatus = status } } } @@ -3471,10 +3466,7 @@ export class OrcaRuntimeService { this.pruneDisconnectedPtyTranscript(pty) } - for (const leaf of this.leaves.values()) { - if (leaf.ptyId !== ptyId) { - continue - } + for (const leaf of this.getLeavesForPty(ptyId)) { this.detachedPreAllocatedLeaves.delete(ptyId) leaf.connected = false leaf.writable = false @@ -9945,6 +9937,7 @@ export class OrcaRuntimeService { this.rememberDetachedPreAllocatedLeaves() this.tabs.clear() this.leaves.clear() + this.leavesByPtyId.clear() this.handles.clear() this.handleByLeafKey.clear() // Why: same as markRendererReloading — pre-allocated CLI handles must @@ -10714,12 +10707,27 @@ export class OrcaRuntimeService { } private leafExistsForPty(ptyId: string): boolean { + return (this.leavesByPtyId.get(ptyId)?.length ?? 0) > 0 + } + + private rebuildLeafPtyIndex(): void { + const next = new Map() for (const leaf of this.leaves.values()) { - if (leaf.ptyId === ptyId) { - return true + if (!leaf.ptyId) { + continue + } + const leaves = next.get(leaf.ptyId) + if (leaves) { + leaves.push(leaf) + } else { + next.set(leaf.ptyId, [leaf]) } } - return false + this.leavesByPtyId = next + } + + private getLeavesForPty(ptyId: string): RuntimeLeafRecord[] { + return this.leavesByPtyId.get(ptyId) ?? [] } private getSummaryForRuntimeWorktreeId( diff --git a/src/main/runtime/rpc/methods/clipboard.test.ts b/src/main/runtime/rpc/methods/clipboard.test.ts index 872bb063dc1..d8b729d317c 100644 --- a/src/main/runtime/rpc/methods/clipboard.test.ts +++ b/src/main/runtime/rpc/methods/clipboard.test.ts @@ -1,4 +1,4 @@ -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { RpcDispatcher } from '../dispatcher' import type { RpcRequest } from '../core' import type { OrcaRuntimeService } from '../../orca-runtime' @@ -11,23 +11,38 @@ vi.mock('../../../window/clipboard-image-temp-file', () => ({ saveClipboardImageBufferAsTempFile })) -import { CLIPBOARD_METHODS } from './clipboard' +import { + CLIPBOARD_IMAGE_UPLOAD_CHUNK_BASE64_CHARS, + CLIPBOARD_IMAGE_UPLOAD_MAX_CONCURRENT, + CLIPBOARD_METHODS, + resetClipboardImageUploadsForTest +} from './clipboard' function makeRequest(method: string, params?: unknown): RpcRequest { return { id: 'req-1', authToken: 'tok', method, params } } +function makeDispatcher(): RpcDispatcher { + const runtime = { getRuntimeId: () => 'test-runtime' } as unknown as OrcaRuntimeService + return new RpcDispatcher({ runtime, methods: CLIPBOARD_METHODS }) +} + describe('clipboard RPC methods', () => { beforeEach(() => { saveClipboardImageBufferAsTempFile.mockReset() + resetClipboardImageUploadsForTest() + }) + + afterEach(() => { + vi.useRealTimers() + resetClipboardImageUploadsForTest() }) it('saves browser-provided clipboard image bytes on the runtime host', async () => { saveClipboardImageBufferAsTempFile.mockResolvedValue( 'C:\\Users\\alice\\AppData\\Local\\Temp\\orca-paste-image.png' ) - const runtime = { getRuntimeId: () => 'test-runtime' } as unknown as OrcaRuntimeService - const dispatcher = new RpcDispatcher({ runtime, methods: CLIPBOARD_METHODS }) + const dispatcher = makeDispatcher() const response = await dispatcher.dispatch( makeRequest('clipboard.saveImageAsTempFile', { @@ -46,8 +61,7 @@ describe('clipboard RPC methods', () => { }) it('rejects non-base64 clipboard image payloads', async () => { - const runtime = { getRuntimeId: () => 'test-runtime' } as unknown as OrcaRuntimeService - const dispatcher = new RpcDispatcher({ runtime, methods: CLIPBOARD_METHODS }) + const dispatcher = makeDispatcher() const response = await dispatcher.dispatch( makeRequest('clipboard.saveImageAsTempFile', { @@ -58,4 +72,264 @@ describe('clipboard RPC methods', () => { expect(response.ok).toBe(false) expect(saveClipboardImageBufferAsTempFile).not.toHaveBeenCalled() }) + + it('accepts chunked uploads and forwards the recorded connectionId on commit', async () => { + saveClipboardImageBufferAsTempFile.mockResolvedValue('/tmp/orca-paste-image.png') + const dispatcher = makeDispatcher() + const contentBase64 = Buffer.from('png-bytes').toString('base64') + + const start = await dispatcher.dispatch( + makeRequest('clipboard.startImageUpload', { + expectedBase64Length: contentBase64.length, + connectionId: 'ssh-1' + }) + ) + expect(start.ok).toBe(true) + const uploadId = (start.ok ? start.result : null) as { uploadId: string } + + const firstChunk = contentBase64.slice(0, 4) + const secondChunk = contentBase64.slice(4) + await expect( + dispatcher.dispatch( + makeRequest('clipboard.appendImageUploadChunk', { + uploadId: uploadId.uploadId, + offset: 0, + contentBase64: firstChunk + }) + ) + ).resolves.toMatchObject({ ok: true, result: { receivedBase64Length: 4 } }) + await expect( + dispatcher.dispatch( + makeRequest('clipboard.appendImageUploadChunk', { + uploadId: uploadId.uploadId, + offset: firstChunk.length, + contentBase64: secondChunk + }) + ) + ).resolves.toMatchObject({ ok: true, result: { receivedBase64Length: contentBase64.length } }) + + await expect( + dispatcher.dispatch( + makeRequest('clipboard.commitImageUpload', { uploadId: uploadId.uploadId }) + ) + ).resolves.toMatchObject({ ok: true, result: '/tmp/orca-paste-image.png' }) + expect(saveClipboardImageBufferAsTempFile).toHaveBeenCalledWith(Buffer.from('png-bytes'), { + connectionId: 'ssh-1' + }) + }) + + it('rejects out-of-order chunk offsets', async () => { + const dispatcher = makeDispatcher() + const start = await dispatcher.dispatch( + makeRequest('clipboard.startImageUpload', { + expectedBase64Length: 8, + connectionId: null + }) + ) + const uploadId = (start.ok ? start.result : null) as { uploadId: string } + + const response = await dispatcher.dispatch( + makeRequest('clipboard.appendImageUploadChunk', { + uploadId: uploadId.uploadId, + offset: 4, + contentBase64: 'AAAA' + }) + ) + + expect(response.ok).toBe(false) + expect(saveClipboardImageBufferAsTempFile).not.toHaveBeenCalled() + }) + + it('rejects invalid base64 chunks and oversized chunks', async () => { + const dispatcher = makeDispatcher() + const start = await dispatcher.dispatch( + makeRequest('clipboard.startImageUpload', { + expectedBase64Length: CLIPBOARD_IMAGE_UPLOAD_CHUNK_BASE64_CHARS + 4, + connectionId: null + }) + ) + const uploadId = (start.ok ? start.result : null) as { uploadId: string } + + await expect( + dispatcher.dispatch( + makeRequest('clipboard.appendImageUploadChunk', { + uploadId: uploadId.uploadId, + offset: 0, + contentBase64: 'not base64!' + }) + ) + ).resolves.toMatchObject({ ok: false }) + await expect( + dispatcher.dispatch( + makeRequest('clipboard.appendImageUploadChunk', { + uploadId: uploadId.uploadId, + offset: 0, + contentBase64: 'A'.repeat(CLIPBOARD_IMAGE_UPLOAD_CHUNK_BASE64_CHARS + 4) + }) + ) + ).resolves.toMatchObject({ ok: false }) + }) + + it('rejects uploads beyond the existing total clipboard image limit', async () => { + const dispatcher = makeDispatcher() + + const response = await dispatcher.dispatch( + makeRequest('clipboard.startImageUpload', { + expectedBase64Length: 24 * 1024 * 1024 + 1, + connectionId: null + }) + ) + + expect(response.ok).toBe(false) + }) + + it('rejects commit until all expected bytes arrive', async () => { + const dispatcher = makeDispatcher() + const start = await dispatcher.dispatch( + makeRequest('clipboard.startImageUpload', { + expectedBase64Length: 8, + connectionId: null + }) + ) + const uploadId = (start.ok ? start.result : null) as { uploadId: string } + await dispatcher.dispatch( + makeRequest('clipboard.appendImageUploadChunk', { + uploadId: uploadId.uploadId, + offset: 0, + contentBase64: 'AAAA' + }) + ) + + const response = await dispatcher.dispatch( + makeRequest('clipboard.commitImageUpload', { uploadId: uploadId.uploadId }) + ) + + expect(response.ok).toBe(false) + expect(saveClipboardImageBufferAsTempFile).not.toHaveBeenCalled() + }) + + it('validates the complete base64 payload before saving', async () => { + const dispatcher = makeDispatcher() + const start = await dispatcher.dispatch( + makeRequest('clipboard.startImageUpload', { + expectedBase64Length: 8, + connectionId: null + }) + ) + const uploadId = (start.ok ? start.result : null) as { uploadId: string } + await dispatcher.dispatch( + makeRequest('clipboard.appendImageUploadChunk', { + uploadId: uploadId.uploadId, + offset: 0, + contentBase64: 'AA==' + }) + ) + await dispatcher.dispatch( + makeRequest('clipboard.appendImageUploadChunk', { + uploadId: uploadId.uploadId, + offset: 4, + contentBase64: 'AAAA' + }) + ) + + const response = await dispatcher.dispatch( + makeRequest('clipboard.commitImageUpload', { uploadId: uploadId.uploadId }) + ) + + expect(response.ok).toBe(false) + expect(saveClipboardImageBufferAsTempFile).not.toHaveBeenCalled() + }) + + it('deletes upload state after abort and treats repeated aborts as success', async () => { + const dispatcher = makeDispatcher() + const start = await dispatcher.dispatch( + makeRequest('clipboard.startImageUpload', { + expectedBase64Length: 4, + connectionId: null + }) + ) + const uploadId = (start.ok ? start.result : null) as { uploadId: string } + + await expect( + dispatcher.dispatch( + makeRequest('clipboard.abortImageUpload', { uploadId: uploadId.uploadId }) + ) + ).resolves.toMatchObject({ ok: true, result: { aborted: true } }) + await expect( + dispatcher.dispatch( + makeRequest('clipboard.abortImageUpload', { uploadId: uploadId.uploadId }) + ) + ).resolves.toMatchObject({ ok: true, result: { aborted: true } }) + await expect( + dispatcher.dispatch( + makeRequest('clipboard.commitImageUpload', { uploadId: uploadId.uploadId }) + ) + ).resolves.toMatchObject({ ok: false }) + }) + + it('deletes upload state when saving fails during commit', async () => { + saveClipboardImageBufferAsTempFile.mockRejectedValue(new Error('ssh write failed')) + const dispatcher = makeDispatcher() + const contentBase64 = Buffer.from('png-bytes').toString('base64') + const start = await dispatcher.dispatch( + makeRequest('clipboard.startImageUpload', { + expectedBase64Length: contentBase64.length, + connectionId: 'ssh-1' + }) + ) + const uploadId = (start.ok ? start.result : null) as { uploadId: string } + await dispatcher.dispatch( + makeRequest('clipboard.appendImageUploadChunk', { + uploadId: uploadId.uploadId, + offset: 0, + contentBase64 + }) + ) + + await expect( + dispatcher.dispatch( + makeRequest('clipboard.commitImageUpload', { uploadId: uploadId.uploadId }) + ) + ).resolves.toMatchObject({ ok: false }) + await expect( + dispatcher.dispatch( + makeRequest('clipboard.commitImageUpload', { uploadId: uploadId.uploadId }) + ) + ).resolves.toMatchObject({ ok: false }) + expect(saveClipboardImageBufferAsTempFile).toHaveBeenCalledTimes(1) + }) + + it('bounds concurrent uploads and releases slots through TTL cleanup', async () => { + vi.useFakeTimers() + const dispatcher = makeDispatcher() + for (let index = 0; index < CLIPBOARD_IMAGE_UPLOAD_MAX_CONCURRENT; index++) { + await expect( + dispatcher.dispatch( + makeRequest('clipboard.startImageUpload', { + expectedBase64Length: 4, + connectionId: null + }) + ) + ).resolves.toMatchObject({ ok: true }) + } + await expect( + dispatcher.dispatch( + makeRequest('clipboard.startImageUpload', { + expectedBase64Length: 4, + connectionId: null + }) + ) + ).resolves.toMatchObject({ ok: false }) + + vi.advanceTimersByTime(5 * 60 * 1000 + 1) + + await expect( + dispatcher.dispatch( + makeRequest('clipboard.startImageUpload', { + expectedBase64Length: 4, + connectionId: null + }) + ) + ).resolves.toMatchObject({ ok: true }) + }) }) diff --git a/src/main/runtime/rpc/methods/clipboard.ts b/src/main/runtime/rpc/methods/clipboard.ts index dc24dc6f920..4c5568f4e25 100644 --- a/src/main/runtime/rpc/methods/clipboard.ts +++ b/src/main/runtime/rpc/methods/clipboard.ts @@ -1,8 +1,75 @@ import { z } from 'zod' import { defineMethod, type RpcMethod } from '../core' import { saveClipboardImageBufferAsTempFile } from '../../../window/clipboard-image-temp-file' +import { randomUUID } from 'node:crypto' const MAX_CLIPBOARD_IMAGE_BASE64_CHARS = 24 * 1024 * 1024 +export const CLIPBOARD_IMAGE_UPLOAD_CHUNK_BASE64_CHARS = 512 * 1024 +export const CLIPBOARD_IMAGE_UPLOAD_MAX_CONCURRENT = 8 +const CLIPBOARD_IMAGE_UPLOAD_TTL_MS = 5 * 60 * 1000 +const BASE64_PATTERN = /^[A-Za-z0-9+/]*={0,2}$/ + +type ClipboardImageUpload = { + expectedBase64Length: number + connectionId?: string | null + chunks: string[] + receivedBase64Length: number + expiresAt: number + ttlTimer: ReturnType +} + +const clipboardImageUploads = new Map() + +function isValidBase64(value: string): boolean { + return value.length % 4 !== 1 && BASE64_PATTERN.test(value) +} + +function pruneExpiredUploads(now = Date.now()): void { + for (const [uploadId, upload] of clipboardImageUploads) { + if (upload.expiresAt <= now) { + deleteUpload(uploadId) + } + } +} + +function scheduleUploadExpiry(uploadId: string): ReturnType { + const timer = setTimeout(() => { + clipboardImageUploads.delete(uploadId) + }, CLIPBOARD_IMAGE_UPLOAD_TTL_MS) + if (typeof timer === 'object' && 'unref' in timer) { + timer.unref() + } + return timer +} + +function refreshUploadExpiry(uploadId: string, upload: ClipboardImageUpload): void { + clearTimeout(upload.ttlTimer) + upload.expiresAt = Date.now() + CLIPBOARD_IMAGE_UPLOAD_TTL_MS + upload.ttlTimer = scheduleUploadExpiry(uploadId) +} + +function deleteUpload(uploadId: string): void { + const upload = clipboardImageUploads.get(uploadId) + if (upload) { + clearTimeout(upload.ttlTimer) + } + clipboardImageUploads.delete(uploadId) +} + +function getUpload(uploadId: string): ClipboardImageUpload { + pruneExpiredUploads() + const upload = clipboardImageUploads.get(uploadId) + if (!upload) { + throw new Error('Clipboard image upload was not found') + } + return upload +} + +function assertValidBase64Content(value: string): void { + if (!isValidBase64(value)) { + throw new Error('Clipboard image content must be base64') + } +} const SaveImageAsTempFile = z.object({ contentBase64: z @@ -11,13 +78,40 @@ const SaveImageAsTempFile = z.object({ .refine((value) => value.length <= MAX_CLIPBOARD_IMAGE_BASE64_CHARS, { message: 'Clipboard image is too large' }) - .refine( - (value) => value.length % 4 !== 1 && /^[A-Za-z0-9+/]*={0,2}$/.test(value), - 'Clipboard image content must be base64' - ), + .refine(isValidBase64, 'Clipboard image content must be base64'), connectionId: z.string().min(1).nullable().optional() }) +const StartImageUpload = z.object({ + expectedBase64Length: z + .number() + .int() + .nonnegative() + .max(MAX_CLIPBOARD_IMAGE_BASE64_CHARS, 'Clipboard image is too large'), + connectionId: z.string().min(1).nullable().optional() +}) + +const AppendImageUploadChunk = z.object({ + uploadId: z.string().min(1), + offset: z.number().int().nonnegative(), + contentBase64: z + .unknown() + .refine((v): v is string => typeof v === 'string', { message: 'Missing image content' }) + .refine( + (value) => value.length <= CLIPBOARD_IMAGE_UPLOAD_CHUNK_BASE64_CHARS, + 'Clipboard image chunk is too large' + ) + .refine(isValidBase64, 'Clipboard image content must be base64') +}) + +const CommitImageUpload = z.object({ + uploadId: z.string().min(1) +}) + +const AbortImageUpload = z.object({ + uploadId: z.string().min(1) +}) + export const CLIPBOARD_METHODS: RpcMethod[] = [ defineMethod({ name: 'clipboard.saveImageAsTempFile', @@ -26,5 +120,78 @@ export const CLIPBOARD_METHODS: RpcMethod[] = [ saveClipboardImageBufferAsTempFile(Buffer.from(params.contentBase64, 'base64'), { connectionId: params.connectionId }) + }), + defineMethod({ + name: 'clipboard.startImageUpload', + params: StartImageUpload, + handler: (params) => { + pruneExpiredUploads() + if (clipboardImageUploads.size >= CLIPBOARD_IMAGE_UPLOAD_MAX_CONCURRENT) { + throw new Error('Too many clipboard image uploads are in progress') + } + const uploadId = randomUUID() + clipboardImageUploads.set(uploadId, { + expectedBase64Length: params.expectedBase64Length, + connectionId: params.connectionId, + chunks: [], + receivedBase64Length: 0, + expiresAt: Date.now() + CLIPBOARD_IMAGE_UPLOAD_TTL_MS, + ttlTimer: scheduleUploadExpiry(uploadId) + }) + return { uploadId } + } + }), + defineMethod({ + name: 'clipboard.appendImageUploadChunk', + params: AppendImageUploadChunk, + handler: (params) => { + const upload = getUpload(params.uploadId) + if (params.offset !== upload.receivedBase64Length) { + throw new Error('Clipboard image chunk offset is out of order') + } + const nextLength = upload.receivedBase64Length + params.contentBase64.length + if (nextLength > upload.expectedBase64Length) { + throw new Error('Clipboard image upload exceeded expected size') + } + upload.chunks.push(params.contentBase64) + upload.receivedBase64Length = nextLength + refreshUploadExpiry(params.uploadId, upload) + return { receivedBase64Length: upload.receivedBase64Length } + } + }), + defineMethod({ + name: 'clipboard.commitImageUpload', + params: CommitImageUpload, + handler: async (params) => { + const upload = getUpload(params.uploadId) + try { + if (upload.receivedBase64Length !== upload.expectedBase64Length) { + throw new Error('Clipboard image upload is incomplete') + } + const contentBase64 = upload.chunks.join('') + assertValidBase64Content(contentBase64) + return await saveClipboardImageBufferAsTempFile(Buffer.from(contentBase64, 'base64'), { + connectionId: upload.connectionId + }) + } finally { + // Why: failed SSH or filesystem commits must not leave bounded upload + // memory pinned until TTL cleanup. + deleteUpload(params.uploadId) + } + } + }), + defineMethod({ + name: 'clipboard.abortImageUpload', + params: AbortImageUpload, + handler: (params) => { + deleteUpload(params.uploadId) + return { aborted: true } + } }) ] + +export function resetClipboardImageUploadsForTest(): void { + for (const uploadId of clipboardImageUploads.keys()) { + deleteUpload(uploadId) + } +} diff --git a/src/main/startup/single-instance-lock.test.ts b/src/main/startup/single-instance-lock.test.ts index 4b1fe7a1c12..5b917f6ac4d 100644 --- a/src/main/startup/single-instance-lock.test.ts +++ b/src/main/startup/single-instance-lock.test.ts @@ -1,6 +1,10 @@ import type { App } from 'electron' import { describe, expect, it, vi } from 'vitest' -import { acquireSingleInstanceLock } from './single-instance-lock' +import { + acquireSingleInstanceLock, + logSingleInstanceLockFailure, + SINGLE_INSTANCE_LOCK_FAILURE_MESSAGE +} from './single-instance-lock' type Listener = (...args: unknown[]) => void @@ -65,3 +69,14 @@ describe('acquireSingleInstanceLock', () => { expect(onSecondInstance).toHaveBeenCalledTimes(1) }) }) + +describe('logSingleInstanceLockFailure', () => { + it('emits a production-visible diagnostic for the early quit path', () => { + const logger = { error: vi.fn() } + + logSingleInstanceLockFailure(logger) + + expect(logger.error).toHaveBeenCalledWith(SINGLE_INSTANCE_LOCK_FAILURE_MESSAGE) + expect(logger.error.mock.calls[0]?.[0]).toContain('Electron/macOS single-instance lock failure') + }) +}) diff --git a/src/main/startup/single-instance-lock.ts b/src/main/startup/single-instance-lock.ts index 0227d454cc0..67a32c54ae5 100644 --- a/src/main/startup/single-instance-lock.ts +++ b/src/main/startup/single-instance-lock.ts @@ -1,5 +1,8 @@ import type { App } from 'electron' +export const SINGLE_INSTANCE_LOCK_FAILURE_MESSAGE = + '[single-instance] Another Orca instance is already running for this userData profile; exiting this launch after requesting the existing window. If no Orca process is running, this may be an Electron/macOS single-instance lock failure.' + /** * Why: Orca writes two canonical discovery files into `/`: * `orca-runtime.json` (RPC endpoint + authToken for the bundled CLI) and @@ -25,3 +28,7 @@ export function acquireSingleInstanceLock(app: App, onSecondInstance: () => void app.on('second-instance', onSecondInstance) return true } + +export function logSingleInstanceLockFailure(logger: Pick = console): void { + logger.error(SINGLE_INSTANCE_LOCK_FAILURE_MESSAGE) +} diff --git a/src/main/text-generation/commit-message-text-generation.test.ts b/src/main/text-generation/commit-message-text-generation.test.ts index d267ce15375..14704c7847b 100644 --- a/src/main/text-generation/commit-message-text-generation.test.ts +++ b/src/main/text-generation/commit-message-text-generation.test.ts @@ -610,6 +610,42 @@ describe('generateCommitMessageFromContext', () => { }) }) + it('reports empty remote commit-message output as an empty message', async () => { + let operation = '' + const result = await generateCommitMessageFromContext( + { + branch: 'main', + stagedSummary: 'M\tREADME.md', + stagedPatch: '+hello' + }, + { + agentId: 'custom', + model: '', + customAgentCommand: 'agent' + }, + { + kind: 'remote', + cwd: '/repo', + missingBinaryLocation: 'remote PATH', + execute: async (_plan, _cwd, _timeoutMs, requestedOperation) => { + operation = requestedOperation + return { + stdout: ' \n', + stderr: '', + exitCode: 0, + timedOut: false + } + } + } + ) + + expect(operation).toBe('commit-message') + expect(result).toEqual({ + success: false, + error: 'agent returned an empty message.' + }) + }) + it('sanitizes remote execution transport failures', async () => { const result = await generateCommitMessageFromContext( { @@ -813,6 +849,130 @@ describe('generateCommitMessageFromContext', () => { expect(children[1]?.kill).not.toHaveBeenCalled() }) + it('keeps local pull-request cancellation from stopping commit-message generation', async () => { + const children: { + kill: ReturnType + listeners: Map void> + }[] = [] + spawnMock.mockImplementation(() => { + const listeners = new Map void>() + const child = { + pid: 123 + children.length, + kill: vi.fn(), + stdout: { on: vi.fn((event, callback) => listeners.set(`stdout:${event}`, callback)) }, + stderr: { on: vi.fn((event, callback) => listeners.set(`stderr:${event}`, callback)) }, + stdin: { end: vi.fn() }, + on: vi.fn((event, callback) => listeners.set(event, callback)) + } + children.push({ kill: child.kill, listeners }) + return child as never + }) + + const commit = generateCommitMessageFromContext( + { + branch: 'main', + stagedSummary: 'M\tREADME.md', + stagedPatch: '+hello' + }, + { + agentId: 'custom', + model: '', + customAgentCommand: 'agent' + }, + { + kind: 'local', + cwd: '/repo' + } + ) + const pullRequest = generatePullRequestFieldsFromContext( + { + branch: 'feature/pr-fields', + base: 'main', + branchChangedByPreparation: false, + currentTitle: '', + currentBody: '', + currentDraft: false, + commitSummary: '- feat: update README', + changeSummary: 'M\tREADME.md', + patch: '+hello' + }, + { + agentId: 'custom', + model: '', + customAgentCommand: 'agent' + }, + { + kind: 'local', + cwd: '/repo' + } + ) + + cancelGeneratePullRequestFieldsLocal('/repo') + + expect(children[0]?.kill).not.toHaveBeenCalled() + expect(children[1]?.kill).toHaveBeenCalledWith('SIGKILL') + + const commitStdout = children[0]?.listeners.get('stdout:data') + commitStdout?.(Buffer.from('Update README\n')) + children[0]?.listeners.get('close')?.(0) + children[1]?.listeners.get('close')?.(null) + + await expect(commit).resolves.toEqual({ + success: true, + message: 'Update README', + agentLabel: 'agent' + }) + await expect(pullRequest).resolves.toEqual({ + success: false, + error: 'Generation canceled.', + canceled: true, + branchChangedByPreparation: false + }) + }) + + it('reports empty remote pull-request field output as empty details', async () => { + let operation = '' + const result = await generatePullRequestFieldsFromContext( + { + branch: 'feature/pr-fields', + base: 'main', + branchChangedByPreparation: false, + currentTitle: '', + currentBody: '', + currentDraft: false, + commitSummary: '- feat: update README', + changeSummary: 'M\tREADME.md', + patch: '+hello' + }, + { + agentId: 'custom', + model: '', + customAgentCommand: 'agent' + }, + { + kind: 'remote', + cwd: '/repo', + missingBinaryLocation: 'remote PATH', + execute: async (_plan, _cwd, _timeoutMs, requestedOperation) => { + operation = requestedOperation + return { + stdout: ' \n', + stderr: '', + exitCode: 0, + timedOut: false + } + } + } + ) + + expect(operation).toBe('pull-request-fields') + expect(result).toEqual({ + success: false, + error: 'agent returned an empty details.', + branchChangedByPreparation: false + }) + }) + it('reports branch changes when pull request field output cannot be parsed', async () => { const listeners = new Map void>() spawnMock.mockReturnValue({ diff --git a/src/main/text-generation/commit-message-text-generation.ts b/src/main/text-generation/commit-message-text-generation.ts index c56d309b50c..4bb5192ae80 100644 --- a/src/main/text-generation/commit-message-text-generation.ts +++ b/src/main/text-generation/commit-message-text-generation.ts @@ -84,6 +84,8 @@ export type RemoteCommitMessageExecResult = { spawnError?: string } +export type TextGenerationOperation = 'commit-message' | 'pull-request-fields' | 'branch-name' + export type CommitMessageGenerationTarget = | { kind: 'local'; cwd: string; env?: NodeJS.ProcessEnv } | { @@ -92,7 +94,8 @@ export type CommitMessageGenerationTarget = execute: ( plan: CommitMessagePlan, cwd: string, - timeoutMs: number + timeoutMs: number, + operation: TextGenerationOperation ) => Promise missingBinaryLocation: string } @@ -428,13 +431,11 @@ function killProcessTree(child: ChildProcess): void { } } -type LocalGenerationOperation = 'commit-message' | 'pull-request-fields' | 'branch-name' - // Keying by operation plus `local:${cwd}` keeps local cancellation independent // from SSH worktrees and from other generation features in the same worktree. const cancelTokensByLane = new Map void>() -function localLaneKey(operation: LocalGenerationOperation, cwd: string): string { +function localLaneKey(operation: TextGenerationOperation, cwd: string): string { return `${operation}:local:${cwd}` } @@ -447,7 +448,7 @@ async function runLocalPlan( cwd: string, env: NodeJS.ProcessEnv | undefined, emptyResultName = 'message', - operation: LocalGenerationOperation = 'commit-message' + operation: TextGenerationOperation = 'commit-message' ): Promise { const { binary, args, stdinPayload, label } = plan return new Promise((resolve) => { @@ -621,12 +622,13 @@ function finalizeFromAgentOutput(args: { async function runRemotePlan( plan: CommitMessagePlan, target: Extract, - emptyResultName = 'message' + emptyResultName = 'message', + operation: TextGenerationOperation = 'commit-message' ): Promise { const { binary, label } = plan let result: RemoteCommitMessageExecResult try { - result = await target.execute(plan, target.cwd, GENERATION_TIMEOUT_MS) + result = await target.execute(plan, target.cwd, GENERATION_TIMEOUT_MS, operation) } catch (error) { console.error('[commit-message] Remote generator request failed:', error) return { @@ -709,8 +711,8 @@ export async function generateCommitMessageFromContext( const internalResult = target.kind === 'remote' - ? await runRemotePlan(planned.plan, target, 'details') - : await runLocalPlan(planned.plan, target.cwd, target.env, 'details') + ? await runRemotePlan(planned.plan, target) + : await runLocalPlan(planned.plan, target.cwd, target.env) return formatCommitMessageGenerationResult(internalResult) } @@ -761,7 +763,7 @@ export async function generatePullRequestFieldsFromContext( const internalResult = target.kind === 'remote' - ? await runRemotePlan(planned.plan, target) + ? await runRemotePlan(planned.plan, target, 'details', 'pull-request-fields') : await runLocalPlan(planned.plan, target.cwd, target.env, 'details', 'pull-request-fields') return formatPullRequestFieldsGenerationResult(internalResult, context) } @@ -788,7 +790,7 @@ export async function generateBranchNameFromContext( const internalResult = target.kind === 'remote' - ? await runRemotePlan(planned.plan, target, 'branch name') + ? await runRemotePlan(planned.plan, target, 'branch name', 'branch-name') : await runLocalPlan(planned.plan, target.cwd, target.env, 'branch name', 'branch-name') if (!internalResult.success) { return internalResult diff --git a/src/main/window/attach-main-window-services.ts b/src/main/window/attach-main-window-services.ts index 4f8f981d852..8bc36c3343b 100644 --- a/src/main/window/attach-main-window-services.ts +++ b/src/main/window/attach-main-window-services.ts @@ -33,6 +33,7 @@ import type { RuntimeMarkdownSaveTabResult } from '../../shared/mobile-markdown-document' import type { RuntimeMobileSessionTabMove } from '../../shared/runtime-types' +import type { NativeFileDropPayload } from '../../shared/native-file-drop' import { requestMobileMarkdownFromRenderer } from './mobile-markdown-request-relay' import type { CodexAccountSelectionTarget } from '../codex-accounts/runtime-selection' import type { ClaudeAccountSelectionTarget } from '../claude-accounts/runtime-selection' @@ -373,25 +374,15 @@ function registerRuntimeWindowLifecycle( function registerFileDropRelay(mainWindow: BrowserWindow): void { ipcMain.removeAllListeners('terminal:file-dropped-from-preload') - ipcMain.on( - 'terminal:file-dropped-from-preload', - ( - _event, - args: - | { paths: string[]; target: 'editor' } - | { paths: string[]; target: 'terminal'; tabId?: string } - | { paths: string[]; target: 'composer' } - | { paths: string[]; target: 'file-explorer'; destinationDir: string } - ) => { - if (mainWindow.isDestroyed()) { - return - } - - // Why: relay exactly one IPC event per drop gesture so the renderer - // receives the full batch of paths without timer-based reconstruction. - mainWindow.webContents.send('terminal:file-drop', args) + ipcMain.on('terminal:file-dropped-from-preload', (_event, args: NativeFileDropPayload) => { + if (mainWindow.isDestroyed()) { + return } - ) + + // Why: relay exactly one IPC event per drop gesture so the renderer + // receives the full batch of paths without timer-based reconstruction. + mainWindow.webContents.send('terminal:file-drop', args) + }) } export function registerUpdaterHandlers(_store: Store): void { diff --git a/src/preload/api-types.ts b/src/preload/api-types.ts index fce1dc1d607..89a1fa75910 100644 --- a/src/preload/api-types.ts +++ b/src/preload/api-types.ts @@ -7,6 +7,7 @@ import type { HostedReviewForBranchArgs, HostedReviewInfo } from '../shared/hosted-review' +import type { NativeFileDropPayload } from '../shared/native-file-drop' import type { AppIdentity } from '../shared/app-identity' import type { BaseRefDefaultResult, @@ -1951,15 +1952,7 @@ export type PreloadApi = { writeClipboardText: (text: string) => Promise writeSelectionClipboardText: (text: string) => Promise writeClipboardImage: (dataUrl: string) => Promise - onFileDrop: ( - callback: ( - data: - | { paths: string[]; target: 'editor' } - | { paths: string[]; target: 'terminal'; tabId?: string } - | { paths: string[]; target: 'composer' } - | { paths: string[]; target: 'file-explorer'; destinationDir: string } - ) => void - ) => () => void + onFileDrop: (callback: (data: NativeFileDropPayload) => void) => () => void getZoomLevel: () => number setZoomLevel: (level: number) => void syncTrafficLights: (zoomFactor: number) => void diff --git a/src/preload/index.ts b/src/preload/index.ts index 26e016048c7..580efb1a12f 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -146,27 +146,20 @@ import { ORCA_UPDATER_QUIT_AND_INSTALL_ABORTED_EVENT, ORCA_UPDATER_QUIT_AND_INSTALL_STARTED_EVENT } from '../shared/updater-renderer-events' +import { + NATIVE_FILE_DROP_TARGET, + ORCA_INTERNAL_FILE_DRAG_TYPE, + hasNativeFileDragTypes, + resolveNativeFileDropPath, + type NativeDropResolution, + type NativeFileDropPayload, + type NativeFileDropPathEntry +} from '../shared/native-file-drop' import { subscribeRuntimeEnvironmentFromPreload } from './runtime-environment-subscriptions' import type { RuntimeEnvironmentSubscriptionHandle } from './runtime-environment-subscriptions' import type { HostedReviewForBranchArgs } from '../shared/hosted-review' import type { CrashReportSubmitArgs, CrashReportSubmitResult } from '../shared/crash-reporting' -type NativeDropResolution = - | { target: 'editor' } - | { target: 'terminal'; tabId?: string } - | { target: 'composer' } - | { target: 'file-explorer'; destinationDir: string } - // Why: returned when the explorer marker was found but no destinationDir - // could be resolved. The caller must suppress the drop entirely instead of - // falling back to 'editor' — fail-closed behavior per design §7.1. - | { target: 'rejected' } - -type NativeFileDropPayload = - | { paths: string[]; target: 'editor' } - | { paths: string[]; target: 'terminal'; tabId?: string } - | { paths: string[]; target: 'composer' } - | { paths: string[]; target: 'file-explorer'; destinationDir: string } - type NativeFileDropCallback = (data: NativeFileDropPayload) => void const nativeFileDropCallbacks: NativeFileDropCallback[] = [] @@ -298,43 +291,17 @@ function disposeCachedNotificationSound(): void { * "inside this folder". */ function resolveNativeFileDrop(event: DragEvent): NativeDropResolution | null { - const path = event.composedPath() - let foundExplorer = false - let destinationDir: string | undefined - - for (const entry of path) { - if (!(entry instanceof HTMLElement)) { - continue - } - - const target = entry.dataset.nativeFileDropTarget - if (target === 'terminal') { - return { target, tabId: entry.dataset.terminalTabId } - } - if (target === 'editor' || target === 'composer') { - return { target } - } - if (target === 'file-explorer') { - foundExplorer = true - } - - // Pick the nearest (innermost) destination directory marker - if (destinationDir === undefined && entry.dataset.nativeFileDropDir) { - destinationDir = entry.dataset.nativeFileDropDir + const pathEntries: NativeFileDropPathEntry[] = [] + for (const entry of event.composedPath()) { + if (entry instanceof HTMLElement) { + pathEntries.push({ + nativeFileDropTarget: entry.dataset.nativeFileDropTarget, + nativeFileDropDir: entry.dataset.nativeFileDropDir, + terminalTabId: entry.dataset.terminalTabId + }) } } - - if (foundExplorer) { - // Why: routing must fail closed for explorer drops. If preload sees the - // explorer target marker but cannot resolve a destinationDir, it rejects - // the gesture and emits no fallback editor drop event. - if (!destinationDir) { - return { target: 'rejected' } - } - return { target: 'file-explorer', destinationDir } - } - - return null + return resolveNativeFileDropPath(pathEntries) } // --------------------------------------------------------------------------- @@ -347,7 +314,7 @@ document.addEventListener( (e) => { // Let in-app drags (e.g. file explorer drag-to-move) through to React handlers // so they can set their own dropEffect. Only override for native OS file drops. - if (e.dataTransfer?.types.includes('text/x-orca-file-path')) { + if (e.dataTransfer && !hasNativeFileDragTypes(e.dataTransfer.types)) { return } e.preventDefault() @@ -362,7 +329,7 @@ document.addEventListener( 'drop', (e) => { // Let in-app drags (e.g. file explorer → terminal) through to React handlers - if (e.dataTransfer?.types.includes('text/x-orca-file-path')) { + if (e.dataTransfer?.types.includes(ORCA_INTERNAL_FILE_DRAG_TYPE)) { return } @@ -398,20 +365,20 @@ document.addEventListener( // The preload layer already has the full FileList. Re-emitting one IPC // message per path and asking the renderer to reconstruct the gesture via // timing would be both fragile and slower under large drops. - if (resolution?.target === 'file-explorer') { + if (resolution?.target === NATIVE_FILE_DROP_TARGET.fileExplorer) { ipcRenderer.send('terminal:file-dropped-from-preload', { paths, - target: 'file-explorer', + target: NATIVE_FILE_DROP_TARGET.fileExplorer, destinationDir: resolution.destinationDir }) } else { // Why: falls back to 'editor' so drops on surfaces without an explicit - // marker (sidebar, editor body, etc.) preserve the prior open-in-editor - // behavior instead of being silently discarded. + // marker preserve the prior open-in-editor behavior instead of being + // silently discarded. ipcRenderer.send('terminal:file-dropped-from-preload', { paths, - target: resolution?.target ?? 'editor', - ...(resolution?.target === 'terminal' && resolution.tabId + target: resolution?.target ?? NATIVE_FILE_DROP_TARGET.editor, + ...(resolution?.target === NATIVE_FILE_DROP_TARGET.terminal && resolution.tabId ? { tabId: resolution.tabId } : {}) }) diff --git a/src/relay/agent-exec-handler.test.ts b/src/relay/agent-exec-handler.test.ts index da402b7d193..edf9c57bcbf 100644 --- a/src/relay/agent-exec-handler.test.ts +++ b/src/relay/agent-exec-handler.test.ts @@ -253,6 +253,72 @@ describe('AgentExecHandler', () => { }) }) + it('cancels only the matching operation lane for a cwd', async () => { + const commitChild = createFakeChild() + const pullRequestChild = createFakeChild() + pullRequestChild.pid = 12346 + spawnMock + .mockReturnValueOnce(commitChild as never) + .mockReturnValueOnce(pullRequestChild as never) + const handlers = createHandlers() + + const commit = handlers.get('agent.execNonInteractive')!( + { + binary: 'agent', + args: [], + cwd: '/repo', + stdin: null, + timeoutMs: 5_000, + operation: 'commit-message' + }, + requestContext() + ) + const pullRequest = handlers.get('agent.execNonInteractive')!( + { + binary: 'agent', + args: [], + cwd: '/repo', + stdin: null, + timeoutMs: 5_000, + operation: 'pull-request-fields' + }, + requestContext() + ) + + await expect( + handlers.get('agent.cancelExec')!( + { cwd: '/repo', operation: 'commit-message' }, + requestContext() + ) + ).resolves.toEqual({ canceled: true }) + + if (process.platform === 'win32') { + expect(execMock).toHaveBeenCalledWith('taskkill /pid 12345 /T /F', expect.any(Function)) + expect(execMock).not.toHaveBeenCalledWith('taskkill /pid 12346 /T /F', expect.any(Function)) + } else { + expect(commitChild.kill).toHaveBeenCalledWith('SIGKILL') + expect(pullRequestChild.kill).not.toHaveBeenCalled() + } + + commitChild.emit('close', null) + pullRequestChild.stdout.emit( + 'data', + Buffer.from('{"base":"main","title":"Update README","body":"Details","draft":false}') + ) + pullRequestChild.emit('close', 0) + + await expect(commit).resolves.toMatchObject({ + exitCode: null, + timedOut: false, + canceled: true + }) + await expect(pullRequest).resolves.toMatchObject({ + exitCode: 0, + timedOut: false, + canceled: false + }) + }) + it('reports when cancellation has no matching in-flight command', async () => { const handlers = createHandlers() diff --git a/src/relay/agent-exec-handler.ts b/src/relay/agent-exec-handler.ts index 38dc75601a4..2a0ca4ae61a 100644 --- a/src/relay/agent-exec-handler.ts +++ b/src/relay/agent-exec-handler.ts @@ -94,12 +94,21 @@ type ExecParams = { stdin: unknown timeoutMs: unknown env: unknown + operation: unknown } type CancelParams = { cwd: unknown + operation: unknown } +function laneKeyFor(cwd: string, operation: unknown): string { + const op = typeof operation === 'string' && operation ? operation : 'default' + return JSON.stringify([op, cwd]) +} + +type InFlightExec = { child: ChildProcess; markCanceled: () => void } + type ExecResult = { stdout: string stderr: string @@ -119,10 +128,13 @@ type ExecResult = { * and a clean exit code instead of an interactive session. */ export class AgentExecHandler { - // Why: a single in-flight exec per cwd is enough — the renderer prevents - // double-clicks. Keying by cwd lets `agent.cancelExec({cwd})` find the - // child without the client having to track relay-internal request ids. - private inFlightByCwd = new Map void }>() + // Why: commit-message and PR-field generation can run together for one cwd; + // operation lanes let cancel target only the user-visible job that stopped. + private inFlightByLane = new Map() + + private laneKey(cwd: string, operation: unknown): string { + return laneKeyFor(cwd, operation) + } constructor(dispatcher: RelayDispatcher) { dispatcher.onRequest('agent.execNonInteractive', (p) => this.exec(p as ExecParams)) @@ -131,7 +143,7 @@ export class AgentExecHandler { private async cancel(params: CancelParams): Promise<{ canceled: boolean }> { const cwd = typeof params.cwd === 'string' ? params.cwd : '' - const entry = this.inFlightByCwd.get(cwd) + const entry = this.inFlightByLane.get(this.laneKey(cwd, params.operation)) if (!entry) { return { canceled: false } } @@ -185,24 +197,26 @@ export class AgentExecHandler { let timedOut = false let canceled = false let settled = false - const laneKey = typeof cwd === 'string' ? cwd : '' + const laneKey = typeof cwd === 'string' ? this.laneKey(cwd, params.operation) : '' + let entry: InFlightExec | null = null const finish = (result: ExecResult): void => { if (settled) { return } settled = true - if (laneKey) { - this.inFlightByCwd.delete(laneKey) + if (laneKey && entry && this.inFlightByLane.get(laneKey) === entry) { + this.inFlightByLane.delete(laneKey) } resolve(result) } if (laneKey) { - this.inFlightByCwd.set(laneKey, { + entry = { child, markCanceled: () => { canceled = true } - }) + } + this.inFlightByLane.set(laneKey, entry) } const timer = setTimeout(() => { diff --git a/src/relay/git-handler.test.ts b/src/relay/git-handler.test.ts index f99c8acc5c4..326e42059a4 100644 --- a/src/relay/git-handler.test.ts +++ b/src/relay/git-handler.test.ts @@ -448,6 +448,37 @@ describe('GitHandler', () => { await expect(fs.access(path.join(tmpDir, 'new.txt'))).rejects.toThrow() }) + it('treats untracked discard paths with Git glob characters as literal paths', async () => { + gitInit(tmpDir) + writeFileSync(path.join(tmpDir, '.gitignore'), 'ignored.log\n') + gitCommit(tmpDir, 'initial') + writeFileSync(path.join(tmpDir, '*.log'), 'selected') + writeFileSync(path.join(tmpDir, 'keep.log'), 'unrelated') + writeFileSync(path.join(tmpDir, 'ignored.log'), 'ignored') + + await dispatcher.callRequest('git.discard', { worktreePath: tmpDir, filePath: '*.log' }) + + await expect(fs.access(path.join(tmpDir, '*.log'))).rejects.toThrow() + await expect(fs.access(path.join(tmpDir, 'keep.log'))).resolves.toBeUndefined() + await expect(fs.access(path.join(tmpDir, 'ignored.log'))).resolves.toBeUndefined() + }) + + it('treats tracked discard paths with Git glob characters as literal paths', async () => { + gitInit(tmpDir) + writeFileSync(path.join(tmpDir, '*.log'), 'selected') + writeFileSync(path.join(tmpDir, 'keep.log'), 'keep') + gitCommit(tmpDir, 'track log fixtures') + writeFileSync(path.join(tmpDir, '*.log'), 'selected modified') + writeFileSync(path.join(tmpDir, 'keep.log'), 'keep modified') + + await dispatcher.callRequest('git.discard', { worktreePath: tmpDir, filePath: '*.log' }) + + await expect(fs.readFile(path.join(tmpDir, '*.log'), 'utf-8')).resolves.toBe('selected') + await expect(fs.readFile(path.join(tmpDir, 'keep.log'), 'utf-8')).resolves.toBe( + 'keep modified' + ) + }) + it('bulk discards tracked and untracked files', async () => { gitInit(tmpDir) writeFileSync(path.join(tmpDir, 'a.txt'), 'a') diff --git a/src/relay/git-handler.ts b/src/relay/git-handler.ts index 9956a4e7668..b532ab070ff 100644 --- a/src/relay/git-handler.ts +++ b/src/relay/git-handler.ts @@ -236,14 +236,20 @@ export class GitHandler { let tracked = false try { - await this.git(['ls-files', '--error-unmatch', '--', filePath], worktreePath) + await this.git( + ['ls-files', '--error-unmatch', '--', this.literalPathspec(filePath)], + worktreePath + ) tracked = true } catch { // untracked } if (tracked) { - await this.git(['restore', '--worktree', '--source=HEAD', '--', filePath], worktreePath) + await this.git( + ['restore', '--worktree', '--source=HEAD', '--', this.literalPathspec(filePath)], + worktreePath + ) return } @@ -266,7 +272,10 @@ export class GitHandler { const trackedPathSpecs: string[] = [] for (let i = 0; i < filePaths.length; i += BULK_CHUNK_SIZE) { const chunk = filePaths.slice(i, i + BULK_CHUNK_SIZE) - const { stdout } = await this.git(['ls-files', '-z', '--', ...chunk], worktreePath) + const { stdout } = await this.git( + ['ls-files', '-z', '--', ...chunk.map((p) => this.literalPathspec(p))], + worktreePath + ) trackedPathSpecs.push(...stdout.split('\0').filter(Boolean)) } @@ -283,18 +292,35 @@ export class GitHandler { async () => { for (let i = 0; i < trackedPaths.length; i += BULK_CHUNK_SIZE) { const chunk = trackedPaths.slice(i, i + BULK_CHUNK_SIZE) - await this.git(['restore', '--worktree', '--source=HEAD', '--', ...chunk], worktreePath) + await this.git( + [ + 'restore', + '--worktree', + '--source=HEAD', + '--', + ...chunk.map((p) => this.literalPathspec(p)) + ], + worktreePath + ) } } ) } + private literalPathspec(filePath: string): string { + // Why: source-control selections are concrete paths, not user-authored Git globs. + return `:(literal)${filePath}` + } + private async cleanUntrackedPaths(worktreePath: string, filePaths: readonly string[]) { for (let i = 0; i < filePaths.length; i += BULK_CHUNK_SIZE) { const chunk = filePaths.slice(i, i + BULK_CHUNK_SIZE) if (chunk.length > 0) { // Why: Git pathspec cleanup avoids raw recursive deletion through symlinked parents. - await this.git(['clean', '-ffdx', '--', ...chunk], worktreePath) + await this.git( + ['clean', '-ffdx', '--', ...chunk.map((p) => this.literalPathspec(p))], + worktreePath + ) } } } diff --git a/src/renderer/src/components/FirstLaunchBanner.tsx b/src/renderer/src/components/FirstLaunchBanner.tsx index 8a03379786e..ed93d57168e 100644 --- a/src/renderer/src/components/FirstLaunchBanner.tsx +++ b/src/renderer/src/components/FirstLaunchBanner.tsx @@ -33,7 +33,7 @@ // off), the notice never returns, because the cohort condition // (`optedIn === null`) clears in all three resolving paths. -import { useState } from 'react' +import { useEffect, useRef, useState } from 'react' import { X } from 'lucide-react' import { Button } from './ui/button' @@ -57,6 +57,14 @@ export function FirstLaunchBanner({ // wasted IPC round-trip, but the guard also blocks a Turn-off click // arriving mid-flight after an acknowledge (or vice versa). const [inFlight, setInFlight] = useState(false) + const mountedRef = useRef(true) + + useEffect(() => { + mountedRef.current = true + return () => { + mountedRef.current = false + } + }, []) const handleAcknowledge = async (): Promise => { if (inFlight) { @@ -74,13 +82,17 @@ export function FirstLaunchBanner({ try { await acknowledgeBanner() await fetchSettings() - onResolve() + if (mountedRef.current) { + onResolve() + } } finally { // Why: if `fetchSettings` rejects (IPC error during shutdown, // settings file lock, etc.), `onResolve` never runs and the banner // stays mounted. Without resetting `inFlight`, every button stays // permanently disabled for the rest of the session. - setInFlight(false) + if (mountedRef.current) { + setInFlight(false) + } } } @@ -97,9 +109,13 @@ export function FirstLaunchBanner({ try { await telemetrySetOptIn(false) await fetchSettings() - onResolve() + if (mountedRef.current) { + onResolve() + } } finally { - setInFlight(false) + if (mountedRef.current) { + setInFlight(false) + } } } diff --git a/src/renderer/src/components/GitHubItemDialog.tsx b/src/renderer/src/components/GitHubItemDialog.tsx index 8144b3bbc45..fdeeec7877e 100644 --- a/src/renderer/src/components/GitHubItemDialog.tsx +++ b/src/renderer/src/components/GitHubItemDialog.tsx @@ -3112,6 +3112,14 @@ function CommentReplyForm({ const [body, setBody] = useState('') const [submitting, setSubmitting] = useState(false) const textareaRef = useRef(null) + const mountedRef = useRef(true) + + useEffect(() => { + mountedRef.current = true + return () => { + mountedRef.current = false + } + }, []) useEffect(() => { textareaRef.current?.focus() @@ -3125,11 +3133,16 @@ function CommentReplyForm({ setSubmitting(true) try { const ok = await onSubmit(trimmed) + if (!mountedRef.current) { + return + } if (ok) { setBody('') } } finally { - setSubmitting(false) + if (mountedRef.current) { + setSubmitting(false) + } } }, [body, onSubmit, submitting]) @@ -5008,6 +5021,14 @@ function GHCommentComposer({ const [body, setBody] = useState('') const [submitting, setSubmitting] = useState(false) const textareaRef = useRef(null) + const mountedRef = useRef(true) + + useEffect(() => { + mountedRef.current = true + return () => { + mountedRef.current = false + } + }, []) const autoGrow = useCallback(() => { const el = textareaRef.current @@ -5032,6 +5053,9 @@ function GHCommentComposer({ body: trimmed, type: itemType }) + if (!mountedRef.current) { + return + } if (result.ok) { setBody('') requestAnimationFrame(autoGrow) @@ -5042,9 +5066,13 @@ function GHCommentComposer({ toast.error(result.error ?? 'Failed to add comment') } } catch (err) { - toast.error(err instanceof Error ? err.message : 'Failed to add comment') + if (mountedRef.current) { + toast.error(err instanceof Error ? err.message : 'Failed to add comment') + } } finally { - setSubmitting(false) + if (mountedRef.current) { + setSubmitting(false) + } } }, [autoGrow, body, repoPath, repoId, issueNumber, itemType, onCommentAdded]) @@ -5440,6 +5468,12 @@ export default function GitHubItemDialog({ const files = details?.files ?? [] const checks = details?.checks ?? [] const [pendingViewedPaths, setPendingViewedPaths] = useState>(() => new Set()) + // Why: clipboard IPC can resolve after the dialog unmounts; skip copied-state + // feedback instead of starting its reset timer on a stale surface. + const linkCopyMountedRef = useRef(false) + const setLinkCopyButtonRef = useCallback((node: HTMLButtonElement | null) => { + linkCopyMountedRef.current = node !== null + }, []) useEffect(() => { setLinkCopied(false) @@ -5461,6 +5495,9 @@ export default function GitHubItemDialog({ // Why: Electron's clipboard IPC is reliable even when browser clipboard // APIs lose focus/activation inside nested overlay surfaces. await window.api.ui.writeClipboardText(workItem.url) + if (!linkCopyMountedRef.current) { + return + } setLinkCopied(true) toast.success('GitHub link copied') } catch { @@ -5580,6 +5617,7 @@ export default function GitHubItemDialog({