diff --git a/src/main/lazy-worker-thread-host.ts b/src/main/lazy-worker-thread-host.ts index 361374f6bc7..a218a9e7e93 100644 --- a/src/main/lazy-worker-thread-host.ts +++ b/src/main/lazy-worker-thread-host.ts @@ -8,13 +8,7 @@ export type WorkerRequestTransport = Pick WorkerRequestTransport -/** - * Owns the lifetime of one lazily-spawned worker thread: spawn on demand, - * listener wiring, teardown, and idle expiry. It holds no request state, so - * every decision about which call a message belongs to stays with its client, - * and a failed spawn is reported rather than thrown so the client can fail its - * queued calls closed instead of moving the work back onto the main thread. - */ +/** Clients own request state and must fail queued calls when spawning fails. */ export class LazyWorkerThreadHost { private worker: WorkerRequestTransport | null = null private idleTimer: NodeJS.Timeout | null = null @@ -29,14 +23,11 @@ export class LazyWorkerThreadHost { onMessage: (response: TResponse) => void onError: (error: Error) => void onExit: (code: number) => void - /** Nothing active and nothing queued, checked again when the idle timer fires. */ + /** No active or queued requests; checked again at idle expiry. */ isIdle: () => boolean - /** First spawn failure only: a repeating one must not repeat the log. */ + /** Called only for the first spawn failure. */ onUnavailable: (error: unknown) => void - /** - * No replacement until a terminated worker has exited (#24572): terminate() - * cannot interrupt a native call, so respawning would stack threads on it. - */ + /** Native calls can delay termination; wait for exit before spawning another worker (#24572). */ awaitRetirement?: boolean } ) {} @@ -45,12 +36,11 @@ export class LazyWorkerThreadHost { return this.worker } - /** A terminated worker has not exited yet, so `ensure` refuses to spawn. */ get isRetiring(): boolean { return this.retiring !== null } - /** The live worker, spawning one if needed; null when no worker can be had. */ + /** Returns null if spawning fails or a previous worker is still retiring. */ ensure(): WorkerRequestTransport | null { if (this.worker) { return this.worker @@ -110,8 +100,7 @@ export class LazyWorkerThreadHost { } this.idleTimer = setTimeout(() => { this.idleTimer = null - // Re-checked here: a request arriving as the timer fires must never be - // lost to a self-exiting worker. + // Recheck idle state so requests arriving during the delay survive. if (this.options.isIdle()) { this.destroy() } diff --git a/src/main/linux-lid-sleep-assertion.ts b/src/main/linux-lid-sleep-assertion.ts index e01a3268ae5..0fe3312e0c0 100644 --- a/src/main/linux-lid-sleep-assertion.ts +++ b/src/main/linux-lid-sleep-assertion.ts @@ -72,8 +72,7 @@ export class LinuxLidSleepAssertion { let child: SystemdInhibitProcess try { - // logind's lid switch handling ignores ordinary sleep inhibitors on many systems, - // so Linux needs both a sleep lock and a handle-lid-switch lock. + // logind may ignore sleep inhibitors for lid events; acquire both locks. child = this.spawn( 'systemd-inhibit', [ diff --git a/src/main/linux-package-install-command.test.ts b/src/main/linux-package-install-command.test.ts index 5cc86cef8cd..196db14b4c7 100644 --- a/src/main/linux-package-install-command.test.ts +++ b/src/main/linux-package-install-command.test.ts @@ -5,7 +5,6 @@ const { statSyncMock } = vi.hoisted(() => ({ statSyncMock: vi.fn() })) vi.mock('node:fs', () => ({ statSync: statSyncMock })) -// Executables the fake filesystem exposes, keyed by absolute path. let executables: Map function install(absolutePath: string, options?: { file?: boolean; mode?: number }): void { @@ -122,7 +121,7 @@ describe('resolveTrustedExecutable', () => { describe('buildLinuxPackageInstallCommand', () => { it('refuses a non-absolute package path before looking for anything', async () => { - // Why: zypper/dnf/yum/rpm take no `--`, so a relative or dash-leading path would become an option. + // Managers without -- support require absolute paths to prevent option injection. install('/usr/bin/sudo') install('/usr/bin/apt') const { buildLinuxPackageInstallCommand } = await loadCommandModule() @@ -265,7 +264,7 @@ describe('hasTrustedPackageManagerFor', () => { expect(hasTrustedPackageManagerFor('deb')).toBe(true) }) - // The #17702 case: an Arch rebuild of the .deb inherits the marker but has no deb tooling. + // Repackagers can inherit deb/rpm markers without the corresponding tooling (#17702). it('rejects a deb marker on a host with only pacman', async () => { install('/usr/bin/pacman') install('/usr/bin/sudo') diff --git a/src/main/linux-package-install-command.ts b/src/main/linux-package-install-command.ts index b9c9191b2de..1f7b2056790 100644 --- a/src/main/linux-package-install-command.ts +++ b/src/main/linux-package-install-command.ts @@ -3,7 +3,7 @@ import { statSync } from 'node:fs' import path from 'node:path' import type { LinuxRootPackageType } from '../shared/update-status-types' -// Why: an absolute but user-writable PATH entry must never be treated as a trusted package manager. +// User-writable PATH entries cannot be trusted for privileged package installation. const TRUSTED_EXECUTABLE_DIRECTORIES = ['/usr/bin', '/bin', '/usr/sbin', '/sbin'] const DEB_PACKAGE_MANAGERS: { name: string; args: string[] }[] = [ @@ -11,8 +11,7 @@ const DEB_PACKAGE_MANAGERS: { name: string; args: string[] }[] = [ { name: 'dpkg', args: ['-i', '--'] } ] -// No `--` terminator: these tools do not accept one. Safe because capture requires an absolute path, -// so the argument can never be read as an option. +// These tools lack -- support; absolute package paths prevent option injection. const RPM_PACKAGE_MANAGERS: { name: string; args: string[] }[] = [ { name: 'zypper', @@ -27,47 +26,31 @@ export type LinuxPackageInstallCommandResult = | { ok: true; command: string } | { ok: false; reason: 'no-sudo' | 'no-package-manager' | 'invalid-package-path' } -/** - * Resolves an executable strictly from the trusted system directories. A symlink inside those - * directories is fine — its target is what `statSync` checks — but nothing outside them is consulted - * and no shell is ever invoked for discovery. - */ export function resolveTrustedExecutable(name: string): string | null { for (const directory of TRUSTED_EXECUTABLE_DIRECTORIES) { - // posix.join: these are POSIX paths, and this module only ever runs on Linux. const candidate = path.posix.join(directory, name) try { const stats = statSync(candidate) if (stats.isFile() && (stats.mode & 0o111) !== 0) { return candidate } - } catch { - // Absent here; keep looking in the remaining trusted directories. - } + } catch {} } return null } -/** - * Whether this host has any package manager able to install the marker's format. A repackaged - * install (AUR, Nix, a container rebuild) inherits the `package-type` marker from the .deb/.rpm it - * was built from, so the marker alone never proves the host can act on it. - */ +/** Repackaged installs can inherit a deb/rpm marker without a matching package manager. */ export function hasTrustedPackageManagerFor(packageType: LinuxRootPackageType): boolean { const candidates = packageType === 'deb' ? DEB_PACKAGE_MANAGERS : RPM_PACKAGE_MANAGERS return candidates.some((candidate) => resolveTrustedExecutable(candidate.name) !== null) } -/** - * Builds the interactive command the user pastes into their own terminal. Every token except the - * package path is a fixed literal, and the path is POSIX-single-quoted — Orca never runs this. - */ +/** Produces a command for the user to run and confirm in their terminal. */ export function buildLinuxPackageInstallCommand( packageType: LinuxRootPackageType, packagePath: string ): LinuxPackageInstallCommandResult { - // Why: several package managers accept no `--` terminator, so a relative or dash-leading path would - // be read as an option. Hold that property here rather than relying on a caller two modules away. + // Absolute paths prevent option injection in managers that lack -- support. if (!path.isAbsolute(packagePath)) { return { ok: false, reason: 'invalid-package-path' } } diff --git a/src/main/linux-package-install-diagnostic.ts b/src/main/linux-package-install-diagnostic.ts index c65c62679fa..39f83143ebe 100644 --- a/src/main/linux-package-install-diagnostic.ts +++ b/src/main/linux-package-install-diagnostic.ts @@ -62,7 +62,7 @@ export function redactLinuxPackageInstallText( if (homeDir) { text = replaceAllLiteral(text, homeDir, '') } - // Why: privilege tools can name the user without including their home directory. + // Privilege tools can name the user without including their home directory. const userName = readUserName() if (userName && userName.length >= MIN_REDACTED_USERNAME_LENGTH) { text = replaceAllLiteral(text, userName, '') diff --git a/src/main/linux-package-update-recovery.test.ts b/src/main/linux-package-update-recovery.test.ts index 859e80e74b2..d31c8f70be8 100644 --- a/src/main/linux-package-update-recovery.test.ts +++ b/src/main/linux-package-update-recovery.test.ts @@ -20,7 +20,6 @@ vi.mock('./linux-package-install-command', () => ({ buildLinuxPackageInstallCommand: buildCommandMock })) -// Counts hash passes without changing read behavior. vi.mock('node:fs', async (importOriginal) => { const actual = await importOriginal() return { @@ -32,7 +31,7 @@ vi.mock('node:fs', async (importOriginal) => { } }) -// Real symlinks, POSIX modes, and the XDG cache layout make these cases meaningless on Windows. +// These fixtures require POSIX modes, symlinks, and XDG cache paths. const describePosix = describe.skipIf(process.platform === 'win32') const VERSION = '1.2.3' @@ -62,7 +61,6 @@ async function writePackage(name: string, contents = PAYLOAD): Promise { return filePath } -/** Captures a well-formed downloaded event unless a field is overridden. */ function capture(overrides: Record = {}): LinuxPackageArtifact | null { const downloadedFile = (overrides.downloadedFile ?? path.join(downloadDir, 'orca.deb')) as string return recovery.captureLinuxPackageArtifact({ @@ -81,7 +79,6 @@ beforeEach(async () => { tempRoot = await fsp.mkdtemp(path.join(os.tmpdir(), 'orca-recovery-')) cacheRoot = path.join(tempRoot, 'cache') updaterDir = path.join(cacheRoot, 'orca-updater') - // The only shape electron-updater downloads into: //pending. downloadDir = path.join(updaterDir, 'pending') outsideDir = path.join(tempRoot, 'outside') await fsp.mkdir(downloadDir, { recursive: true }) @@ -197,7 +194,7 @@ describe('captureLinuxPackageArtifact', () => { }) it('refuses a digest that cannot decode to 64 bytes', () => { - // Why: an undecodable digest is a metadata problem — arming here would blame the user's file. + // Invalid release metadata must not arm recovery that blames the downloaded file. capture({ files: [{ url: 'orca.deb', sha512: createHash('sha256').update(PAYLOAD).digest('base64') }] }) @@ -205,7 +202,7 @@ describe('captureLinuxPackageArtifact', () => { }) it('refuses a digest with characters outside the base64 alphabet', () => { - // Buffer.from would silently drop the '!', so the round-trip check must reject this. + // Buffer.from silently drops invalid characters unless checked by a round trip. capture({ files: [{ url: 'orca.deb', sha512: `${SHA512.slice(0, 4)}!${SHA512.slice(4)}` }] }) expect(recovery.getTrackedLinuxPackageArtifact()).toBeNull() }) @@ -491,7 +488,6 @@ describePosix('cache containment', () => { }) it('uses the home cache when XDG_CACHE_HOME is unset', async () => { - // Why: the fallback is the only reason an unset XDG_CACHE_HOME still finds the real download. const homeCache = path.join(tempRoot, 'home') const homeDownloadDir = path.join(homeCache, '.cache', 'orca-updater', 'pending') await fsp.mkdir(homeDownloadDir, { recursive: true }) @@ -508,11 +504,7 @@ describePosix('cache containment', () => { }) }) -// electron-updater builds `downloadedFile` from the cache's own writable update-info.json without -// calling basename, so these pin the accepted shape to the one directory it actually downloads into. -// Defence in depth only — a same-uid process can create its own `pending` directory, and per the -// design's trust model it can already overwrite the real cached package. The load-bearing check is the -// cache-root anchor covered by the `cache containment` describe above. +// Pending-directory shape is defense in depth; cache-root containment excludes other users’ swap locations. describePosix('pending-directory anchoring', () => { it('rejects a package in an attacker-owned directory under the cache root', async () => { const evilDir = path.join(cacheRoot, 'evil') @@ -561,7 +553,6 @@ describePosix('pending-directory anchoring', () => { }) it('accepts any updater directory name that holds the pending directory', async () => { - // The cache directory name comes from the app, so only its position is fixed. const otherPending = path.join(cacheRoot, 'orca-updater-next', 'pending') await fsp.mkdir(otherPending, { recursive: true }) const filePath = path.join(otherPending, 'orca.deb') diff --git a/src/main/linux-package-update-recovery.ts b/src/main/linux-package-update-recovery.ts index 4722a2e13ae..075969bc757 100644 --- a/src/main/linux-package-update-recovery.ts +++ b/src/main/linux-package-update-recovery.ts @@ -11,7 +11,6 @@ import { getLinuxRootPackageType } from './linux-update-package-type' import { buildLinuxPackageInstallCommand } from './linux-package-install-command' const SHA512_BYTE_LENGTH = 64 -// electron-updater downloads into //pending; nothing else is trusted. const PENDING_DIRECTORY_NAME = 'pending' export type LinuxPackageArtifact = { @@ -43,7 +42,7 @@ type ValidationResult = | { ok: false; reason: 'missing' | 'not-regular' | 'hash-mismatch' | 'read-failed' } let trackedArtifact: LinuxPackageArtifact | null = null -// Why: the renderer debounces clicks, but the IPC boundary must not allow parallel hashing of a 160 MB package. +// Deduplicate hashing at the IPC boundary even if clients debounce clicks. const inFlightValidations = new WeakMap>() export function getTrackedLinuxPackageArtifact(): LinuxPackageArtifact | null { @@ -54,12 +53,11 @@ export function clearTrackedLinuxPackageArtifact(): void { trackedArtifact = null } -/** Clears the artifact only when a different version demonstrably takes over the update cycle. */ export function clearTrackedLinuxPackageArtifactForOtherVersion(version: unknown): void { if (!trackedArtifact) { return } - // Why: an unknown/empty version is not evidence of another cycle and must not destroy recovery. + // Missing version metadata does not prove another update cycle has started. if (typeof version !== 'string' || version.length === 0) { return } @@ -69,12 +67,7 @@ export function clearTrackedLinuxPackageArtifactForOtherVersion(version: unknown trackedArtifact = null } -/** - * Mirrors electron-updater's cache-name rule: resolve the manifest URL against a dummy base, decode - * the pathname, require the package extension, and match the basename of the downloaded file. - * A malformed encoding or an ambiguous file/hash pairing disables cached-package recovery rather - * than guessing a digest. - */ +/** Match electron-updater’s decoded cache filename; reject malformed or conflicting digests. */ function resolveExpectedSha512( files: unknown, downloadedFile: string, @@ -115,10 +108,7 @@ function resolveExpectedSha512( return resolved } -/** - * Retains the downloaded package and its release digest so manual actions do not repeat the 160 MB - * transfer. Only the in-memory event metadata is trusted for the digest. - */ +/** Trust only the release event’s digest; retain the download for manual recovery. */ export function captureLinuxPackageArtifact(event: unknown): LinuxPackageArtifact | null { const packageType = getLinuxRootPackageType() if (!packageType) { @@ -140,12 +130,9 @@ export function captureLinuxPackageArtifact(event: unknown): LinuxPackageArtifac downloadedFile, packageType ) - // Why: a malformed digest can never validate. Arming recovery on it would send the user to the - // alarming "no longer matches the verified release" path for what is a release-metadata problem. + // Reject malformed release metadata before offering recovery that cannot validate. if (!sha512 || !decodeExpectedDigest(sha512)) { - // Why: an unresolvable digest only means THIS event cannot arm recovery. A previously retained - // artifact carries its own digest and is revalidated on every use, so dropping it would force a - // needless 160 MB redownload of a file that is still on disk and still verifiable. + // Keep the prior artifact: it has its own digest and is revalidated on use. return null } const artifact = { packageType, version, path: downloadedFile, sha512 } @@ -158,27 +145,12 @@ function isInsideDirectory(root: string, target: string): boolean { return relative.length > 0 && !relative.startsWith('..') && !path.isAbsolute(relative) } -/** - * The cache root electron-updater downloads into. Mirrors its own `getAppCacheDir()` rule, which on - * Linux resolves to the same directory Electron reports as the `cache` path. - */ function getUpdaterCacheRoot(): string { - // Mirrors upstream exactly, including its lack of an absoluteness check — diverging here would put - // the real download outside the directory this module treats as the cache. + // Match electron-updater’s XDG_CACHE_HOME handling, including relative paths. return process.env.XDG_CACHE_HOME || path.join(os.homedir(), '.cache') } -/** - * Rejects traversal and a symlinked parent that escapes the updater cache. - * - * The security boundary is the cache-root anchor below: electron-updater builds `downloadedFile` from - * the `fileName` in the cache's own writable `update-info.json` without calling `basename`, so without - * it a manipulated entry could point at `/tmp` or `/dev/shm` where a DIFFERENT principal can win the - * swap race. The `//pending` shape check narrows the accepted paths to - * the one place electron-updater actually downloads into; it is defence in depth, not a boundary — a - * same-uid process can create its own `pending` directory, and per the design's trust model such a - * process can already overwrite the real cached package anyway. - */ +/** Anchor paths under the real cache root to exclude other users’ swap locations; same-user writes remain possible. */ export async function isInUpdaterPendingCache(filePath: string): Promise { const cacheRoot = path.resolve(getUpdaterCacheRoot()) if (!isInsideDirectory(cacheRoot, path.resolve(filePath))) { @@ -202,9 +174,7 @@ function decodeExpectedDigest(sha512: string): Buffer | null { if (decoded.byteLength !== SHA512_BYTE_LENGTH) { return null } - // Why: Buffer.from silently drops invalid base64 characters; round-tripping rejects malformed input. - // The round-trip emits standard base64, so a URL-safe digest would be rejected — electron-updater's - // latest-linux.yml is standard base64, and failing closed on an unrecognized encoding is correct. + // Round-trip standard base64: Buffer.from silently accepts malformed input. return decoded.toString('base64') === trimmed ? decoded : null } @@ -245,7 +215,6 @@ async function validateArtifact(artifact: LinuxPackageArtifact): Promise { const inFlight = inFlightValidations.get(artifact) if (inFlight) { @@ -260,11 +229,7 @@ function runValidation(artifact: LinuxPackageArtifact): Promise { diff --git a/src/main/linux-update-package-type.test.ts b/src/main/linux-update-package-type.test.ts index ed1abf90b47..405ea4c33ac 100644 --- a/src/main/linux-update-package-type.test.ts +++ b/src/main/linux-update-package-type.test.ts @@ -221,7 +221,6 @@ describe('getLinuxRootPackageType', () => { }) it('returns unusable when the marker is unreadable', async () => { - // A directory in the marker's place makes fail with EISDIR. await fsp.mkdir(path.join(resourcesDir, 'package-type')) const module = await loadPackageType() expect(module.getLinuxPackageType()).toBe('unusable') @@ -287,7 +286,6 @@ describe('getLinuxRootPackageType', () => { it('reads the marker once and warns once per process', async () => { const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) - // A directory in place of the marker is readable-but-unusable, which is the case worth reporting. await fsp.mkdir(path.join(resourcesDir, 'package-type')) const module = await loadPackageType() module.getLinuxPackageType() @@ -315,7 +313,6 @@ describe('getLinuxRootPackageType', () => { }) describe('isExternallyManagedLinuxInstall', () => { - // The #17702 case: an AUR/Nix/container rebuild of the .deb inherits `package-type` verbatim. it('reports a deb marker with no deb package manager as externally managed', async () => { hasTrustedPackageManagerForMock.mockReturnValue(false) await writeMarker('deb') diff --git a/src/main/linux-update-package-type.ts b/src/main/linux-update-package-type.ts index f58c6cf16ad..8e8b1501189 100644 --- a/src/main/linux-update-package-type.ts +++ b/src/main/linux-update-package-type.ts @@ -6,14 +6,11 @@ import type { LinuxRootPackageType } from '../shared/update-status-types' export type { LinuxRootPackageType } -/** The packaged Linux format that controls how updates may be installed. */ export type LinuxPackageType = LinuxRootPackageType | 'non-root' | 'unusable' -// Why: `undefined` means "not resolved yet"; every other value is stable for this process. let cachedPackageType: LinuxPackageType | undefined let cachedExternallyManaged: boolean | undefined -// Bounded by construction: the marker is read at most once per process. function warnMarkerUnusable(detail: string): void { console.warn(`[updater] linux package-type marker unusable: ${detail}`) } @@ -98,11 +95,7 @@ function readPackageTypeMarker(): LinuxPackageType { return 'unusable' } -/** - * Resolves the installed Linux package format. Packaged builds fail closed when their marker is - * missing or unusable unless the legacy APPIMAGE runtime identity is valid. Unpackaged, non-Linux, - * and identified AppImage runs are non-root. - */ +/** Packaged Linux builds require a valid marker, except legacy AppImages with a missing marker. */ export function getLinuxPackageType(): LinuxPackageType { if (cachedPackageType === undefined) { cachedPackageType = readPackageTypeMarker() @@ -110,23 +103,12 @@ export function getLinuxPackageType(): LinuxPackageType { return cachedPackageType } -/** Returns a root package type when this build supports manual package recovery. */ export function getLinuxRootPackageType(): LinuxRootPackageType | null { const packageType = getLinuxPackageType() return packageType === 'deb' || packageType === 'rpm' ? packageType : null } -/** - * Whether the marker claims a root package format this host cannot install. Repackagers (AUR, Nix, - * container rebuilds) unpack Orca's .deb and inherit its `package-type` verbatim, so the marker - * describes the artifact Orca was built as, never the system that now owns the install. Without a - * matching package manager no downloaded package can ever be applied here. - * - * A false positive is impossible by construction: this reuses the exact manager lists and resolver - * that `buildLinuxPackageInstallCommand` already loops over, so any host flagged here would have - * failed with `no-package-manager` after the download anyway. The gate only moves that verdict - * earlier — it never refuses a host that could have installed the update. - */ +/** Repackagers can inherit deb/rpm markers; require the same manager discovery used for installation. */ export function isExternallyManagedLinuxInstall(): boolean { if (cachedExternallyManaged === undefined) { const packageType = getLinuxRootPackageType() diff --git a/src/main/local-project-runtime-resolution.ts b/src/main/local-project-runtime-resolution.ts index 09dbc5754a0..6024d963a7e 100644 --- a/src/main/local-project-runtime-resolution.ts +++ b/src/main/local-project-runtime-resolution.ts @@ -29,15 +29,7 @@ import { import { resolveManagedSshHostLoginShell } from './pty/managed-ssh-host-login-shell' import { getRepoExecutionHostId, LOCAL_EXECUTION_HOST_ID } from '../shared/execution-host' -/** - * The slice of the store runtime resolution actually reads. Structural rather - * than the full `Store` so narrowed stores -- the Orca runtime's `RuntimeStore`, - * worktree root preparation -- resolve the same runtime the create path does - * instead of silently falling back to host placement. - * - * Members stay optional because those stores declare them optional; the - * `typeof` guards below are what actually decide whether resolution can run. - */ +// Structural typing lets narrowed runtime stores use the same resolution as workspace creation. export type ProjectRuntimeResolutionStore = { getProjects?: Store['getProjects'] getRepo?: Store['getRepo'] @@ -99,8 +91,7 @@ export function resolveLocalProjectRuntimeForRepo( return resolveLocalProjectRuntime(store, project) } -/** Same choice the renderer makes for a workspace: the project's runtime, else the distro of a - * WSL-share path, else the global default. Works for a repo with no project too. */ +/** Match renderer precedence: project preference, WSL-share distro, then global default. */ export function resolveLocalWorkspaceRuntime( store: ProjectRuntimeResolutionStore, repo: Repo, @@ -117,8 +108,7 @@ export function resolveLocalWorkspaceRuntime( project?.localWindowsRuntimePreference, workspacePath ) - // A `\\wsl$` path proves its distro exists, and main's distro cache can be stale; the window - // checks such a path against no cache either. + // WSL-share paths supply their distro; ignore potentially stale discovery caches for inferred preferences. return resolveLocalProjectRuntime( store, { id: project?.id ?? repo.id, localWindowsRuntimePreference: preference }, @@ -127,8 +117,7 @@ export function resolveLocalWorkspaceRuntime( ) } -/** The one local repo whose project runs a folder workspace on this machine; none for a folder - * pinned to another host or spanning several repos. */ +/** A folder inherits a runtime only from a single repo on the local host. */ export function findLocalFolderWorkspaceRuntimeRepo( state: FolderWorkspaceHostState, folderWorkspaceId: string @@ -155,8 +144,7 @@ export type LaunchScopeRuntimeStore = ProjectRuntimeResolutionStore & { getProjectGroups?: () => ProjectGroup[] } -/** A launch scope's runtime, as the window reads it for a workspace. A remote scope has none: its - * host plans for itself. */ +/** Remote scopes resolve their runtime on their own host. */ export function resolveLocalLaunchScopeRuntime( store: LaunchScopeRuntimeStore, scope: { @@ -210,8 +198,7 @@ export function resolveLocalProjectRuntimesForRepos( if (matchingRepoIds.length === 0) { continue } - // Why: one project runtime applies to every source repo in that project; - // resolving it once prevents mobile polls from rescanning project settings. + // Resolve once per project to avoid repeated settings scans during mobile polls. const runtime = resolveLocalProjectRuntime(store, project, settings) for (const repoId of matchingRepoIds) { runtimeByRepoId.set(repoId, runtime) @@ -238,7 +225,7 @@ type LocalPtySettings = Partial< Pick > -/** A local PTY's shell and distro. Throws for a runtime that needs repair. */ +/** Throws for a Windows runtime that needs repair. */ export function resolveLocalPtyOptions( settings: LocalPtySettings | undefined, projectRuntime: ProjectExecutionRuntimeResolution | undefined, @@ -260,8 +247,7 @@ export function resolveLocalPtyOptions( } } -/** A plain terminal's PTY: the project-only runtime and `COMSPEC`, as the spawn picks when no - * agent plan brought its own. */ +/** Plain terminals use the project runtime and fall back to COMSPEC for the Windows host shell. */ export function resolvePlainTerminalPtyOptions( store: ProjectRuntimeResolutionStore | undefined, settings: LocalPtySettings | undefined, diff --git a/src/main/local-worktree-filesystem-wsl-banner.wsl.test.ts b/src/main/local-worktree-filesystem-wsl-banner.wsl.test.ts index a4e7e4a40f9..ee8f95b9dcf 100644 --- a/src/main/local-worktree-filesystem-wsl-banner.wsl.test.ts +++ b/src/main/local-worktree-filesystem-wsl-banner.wsl.test.ts @@ -23,7 +23,6 @@ async function wsl(command: string, ...args: string[]): Promise { return stdout.trim() } -/** What an unfenced login-shell read returns — the shape this suite exists to rule out. */ async function readThroughRawLoginShell(linuxPath: string): Promise { const { stdout } = await execFileAsync( 'wsl.exe', @@ -57,9 +56,7 @@ describe.skipIf(!runRealWsl)('WSL worktree reads carry no shell chatter', () => const { readPath } = getLocalWorktreePathAccess({ wslDistro: DISTRO }) const raw = await readThroughRawLoginShell(`${fixtureRoot}/file.txt`) - // Contrast: routed through a login shell the read carries whatever the rc - // files printed (stock Ubuntu ships a sudo hint). These reads use a plain - // `sh -c`, which runs no rc at all, so they are exactly the file. + // A login shell may print rc banners; plain sh must return only the file’s bytes. expect(raw.endsWith(FILE_CONTENTS)).toBe(true) expect(await readPath(unc(`${fixtureRoot}/file.txt`))).toBe(FILE_CONTENTS) }, 60_000) diff --git a/src/main/local-worktree-filesystem.test.ts b/src/main/local-worktree-filesystem.test.ts index 8cc1584c380..e235ad4fdea 100644 --- a/src/main/local-worktree-filesystem.test.ts +++ b/src/main/local-worktree-filesystem.test.ts @@ -7,8 +7,7 @@ const { runProcessMock, lstatMock, readFileMock, rmMock } = vi.hoisted(() => ({ rmMock: vi.fn() })) -// Why mock the chokepoint: encoding, timeout and the hidden console are its -// contract now, so this suite asserts what the distro is asked to run. +// Process-host tests own encoding, timeouts, and console hiding; assert guest commands here. vi.mock('@orca/process-host', () => ({ runProcess: runProcessMock })) @@ -190,9 +189,7 @@ describe('local worktree filesystem runtime access', () => { expect.objectContaining({ program: 'wsl.exe', args: expect.arrayContaining(['-d', 'Ubuntu']), - // Why a concrete directory (#16463): the guest path is inside the - // command, and these run while a worktree is being removed -- which is - // the cwd an omitted one would inherit. + // The inherited cwd may be the worktree being removed (#16463). cwd: expect.any(String) }) ) @@ -208,9 +205,7 @@ describe('local worktree filesystem runtime access', () => { completeExecFile('file') await getLocalWorktreePathAccess({ wslDistro: 'Ubuntu' }).statPath('/home/me/repo/.git') - // Why: a login/interactive shell is what puts the distro's rc banner on the - // stdout these callers parse. cat/stat/rm need nothing from the user's PATH, - // so the shell mode is the fix rather than filtering what it prints. + // Avoid login-shell banners instead of filtering potentially valid file content. const args = runProcessMock.mock.calls[0]?.[0].args as string[] expect(args).toContain('-c') expect(args).not.toContain('-lc') @@ -250,7 +245,6 @@ describe('local worktree filesystem runtime access', () => { const access = getLocalWorktreePathAccess({ wslDistro: 'Ubuntu' }) await expect(access.statPath('/mnt/c/repo/locked')).rejects.toThrow('Permission denied') - // A delete that did not happen still has to look like a failure, or cleanup silently "succeeds". await expect(isWorktreePathMissing('/mnt/c/repo/locked', access.statPath)).resolves.toBe( false ) @@ -270,8 +264,7 @@ describe('local worktree filesystem runtime access', () => { it('reports a translated stat failure as a failure rather than as absence', async () => { await withPlatform('win32', async () => { - // Synthetic: no host available here ships a non-English locale, and the probe pins LC_ALL=C - // anyway. The point is the direction it errs if that pin ever comes off. + // Synthetic localization verifies that unknown errors cannot prove absence if LC_ALL=C regresses. failExecFile(1, "stat: '/mnt/c/repo/missing': \n") const access = getLocalWorktreePathAccess({ wslDistro: 'Ubuntu' }) diff --git a/src/main/local-worktree-filesystem.ts b/src/main/local-worktree-filesystem.ts index 65491b3fd26..4f2544b3b71 100644 --- a/src/main/local-worktree-filesystem.ts +++ b/src/main/local-worktree-filesystem.ts @@ -24,21 +24,12 @@ function shouldUseWslFilesystem(options: LocalWorktreeFilesystemOptions): boolea return process.platform === 'win32' && !!options.wslDistro?.trim() } -/** - * Run a filesystem command inside the distro. - * - * Why no login shell: these are coreutils at standard paths plus shell builtins, - * and need nothing from the user's PATH. A login shell would only add its rc/motd - * output to the stdout these callers parse -- the banner problem -- so the fix is - * to not start one rather than to fence what it prints. - */ +// Coreutils need no login shell; rc/motd output would corrupt parsed stdout. async function runWslCommand(distro: string, command: string): Promise { const result = await runProcess({ program: 'wsl.exe', args: buildWslExecArgs(distro, ['sh', '-c', command]), - // Why explicit (#16463): the guest path is inside `command`, so this only - // decides whether CreateProcessW succeeds -- and these calls run while a - // worktree is being removed, which is the cwd an inherited one would be. + // An inherited cwd may be the worktree currently being deleted (#16463). cwd: resolveWslInteropSpawnCwd(), timeoutMs: WSL_FILE_OPERATION_TIMEOUT_MS }) @@ -54,12 +45,7 @@ async function runWslCommand(distro: string, command: string): Promise { return result.stdout } -/** - * Only stat's trailing strerror text is portable: GNU coreutils says `cannot statx`, - * BusyBox says `can't stat`, so matching the verb made a BusyBox distro report a - * successful cleanup as a permanent failure. The tail stays English because the probe - * pins LC_ALL=C, and stat is the only thing in that probe that writes to stderr. - */ +// LC_ALL=C fixes strerror text; match only its tail because GNU and BusyBox use different verbs. const WSL_MISSING_PATH_STDERR = /: (?:No such file or directory|Not a directory)\r?\n?$/ function isWslMissingPathError(error: unknown): boolean { @@ -130,7 +116,6 @@ export async function removeLocalWorktreePath( return } - // Why: WSL-owned worktree directories may be POSIX paths that Node on - // Windows cannot delete safely. Run the deletion inside the selected distro. + // Delete guest paths inside their distro; Windows Node cannot safely remove them. await runWslCommand(distro, `rm -rf -- ${quotePosixShell(toLinuxPath(targetPath))}`) } diff --git a/src/main/local-worktree-metadata-prune-gate.test.ts b/src/main/local-worktree-metadata-prune-gate.test.ts index 9ae5d3b9f8a..a66c0c7492b 100644 --- a/src/main/local-worktree-metadata-prune-gate.test.ts +++ b/src/main/local-worktree-metadata-prune-gate.test.ts @@ -80,7 +80,6 @@ describe('local worktree metadata prune gate', () => { forgetLocalWorktreeMetadataPruneGate('repo-1') expect(isLocalWorktreeMetadataPruneDue('repo-1')).toBe(true) - // The forgotten fingerprint must not later read as a change against a stale entry. recordLocalWorktreeListingForPruneGate('repo-1', ['/a']) markLocalWorktreeMetadataPruneStarted('repo-1') expect(isLocalWorktreeMetadataPruneDue('repo-1')).toBe(false) diff --git a/src/main/local-worktree-metadata-prune-gate.ts b/src/main/local-worktree-metadata-prune-gate.ts index 0daa8a41b9f..0d2e55847fb 100644 --- a/src/main/local-worktree-metadata-prune-gate.ts +++ b/src/main/local-worktree-metadata-prune-gate.ts @@ -1,66 +1,30 @@ -/** - * Decides whether a detected-worktree scan owes the store a metadata-hygiene pass. - * - * The pass captures a prune expectation over the repo's whole metadata table, `stat`s every - * path-missing candidate, then prunes metadata and lineage. That is O(all rows) and destructive, so - * it must not ride a polled read path: on a store whose dangling rows outnumber live worktrees - * ~100:1 it re-derives an answer it already has, forever, pinning the main process in filesystem - * completion callbacks (#17775). Rows pinned by a persisted session are never removable, so the - * repetition cannot even make progress. - * - * Nothing the pass reads changes on its own, so it is gated on evidence rather than a clock: - * - a worktree lifecycle event for the repo (the existing worktree-change invalidator hub), - * - a mutation that can make some row *more* removable (see `invalidate…PruneInputs`), - * - the repo's git listing differing from the one the last pass ran against. - * With none of those, the pass is a provable repeat and is skipped outright. - * - * Why only "more removable" mutations count: the listing path itself writes worktree metadata - * (discovery backfill, host-ownership stamping), so bumping on every metadata write would re-arm - * the gate from the very scan it gates and restore the storm. Additions and updates can only - * preserve more rows, so staleness there is harmless. - * - * The failure direction is deliberate. A signal we miss leaves a dangling row in place until the - * next one arrives — hygiene lags, and nothing is deleted that would not have been deleted anyway. - */ +// Pruning scans all metadata: rerun on removal evidence, never on every listing poll (#17775). +// Ignore additions/updates: discovery writes metadata itself, and retaining extra rows is safe. -/** Bumped by evidence that some row may have become removable; repos re-run the pass once each. */ let pruneInputsGeneration = 0 const observedGenerationByRepo = new Map() const listingFingerprintByRepo = new Map() -/** True until the repo has run a pass against the current generation — so also on the first scan. */ +/** Unseen repos are due on their first scan. */ export function isLocalWorktreeMetadataPruneDue(repoId: string): boolean { return observedGenerationByRepo.get(repoId) !== pruneInputsGeneration } -/** - * Claim the pending pass. Recorded when the expectation is captured rather than when the prune - * finishes: a scan that is invalidated or fails mid-flight must not re-arm the full capture on the - * very next listing poll, and any real change re-bumps the generation anyway. - */ +/** Claim before pruning so failures do not retry on every poll; new evidence rearms the pass. */ export function markLocalWorktreeMetadataPruneStarted(repoId: string): void { observedGenerationByRepo.set(repoId, pruneInputsGeneration) } -/** One repo's worktrees changed (create/remove/rename). */ export function requireLocalWorktreeMetadataPrune(repoId: string): void { observedGenerationByRepo.delete(repoId) } -/** - * Some metadata row may have become removable — a worktree-meta/identity/lineage row was deleted, - * or a session, lease, automation or selection released its claim on a workspace. Repo-agnostic - * because ownership is global state: a session release can unpin a row in any repo. - */ +/** Deletions and released claims can unpin rows in any repo; invalidate globally. */ export function invalidateLocalWorktreeMetadataPruneInputs(): void { pruneInputsGeneration += 1 } -/** - * Backstop for changes no event reported: if Git now lists a different set of worktrees than the - * last pass ran against, that pass's conclusions no longer describe this repo. Costs one join over - * a list we already hold. - */ +/** Changed Git listings catch worktree mutations that emitted no event. */ export function recordLocalWorktreeListingForPruneGate( repoId: string, worktreePaths: readonly string[] @@ -71,14 +35,12 @@ export function recordLocalWorktreeListingForPruneGate( return } listingFingerprintByRepo.set(repoId, fingerprint) - // Why not on the first listing: a repo we have never scanned is already due by default, and the - // scan that produced this listing is the pass that covers it. Re-arming here would double it. + // First listings are already due; rearming would duplicate their pass. if (previous !== undefined) { requireLocalWorktreeMetadataPrune(repoId) } } -/** A deregistered repo leaves no reason to retain its gate state. */ export function forgetLocalWorktreeMetadataPruneGate(repoId: string): void { observedGenerationByRepo.delete(repoId) listingFingerprintByRepo.delete(repoId) @@ -90,8 +52,7 @@ export function __resetLocalWorktreeMetadataPruneGateForTests(): void { listingFingerprintByRepo.clear() } -/** Repo teardown: a full removal retires this repo's gate, and either shape can unpin rows in - * other repos, so the shared inputs are always re-armed. */ +/** Removing either local or remote ownership can unpin metadata in other repos. */ export function retireLocalWorktreeMetadataPruneStateForRepo( repoId: string, hostId: string | null diff --git a/src/main/local-worktree-path-presence.ts b/src/main/local-worktree-path-presence.ts index b6c28cf177b..ed2757168a5 100644 --- a/src/main/local-worktree-path-presence.ts +++ b/src/main/local-worktree-path-presence.ts @@ -3,7 +3,7 @@ import { mapWithConcurrency } from '../shared/map-with-concurrency' import { PrioritySemaphore } from '../shared/priority-semaphore' import { abortSignalWithTimeout } from './lib/abort-signal-with-timeout' -// Why: share one bound across polling callers so slow mounts cannot multiply libuv work. +// Share a concurrency bound across polling callers so slow mounts cannot multiply libuv work. const LOCAL_WORKTREE_PATH_PROBE_CONCURRENCY = 2 export const LOCAL_WORKTREE_PATH_PROBE_TIMEOUT_MS = 2_000 const localWorktreePathProbeSemaphore = new PrioritySemaphore(LOCAL_WORKTREE_PATH_PROBE_CONCURRENCY) diff --git a/src/main/local-worktree-removal-recovery-live.test.ts b/src/main/local-worktree-removal-recovery-live.test.ts index d5301bbb51d..54ab64f9719 100644 --- a/src/main/local-worktree-removal-recovery-live.test.ts +++ b/src/main/local-worktree-removal-recovery-live.test.ts @@ -70,8 +70,7 @@ describe('local Windows worktree removal recovery (live Git)', () => { git(repoPath, ['worktree', 'add', '--quiet', '-b', 'feature/churn', worktreePath]) const head = git(worktreePath, ['rev-parse', 'HEAD']) - // Why: a producer racing Git's recursive delete deterministically exposes - // the Windows ENOTEMPTY state reported by real workspace deletions. + // Race Git’s recursive deletion to reproduce Windows ENOTEMPTY deterministically. const churn = spawn( process.execPath, [ diff --git a/src/main/local-worktree-removal-recovery.ts b/src/main/local-worktree-removal-recovery.ts index bff0d2d02a7..a5d00d207f9 100644 --- a/src/main/local-worktree-removal-recovery.ts +++ b/src/main/local-worktree-removal-recovery.ts @@ -90,8 +90,7 @@ async function removeRequiredGitWorktreeRegistration( removalError = error } - // Why: Git prune exits successfully while retaining locked registrations; - // a failed remove can also have detached the row before filesystem cleanup. + // Successful prune can retain locked registrations; verify the row is gone. try { await verifyGitWorktreeRegistrationRemoved( args.repoPath, @@ -104,26 +103,21 @@ async function removeRequiredGitWorktreeRegistration( ? staleRegistrationRecoveryError(removalError, args.canonicalWorktreePath, forceForError) : verificationError } - // Why: if Git detached the row before reporting its filesystem error, keep - // the branch rather than guessing whether the normal branch cleanup ran. + // Preserve the branch when Git may have detached the row before reporting a filesystem error. return result ?? preservedBranchResult(args.registeredWorktree, args.deleteBranch) } export async function recoverLocalWindowsWorktreeRemoval( args: LocalWindowsRemovalRecoveryArgs ): Promise { - // Why: recovery can recursively delete the remaining directory, so a Git - // lock must reject the attempt before classification or any side effects. + // Reject Git locks before recovery can recursively delete files. assertWorktreeUnlockedForRemoval(args.registeredWorktree) if (!(await isRecoverableWindowsFilesystemRemovalFailure(args))) { return undefined } - // Why: this error means Git accepted removal and started deleting the path; - // finish that partial success with Windows retries, then verify Git metadata. - // Why: Windows recovery recursively deletes the remaining directory, so it - // must fail closed while a watcher process may still own a native handle. + // Release watcher handles before recursively finishing Git’s partial removal; abort if closing fails. await args.closeWatcher(args.canonicalWorktreePath) try { await removeLocalWorktreePath(args.canonicalWorktreePath, args.localWorktreeGitOptions) @@ -142,8 +136,7 @@ async function isRecoverableWindowsFilesystemRemovalFailure( try { const worktrees = await listWorktreesStrict(args.repoPath, args.localWorktreeGitOptions) - // Why: error prose can be localized or ambiguous. Only a missing Git row - // proves removal started and makes recursive Windows cleanup safe. + // Localized error text cannot prove removal started; require the Git row to be absent. return !worktrees.some((worktree) => areWorktreePathsEqual(worktree.path, args.canonicalWorktreePath) ) diff --git a/src/main/local-worktree-scan-generation.ts b/src/main/local-worktree-scan-generation.ts index 30cd736c9f0..5791e379942 100644 --- a/src/main/local-worktree-scan-generation.ts +++ b/src/main/local-worktree-scan-generation.ts @@ -4,16 +4,13 @@ import type { WorktreeCatalogVersion } from '../shared/worktree/catalog-version' const generationByRepoId = new Map() let generationSequence = 0 let mutationRevision = 0 -// Why per process: generations restart with the process, so a client ordering catalog -// publications across a host restart must see the restart as a new catalog, not an older one. +// A new epoch lets clients distinguish a host restart from older catalog publications. const LOCAL_WORKTREE_CATALOG_EPOCH = randomUUID() -/** The catalog version a scan that began at `generation` describes. */ export function localWorktreeCatalogVersionAt(generation: number): WorktreeCatalogVersion { return { epoch: LOCAL_WORKTREE_CATALOG_EPOCH, sequence: generation } } -/** The repo's current catalog version: what a mutation reply issued now describes. */ export function getLocalWorktreeCatalogVersion(repoId: string): WorktreeCatalogVersion { return localWorktreeCatalogVersionAt(getLocalWorktreeScanGeneration(repoId)) } @@ -42,17 +39,7 @@ export function retireLocalWorktreeScanGeneration(repoId: string): void { forgetLocalWorktreeScanGeneration(repoId) } -/** - * Advances on every event above that can change what a worktree scan would find — repo add, - * removal, update, and scan-cache invalidation — and on nothing else. A cache that must not answer - * for repos it never saw compares this in O(1) instead of walking the repo list. - * - * Why not `generationSequence`: that also advances when `getLocalWorktreeScanGeneration` mints a key - * for a repo id nothing has scanned yet, which is a read. Keying a snapshot on it would let a read - * path discard a snapshot that is still perfectly valid. - * - * Ordering-only: the value means nothing outside a same-process comparison. - */ +/** Same-process mutation counter; unlike generationSequence, reads cannot advance it. */ export function getWorktreeScanMutationRevision(): number { return mutationRevision } diff --git a/src/main/localhost-worktree-label-proxy.test.ts b/src/main/localhost-worktree-label-proxy.test.ts index 7d499209847..5c69db7236a 100644 --- a/src/main/localhost-worktree-label-proxy.test.ts +++ b/src/main/localhost-worktree-label-proxy.test.ts @@ -27,8 +27,7 @@ function fetchThroughProxy( ): Promise<{ status: number; body: string; headers: http.IncomingHttpHeaders }> { const url = new URL(labeledUrl) return new Promise((resolve, reject) => { - // Why: *.orca.localhost is not resolvable DNS; connect to the proxy on - // loopback and carry the label through the Host header instead. + // Bypass DNS with loopback; route the worktree label through Host. const request = http.request( { host: '127.0.0.1', @@ -84,8 +83,6 @@ describe('localhost worktree label proxy', () => { const result = await fetchThroughProxy(url) expect(result.status).toBe(200) - // The proxy only relabels the hostname; the page is delivered verbatim - // with no injected favicon/title and the CSP header intact. expect(result.body).toBe('hello') expect(result.headers['content-security-policy']).toBe("default-src 'self'") }) @@ -149,7 +146,6 @@ describe('localhost worktree label proxy', () => { proxy.unregisterWorktree('wt-a') - // The removed worktree's label no longer routes; the other worktree is unaffected. expect((await fetchThroughProxy(a.url)).status).toBe(404) expect((await fetchThroughProxy(b.url)).status).toBe(200) }) diff --git a/src/main/localhost-worktree-label-proxy.ts b/src/main/localhost-worktree-label-proxy.ts index acc1164d1ee..30a64a2e3c7 100644 --- a/src/main/localhost-worktree-label-proxy.ts +++ b/src/main/localhost-worktree-label-proxy.ts @@ -49,9 +49,6 @@ export class LocalhostWorktreeLabelProxy { } } - // Why: routes are added per (worktreeId, targetUrl) but were never removed, so - // labels for deleted worktrees accumulated in both maps for the whole session. - // Drop them when the worktree is torn down. The shared http.Server stays up. unregisterWorktree(worktreeId: string): void { for (const [label, route] of this.routes) { if (route.worktreeId === worktreeId) { @@ -135,22 +132,17 @@ export class LocalhostWorktreeLabelProxy { headers: requestHeadersForTarget(request, route.target) }) - // Why: a client abort or downstream socket error must tear down the - // upstream request instead of surfacing as an uncaught exception/leak. + // Downstream errors must release upstream work rather than leak requests or crash main. request.on('error', () => proxyRequest.destroy()) response.on('error', () => proxyRequest.destroy()) - // Why: the proxy only relabels the hostname; responses are streamed - // through untouched so app headers (CSP, cookies) and bodies are - // preserved exactly as the dev server sent them. proxyRequest.on('response', (proxyResponse) => { proxyResponse.on('error', () => response.destroy()) response.writeHead(proxyResponse.statusCode ?? 502, proxyResponse.headers) proxyResponse.pipe(response) }) proxyRequest.on('error', (error) => { - // Why: once headers/bytes are flushed we can't write a 502, so tear the - // socket down to avoid an ERR_HTTP_HEADERS_SENT crash. + // After headers are sent, destroy the response instead of attempting a 502. if (response.headersSent) { response.destroy(error) return diff --git a/src/main/macos-press-and-hold-default.defaults-domain.test.ts b/src/main/macos-press-and-hold-default.defaults-domain.test.ts index f05db4bcd27..13bc5334058 100644 --- a/src/main/macos-press-and-hold-default.defaults-domain.test.ts +++ b/src/main/macos-press-and-hold-default.defaults-domain.test.ts @@ -15,22 +15,10 @@ import { type PressAndHoldRecord } from './macos-press-and-hold-default' -/** - * Runs against the real `/usr/bin/defaults` on a throwaway Orca-owned domain. - * - * The whole design rests on one claim the mocks cannot make: a domain that has never been written - * is distinguishable from one explicitly set to `false`. Electron's `systemPreferences` cannot tell - * them apart, so if `defaults` could not either, "only write when unset" would be unimplementable. - * - * Every domain used here is a throwaway UUID under Orca's own prefix, deleted along with its plist - * in `afterEach`; the real `com.stablyai.orca` domain is never read or written. - */ +// Verify real defaults distinguishes absence from false using disposable Orca-prefixed domains. -// Why a real Orca-owned domain shape: the ownership guard rejects anything else, so a fake prefix -// would exercise a different branch than production. const domains: string[] = [] -/** Pinned against the real binary below, then reused by the platform-agnostic unit tests. */ const DEFAULTS_MISSING_EXIT_CODE = 1 function throwawayDomain(): string { @@ -71,11 +59,8 @@ describe.skipIf(process.platform !== 'darwin')( for (const domain of domains.splice(0)) { try { execFileSync('/usr/bin/defaults', ['delete', domain], { stdio: 'ignore' }) - } catch { - // The domain may never have been created; nothing to clean up. - } - // Why the file too: `defaults delete` empties the domain but leaves its plist behind, so - // without this every run litters the developer's ~/Library/Preferences. + } catch {} + // defaults delete empties the domain but leaves its plist behind. rmSync(join(homedir(), 'Library', 'Preferences', `${domain}.plist`), { force: true }) } }) @@ -116,8 +101,6 @@ describe.skipIf(process.platform !== 'darwin')( }) it('refuses to call a failed probe "unset"', () => { - // Real runs, not hand-built results: a binary that is not there must come back 'unknown' - // through the same path production takes, not be mistaken for "the key does not exist". const spawnFailure = (): ReturnType => runProcessSync({ program: '/usr/bin/defaults-does-not-exist', @@ -132,16 +115,13 @@ describe.skipIf(process.platform !== 'darwin')( args: ['read', throwawayDomain(), PRESS_AND_HOLD_KEY], stdio: ['ignore', 'ignore', 'ignore'] }) - // The one exit code the whole design reads as unset; every other outcome is 'unknown'. expect(missingKey.code).toBe(DEFAULTS_MISSING_EXIT_CODE) expect(missingKey.timedOut).toBe(false) expect(interpretDefaultsRead(() => missingKey)).toBe('unset') }) - // Skipped as root, where the read-only directory below would still be writable. + // Root could still write the read-only fixture directory. it.skipIf(process.getuid?.() === 0)('reports a write the binary refused', () => { - // Why a real refusal: `defaults write` exits non-zero instead of throwing, so a caller that - // only caught exceptions would record 'applied' for a value that never reached the plist. const readOnly = join(mkdtempSync(join(tmpdir(), 'orca-press-hold-ro-')), 'locked') mkdirSync(readOnly) chmodSync(readOnly, 0o500) @@ -159,7 +139,6 @@ describe.skipIf(process.platform !== 'darwin')( execFileSync('/usr/bin/defaults', ['delete', domain, PRESS_AND_HOLD_KEY], { stdio: 'ignore' }) expect(readDomainPressAndHoldPreference(domain)).toBe('unset') - // A launch that already decided must not rewrite it, which is what makes the delete stick. const decided: PressAndHoldRecord = { version: 1, decision: 'applied', diff --git a/src/main/macos-press-and-hold-default.test.ts b/src/main/macos-press-and-hold-default.test.ts index 29cc0657ec9..fb90c5db47c 100644 --- a/src/main/macos-press-and-hold-default.test.ts +++ b/src/main/macos-press-and-hold-default.test.ts @@ -18,7 +18,6 @@ const ORCA_DOMAIN = 'com.stablyai.orca' type HostOverrides = Partial & { record?: PressAndHoldRecord | null } -/** A macOS host whose domain has no explicit value — the state every affected user is in. */ function createHost(overrides: HostOverrides = {}): { host: PressAndHoldHost writes: { domain: string; value: boolean }[] @@ -71,8 +70,6 @@ describe('ensureMacPressAndHoldDefault', () => { }) expect(ensureMacPressAndHoldDefault(host)).toBe('not-macos') - // Why assert the reads too: "inert" means no `defaults` subprocess and no userData I/O, - // not merely no write. expect(probe).not.toHaveBeenCalled() expect(readRecord).not.toHaveBeenCalled() expect(writes).toEqual([]) @@ -83,9 +80,7 @@ describe('ensureMacPressAndHoldDefault', () => { describe('respects an explicit user setting', () => { it('leaves the domain alone when the user already set a value', () => { - // One arm covers both `true` and `false`: the probe deliberately reports only presence, so - // whichever way the user's value points it is theirs. That the probe really does report - // presence for both is pinned against the real binary in the .defaults-domain test. + // The real-binary suite verifies that both true and false report set. const { host, writes, records } = createHost({ readDomainPreference: () => 'set' }) expect(ensureMacPressAndHoldDefault(host)).toBe('kept-user-preference') @@ -108,8 +103,7 @@ describe('ensureMacPressAndHoldDefault', () => { }) it('stays out of the way after the user deletes the key we wrote', () => { - // The documented way back to the accent picker is `defaults delete`, which returns the - // domain to "unset". Without the recorded decision the next launch would rewrite it. + // Deleting the preference restores the accent picker; the recorded decision must prevent rewriting it. const { host, writes } = createHost({ record: terminalRecord('applied'), readDomainPreference: () => 'unset' @@ -145,7 +139,6 @@ describe('ensureMacPressAndHoldDefault', () => { expect(isOrcaPreferencesDomain('com.stablyai.orca')).toBe(true) expect(isOrcaPreferencesDomain('com.stablyai.orca.dev')).toBe(true) expect(isOrcaPreferencesDomain('com.github.Electron')).toBe(false) - // Why: a prefix test without the dot would accept a lookalike bundle id. expect(isOrcaPreferencesDomain('com.stablyai.orcafake')).toBe(false) }) }) @@ -182,11 +175,6 @@ describe('ensureMacPressAndHoldDefault', () => { }) }) -/** - * Why here and not only in the .defaults-domain file: that file is macOS-only and CI has no macOS - * runner, so this three-way rule would otherwise be enforced nowhere in CI. These cases need no - * `defaults` binary — the sibling file pins that the real binary produces them. - */ describe('interpretDefaultsRead', () => { function probe(overrides: Partial): () => ProcessResult { return () => ({ code: 0, signal: null, stdout: '', stderr: '', timedOut: false, ...overrides }) @@ -201,7 +189,6 @@ describe('interpretDefaultsRead', () => { }) describe('refuses to call a failed probe unset', () => { - // Each of these would otherwise overwrite a value the user deliberately set. const failures: [string, () => ProcessResult][] = [ ['killed before it exited', probe({ code: null, signal: 'SIGTERM' })], ['timed out', probe({ code: null, timedOut: true })], @@ -285,8 +272,7 @@ describe('startup wiring', () => { expect(callIndex).toBeGreaterThanOrEqual(0) expect(readyIndex).toBeGreaterThanOrEqual(0) expect(preflightCall).toBeGreaterThanOrEqual(0) - // Why after initDataPath: the record lives beside orca-data.json, and the canonical userData - // path is only captured there. + // initDataPath captures the canonical userData path where the decision record belongs. expect(callIndex).toBeGreaterThan(initDataPathIndex) expect(preflightCall).toBeLessThan(readyIndex) }) diff --git a/src/main/macos-press-and-hold-default.ts b/src/main/macos-press-and-hold-default.ts index 63123ae5f7b..d597f855615 100644 --- a/src/main/macos-press-and-hold-default.ts +++ b/src/main/macos-press-and-hold-default.ts @@ -4,64 +4,32 @@ import { runProcessSync } from '@orca/process-host' import type { ProcessResult } from '@orca/process-host/process-spec' import { writeFileAtomically } from './codex-accounts/fs-utils' -/** - * Turns off the macOS accent picker for Orca's own preferences domain (#14746). - * - * macOS routes press-and-hold to the accent popup unless an app opts out via - * `ApplePressAndHoldEnabled`, so holding `j` in vim inserts one character instead of repeating. - * The key is unset by default, which is why every terminal-hosting Mac app ships this opt-out. - * - * Written once and never again: a user who wants the accent picker back sets - * `defaults write com.stablyai.orca ApplePressAndHoldEnabled -bool true` (or deletes the key), and - * the recorded decision below keeps a later launch from overwriting that choice. - * - * A fresh write is assumed to land for the *next* launch, not the current one: it goes out through - * a separate `defaults` process, so this app's own cached copy may not observe it. Whether AppKit - * re-reads sooner is untested, so the conservative assumption is the one encoded here. - * - * If this is ever reverted, delete the key as well — AppKit reads the plist, not this file, so - * removing the code alone leaves press-and-hold disabled forever for everyone who ran an affected - * build, with nothing left in the tree to explain it. - * - * The `defaults`-binary behaviour this relies on is pinned by the sibling test file, which runs - * only on a developer Mac: CI has no macOS test runner, so those tests never execute there. - */ +// Opt out of the accent picker once so terminal keys repeat (#14746); later user choices win. +// The separate defaults process may affect only the next launch. Reverting this also requires deleting the key. export const PRESS_AND_HOLD_KEY = 'ApplePressAndHoldEnabled' export const PRESS_AND_HOLD_RECORD_FILE = 'macos-press-and-hold-default.json' export const PRESS_AND_HOLD_RECORD_VERSION = 1 -/** Why not `systemPreferences.getUserDefault`: it reads through the whole NSUserDefaults search - * list and is typed non-nullable, so an unset key and an explicit `false` both come back `false` — - * and the system default is unset, so it reports `false` on a Mac where press-and-hold is on. - * `defaults read ` is domain-scoped and exits 1 when the key is absent. */ +// defaults read distinguishes an absent key from explicit false; systemPreferences.getUserDefault cannot. const DEFAULTS_BINARY = '/usr/bin/defaults' -/** A hang cutoff, not a latency budget: the probe measures ~6ms, and cutting a slow-but-working - * `defaults` short costs the user the feature for that launch. The shared 30s default is what - * must not apply — this runs before `ready`, so a 30s block is a hung app. */ +// This runs before ready; bound a wedged defaults process more tightly than the shared 30s timeout. const DEFAULTS_TIMEOUT_MS = 5_000 -/** Why: `defaults` exits 1 for "does not exist"; anything else means the probe itself failed. */ +// defaults uses exit 1 for an absent key; other failures leave its presence unknown. const DEFAULTS_MISSING_STATUS = 1 const ORCA_BUNDLE_ID = 'com.stablyai.orca' export type PressAndHoldDecision = - /** Not macOS — nothing is read or written. */ | 'not-macos' - /** A previous launch already decided; the domain is never touched again. */ | 'already-decided' - /** The running bundle is not Orca's (e.g. a bare `Electron.app`), whose domain we do not own. */ | 'foreign-bundle' - /** `defaults read` could not answer, so we cannot tell an unset key from a user's choice. */ | 'probe-failed' - /** `defaults write` failed. */ | 'write-failed' - /** The domain already carries an explicit value; the user's choice stands. */ | 'kept-user-preference' - /** The key was unset and we wrote `false`. */ | 'applied' -/** Decisions about the domain itself. Anything else is a condition that can change by next launch. */ +// Transient failures are retried on later launches; completed decisions preserve user changes. const TERMINAL_DECISIONS = new Set(['applied', 'kept-user-preference']) export type PressAndHoldRecord = { @@ -81,13 +49,11 @@ export type PressAndHoldHost = { now: () => string } -/** Only Orca's own bundle: an unpackaged run is `com.github.Electron`, shared with every other - * unpackaged Electron app on the machine. */ +// Unpackaged Electron apps share com.github.Electron; do not change that domain. export function isOrcaPreferencesDomain(domain: string): boolean { return domain === ORCA_BUNDLE_ID || domain.startsWith(`${ORCA_BUNDLE_ID}.`) } -/** `/Contents/MacOS/` → `/Contents/Info.plist`. */ export function readBundleIdentifierFromExecutablePath(execPath: string): string | null { try { const plist = readFileSync(join(dirname(dirname(execPath)), 'Info.plist'), 'utf8') @@ -99,20 +65,15 @@ export function readBundleIdentifierFromExecutablePath(execPath: string): string } } -/** - * Why this distinction carries the whole design: only a real "does not exist" answer may be read as - * unset. A killed or timed-out probe never answered, and calling that unset would let a broken - * probe overwrite a value the user chose. - */ +/** Failed probes cannot establish absence and must never authorize overwriting a preference. */ export function interpretDefaultsRead(probe: () => ProcessResult): 'set' | 'unset' | 'unknown' { let result: ProcessResult try { result = probe() } catch { - // `runProcessSync` throws only when the child never started, which answers nothing at all. return 'unknown' } - // Why before the code: a timeout kills mid-run, so any exit it leaves behind is not an answer. + // A timeout invalidates any exit status from the interrupted probe. if (result.timedOut) { return 'unknown' } @@ -123,8 +84,7 @@ export function interpretDefaultsRead(probe: () => ProcessResult): 'set' | 'unse } export function readDomainPressAndHoldPreference(domain: string): 'set' | 'unset' | 'unknown' { - // Why no captured output: the exit code is the whole answer, and reading only presence is what - // makes an explicit `true` and an explicit `false` both count as the user's. + // Presence alone preserves both explicit true and explicit false as user choices. return interpretDefaultsRead(() => runProcessSync({ program: DEFAULTS_BINARY, @@ -137,8 +97,6 @@ export function readDomainPressAndHoldPreference(domain: string): 'set' | 'unset export function writeDomainPressAndHoldPreference(domain: string, value: boolean): boolean { try { - // Why the code check: a non-zero `defaults write` returns rather than throwing, so treating a - // completed run as success would record 'applied' for a write that never landed. return ( runProcessSync({ program: DEFAULTS_BINARY, @@ -169,13 +127,7 @@ function parseRecord(raw: string): PressAndHoldRecord | null { } } -/** - * Apply Orca's press-and-hold default at most once, leaving any explicit user value alone. - * - * Returns the decision so startup can log it; the same value is persisted for support triage. - */ export function ensureMacPressAndHoldDefault(host: PressAndHoldHost): PressAndHoldDecision { - // Why first and without touching disk: Windows and Linux must do no work at all here. if (host.platform !== 'darwin') { return 'not-macos' } @@ -186,7 +138,7 @@ export function ensureMacPressAndHoldDefault(host: PressAndHoldHost): PressAndHo } const record = (decision: PressAndHoldDecision, domain: string | null): PressAndHoldDecision => { - // Why skip an unchanged rewrite: the non-terminal decisions are re-evaluated every launch. + // Transient decisions recur on every launch; persist only changed results. if (!previous || previous.decision !== decision || previous.domain !== domain) { host.writeRecord({ version: PRESS_AND_HOLD_RECORD_VERSION, @@ -216,15 +168,12 @@ export function ensureMacPressAndHoldDefault(host: PressAndHoldHost): PressAndHo return record('applied', domain) } -/** Why log: silently rewriting a macOS default for the user's login account should leave a trace, - * and the failure paths are otherwise invisible. The quiet decisions are the steady state. */ const REPORTED_DECISIONS: Partial> = { applied: `set ${PRESS_AND_HOLD_KEY}=false so held keys repeat; takes effect next launch`, 'probe-failed': `could not read ${PRESS_AND_HOLD_KEY}; leaving it alone`, 'write-failed': `could not write ${PRESS_AND_HOLD_KEY}; held keys will not repeat` } -/** Wires {@link ensureMacPressAndHoldDefault} to the real bundle, `defaults`, and userData. */ export function applyMacPressAndHoldDefaultAtStartup(userDataPath: string): PressAndHoldDecision { const recordPath = pressAndHoldRecordPath(userDataPath) const decision = ensureMacPressAndHoldDefault({ @@ -239,11 +188,11 @@ export function applyMacPressAndHoldDefaultAtStartup(userDataPath: string): Pres }, writeRecord: (record) => { try { - // Why: this runs before `ready`, and Electron has not necessarily created userData yet. + // Before ready, Electron may not have created userData yet. mkdirSync(userDataPath, { recursive: true }) writeFileAtomically(recordPath, `${JSON.stringify(record, null, 2)}\n`) } catch { - // Best-effort: an unwritten record only costs one repeated probe next launch. + // If persistence fails, a later launch safely probes the preference again. } }, readDomainPreference: readDomainPressAndHoldPreference, diff --git a/src/main/macos-tcc-log-stream-orphans.test.ts b/src/main/macos-tcc-log-stream-orphans.test.ts index a5c8f2ab7fd..71270c1dc13 100644 --- a/src/main/macos-tcc-log-stream-orphans.test.ts +++ b/src/main/macos-tcc-log-stream-orphans.test.ts @@ -30,7 +30,7 @@ describe('findOrphanedTccLogStreams', () => { row(11, 4321), row(12, 1, '/usr/bin/log stream --predicate subsystem == "com.apple.TCC" --style compact'), row(13, 1, '/usr/bin/log show --last 1m'), - // A pid recycled between the args and comm sweeps, or an unreadable comm. + // Simulate PID reuse between argv and executable snapshots, plus unreadable executable identity. row(14, 1, LEGACY_ARGS, '/usr/bin/yes'), row(15, 1, LEGACY_ARGS, '') ]).map((match) => match.pid) diff --git a/src/main/macos-tcc-log-stream-orphans.ts b/src/main/macos-tcc-log-stream-orphans.ts index 8503fab6354..ee2e38eefba 100644 --- a/src/main/macos-tcc-log-stream-orphans.ts +++ b/src/main/macos-tcc-log-stream-orphans.ts @@ -7,31 +7,25 @@ import { } from './opencode/opencode-client-sweep' import { LOG_PREDICATE } from './macos-tcc-prompt-watch' -/** The exact argv that Orca builds before the sh wrapper spawned `log stream` directly. */ +// Match legacy builds that spawned log stream without a lifetime wrapper. const LEGACY_LOG_STREAM_COMMAND = splitCommandLineArgv( ['/usr/bin/log', 'stream', '--predicate', LOG_PREDICATE, '--info', '--style', 'compact'].join(' ') ).join(' ') -// Why: `etime` has 1 s resolution and two reads of it can straddle a tick. +// etime has one-second resolution; separate snapshots may straddle a tick. const START_TIME_TOLERANCE_MS = 2_000 function isLegacyOrphan(row: ProcessIdentityRow): boolean { return row.ppid === 1 && row.argv.join(' ') === LEGACY_LOG_STREAM_COMMAND } -/** - * Why: older builds left this `log stream` running under launchd whenever Orca died - * uncleanly, and those orphans outlive every later launch. A live watcher's - * `log stream` is parented by its wrapper, so only launchd's children (ppid 1) match. - * `executable` comes from a later `ps` than argv, so a pid recycled between them fails. - */ +/** Only legacy streams reparented to launchd qualify; live streams belong to their wrapper. */ export function findOrphanedTccLogStreams( rows: readonly ProcessIdentityRow[] ): ProcessIdentityRow[] { return rows.filter((row) => row.executable === '/usr/bin/log' && isLegacyOrphan(row)) } -/** One-snapshot re-read of a single pid, so its ppid, start time and argv agree. */ async function readProcessIdentity(pid: number): Promise { const result = await runProcess({ program: 'ps', @@ -59,8 +53,7 @@ export async function reclaimOrphanedTccLogStreams( await (deps.sweep ?? sweepProcessIdentities)() )) { try { - // Why: the sweep joins two `ps` snapshots, so re-read right before signalling - // and skip anything whose identity changed or can no longer be confirmed. + // Recheck identity immediately before signalling; the sweep joins separate ps snapshots. const current = await readProcess(candidate.pid) if ( !current || @@ -71,9 +64,7 @@ export async function reclaimOrphanedTccLogStreams( } kill(candidate.pid, 'SIGTERM') reclaimed++ - } catch { - // Already gone, or another user's process. - } + } catch {} } if (reclaimed > 0) { console.info(`[tcc-prompt-watch] reclaimed ${reclaimed} orphaned log stream process(es)`) diff --git a/src/main/macos-tcc-prompt-notice.ts b/src/main/macos-tcc-prompt-notice.ts index 595b8fe9363..7f282dcebda 100644 --- a/src/main/macos-tcc-prompt-notice.ts +++ b/src/main/macos-tcc-prompt-notice.ts @@ -6,19 +6,12 @@ import { getCanonicalUserDataPath } from './persistence' import { MacosTccPromptWatch } from './macos-tcc-prompt-watch' import { reclaimOrphanedTccLogStreams } from './macos-tcc-log-stream-orphans' -/** - * Surfaces Full Disk Access guidance only to users macOS is actually prompting - * (#9756), instead of nudging every Mac user. Counts Orca-attributed TCC - * dialogs across launches and tells the renderer when the first one lands. - */ - -/** Why: the first detected dialog identifies an affected user; the notice remains one-time. */ export const TCC_PROMPT_NOTICE_THRESHOLD = 1 -/** Why: re-arm users who may have trusted the former false-positive Full Disk Access badge. */ +// Re-arm users who may have trusted the former false-positive Full Disk Access badge. export const TCC_PROMPT_NOTICE_VERSION = 2 -/** Why: preserve detection when Electron never emits `ready-to-show` without competing with startup. */ +// Fall back when ready-to-show never fires, while keeping detection out of startup’s critical path. export const TCC_PROMPT_WATCH_START_FALLBACK_MS = 10_000 export const TCC_PROMPT_NOTICE_CHANNEL = 'macosTccPrompts:threshold' @@ -77,7 +70,7 @@ function loadTally(): TccPromptTally { return { ...EMPTY_TALLY } } if (typeof parsed.acknowledgedAfterClose !== 'boolean') { - // Why: an incomplete tally proves only a past prompt, not that access is still missing. + // An incomplete tally proves a past prompt, not that access is still missing. return { ...EMPTY_TALLY } } const acknowledgedAfterClose = parsed.acknowledgedAfterClose === true @@ -97,7 +90,7 @@ function saveTally(): void { try { writeFileAtomically(tallyPath(), `${JSON.stringify(tally, null, 2)}\n`) } catch { - // Best-effort: losing the count only means the notice arrives a launch later. + // Failed persistence only delays the notice until another prompt. } } @@ -154,7 +147,6 @@ export function releasePendingTccPromptNotice(ownerToken: number, claimId?: numb } } -/** Permanently stops the notice for this user; the watcher shuts down with it. */ export function dismissTccPromptNotice(): void { pendingClaim = null tally = { ...tally, dismissed: true, notified: true, acknowledgedAfterClose: true } @@ -178,7 +170,7 @@ function sendTccPromptNotice(mainWindow: BrowserWindow, payload: TccPromptNotice try { mainWindow.webContents.send(TCC_PROMPT_NOTICE_CHANNEL, payload) } catch { - // Why: the durable renderer pull recovers a send lost during renderer teardown. + // The durable renderer pull recovers sends lost during teardown. } } @@ -227,7 +219,7 @@ export function initTccPromptNotice( } if (!orphanReclaimScheduled) { orphanReclaimScheduled = true - // Why: runs even after the notice is dismissed, since older builds' orphans are still alive. + // Legacy orphan cleanup remains necessary after the notice is dismissed. setTimeout( () => void reclaimOrphanedTccLogStreams(), TCC_PROMPT_WATCH_START_FALLBACK_MS @@ -263,7 +255,7 @@ export function initTccPromptNotice( if (target) { sendTccPromptNotice(target, payload) } - // Why: pending state is renderer-acknowledged, so the log child can stop at threshold. + // The persisted pending notice survives until renderer acknowledgement, so logging can stop now. stopTccPromptNotice() } }) diff --git a/src/main/macos-tcc-prompt-watch.test.ts b/src/main/macos-tcc-prompt-watch.test.ts index 025cbc843f4..f90491c3f16 100644 --- a/src/main/macos-tcc-prompt-watch.test.ts +++ b/src/main/macos-tcc-prompt-watch.test.ts @@ -8,12 +8,11 @@ import { parseTccPromptEvent } from './macos-tcc-prompt-watch' -// Captured verbatim from `log stream --predicate 'subsystem == "com.apple.TCC"'` -// on macOS 26.5 while a real consent dialog was displayed and denied. +// Captured from log stream on macOS 26.5 while displaying and denying a real consent dialog. const REAL_PROMPT_LINE = '2026-07-27 15:35:26.136 Df tccd[79149:c81551c] [com.apple.TCC:access] AUTHREQ_PROMPTING: msgID=80871.81, service=kTCCServiceSystemPolicyDocumentsFolder, subject=Sub:{com.orca.tccprobe.shapecapture}Resp:{TCCDProcess: identifier=com.orca.tccprobe.shapecapture, pid=74171, auid=501, euid=501, binary_path=/private/tmp/tccprobe/TccProbe.app/Contents/MacOS/TccProbe},' -// Same shape, but the #9756 case: an agent CLI accesses, Orca is held responsible. +// Derived fixture: an agent accesses files while Orca is held responsible (#9756). const ORCA_APPDATA_LINE = '2026-07-27 15:40:02.001 Df tccd[79149:c81551c] [com.apple.TCC:access] AUTHREQ_PROMPTING: msgID=80871.99, service=kTCCServiceSystemPolicyAppData, subject=Sub:{node-5555494487fbc7467d473fd8b0a397018cbf954b}Resp:{TCCDProcess: identifier=com.stablyai.orca, pid=47548, auid=501, euid=501, binary_path=/opt/homebrew/Cellar/node/26.5.0/bin/node},' @@ -33,7 +32,6 @@ describe('parseTccPromptEvent', () => { it('separates the accessing binary from the responsible app', () => { const event = parseTccPromptEvent(ORCA_APPDATA_LINE) - // The whole point of #9756: the dialog says Orca, but node did the access. expect(event?.responsibleIdentifier).toBe('com.stablyai.orca') expect(event?.accessingIdentifier).toBe('node-5555494487fbc7467d473fd8b0a397018cbf954b') expect(event?.binaryPath).toBe('/opt/homebrew/Cellar/node/26.5.0/bin/node') @@ -43,7 +41,6 @@ describe('parseTccPromptEvent', () => { expect(parseTccPromptEvent(PREFLIGHT_LINE)).toBeNull() expect(parseTccPromptEvent('Filtering the log data using "subsystem == ..."')).toBeNull() expect(parseTccPromptEvent('')).toBeNull() - // Has the marker but no parseable identities — must not yield a partial event. expect(parseTccPromptEvent('AUTHREQ_PROMPTING: msgID=1.2,')).toBeNull() }) }) @@ -111,9 +108,7 @@ function createFakeLogStream(): { return { child, stdin, stdout, killed } } -// Why: the watcher is darwin-gated, so CI (Linux) would silently no-op every -// assertion below unless the platform is pinned. The gate itself is covered by -// the non-darwin case at the end of this block. +// Pin Darwin to exercise the watcher on every host; test the platform guard separately. const originalPlatform = Object.getOwnPropertyDescriptor(process, 'platform') function setPlatform(value: NodeJS.Platform): void { @@ -167,7 +162,7 @@ describe('MacosTccPromptWatch', () => { watch.start() watch.stop() expect(killed).toEqual(['SIGTERM']) - // Why: closing the parent-death pipe is the backstop if the wrapper missed the signal. + // Closing stdin is the parent-death backstop if the wrapper missed SIGTERM. expect(stdin.destroyed).toBe(true) }) @@ -239,7 +234,6 @@ describe('MacosTccPromptWatch', () => { }) afterStop.stop() afterStop.start() - // Why: quit ordering can call stop() before start(); it must not leave a live child behind. expect(afterStop['child']).toBeNull() }) }) diff --git a/src/main/macos-tcc-prompt-watch.ts b/src/main/macos-tcc-prompt-watch.ts index 95d65a99e31..11906b86a02 100644 --- a/src/main/macos-tcc-prompt-watch.ts +++ b/src/main/macos-tcc-prompt-watch.ts @@ -2,22 +2,12 @@ import { spawn, type ChildProcessByStdio } from 'node:child_process' import { createInterface, type Interface } from 'node:readline' import type { Readable, Writable } from 'node:stream' -/** Why: stdin is a never-written pipe whose EOF tells the wrapper that Orca died. */ +/** Stdin EOF tells the wrapper that Orca died. */ export type LogStreamChild = ChildProcessByStdio -/** - * Counts the macOS TCC consent dialogs that name Orca as the responsible - * process (#9756). Terminal children — agent CLIs and anything else the user - * runs — perform the access, but TCC walks the responsibility chain back to - * Orca and puts Orca's name on the dialog, so users read it as Orca snooping. - * - * tccd emits one `AUTHREQ_PROMPTING` line per dialog it actually displays, - * carrying the service and both identities, so this never has to correlate - * across lines or guess whether a dialog was shown. Routine preflight checks - * (the overwhelming majority of TCC log traffic) do not emit it. - */ +// AUTHREQ_PROMPTING identifies actual dialogs, including access attributed to Orca’s terminal children (#9756). -/** Why: terminals run from the detached helper, which TCC can hold responsible independently. */ +// TCC can attribute terminal access to the detached helper independently of the main app. const ORCA_RESPONSIBLE_IDENTIFIERS = new Set([ 'com.stablyai.orca', 'com.stablyai.orca.helper', @@ -27,7 +17,6 @@ const ORCA_RESPONSIBLE_IDENTIFIERS = new Set([ 'com.stablyai.orca.local.helper' ]) -/** Why: the prompt classes #9756 is about — other-apps' data plus the protected home folders agents sweep. */ const WATCHED_SERVICES = new Set([ 'kTCCServiceSystemPolicyAppData', 'kTCCServiceSystemPolicyAllFiles', @@ -40,20 +29,14 @@ export const LOG_PREDICATE = 'subsystem == "com.apple.TCC" AND eventMessage CONTAINS "AUTHREQ_PROMPTING"' export type TccPromptEvent = { - /** TCC service the dialog was raised for, e.g. `kTCCServiceSystemPolicyAppData`. */ service: string - /** Bundle id or hashed identity of the process performing the access (often an agent CLI). */ + /** Bundle ID or hashed identity of the process performing the access. */ accessingIdentifier: string - /** Bundle id macOS holds responsible, and therefore names in the dialog. */ + /** Bundle ID named as responsible in the dialog. */ responsibleIdentifier: string - /** Absolute path of the accessing binary, when tccd reports one. */ binaryPath?: string } -/** - * Parses one `AUTHREQ_PROMPTING` log line. Returns null for anything else, so - * callers can feed the raw stream (which includes a header line) straight in. - */ export function parseTccPromptEvent(line: string): TccPromptEvent | null { if (!line.includes('AUTHREQ_PROMPTING')) { return null @@ -73,7 +56,6 @@ export function parseTccPromptEvent(line: string): TccPromptEvent | null { } } -/** True when this dialog is one macOS raised in Orca's name for a watched file-access service. */ export function isOrcaAttributedPrompt(event: TccPromptEvent): boolean { return ( ORCA_RESPONSIBLE_IDENTIFIERS.has(event.responsibleIdentifier) && @@ -83,18 +65,13 @@ export function isOrcaAttributedPrompt(event: TccPromptEvent): boolean { export type TccPromptWatchOptions = { onPrompt: (event: TccPromptEvent) => void - /** Injected in tests; defaults to spawning `log stream`. */ spawnLogStream?: () => LogStreamChild - /** Injected in tests; production waits before its single recovery attempt. */ restartDelayMs?: number } -// Why: `log stream` never notices its parent dying (it only writes on a rare -// match), so a crashed or SIGKILLed Orca orphaned it forever. The wrapper kills -// it when our stdin pipe hits EOF, and exits when it exits. fd 3 is needed -// because async lists in a non-interactive sh get stdin=/dev/null. The trap is -// armed before the fork; `term` covers a signal that lands before `log_pid` is set. -// `set +eu` because an inherited SHELLOPTS=errexit would end the reader on read's EOF status. +// log stream ignores parent death; stdin EOF makes the wrapper terminate it. +// Preserve stdin on fd 3 because sh backgrounds async lists with /dev/null; arm the trap before forking. +// Disable inherited errexit/nounset so EOF and early signals still reach cleanup. const LOG_STREAM_WRAPPER_SCRIPT = `set +eu exec 3<&0 log_pid= @@ -111,17 +88,13 @@ kill "$reader_pid" 2>/dev/null exit "$status"` function spawnDefaultLogStream(): LogStreamChild { - // Why: the predicate is evaluated inside the logging system, so this delivers - // ~1 line per real dialog instead of the ~30/s the TCC subsystem emits raw. + // Filter within logd to avoid streaming routine TCC traffic into main. return spawn('/bin/sh', ['-c', LOG_STREAM_WRAPPER_SCRIPT, 'sh', LOG_PREDICATE], { stdio: ['pipe', 'pipe', 'pipe'] }) } -/** - * Watches for Orca-attributed TCC dialogs. macOS-only; `start()` is a no-op - * elsewhere so callers don't need their own platform guard. - */ +/** start() is a no-op outside macOS. */ export class MacosTccPromptWatch { private child: LogStreamChild | null = null private reader: Interface | null = null @@ -140,8 +113,7 @@ export class MacosTccPromptWatch { try { child = spawnLogStream() } catch { - // Why: diagnostics only — a missing or restricted `log` binary must never - // break startup, and there is nothing actionable to tell the user. + // Optional diagnostics must not prevent startup if log is unavailable. return } this.child = child @@ -158,13 +130,12 @@ export class MacosTccPromptWatch { this.reader?.close() this.reader = null this.child = null - // Why: EOF on stdin makes any surviving wrapper reader kill its log stream. + // EOF makes any surviving wrapper reader terminate its log stream. child.stdin.destroy() if (this.stopped || this.restartAttempted) { return } this.restartAttempted = true - // Why: recover one transient logd failure without respawning forever when logging is unavailable. this.restartTimer = setTimeout(() => { this.restartTimer = null this.start() @@ -187,7 +158,7 @@ export class MacosTccPromptWatch { } this.reader?.close() this.reader = null - // Why: log stream ignores a closed stdout, so the child needs an explicit kill or it outlives quit. + // Closing stdout does not stop log stream; terminate it explicitly. this.child?.kill('SIGTERM') this.child?.stdin.destroy() this.child = null diff --git a/src/main/macos-tcc-reset.ts b/src/main/macos-tcc-reset.ts index 65b1ea34e77..cc82c4c1889 100644 --- a/src/main/macos-tcc-reset.ts +++ b/src/main/macos-tcc-reset.ts @@ -1,20 +1,12 @@ -// Clearing a macOS TCC row, plus the bundle id every row is keyed by. Shared because two remedies -// must issue the identical `tccutil reset`: the computer-use helper's stale-grant reset and the -// daemon folder-access fix (STA-7948), where clearing the row is what makes macOS ask again. - import { join } from 'node:path' import { runProcess } from '@orca/process-host' import type { ProcessResult } from '@orca/process-host/process-spec' -/** Bounded so a wedged helper cannot hold a caller: neither binary prompts, so neither lingers. */ const TCC_COMMAND_TIMEOUT_MS = 10_000 export type MacosTccResetResult = { ok: true } | { ok: false; detail: string } -/** - * The bundle's `CFBundleIdentifier`, or null when it cannot be read — Info.plist is usually a - * binary plist, so PlistBuddy is the only reader that works on both encodings. - */ +/** PlistBuddy handles both binary and XML Info.plist encodings. */ export async function readMacosBundleId(appBundlePath: string): Promise { try { const result = await runProcess({ @@ -31,13 +23,7 @@ export async function readMacosBundleId(appBundlePath: string): Promise { - // APFS clone first; a failed clone can leave a partial tree on other filesystems. - // Clearing also keeps `cp -R` from nesting `source` inside an existing `destination`. + // Clear partial clones before fallback, and prevent cp -R from nesting into an existing destination. for (const flags of ['-cR', '-R']) { await rm(destination, { recursive: true, force: true }) - // Why no timeout: a large tree at background I/O priority can outlast the 30 s default. + // Large trees copied at background I/O priority can exceed the default timeout. const result = await runProcess({ program: '/bin/cp', args: [flags, source, destination], diff --git a/src/main/orca-chromium-process-pids.ts b/src/main/orca-chromium-process-pids.ts index 3282f3a4d66..710ad10580c 100644 --- a/src/main/orca-chromium-process-pids.ts +++ b/src/main/orca-chromium-process-pids.ts @@ -2,38 +2,9 @@ import { getAppEnvironment, hasAppEnvironment } from '../shared/app-environment' import { recordCoalescedDurableCrashBreadcrumb } from './crash-reporting/durable-crash-breadcrumb' /** - * PIDs of Orca's own Chromium processes — browser, renderers, GPU, utilities. - * - * Why: `taskkill /T /F` aimed at one of these kills a renderer we depend on, and - * the `render-process-gone` it produces is indistinguishable from an external - * kill in every field Orca records (#10680). A pid in this set is proof the - * target is ours to keep, not ours to tear down. - * - * Empty on a Node host and empty on failure: that is "no refusal proven", never - * "safe to kill" — callers must keep every other guard they already have. - * - * The other direction is real too, and bounded by design: `getAppMetrics()` can - * still list a renderer Electron has not finished reaping, so on Windows a pid - * already recycled onto an unrelated child of ours reads as `own` and its tree - * walk is refused. That is why a refusal only blocks the pid-addressed walk and - * every gated site still kills its own root through the child handle. - * - * Why failure stays open rather than refusing everything: a refusal is not free. - * `terminateWindowsProcessTree` resolves without killing, and - * `killSourceControlAgentProcess` returns that straight to a caller that then - * releases the managed-home lock, so failing closed would trade one unreadable - * metrics table for every PTY, git, codex and notebook tree in main leaking at - * once. The `own_chromium_pids_unreadable` crumb is the price of that choice: - * without it a throw is byte-identical to "no Chromium on this host". - * - * Host coverage: only Electron main installs a Chromium-backed AppEnvironment - * (main-process-preflight). The standalone daemon installs none and `orcad` - * installs a Node one whose `getAppMetrics()` is `[]`, so this set is empty in - * both — and that is sound, not a hole: the pid-addressed kills those hosts - * issue go through `classifyWindowsTreeKillTarget`, which walks ancestry back to - * the *killing* process's own pid. Orca's Chromium processes are children of - * Electron main, so they never classify `own` from a daemon or orcad host, and - * on an SSH/serve host there is no Chromium on the machine at all. + * Guard Windows PID-based kills; stale metrics must not prevent root termination through its handle. + * Missing metrics leave other kill guards responsible: refusing all kills would leak managed processes. + * Daemon/Node hosts have no Chromium metrics and instead classify targets against their own ancestry. */ export function readOrcaChromiumProcessPids(): ReadonlySet { if (!hasAppEnvironment()) { @@ -51,8 +22,7 @@ export function readOrcaChromiumProcessPids(): ReadonlySet { } } -// Why coalesced: the gate reads this set on every tree kill, so a persistently -// broken metrics table would otherwise flood the 30-slot ring it shares. +// Repeated kill attempts must not flood the shared breadcrumb ring when metrics remain unreadable. const UNREADABLE_METRICS_COALESCE_MS = 60_000 function recordUnreadableOwnChromiumMetrics(error: unknown): void { @@ -64,7 +34,6 @@ function recordUnreadableOwnChromiumMetrics(error: unknown): void { minIntervalMs: UNREADABLE_METRICS_COALESCE_MS }) } catch { - // Diagnostics must never turn an admitted kill into a thrown one: callers - // read this set outside their own try. + // Diagnostic failures must not interrupt callers’ termination paths. } } diff --git a/src/main/own-chromium-tree-kill-guard.test.ts b/src/main/own-chromium-tree-kill-guard.test.ts index 1b8a2c3b96d..e518949c2e2 100644 --- a/src/main/own-chromium-tree-kill-guard.test.ts +++ b/src/main/own-chromium-tree-kill-guard.test.ts @@ -28,10 +28,8 @@ import { _resetTracerForTests, setActiveSink } from './observability/tracer' const ORCA_MAIN_PID = 1000 const RENDERER_PID = 1001 -/** The standalone daemon is a sibling of the renderers, spawned by main. */ const DAEMON_PID = 1500 -/** Orca's renderer is a direct child of the main process, so the ppid walk says `own`. */ const PROCESS_ROWS = [ { pid: RENDERER_PID, ppid: ORCA_MAIN_PID }, { pid: ORCA_MAIN_PID, ppid: 900 } @@ -90,9 +88,7 @@ describe('refusing to tree-kill our own Chromium processes', () => { ])( 'refuses an Orca renderer from a daemon host with %s, because no Chromium descends from it', (_case, ownChromiumPids) => { - // The standalone daemon and orcad install no Chromium-backed AppEnvironment, - // so this set is empty there. The ancestry walk is what refuses instead: it - // ends at the *killing* process's pid, and the renderer's chain reaches main. + // Daemon ancestry stops at the daemon PID; Chromium siblings fail that check without metrics. const rows = [...PROCESS_ROWS, { pid: DAEMON_PID, ppid: ORCA_MAIN_PID }] expect(classifyWindowsTreeKillTarget(RENDERER_PID, rows, DAEMON_PID, ownChromiumPids)).toBe( @@ -102,8 +98,7 @@ describe('refusing to tree-kill our own Chromium processes', () => { ) it('is the only thing standing between Electron main and its own renderer', () => { - // Falsifiable counterpart to the daemon case above: in main the ancestry walk - // says `own`, so the pid set is load-bearing here and nowhere else. + // In main, renderer ancestry passes; Chromium metrics provide the refusal. expect( classifyWindowsTreeKillTarget(RENDERER_PID, PROCESS_ROWS, ORCA_MAIN_PID, new Set()) ).toBe('own') @@ -159,8 +154,7 @@ describe('refusing to tree-kill our own Chromium processes', () => { spawnImpl: spawnImpl as never }) - // The deadline timer fires on `child.pid` alone; a reaped-then-recycled pid - // is the stale-pid mechanism this gate exists to stop. + // Deadline kills have only a PID, which may have been reused after the child exited. expect(spawnImpl).not.toHaveBeenCalled() expect(getCrashBreadcrumbSnapshot()).toEqual([ expect.objectContaining({ name: 'self_tree_kill_refused_own_chromium' }) @@ -182,19 +176,13 @@ describe('refusing to tree-kill our own Chromium processes', () => { }) }) - /** - * Fail-open is the deliberate choice — see `orca-chromium-process-pids.ts` for - * why refusing everything is worse — so the crumb is the only thing that keeps - * an unreadable metrics table distinguishable from a host that has no Chromium. - */ it('leaves proof, and still admits the kill, when the Chromium metrics cannot be read', () => { appMetricsMock.mockImplementation(() => { throw new Error('getAppMetrics unavailable') }) expect([...readOrcaChromiumProcessPids()]).toEqual([]) - // Coalesced: the gate reads this set on every kill, so a broken table must - // not evict the ring it shares with the refusal crumb. + // Coalesce repeated failures so they cannot evict refusal evidence from the shared ring. expect([...readOrcaChromiumProcessPids()]).toEqual([]) expect( admitSelfInitiatedTreeKill({ diff --git a/src/main/own-chromium-tree-kill-guard.ts b/src/main/own-chromium-tree-kill-guard.ts index 6f1791d1e3d..e5db64f71d2 100644 --- a/src/main/own-chromium-tree-kill-guard.ts +++ b/src/main/own-chromium-tree-kill-guard.ts @@ -6,40 +6,13 @@ import { import { readOrcaChromiumProcessPids } from './orca-chromium-process-pids' import { setProcessTreeKillGate } from '@orca/process-host/process-tree-kill-gate' -/** - * Gate every main-process tree-kill through one decision: refuse a pid-addressed - * walk when Electron is currently accounting for the pid, otherwise put the - * kill on the record. - * - * Why a shared gate rather than a check inside `terminateWindowsProcessTree`: - * five other families in main run their own `taskkill /T /F` with different - * lifetimes (sync, fire-and-forget, timeout ladder), and three more live in - * `src/shared` and reach this through `process-tree-kill-gate`, so a guard that - * only lived in the tree-kill helper would cover one of nine. - * Returns false - * when the caller must not walk that pid's tree; the caller still kills its own - * root through the child handle (`refused-tree-kill-root-termination.test.ts`), - * so a refusal is never a process leak. - * - * Electron main only, by construction. `terminateWindowsProcessTree` also runs - * in the standalone daemon (the `pty-descendant-sweep` site), where - * `readOrcaChromiumProcessPids()` is empty and this always admits. That is not - * the gap it looks like: the daemon reaches that taskkill only through - * `classifyWindowsTreeKillTarget`, whose ancestry walk ends at the daemon's own - * pid, and no Chromium process descends from the daemon. See - * `orca-chromium-process-pids.ts`. - */ +/** Refusal forbids only the PID-based tree walk; callers must still kill the root through its handle. */ export function admitSelfInitiatedTreeKill(target: { pid: number site: string scope: SelfInitiatedTreeKillScope }): boolean { - // Why: no PTY root, codex root or git child is ever one of our own Chromium - // processes, so a pid that is means the caller is about to kill a renderer, - // the GPU or the browser itself (#10680). Only the pid-addressed scope can - // land there: a POSIX group holds only what Orca put in it, so that arm is - // recorded and admitted like every other group kill in main, and a stale - // `getAppMetrics()` entry cannot orphan a macOS/Linux tree. + // Windows PID reuse can target Chromium; POSIX groups contain only processes assigned to them (#10680). const isOwnChromiumPid = target.scope === 'win-taskkill-tree' && readOrcaChromiumProcessPids().has(target.pid) try { @@ -49,13 +22,12 @@ export function admitSelfInitiatedTreeKill(target: { recordSelfInitiatedTreeKill(target) } } catch { - // Recording must never turn a successful termination into a failed one, and - // never flip the decision: it is taken above, before anything can throw. + // Recording failures must neither interrupt termination nor change the guard’s decision. } return !isOwnChromiumPid } -/** Hands the gate to the shared choke points, which cannot import main. */ +/** Shared kill sites cannot import main; install the same guard through their hook. */ export function installMainProcessTreeKillGate(): void { setProcessTreeKillGate((kill) => admitSelfInitiatedTreeKill(kill)) }