From eee4a69e098f7a7cae39f3bd1b2d80a84066d651 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 31 Oct 2026 22:32:30 -0700 Subject: [PATCH] =?UTF-8?q?fix(desktop):=20address=20release=20review=20?= =?UTF-8?q?=E2=80=94=20count=20app=20downloads=20and=20unchecked=20termina?= =?UTF-8?q?ls=20before=20an=20update=20restart,=20send=20the=20page=20refe?= =?UTF-8?q?rrer=20on=20Save=20Image,=20and=20tighten=20sign-in=20and=20peo?= =?UTF-8?q?ple=20edge=20cases?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- apps/desktop/e2e/browser-page-parity.spec.ts | 44 ++++++---- apps/desktop/e2e/fixtures/updater.ts | 1 + .../src/main/browser-agent/context-menu.ts | 36 ++------- .../src/main/browser-agent/referrer.test.ts | 57 +++++++++++++ .../src/main/browser-agent/referrer.ts | 81 +++++++++++++++++++ apps/desktop/src/main/downloads.test.ts | 49 ++++++++++- apps/desktop/src/main/downloads.ts | 15 +++- apps/desktop/src/main/in-flight-work.test.ts | 49 ++++++++++- apps/desktop/src/main/in-flight-work.ts | 39 +++++++-- apps/desktop/src/main/index.ts | 10 ++- apps/desktop/src/main/terminal/index.ts | 60 ++++++++++---- .../src/main/terminal/process-group.test.ts | 15 ++++ .../src/main/terminal/process-group.ts | 43 +++++++--- apps/desktop/src/main/terminal/registry.ts | 14 ++-- .../desktop/src/main/terminal/service.test.ts | 42 +++++++--- apps/desktop/src/main/updater.test.ts | 26 +++++- apps/desktop/src/main/updater.ts | 1 + .../people/[memberId]/person.tsx | 2 +- .../browser-session/browser-notice.tsx | 2 +- .../browser-session/sign-in-hosts.test.ts | 8 ++ .../browser-session/sign-in-hosts.ts | 6 +- .../browser-session/use-continue-sign-in.ts | 9 ++- 22 files changed, 500 insertions(+), 109 deletions(-) create mode 100644 apps/desktop/src/main/browser-agent/referrer.test.ts create mode 100644 apps/desktop/src/main/browser-agent/referrer.ts create mode 100644 apps/desktop/src/main/terminal/process-group.test.ts diff --git a/apps/desktop/e2e/browser-page-parity.spec.ts b/apps/desktop/e2e/browser-page-parity.spec.ts index 63063e18abe..ec1c1256536 100644 --- a/apps/desktop/e2e/browser-page-parity.spec.ts +++ b/apps/desktop/e2e/browser-page-parity.spec.ts @@ -101,6 +101,8 @@ test.describe('browser page parity with Chrome', () => { const held = new Set() /** The Referer each load of the link target arrived with. */ const referrers: (string | undefined)[] = [] + /** The Referer each request for the image arrived with. */ + const imageReferrers: (string | undefined)[] = [] const calls = new Map() const server = createServer(async (request, response) => { const path = new URL(request.url ?? '/', 'http://localhost').pathname @@ -113,7 +115,14 @@ test.describe('browser page parity with Chrome', () => { return } if (path === '/pixel.png') { - response.writeHead(200, { 'Content-Type': 'image/png' }) + imageReferrers.push(request.headers.referer) + // Hotlink-protected, like many image hosts: a request without the page as referrer fails. + if (!request.headers.referer) { + response.writeHead(403) + response.end() + return + } + response.writeHead(200, { 'Content-Type': 'image/png', 'Cache-Control': 'no-store' }) response.end(PNG) return } @@ -358,20 +367,25 @@ test.describe('browser page parity with Chrome', () => { await expect.poll(pageUrl).toBe(`${site}/next`) }) - checkedTest("the page menu's Save Image lands through the browser's downloads", async () => { - for (const file of readdirSync(downloads)) rmSync(join(downloads, file), { force: true }) - await navigate('/image') - await expect - .poll(() => inPage({ kind: 'script', script: 'document.getElementById("pic").complete' })) - .toBe(true) - const labels = await openPageMenu('#pic') - expect(labels).toEqual( - expect.arrayContaining(['Open Image in New Tab', 'Copy Image', 'Copy Image Address']) - ) - await choosePageMenuItem('Save Image') - await expect.poll(() => readdirSync(downloads)).toEqual(['pixel.png']) - expect(readFileSync(join(downloads, 'pixel.png')).equals(PNG)).toBe(true) - }) + checkedTest( + "the page menu's Save Image lands through the browser's downloads, with the page as referrer", + async () => { + for (const file of readdirSync(downloads)) rmSync(join(downloads, file), { force: true }) + await navigate('/image') + await expect + .poll(() => inPage({ kind: 'script', script: 'document.getElementById("pic").complete' })) + .toBe(true) + const labels = await openPageMenu('#pic') + expect(labels).toEqual( + expect.arrayContaining(['Open Image in New Tab', 'Copy Image', 'Copy Image Address']) + ) + imageReferrers.length = 0 + await choosePageMenuItem('Save Image') + await expect.poll(() => readdirSync(downloads)).toEqual(['pixel.png']) + expect(readFileSync(join(downloads, 'pixel.png')).equals(PNG)).toBe(true) + expect(imageReferrers).toEqual([`${site}/image`]) + } + ) checkedTest("the page menu's Open Link in New Tab sends the page as referrer", async () => { referrers.length = 0 diff --git a/apps/desktop/e2e/fixtures/updater.ts b/apps/desktop/e2e/fixtures/updater.ts index 6bd92777510..a964dfcc979 100644 --- a/apps/desktop/e2e/fixtures/updater.ts +++ b/apps/desktop/e2e/fixtures/updater.ts @@ -102,6 +102,7 @@ void app.whenReady().then(async () => { describeInFlightWork: async () => describeInFlightWork({ terminalCommands, + uncheckedTerminalCount: 0, computerUseApp: null, browserToolRunning, agentWorkRunning: false, diff --git a/apps/desktop/src/main/browser-agent/context-menu.ts b/apps/desktop/src/main/browser-agent/context-menu.ts index d4da63e1b40..4043da75842 100644 --- a/apps/desktop/src/main/browser-agent/context-menu.ts +++ b/apps/desktop/src/main/browser-agent/context-menu.ts @@ -22,6 +22,7 @@ import { consumeAgentContextMenu, isDispatchingAgentInput, } from '@/main/browser-agent/cdp' +import { menuReferrer, referrerForRequest } from '@/main/browser-agent/referrer' import { spellingItems } from '@/main/context-menu' import { parseHttpUrl } from '@/main/navigation' @@ -139,33 +140,6 @@ export interface AgentContextMenuHost { const isWebUrl = (url: string) => parseHttpUrl(url) !== null -const REFERRER_POLICIES: ReadonlySet = new Set([ - 'default', - 'unsafe-url', - 'no-referrer-when-downgrade', - 'no-referrer', - 'origin', - 'strict-origin-when-cross-origin', - 'same-origin', - 'strict-origin', -]) - -function isReferrerPolicy(value: unknown): value is Referrer['policy'] { - return REFERRER_POLICIES.has(value) -} - -/** Electron types `referrerPolicy` as a Referrer, but it arrives as the bare policy string. */ -function menuReferrer( - params: Pick -): Referrer { - const policy: unknown = params.referrerPolicy - const url = params.frameURL || params.pageURL - return { - url: isWebUrl(url) ? url : '', - policy: isReferrerPolicy(policy) ? policy : 'default', - } -} - /** Ends the template with one separator, never two in a row or one at the top. */ function endGroup(template: MenuItemConstructorOptions[]): void { if (template.length > 0 && template[template.length - 1].type !== 'separator') { @@ -314,8 +288,12 @@ export function attachAgentContextMenu(contents: WebContents, host: AgentContext openTab: (url) => host.openTab(url, menuReferrer(params)), copyLink: (url) => clipboard.writeText(url), copyImage: () => contents.copyImageAt(params.x, params.y), - // A download, so the browser's download limits and request guards apply. - saveImage: (url) => contents.downloadURL(url), + // A download, so the browser's download limits and request guards apply. Electron maps a + // Referer header into the download's referrer, so referrer-protected images still save. + saveImage: (url) => { + const referrer = referrerForRequest(menuReferrer(params), url) + contents.downloadURL(url, referrer ? { headers: { Referer: referrer } } : undefined) + }, setZoomFactor: (factor) => contents.setZoomFactor(factor), } ) diff --git a/apps/desktop/src/main/browser-agent/referrer.test.ts b/apps/desktop/src/main/browser-agent/referrer.test.ts new file mode 100644 index 00000000000..3f41335f3f5 --- /dev/null +++ b/apps/desktop/src/main/browser-agent/referrer.test.ts @@ -0,0 +1,57 @@ +import { describe, expect, it, vi } from 'vitest' + +vi.mock('electron', () => import('@/test/electron-mock')) + +import { referrerForRequest } from '@/main/browser-agent/referrer' + +describe('referrerForRequest', () => { + const PAGE = 'https://user:secret@news.example.com/story?id=7#comments' + const FULL = 'https://news.example.com/story?id=7' + const ORIGIN = 'https://news.example.com/' + + it('sends the full URL same-origin and only the origin cross-origin by default', () => { + const referrer = { url: PAGE, policy: 'default' } as const + expect(referrerForRequest(referrer, 'https://news.example.com/img.png')).toBe(FULL) + expect(referrerForRequest(referrer, 'https://cdn.example.net/img.png')).toBe(ORIGIN) + }) + + it('never sends credentials or the fragment, even under unsafe-url', () => { + expect( + referrerForRequest({ url: PAGE, policy: 'unsafe-url' }, 'http://cdn.example.net/img.png') + ).toBe(FULL) + }) + + it('sends nothing from an https page to an http image unless the policy allows downgrades', () => { + const image = 'http://cdn.example.net/img.png' + for (const policy of [ + 'default', + 'strict-origin-when-cross-origin', + 'strict-origin', + 'no-referrer-when-downgrade', + ] as const) { + expect(referrerForRequest({ url: PAGE, policy }, image)).toBeUndefined() + } + expect(referrerForRequest({ url: PAGE, policy: 'origin' }, image)).toBe(ORIGIN) + expect(referrerForRequest({ url: PAGE, policy: 'no-referrer-when-downgrade' }, FULL)).toBe(FULL) + }) + + it('honors no-referrer and same-origin', () => { + const crossOrigin = 'https://cdn.example.net/img.png' + expect(referrerForRequest({ url: PAGE, policy: 'no-referrer' }, FULL)).toBeUndefined() + expect(referrerForRequest({ url: PAGE, policy: 'same-origin' }, crossOrigin)).toBeUndefined() + expect(referrerForRequest({ url: PAGE, policy: 'same-origin' }, FULL)).toBe(FULL) + }) + + it('cuts a referrer longer than 4096 characters to its origin', () => { + const longPage = `https://news.example.com/story?q=${'x'.repeat(4096)}` + expect( + referrerForRequest({ url: longPage, policy: 'unsafe-url' }, 'https://cdn.example.net/img.png') + ).toBe(ORIGIN) + }) + + it('sends nothing for a page that is not on the web', () => { + expect( + referrerForRequest({ url: '', policy: 'unsafe-url' }, 'https://cdn.example.net/img.png') + ).toBeUndefined() + }) +}) diff --git a/apps/desktop/src/main/browser-agent/referrer.ts b/apps/desktop/src/main/browser-agent/referrer.ts new file mode 100644 index 00000000000..f897144aad0 --- /dev/null +++ b/apps/desktop/src/main/browser-agent/referrer.ts @@ -0,0 +1,81 @@ +/** + * The referrer a page-menu action carries: the clicked frame's URL and Referrer-Policy, and the + * `Referer` that policy allows for a given request. + */ +import type { ContextMenuParams, Referrer } from 'electron' +import { parseHttpUrl } from '@/main/navigation' + +const isWebUrl = (url: string) => parseHttpUrl(url) !== null + +const REFERRER_POLICIES: ReadonlySet = new Set([ + 'default', + 'unsafe-url', + 'no-referrer-when-downgrade', + 'no-referrer', + 'origin', + 'strict-origin-when-cross-origin', + 'same-origin', + 'strict-origin', +]) + +function isReferrerPolicy(value: unknown): value is Referrer['policy'] { + return REFERRER_POLICIES.has(value) +} + +/** Electron types `referrerPolicy` as a Referrer, but it arrives as the bare policy string. */ +export function menuReferrer( + params: Pick +): Referrer { + const policy: unknown = params.referrerPolicy + const url = params.frameURL || params.pageURL + return { + url: isWebUrl(url) ? url : '', + policy: isReferrerPolicy(policy) ? policy : 'default', + } +} + +/** Longest referrer sent in full; a longer one is cut to its origin, as browsers do. */ +const MAX_REFERRER_LENGTH = 4096 +const LOOPBACK_HOSTS: ReadonlySet = new Set(['localhost', '127.0.0.1', '[::1]']) + +function isPotentiallyTrustworthy(url: URL): boolean { + if (url.protocol === 'https:') return true + return LOOPBACK_HOSTS.has(url.hostname) || url.hostname.endsWith('.localhost') +} + +/** + * The `Referer` a request from `referrer`'s document to `targetUrl` carries under its + * Referrer-Policy (W3C Referrer Policy, "determine request's referrer"), or undefined when it + * sends none. Chromium's `default` is `strict-origin-when-cross-origin`; Electron's `Referrer` has + * no `origin-when-cross-origin`, which {@link menuReferrer} already reads as `default`. + */ +export function referrerForRequest(referrer: Referrer, targetUrl: string): string | undefined { + const source = parseHttpUrl(referrer.url) + const target = parseHttpUrl(targetUrl) + if (!source || !target) return undefined + source.username = '' + source.password = '' + source.hash = '' + const origin = `${source.origin}/` + const full = source.href.length > MAX_REFERRER_LENGTH ? origin : source.href + const sameOrigin = source.origin === target.origin + const downgrade = isPotentiallyTrustworthy(source) && !isPotentiallyTrustworthy(target) + + switch (referrer.policy) { + case 'no-referrer': + return undefined + case 'origin': + return origin + case 'unsafe-url': + return full + case 'same-origin': + return sameOrigin ? full : undefined + case 'no-referrer-when-downgrade': + return downgrade ? undefined : full + case 'strict-origin': + return downgrade ? undefined : origin + default: + if (downgrade) return undefined + return sameOrigin ? full : origin + } +} diff --git a/apps/desktop/src/main/downloads.test.ts b/apps/desktop/src/main/downloads.test.ts index 951bc151f6c..22302421def 100644 --- a/apps/desktop/src/main/downloads.test.ts +++ b/apps/desktop/src/main/downloads.test.ts @@ -1,3 +1,4 @@ +import { EventEmitter } from 'node:events' import { mkdtempSync, symlinkSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' @@ -5,7 +6,35 @@ import { describe, expect, it, vi } from 'vitest' vi.mock('electron', () => import('@/test/electron-mock')) -import { sanitizeFilename, uniqueDownloadPath } from '@/main/downloads' +import type { DownloadItem, Session } from 'electron' +import { + attachDownloadHandling, + countProgressingAppDownloads, + sanitizeFilename, + uniqueDownloadPath, +} from '@/main/downloads' +import type { EventRecorder } from '@/main/observability' + +type DownloadState = ReturnType + +function startAppDownload(session: EventEmitter) { + const item = new EventEmitter() + let state: DownloadState = 'progressing' + Object.assign(item, { + getFilename: () => 'export.csv', + getMimeType: () => 'text/csv', + getSavePath: () => '/Downloads/export.csv', + getState: () => state, + setSaveDialogOptions: vi.fn(), + }) + session.emit('will-download', {}, item, undefined) + return { + finish(finalState: DownloadState) { + state = finalState + item.emit('done', {}, finalState) + }, + } +} describe('sanitizeFilename', () => { it('neutralizes path separators and leading dots', () => { @@ -49,3 +78,21 @@ describe('uniqueDownloadPath', () => { ) }) }) + +describe('countProgressingAppDownloads', () => { + it('counts an app-window download until it completes or is cancelled', () => { + const session = new EventEmitter() + const events = { record: vi.fn() } as unknown as EventRecorder + attachDownloadHandling(session as unknown as Session, events) + const before = countProgressingAppDownloads() + + const completed = startAppDownload(session) + const cancelled = startAppDownload(session) + expect(countProgressingAppDownloads()).toBe(before + 2) + + completed.finish('completed') + expect(countProgressingAppDownloads()).toBe(before + 1) + cancelled.finish('cancelled') + expect(countProgressingAppDownloads()).toBe(before) + }) +}) diff --git a/apps/desktop/src/main/downloads.ts b/apps/desktop/src/main/downloads.ts index a26582f379b..718b3f7429c 100644 --- a/apps/desktop/src/main/downloads.ts +++ b/apps/desktop/src/main/downloads.ts @@ -2,7 +2,7 @@ import { lstat } from 'node:fs/promises' import { basename, extname, join } from 'node:path' import { createLogger } from '@sim/logger' import { generateShortId } from '@sim/utils/id' -import type { Session } from 'electron' +import type { DownloadItem, Session } from 'electron' import { app } from 'electron' import { isAgentWebContents } from '@/main/browser-agent/registry' import type { EventRecorder } from '@/main/observability' @@ -131,6 +131,17 @@ export async function uniqueDownloadPath( return null } +const activeAppDownloads = new Set() + +/** App-window downloads (Files exports and saves) still receiving bytes, which a restart would cut off. */ +export function countProgressingAppDownloads(): number { + let count = 0 + for (const item of activeAppDownloads) { + if (item.getState() === 'progressing') count += 1 + } + return count +} + /** * Wires will-download so exports, blob URLs, and presigned-URL downloads all * get a native save dialog with a sensible default name, and completed @@ -139,11 +150,13 @@ export async function uniqueDownloadPath( export function attachDownloadHandling(session: Session, events: EventRecorder): void { session.on('will-download', (_event, item, contents) => { if (contents && isAgentWebContents(contents)) return + activeAppDownloads.add(item) const filename = suggestedFilename(item.getFilename(), item.getMimeType()) item.setSaveDialogOptions({ defaultPath: join(app.getPath('downloads'), filename), }) item.once('done', (_doneEvent, state) => { + activeAppDownloads.delete(item) if (state === 'completed') { logger.info('Download completed', { filename }) if (process.platform === 'darwin') { diff --git a/apps/desktop/src/main/in-flight-work.test.ts b/apps/desktop/src/main/in-flight-work.test.ts index 54862b54fe8..4a218ab7268 100644 --- a/apps/desktop/src/main/in-flight-work.test.ts +++ b/apps/desktop/src/main/in-flight-work.test.ts @@ -1,6 +1,10 @@ import type { WebContents } from 'electron' import { describe, expect, it, vi } from 'vitest' -import { countWindowsWithUnsavedChanges } from '@/main/in-flight-work' +import { + countWindowsWithUnsavedChanges, + describeInFlightWork, + type InFlightWork, +} from '@/main/in-flight-work' vi.mock('electron', () => import('@/test/electron-mock')) @@ -24,3 +28,46 @@ describe('countWindowsWithUnsavedChanges', () => { expect(counts).toEqual({ unsavedWindowCount: 1, unresponsiveWindowCount: 1 }) }) }) + +describe('describeInFlightWork', () => { + const IDLE: InFlightWork = { + terminalCommands: [], + uncheckedTerminalCount: 0, + computerUseApp: null, + browserToolRunning: false, + agentWorkRunning: false, + downloadCount: 0, + unsavedWindowCount: 0, + unresponsiveWindowCount: 0, + } + + it('warns about a terminal it could not check rather than restarting silently', () => { + expect(describeInFlightWork({ ...IDLE, uncheckedTerminalCount: 2 })).toEqual([ + "2 terminals couldn't be checked and may still be running commands.", + ]) + expect(describeInFlightWork({ ...IDLE, uncheckedTerminalCount: 1 })).toEqual([ + "A terminal couldn't be checked and may still be running a command.", + ]) + }) + + it('starts the sentence for an unannounced command with a capital', () => { + expect(describeInFlightWork({ ...IDLE, terminalCommands: [''] })).toEqual([ + 'A command is still running in a terminal.', + ]) + expect(describeInFlightWork({ ...IDLE, terminalCommands: ['npm run dev'] })).toEqual([ + 'npm is still running in a terminal.', + ]) + }) + + it('counts unannounced commands rather than naming "a command" twice', () => { + expect(describeInFlightWork({ ...IDLE, terminalCommands: ['', ''] })).toEqual([ + '2 terminal commands are still running.', + ]) + expect(describeInFlightWork({ ...IDLE, terminalCommands: ['', 'npm run dev'] })).toEqual([ + 'npm and a command are still running in terminals.', + ]) + expect( + describeInFlightWork({ ...IDLE, terminalCommands: ['', 'npm run dev', 'claude'] }) + ).toEqual(['3 terminal commands are still running.']) + }) +}) diff --git a/apps/desktop/src/main/in-flight-work.ts b/apps/desktop/src/main/in-flight-work.ts index 82f6219f36c..3203b675a85 100644 --- a/apps/desktop/src/main/in-flight-work.ts +++ b/apps/desktop/src/main/in-flight-work.ts @@ -62,31 +62,54 @@ export async function countWindowsWithUnsavedChanges( export interface InFlightWork { /** The command line holding each busy terminal's foreground (`''` when unannounced). */ terminalCommands: readonly string[] + /** Terminals whose foreground could not be read, which may be running a command. */ + uncheckedTerminalCount: number /** The app a computer-use action is driving, `''` when it has no name, or null when idle. */ computerUseApp: string | null /** Whether an agent has a browser action queued or running. */ browserToolRunning: boolean /** Whether a chat has desktop work claimed on this machine. */ agentWorkRunning: boolean - /** Agent-browser downloads still in progress. */ + /** Agent-browser and app-window downloads still in progress. */ downloadCount: number unsavedWindowCount: number unresponsiveWindowCount: number } +/** + * One sentence for the busy terminals. Commands are named while at most two are running and + * at most one is unannounced; past that, two "a command"s would say nothing, so they are counted. + */ +function describeTerminalCommands(commands: readonly string[]): string | null { + if (commands.length === 0) return null + const named = commands.filter((command) => command.trim()).map(describeRunningCommand) + const unnamed = commands.length - named.length + if (unnamed > 1 || commands.length > MAX_NAMED_TERMINAL_COMMANDS) { + return `${commands.length} terminal commands are still running.` + } + const labels = unnamed === 1 ? [...named, describeRunningCommand('')] : named + const sentence = + labels.length > 1 + ? `${labels.join(' and ')} are still running in terminals.` + : `${labels[0]} is still running in a terminal.` + // Program names keep their own case; only the generic "a command" label is capitalized. + return named.length === 0 ? sentence.charAt(0).toUpperCase() + sentence.slice(1) : sentence +} + /** * Names the work a restart would interrupt, one sentence per kind, so a * restart prompt says exactly what is at stake. Empty when nothing is. */ export function describeInFlightWork(work: InFlightWork): string[] { const sentences: string[] = [] - const commands = work.terminalCommands.map(describeRunningCommand) - if (commands.length > MAX_NAMED_TERMINAL_COMMANDS) { - sentences.push(`${commands.length} terminal commands are still running.`) - } else if (commands.length > 1) { - sentences.push(`${commands.join(' and ')} are still running in terminals.`) - } else if (commands.length === 1) { - sentences.push(`${commands[0]} is still running in a terminal.`) + const terminalSentence = describeTerminalCommands(work.terminalCommands) + if (terminalSentence) sentences.push(terminalSentence) + if (work.uncheckedTerminalCount === 1) { + sentences.push("A terminal couldn't be checked and may still be running a command.") + } else if (work.uncheckedTerminalCount > 1) { + sentences.push( + `${work.uncheckedTerminalCount} terminals couldn't be checked and may still be running commands.` + ) } if (work.computerUseApp !== null) { diff --git a/apps/desktop/src/main/index.ts b/apps/desktop/src/main/index.ts index e61e895fa47..a0efc04faf5 100644 --- a/apps/desktop/src/main/index.ts +++ b/apps/desktop/src/main/index.ts @@ -75,7 +75,7 @@ import { createDesktopToolRunner } from '@/main/desktop-executor/runner' import { createDesktopExecutorService } from '@/main/desktop-executor/service' import { createSleepBlocker } from '@/main/desktop-executor/sleep-blocker' import { createDesktopSettingsService } from '@/main/desktop-settings' -import { attachDownloadHandling } from '@/main/downloads' +import { attachDownloadHandling, countProgressingAppDownloads } from '@/main/downloads' import { createAuthFlow, createConnectFlow, createHandoffManager } from '@/main/handoff' import { installDocumentationHelpSearch, @@ -610,6 +610,7 @@ function main(): void { const routes: unknown = config.get('updateRelaunchRoutes') if (routes === undefined) return config.set('updateRelaunchRoutes', undefined) + config.flush() if (!Array.isArray(routes)) return for (const route of routes.slice(0, MAX_RELAUNCH_WINDOWS)) { if (typeof route === 'string' && decideStartRoute(route) === route) { @@ -1174,14 +1175,15 @@ function main(): void { const windows = await countWindowsWithUnsavedChanges( getWindows().map((win) => win.webContents) ) - const terminalCommands = await terminal.runningCommands() + const terminalWork = await terminal.runningWork() const computerUseActivity = computerUse.getActivity() return describeInFlightWork({ - terminalCommands, + terminalCommands: terminalWork.commands, + uncheckedTerminalCount: terminalWork.uncheckedCount, computerUseApp: computerUseActivity ? (computerUseActivity.appName ?? '') : null, browserToolRunning: isBrowserToolRunning(), agentWorkRunning, - downloadCount: countProgressingBrowserDownloads(), + downloadCount: countProgressingBrowserDownloads() + countProgressingAppDownloads(), ...windows, }) }, diff --git a/apps/desktop/src/main/terminal/index.ts b/apps/desktop/src/main/terminal/index.ts index a11d90157b1..9aafbf7ff2c 100644 --- a/apps/desktop/src/main/terminal/index.ts +++ b/apps/desktop/src/main/terminal/index.ts @@ -37,7 +37,11 @@ import { isResourceTabSelectionShortcut, resourceTabTargetIndex, } from '@/main/resource-shortcuts' -import { readForegroundProcessGroup, signalProcessGroup } from '@/main/terminal/process-group' +import { + type ForegroundProcessGroup, + readForegroundProcessGroup, + signalProcessGroup, +} from '@/main/terminal/process-group' import type { RunLedger } from '@/main/terminal/run-ledger' import { elide, type ShellStartupBounds, TerminalSession } from '@/main/terminal/session' import { @@ -167,7 +171,7 @@ export interface TerminalServiceOptions { } interface TerminalProcessGroups { - foreground(shellPid: number): Promise + foreground(shellPid: number): Promise signal(pgid: number, signal: NodeJS.Signals): void } @@ -176,6 +180,22 @@ const OS_PROCESS_GROUPS: TerminalProcessGroups = { signal: signalProcessGroup, } +/** The terminal work a restart would cut off. */ +export interface TerminalWork { + /** The command line holding each busy terminal's foreground, `''` when unannounced. */ + commands: string[] + /** Terminals whose foreground could not be read, which may be running anything. */ + uncheckedCount: number +} + +async function foregroundGroupId( + groups: TerminalProcessGroups, + shellPid: number +): Promise { + const foreground = await groups.foreground(shellPid) + return foreground.kind === 'group' ? foreground.pgid : null +} + /** * One tool call's Stop: remembered if it arrives before the call's command starts, and handed to * whatever is running once it does. @@ -304,21 +324,29 @@ export class TerminalService { } /** - * The command line holding each busy terminal's foreground, `''` when unannounced. A shell - * without integration (fish, sh) announces nothing, so the OS's foreground process group - * answers for it; an unreadable group (null) counts as idle. + * What each terminal is running. A shell without integration (fish, sh) announces nothing, so + * the OS's foreground process group answers for it; one the OS could not be asked about is + * reported as unchecked rather than idle. */ - async runningCommands(): Promise { + async runningWork(): Promise { const groups = this.options.processGroups ?? OS_PROCESS_GROUPS - const commands = await Promise.all( - [...this.sessions.values()].map(async (session) => { - if (session.isBusy) return [session.foreground ?? ''] - if (session.hasShellIntegration) return [] - const pgid = await groups.foreground(session.pid) - return pgid !== null && pgid !== session.pid ? [''] : [] - }) + const perSession = await Promise.all( + [...this.sessions.values()].map( + async (session): Promise<{ command: string | null; unchecked: boolean }> => { + if (session.isBusy) return { command: session.foreground ?? '', unchecked: false } + if (session.hasShellIntegration) return { command: null, unchecked: false } + const foreground = await groups.foreground(session.pid) + return { + command: foreground.kind === 'group' ? '' : null, + unchecked: foreground.kind === 'unknown', + } + } + ) ) - return commands.flat() + return { + commands: perSession.flatMap(({ command }) => (command === null ? [] : [command])), + uncheckedCount: perSession.filter(({ unchecked }) => unchecked).length, + } } getTabs(): TerminalTabsState { @@ -991,12 +1019,12 @@ export class TerminalService { private async stopCommand(session: TerminalSession, toolCallId: string): Promise { if (session.agentCommandToolCallId !== toolCallId) return const groups = this.options.processGroups ?? OS_PROCESS_GROUPS - const pgid = await groups.foreground(session.pid) + const pgid = await foregroundGroupId(groups, session.pid) if (session.agentCommandToolCallId !== toolCallId) return session.kill('SIGINT') for (const escalation of ['SIGTERM', 'SIGKILL'] as const) { if (await this.commandEnds(session, toolCallId, STOP_ESCALATION_MS)) return - if (pgid === null || (await groups.foreground(session.pid)) !== pgid) return + if (pgid === null || (await foregroundGroupId(groups, session.pid)) !== pgid) return if (session.agentCommandToolCallId !== toolCallId) return logger.info('Stopped command ignored the previous signal; escalating', { toolCallId, diff --git a/apps/desktop/src/main/terminal/process-group.test.ts b/apps/desktop/src/main/terminal/process-group.test.ts new file mode 100644 index 00000000000..efefec57e4b --- /dev/null +++ b/apps/desktop/src/main/terminal/process-group.test.ts @@ -0,0 +1,15 @@ +import { spawn } from 'node:child_process' +import { once } from 'node:events' +import { describe, expect, it } from 'vitest' +import { readForegroundProcessGroup } from '@/main/terminal/process-group' + +describe.skipIf(process.platform === 'win32')('readForegroundProcessGroup', () => { + it('reads a shell that has already exited as idle, not unknown', async () => { + const child = spawn('true') + await once(child, 'exit') + const exitedPid = child.pid + if (exitedPid === undefined) throw new Error('The child never started') + + await expect(readForegroundProcessGroup(exitedPid)).resolves.toEqual({ kind: 'shell' }) + }) +}) diff --git a/apps/desktop/src/main/terminal/process-group.ts b/apps/desktop/src/main/terminal/process-group.ts index 6e5a4e10a87..4c040383cb2 100644 --- a/apps/desktop/src/main/terminal/process-group.ts +++ b/apps/desktop/src/main/terminal/process-group.ts @@ -11,11 +11,25 @@ const logger = createLogger('DesktopTerminalProcessGroup') const LOOKUP_TIMEOUT_MS = 2_000 /** - * The tty's foreground process group, read from `ps`. Null when it cannot be read, or when the - * shell itself holds the foreground (it is sitting at its prompt). + * Who holds a terminal's foreground: the shell at its prompt (or a shell that has already + * exited), another process group, or `unknown` when the OS could not be asked. */ -export function readForegroundProcessGroup(shellPid: number): Promise { - if (!Number.isInteger(shellPid) || shellPid <= 0) return Promise.resolve(null) +export type ForegroundProcessGroup = + | { kind: 'shell' } + | { kind: 'group'; pgid: number } + | { kind: 'unknown' } + +const SHELL: ForegroundProcessGroup = { kind: 'shell' } +const UNKNOWN: ForegroundProcessGroup = { kind: 'unknown' } + +/** + * The tty's foreground process group, read from `ps`. A pid `ps` no longer knows is a shell + * that has exited, so nothing can be running in it. Windows has no `ps` or tty process groups, + * so a Windows shell always reads as holding its own foreground. + */ +export function readForegroundProcessGroup(shellPid: number): Promise { + if (process.platform === 'win32') return Promise.resolve(SHELL) + if (!Number.isInteger(shellPid) || shellPid <= 0) return Promise.resolve(UNKNOWN) return new Promise((resolve) => { let child: ReturnType try { @@ -23,12 +37,12 @@ export function readForegroundProcessGroup(shellPid: number): Promise { + const finish = (value: ForegroundProcessGroup) => { if (settled) return settled = true clearTimeout(timer) @@ -36,15 +50,22 @@ export function readForegroundProcessGroup(shellPid: number): Promise { child.kill('SIGKILL') - finish(null) + finish(UNKNOWN) }, LOOKUP_TIMEOUT_MS) child.stdout?.on('data', (chunk: Buffer) => { stdout += chunk.toString() }) - child.on('error', () => finish(null)) - child.on('close', () => { - const pgid = Number.parseInt(stdout.trim(), 10) - finish(Number.isInteger(pgid) && pgid > 0 && pgid !== shellPid ? pgid : null) + child.on('error', () => finish(UNKNOWN)) + child.on('close', (code) => { + const output = stdout.trim() + if (code !== 0 || !output) { + finish(code === 1 && !output ? SHELL : UNKNOWN) + return + } + const pgid = Number.parseInt(output, 10) + if (!Number.isInteger(pgid)) finish(UNKNOWN) + else if (pgid <= 0 || pgid === shellPid) finish(SHELL) + else finish({ kind: 'group', pgid }) }) }) } diff --git a/apps/desktop/src/main/terminal/registry.ts b/apps/desktop/src/main/terminal/registry.ts index 51b9f8330fb..8686b023c63 100644 --- a/apps/desktop/src/main/terminal/registry.ts +++ b/apps/desktop/src/main/terminal/registry.ts @@ -18,6 +18,7 @@ import { TerminalService, type TerminalServiceOptions, type TerminalSink, + type TerminalWork, } from '@/main/terminal' import type { RunLedger, RunRecord } from '@/main/terminal/run-ledger' import { recordedRunState, stopRecordedRun } from '@/main/terminal/tmux' @@ -375,12 +376,15 @@ export class TerminalRegistry { return true } - /** The command line holding each busy terminal's foreground, across every chat. */ - async runningCommands(): Promise { - const commands = await Promise.all( - [...this.entries.values()].map((entry) => entry.service.runningCommands()) + /** What every chat's terminals are running. */ + async runningWork(): Promise { + const perService = await Promise.all( + [...this.entries.values()].map((entry) => entry.service.runningWork()) ) - return commands.flat() + return { + commands: perService.flatMap((work) => work.commands), + uncheckedCount: perService.reduce((total, work) => total + work.uncheckedCount, 0), + } } /** diff --git a/apps/desktop/src/main/terminal/service.test.ts b/apps/desktop/src/main/terminal/service.test.ts index c44ebb49e7c..32a77f6c7a8 100644 --- a/apps/desktop/src/main/terminal/service.test.ts +++ b/apps/desktop/src/main/terminal/service.test.ts @@ -4,6 +4,7 @@ import { join } from 'node:path' import { sleep } from '@sim/utils/helpers' import { describe, expect, it, vi } from 'vitest' import { TerminalService } from '@/main/terminal' +import type { ForegroundProcessGroup } from '@/main/terminal/process-group' import { createRunLedger } from '@/main/terminal/run-ledger' /** @@ -487,7 +488,7 @@ describe('a shell that ends by itself', () => { }) describe('running work before a restart', () => { - function serviceWithForeground(read: (shellPid: number) => Promise) { + function serviceWithForeground(read: (shellPid: number) => Promise) { const terminal = new TerminalService({ loadCwd: () => '/tmp', processGroups: { foreground: vi.fn(read), signal: vi.fn() }, @@ -499,22 +500,24 @@ describe('running work before a restart', () => { } it('asks the OS about a shell that announces nothing, and trusts one that does', async () => { - const { terminal, session } = serviceWithForeground(async () => 4242) + const { terminal, session } = serviceWithForeground(async () => ({ kind: 'group', pgid: 4242 })) session.setShellIntegration(false) - await expect(terminal.runningCommands()).resolves.toEqual(['']) + await expect(terminal.runningWork()).resolves.toEqual({ commands: [''], uncheckedCount: 0 }) session.setShellIntegration(true) - await expect(terminal.runningCommands()).resolves.toEqual([]) + await expect(terminal.runningWork()).resolves.toEqual({ commands: [], uncheckedCount: 0 }) }) - it('counts a shell at its own prompt, or an unreadable foreground, as idle', async () => { - let read: () => Promise = async () => null - const { terminal, session } = serviceWithForeground(() => read()) + it('counts a shell at its own prompt as idle', async () => { + const { terminal, session } = serviceWithForeground(async () => ({ kind: 'shell' })) session.setShellIntegration(false) - await expect(terminal.runningCommands()).resolves.toEqual([]) + await expect(terminal.runningWork()).resolves.toEqual({ commands: [], uncheckedCount: 0 }) + }) - read = async () => session.pid - await expect(terminal.runningCommands()).resolves.toEqual([]) + it('reports a terminal whose foreground could not be read as unchecked, never idle', async () => { + const { terminal, session } = serviceWithForeground(async () => ({ kind: 'unknown' })) + session.setShellIntegration(false) + await expect(terminal.runningWork()).resolves.toEqual({ commands: [], uncheckedCount: 1 }) }) }) @@ -525,7 +528,10 @@ describe('stopping a tool call', () => { /** The tty's foreground group as the OS would report it at each read. */ let foreground: number | null = COMMAND_GROUP const processGroups = { - foreground: vi.fn(async (_shellPid: number) => foreground), + foreground: vi.fn( + async (_shellPid: number): Promise => + foreground === null ? { kind: 'unknown' } : { kind: 'group', pgid: foreground } + ), signal: vi.fn((_pgid: number, _signal: NodeJS.Signals) => {}), } const terminal = new TerminalService({ loadCwd: () => '/tmp', processGroups }) @@ -576,6 +582,20 @@ describe('stopping a tool call', () => { await expect(running).resolves.toMatchObject({ ok: true, result: { exitCode: 143 } }) }) + it('interrupts but never escalates when the foreground cannot be read', async () => { + const { terminal, session, processGroups, setForeground } = cancellableService() + session.setInterruptible(false) + const { running } = await startRun(terminal, session, 'call-unreadable') + setForeground(null) + + await terminal.cancelTool('call-unreadable') + + expect(session.kill).toHaveBeenCalledWith('SIGINT') + expect(processGroups.signal).not.toHaveBeenCalled() + session.finishRun(0) + await running + }) + it('never signals a different command that took the foreground meanwhile', async () => { const { terminal, session, processGroups, setForeground } = cancellableService() session.setInterruptible(false) diff --git a/apps/desktop/src/main/updater.test.ts b/apps/desktop/src/main/updater.test.ts index 8935167ace8..723cc3d38ae 100644 --- a/apps/desktop/src/main/updater.test.ts +++ b/apps/desktop/src/main/updater.test.ts @@ -114,6 +114,8 @@ describe('initUpdater state machine', () => { setRelaunchPending?: (pending: boolean) => void /** Work the restart would interrupt, as the in-flight check names it. */ withRunningWork?: string + /** Overrides the in-flight check, e.g. to make it fail. */ + describeInFlightWork?: () => Promise }) { const states: DesktopUpdateState[] = [] const handle = initUpdater({ @@ -129,7 +131,9 @@ describe('initUpdater state machine', () => { platform: 'darwin', beforeInstall: options?.beforeInstall, setRelaunchPending: options?.setRelaunchPending, - describeInFlightWork: async () => (options?.withRunningWork ? [options.withRunningWork] : []), + describeInFlightWork: + options?.describeInFlightWork ?? + (async () => (options?.withRunningWork ? [options.withRunningWork] : [])), }) // Engine selection (signature detection) resolves asynchronously. await vi.advanceTimersByTimeAsync(0) @@ -217,6 +221,26 @@ describe('initUpdater state machine', () => { expect(autoUpdaterMock.autoInstallOnAppQuit).toBe(true) }) + it('asks before restarting when the check for running work fails', async () => { + const { handle } = await createUpdater({ + describeInFlightWork: async () => { + throw new Error('terminal probe failed') + }, + }) + await stageUpdate(handle, '2.0.0') + vi.mocked(dialog.showMessageBox).mockResolvedValueOnce({ response: 1, checkboxChecked: false }) + + handle.install() + await vi.advanceTimersByTimeAsync(0) + + expect(dialog.showMessageBox).toHaveBeenCalledWith( + expect.objectContaining({ + message: "Sim couldn't check whether restarting would interrupt any work.", + }) + ) + expect(autoUpdaterMock.quitAndInstall).not.toHaveBeenCalled() + }) + it('installs a required update without offering to wait for running work', async () => { const { handle } = await createUpdater({ withRunningWork: 'claude is still running in a terminal.', diff --git a/apps/desktop/src/main/updater.ts b/apps/desktop/src/main/updater.ts index caaff3e9c78..1c084971a27 100644 --- a/apps/desktop/src/main/updater.ts +++ b/apps/desktop/src/main/updater.ts @@ -493,6 +493,7 @@ export function initUpdater(deps: UpdaterDeps): UpdaterHandle { logger.warn('Could not check for work a restart would interrupt', { message: getErrorMessage(error, 'unknown'), }) + work = ["Sim couldn't check whether restarting would interrupt any work."] } } if (work.length > 0) { diff --git a/apps/sim/app/o/[organizationId]/people/[memberId]/person.tsx b/apps/sim/app/o/[organizationId]/people/[memberId]/person.tsx index eacbb710d1c..772f0bbfd5e 100644 --- a/apps/sim/app/o/[organizationId]/people/[memberId]/person.tsx +++ b/apps/sim/app/o/[organizationId]/people/[memberId]/person.tsx @@ -163,7 +163,7 @@ export function Person({ organizationId, memberId }: PersonProps) { diff --git a/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-content/components/browser-session/sign-in-hosts.test.ts b/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-content/components/browser-session/sign-in-hosts.test.ts index 2263d1cdacd..c6d81c575cd 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-content/components/browser-session/sign-in-hosts.test.ts +++ b/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-content/components/browser-session/sign-in-hosts.test.ts @@ -50,4 +50,12 @@ describe('continuablePage', () => { expect(continuablePage(`${APP_ORIGIN}/workspace/w1/home`, APP_ORIGIN)).toBeNull() expect(continuablePage('https://sim.ai/login', APP_ORIGIN)?.hostname).toBe('sim.ai') }) + + it.each([ + 'https://user@accounts.google.com/signin', + 'https://user:secret@accounts.google.com/signin', + 'https://:secret@accounts.google.com/signin', + ])('never offers %s, whose embedded credentials the shell refuses', (url) => { + expect(continuablePage(url, APP_ORIGIN)).toBeNull() + }) }) diff --git a/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-content/components/browser-session/sign-in-hosts.ts b/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-content/components/browser-session/sign-in-hosts.ts index 7b8e96da150..e8f35ff2bc0 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-content/components/browser-session/sign-in-hosts.ts +++ b/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-content/components/browser-session/sign-in-hosts.ts @@ -27,13 +27,15 @@ const SIGN_IN_HOSTS: readonly SignInHost[] = [ ] /** - * The page as a URL when another browser can continue it: https, and not one of - * Sim's own pages on `appOrigin`, which the shell refuses to hand off. + * The page as a URL when another browser can continue it: https without embedded + * credentials, and not one of Sim's own pages on `appOrigin`, all of which the + * shell refuses to hand off. */ export function continuablePage(url: string | undefined, appOrigin: string): URL | null { if (!url) return null try { const parsed = new URL(url) + if (parsed.username || parsed.password) return null return parsed.protocol === 'https:' && parsed.origin !== appOrigin ? parsed : null } catch { return null diff --git a/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-content/components/browser-session/use-continue-sign-in.ts b/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-content/components/browser-session/use-continue-sign-in.ts index a70d1979562..2211c6817c2 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-content/components/browser-session/use-continue-sign-in.ts +++ b/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-content/components/browser-session/use-continue-sign-in.ts @@ -51,7 +51,7 @@ interface UseContinueSignInProps { export function useContinueSignIn({ scopeId, pageUrl }: UseContinueSignInProps) { const supported = supportsContinueSignIn() const { data: profiles = [] } = useBrowserImportProfiles(supported) - const { data: pendingData = null } = usePendingSignIn(scopeId, supported) + const { data: pendingData = null, isSuccess: pendingKnown } = usePendingSignIn(scopeId, supported) const pending = supported ? pendingData : null const { mutateAsync: startHandoff, isPending: starting } = useStartSignInHandoff(scopeId) const { mutateAsync: importBack, isPending: importing } = useBringSignInBack(scopeId) @@ -64,8 +64,13 @@ export function useContinueSignIn({ scopeId, pageUrl }: UseContinueSignInProps) // also support, so cached profiles alone must not reveal this feature. const canContinue = supported && profiles.length > 0 && page !== null const hintHost = page ? signInHintHost(page) : null + // Until the shell answers, an existing handoff may be pending; the hint's Continue would replace it. const hintVisible = - canContinue && pending === null && hintHost !== null && !dismissedHintHosts.has(hintHost) + canContinue && + pendingKnown && + pending === null && + hintHost !== null && + !dismissedHintHosts.has(hintHost) const continueIn = useCallback( async (profile: BrowserImportProfile) => {