diff --git a/src/cli/runtime/client-execution-host.test.ts b/src/cli/runtime/client-execution-host.test.ts index 82c608c3911..bf72a6be448 100644 --- a/src/cli/runtime/client-execution-host.test.ts +++ b/src/cli/runtime/client-execution-host.test.ts @@ -14,7 +14,6 @@ const pairingCode = encodePairingOffer({ publicKeyB64: 'fixture-key', scope: 'runtime' }) -// Each selector names a real paired server, so honouring it routes there. const selection = { ORCA_ENVIRONMENT: 'other-server', ORCA_PAIRING_CODE: pairingCode, @@ -27,7 +26,6 @@ afterEach(() => { roots.splice(0).forEach((root) => rmSync(root, { recursive: true, force: true })) }) -/** This server's profile, with another server paired in it; the CLI starts with `env`. */ function serverProfileWith(env: Partial>): string { const root = mkdtempSync(join(tmpdir(), 'cli-owner-')) roots.push(root) @@ -40,7 +38,7 @@ function serverProfileWith(env: Partial { - // The agent environment sets all three empty so a profile's export cannot win. + // Empty selectors prevent profile exports from routing the agent to another server. const root = serverProfileWith({ ORCA_ENVIRONMENT: '', ORCA_PAIRING_CODE: '', diff --git a/src/cli/runtime/client.ts b/src/cli/runtime/client.ts index 339e2ca8534..cddf11bb215 100644 --- a/src/cli/runtime/client.ts +++ b/src/cli/runtime/client.ts @@ -59,9 +59,7 @@ export class RuntimeClient { ) private readonly runtimeSourceFence: RuntimeSourceFence - // Why: browser commands trigger first-time session init (agent-browser connect + - // CDP proxy setup) which can take 15-30s. 60s accommodates cold start without - // being so large that genuine hangs go unnoticed. + // Browser session cold starts can take 15–30 seconds; allow time for initialization. constructor( userDataPath = getDefaultUserDataPath(), requestTimeoutMs = 60_000, @@ -165,7 +163,7 @@ export class RuntimeClient { return response } const metadata = readMetadata(this.userDataPath) - // Why outside recover(): a refusal here happens before anything is sent, so there is nothing to retry. + // Refusal precedes sending, so there is no request to recover. const expectedRuntimeSource = await this.runtimeSourceFence.check(metadata, method) let response try { @@ -186,11 +184,7 @@ export class RuntimeClient { return response } - // Why: centralises the per-method timeout policy. Long-poll inner waiter - // budgets live in `params.timeoutMs`; widen the client-side socket timeout - // to `timeoutMs + grace` so it doesn't fire before the server has a chance - // to resolve. Without this, a 5 min wait would still die at the 60 s default. - // See design doc §3.1. + // Give server-side waits time to finish before the client transport times out. private resolveMethodTimeoutMs(method: string, params?: unknown): number { if (method === 'orchestration.workerStart') { const requestedValue = getTimeoutMsParam(params) @@ -260,7 +254,7 @@ export class RuntimeClient { this.orchestrationContractCheck = this.checkOrchestrationContractCompatibility( timeoutMs ).catch((error: unknown) => { - // Why: a failed probe must not be cached, or a retry after a brief outage never reaches the app. + // Failed probes must be retried after transient outages. this.orchestrationContractCheck = null throw error }) @@ -288,8 +282,7 @@ export class RuntimeClient { return initial } - // Why: a blocked runtime can't open a window, so spawning the app would - // only hit the single-instance lock and exit — bail before launching. + // A blocked runtime cannot open a window; a second app would only hit its instance lock. if (initial.result.app.desktopWindowStatus === 'blocked') { throwDesktopActivationBlocked() } diff --git a/src/cli/runtime/electron-sandbox-switch.ts b/src/cli/runtime/electron-sandbox-switch.ts index ae5c98a8ea8..93d3554c51d 100644 --- a/src/cli/runtime/electron-sandbox-switch.ts +++ b/src/cli/runtime/electron-sandbox-switch.ts @@ -5,7 +5,6 @@ import { ELECTRON_NO_SANDBOX_REQUEST_ENV } from '../../shared/electron-no-sandbo const USER_NAMESPACE_PROBE_TIMEOUT_MS = 2_000 -/** Whether an Electron this CLI starts needs `--no-sandbox`. */ export function needsElectronNoSandbox(executable: string): boolean { return ( process.env[ELECTRON_NO_SANDBOX_REQUEST_ENV] === '1' || diff --git a/src/cli/runtime/envelope-schema.test.ts b/src/cli/runtime/envelope-schema.test.ts index 0e83de3ab44..75a00185cde 100644 --- a/src/cli/runtime/envelope-schema.test.ts +++ b/src/cli/runtime/envelope-schema.test.ts @@ -85,9 +85,7 @@ describe('RuntimeRpcEnvelopeSchema', () => { }) it('accepts a failure envelope without _meta', () => { - // Why: the runtime may fail before it has resolved its own runtimeId, in - // which case _meta is omitted. The schema must tolerate that rather than - // rejecting a legitimate failure as an invalid frame. + // Failures before runtimeId resolution legitimately omit _meta. const parsed = RuntimeRpcEnvelopeSchema.safeParse({ id: 'req-1', ok: false, diff --git a/src/cli/runtime/environments.test.ts b/src/cli/runtime/environments.test.ts index aad4e9b42f3..974b26ed1ca 100644 --- a/src/cli/runtime/environments.test.ts +++ b/src/cli/runtime/environments.test.ts @@ -55,8 +55,7 @@ describe('CLI runtime environments', () => { now: 100 }) - // Why: NTFS mode bits do not prove Windows ACL hardening; shared secure-file - // tests cover that path, while POSIX hosts must keep the token store at 0600. + // POSIX mode bits cannot prove Windows ACL hardening; secure-file tests cover it. expect((statSync(getEnvironmentStorePath(userDataPath)).mode & 0o777).toString(8)).toBe('600') }) diff --git a/src/cli/runtime/launch-serve-replaced-app-restart.test.ts b/src/cli/runtime/launch-serve-replaced-app-restart.test.ts index 76ba0d5b3b7..a1119426bc4 100644 --- a/src/cli/runtime/launch-serve-replaced-app-restart.test.ts +++ b/src/cli/runtime/launch-serve-replaced-app-restart.test.ts @@ -20,7 +20,7 @@ vi.mock('./serve-replaced-app-restart', async (importOriginal) => ({ restartThisServeSupervisorIfReplaced: restartMock })) -// Why: the real check lists processes for the installer, which the spawn mock cannot answer. +// The spawn mock cannot answer the real installer process check. vi.mock('./serve-install-hold', async (importOriginal) => ({ ...(await importOriginal()), readServeInstallEvidence: async () => ({ kind: 'clear' }) @@ -82,7 +82,6 @@ describe('serveOrcaApp restarts from a replaced app before any server starts', ( beforeEach(async () => { root = await mkdtemp(join(tmpdir(), 'orca-serve-replaced-app-')) app = join(root, 'Orca.app') - // The update is already on disk unless a test swaps it in. await installApp(app, NEW) for (const key of ENV_KEYS) { savedEnv[key] = process.env[key] @@ -117,7 +116,6 @@ describe('serveOrcaApp restarts from a replaced app before any server starts', ( await rm(root, { recursive: true, force: true }) }) - /** Each restart check must precede its own spawn: none may start a server unchecked. */ function expectCheckedBeforeEverySpawn(spawns: number): void { expect(spawnMock).toHaveBeenCalledTimes(spawns) expect(restartMock).toHaveBeenCalledTimes(spawns) @@ -182,7 +180,6 @@ describe('serveOrcaApp restarts from a replaced app before any server starts', ( expect(restartMock).not.toHaveBeenCalled() }) - // The real decision, for a supervisor that started on the old app before the install swapped it. async function withRealRestart() { const actual = await vi.importActual('./serve-replaced-app-restart') restartMock.mockImplementation(actual.restartThisServeSupervisorIfReplaced) @@ -229,7 +226,6 @@ describe('serveOrcaApp restarts from a replaced app before any server starts', ( expect(await resumeAcrossInstall(false)).not.toHaveBeenCalled() }) - // A later attempt of the service loop (here after a crash; also after a park) has no handoff. it('restarts before a supervised service starts its next server from a replaced app', async () => { const execve = await withRealRestart() const crashed = new FakeChild(4101) diff --git a/src/cli/runtime/launch-serve-runtime.test.ts b/src/cli/runtime/launch-serve-runtime.test.ts index 9a64d824c9f..1d9e882f2f1 100644 --- a/src/cli/runtime/launch-serve-runtime.test.ts +++ b/src/cli/runtime/launch-serve-runtime.test.ts @@ -14,7 +14,7 @@ const { spawnMock, resolveLocalServeRuntimeMock, serveWithOrcadMock, refuseIfPro })) vi.mock('child_process', () => ({ spawn: spawnMock, spawnSync: vi.fn() })) -// Why: these cover host selection; the pre-exec profile check would read this machine's own profile. +// Isolate host selection from the preflight that reads this machine’s profile. vi.mock('./serve-electron-preflight', () => ({ preflightElectronServe: () => null, recordElectronServeStartupAbort: vi.fn(), @@ -36,7 +36,6 @@ class FakeChildProcess extends EventEmitter { pid = 4101 } -/** Electron serve that exits cleanly once spawned, whenever selection gets to spawning it. */ function electronChild(): void { spawnMock.mockImplementation(() => { const child = new FakeChildProcess() @@ -96,7 +95,6 @@ describe('orca serve host selection', () => { } }) - // #30100: the slot carries no browser client, so orcad must be handed the app's. it("serves on orcad by default, handing it the app's browser client", async () => { const appRoot = await mkdtemp(join(tmpdir(), 'orca-serve-orcad-web-')) try { @@ -117,7 +115,6 @@ describe('orca serve host selection', () => { expect.any(String), expect.not.objectContaining({ ELECTRON_RUN_AS_NODE: expect.anything() }), join(appRoot, 'out', 'web'), - // #31174: and the app's speech recognizer. process.platform === 'win32' ? undefined : join(appRoot, 'node_modules', recognizer) ) expect(spawnMock).not.toHaveBeenCalled() @@ -216,7 +213,7 @@ describe('orca serve host selection', () => { expect(resolveLocalServeRuntimeMock).toHaveBeenCalledWith( expect.objectContaining({ userDataPath: profile }) ) - // A dev build ignores --user-data-dir and reads this instead. + // Dev builds read the profile from the environment rather than --user-data-dir. expect(electronSpawn().env).toMatchObject({ ORCA_DEV_USER_DATA_PATH: profile, ORCA_SERVE_USER_DATA_PATH: profile diff --git a/src/cli/runtime/launch-user-data-path.ts b/src/cli/runtime/launch-user-data-path.ts index 658108a0860..88dc6f5e7a5 100644 --- a/src/cli/runtime/launch-user-data-path.ts +++ b/src/cli/runtime/launch-user-data-path.ts @@ -2,21 +2,17 @@ import { dirname, join, resolve } from 'node:path' import { SERVE_USER_DATA_PATH_ENV } from '../../shared/serve-user-data-path' import { getPlatformUserDataPath } from './metadata' -/** An unpackaged checkout build, which takes its profile from ORCA_DEV_USER_DATA_PATH, not --user-data-dir. */ +/** Dev builds read ORCA_DEV_USER_DATA_PATH rather than --user-data-dir. */ function launchesDevApp(env: NodeJS.ProcessEnv): boolean { return env.ORCA_APP_EXECUTABLE_NEEDS_APP_ROOT === '1' } -/** - * The one profile a CLI launch both reads and hands to the app it starts. - * Absolute, because the child runs from the app root, not the caller's cwd. - */ +/** Resolve absolutely: the launched child runs from the app root, not the caller’s cwd. */ export function resolveLaunchUserDataPath(env: NodeJS.ProcessEnv = process.env): string { if (env.ORCA_USER_DATA_PATH) { return resolve(env.ORCA_USER_DATA_PATH) } if (launchesDevApp(env)) { - // Mirrors the dev app's own default (`/orca-dev`). return resolve( env.ORCA_DEV_USER_DATA_PATH || join(dirname(getPlatformUserDataPath()), 'orca-dev') ) @@ -24,7 +20,7 @@ export function resolveLaunchUserDataPath(env: NodeJS.ProcessEnv = process.env): return getPlatformUserDataPath() } -/** Pins a launched app to `userDataPath` in every form a packaged or dev build reads. */ +/** Pin the profile through both packaged and dev entry points. */ export function pinLaunchUserDataPath( args: string[], env: NodeJS.ProcessEnv, @@ -37,7 +33,7 @@ export function pinLaunchUserDataPath( return { args: [...args, `--user-data-dir=${userDataPath}`], env: pinnedEnv } } -/** Serve additionally refuses to start, before the instance lock, if it lands anywhere else. */ +/** Serve checks this profile before acquiring its instance lock. */ export function pinServeUserDataPath( args: string[], env: NodeJS.ProcessEnv, diff --git a/src/cli/runtime/launch.test.ts b/src/cli/runtime/launch.test.ts index 894f264fbe3..fc647d9ce48 100644 --- a/src/cli/runtime/launch.test.ts +++ b/src/cli/runtime/launch.test.ts @@ -74,7 +74,6 @@ const INVALID_SSH_RECIPE_JSON = SSH_RECIPE_JSON.replace('/workspace/repo', 'rela const IGNORED_NON_RECIPE_STDOUT = '[serve] ignored non-recipe stdout' const PROFILE = resolve('/profiles/serve') const PIN_PROFILE = `--user-data-dir=${PROFILE}` -// Linux CI runners get the X11 pin; macOS and Windows hosts get nothing. const HOST_SERVE_OZONE_SWITCHES = headlessServeOzonePlatformSwitches(process.platform) function restoreEnv(key: string, value: string | undefined): void { @@ -112,7 +111,7 @@ describe('serveOrcaApp', () => { spawnMock.mockReset() spawnSyncMock.mockReset() process.env.ORCA_APP_EXECUTABLE = '/Applications/Orca.app/Contents/MacOS/Orca' - // These cover Electron serve itself; the orcad default is in launch-serve-runtime.test.ts. + // Test Electron here; launch-serve-runtime.test.ts covers orcad selection. process.env.ORCA_SERVE_RUNTIME = 'electron' process.env.ORCA_USER_DATA_PATH = PROFILE }) diff --git a/src/cli/runtime/launch.ts b/src/cli/runtime/launch.ts index 710ae1403fe..526c689e764 100644 --- a/src/cli/runtime/launch.ts +++ b/src/cli/runtime/launch.ts @@ -64,7 +64,7 @@ export function launchOrcaApp(): void { return } - // Why: `openOrca` waits on this profile's metadata, so the app must start on it too. + // Launch on the same profile whose metadata openOrca awaits. const userDataPath = resolveLaunchUserDataPath() const overrideExecutable = process.env.ORCA_APP_EXECUTABLE if (typeof overrideExecutable === 'string' && overrideExecutable.trim().length > 0) { @@ -84,9 +84,7 @@ export function launchOrcaApp(): void { if (process.platform === 'darwin') { const appBundlePath = getMacAppBundlePath(process.execPath) if (appBundlePath) { - // Why: launching the inner MacOS binary directly can trigger macOS app - // launch failures and bypass normal bundle lifecycle. The public - // packaged CLI should re-open the .app the same way Finder does. + // Launch the macOS bundle through Launch Services to preserve its app lifecycle. spawnDetached('open', getMacOpenArgs(appBundlePath, userDataPath), { env: stripElectronRunAsNode(process.env) }) @@ -109,10 +107,7 @@ export function launchOrcaApp(): void { ) } -/** - * Plain `open` activates whichever instance already runs, which is the default profile's. - * Why `-n` only off the default: `open` drops our env, and `--args` reach only a new instance. - */ +/** Nondefault profiles need a new instance: open drops env and only new instances receive --args. */ export function getMacOpenArgs(appBundlePath: string, userDataPath: string): string[] { if (isSameUserDataPath(userDataPath, getPlatformUserDataPath())) { return [appBundlePath] @@ -126,21 +121,20 @@ function spawnDetached(command: string, args: string[], options: SpawnOptions): stdio: 'ignore', ...options }) - // Why: detached launch errors are reported asynchronously after this function - // returns; openOrca already reports the user-facing timeout if startup fails. + // Handle asynchronous spawn errors; openOrca reports startup failure via its timeout. child.once('error', () => {}) child.unref() } export function serveOrcaApp(args: ServeOrcaAppArgs = {}): Promise { - // Why first: as a container or PrivatePIDs init, nothing would reap orphaned descendants. + // Install the PID 1 reaper before spawning so orphaned descendants are collected. reexecThisServeUnderInitReaper() noteServeSupervisorStart() const executable = resolveForegroundOrcaExecutable() - // Why one value: the selector, orcad and Electron must all serve the profile the caller chose. + // Runtime selection, orcad, and Electron must use the same profile. const userDataPath = resolveLaunchUserDataPath() if (args.supervise) { - // Why: a successful relocation replaces this process and never resolves. + // Successful relocation replaces this process and never resolves. return relocateThisServeSupervisor(resolveAppRoot(), userDataPath).then(() => { let attempt = 0 return superviseServeService({ @@ -150,7 +144,7 @@ export function serveOrcaApp(args: ServeOrcaAppArgs = {}): Promise { force: args.force === true, park: openServeBundlePark({ executable }), appImageUpdate: supervisedAppImageUpdate(executable, userDataPath, args.force === true), - // Why first only: --force overrides a remembered abort once; the brake owns later attempts. + // --force overrides the brake once; later attempts use its updated state. runOnce: () => serveOnce( executable, @@ -168,7 +162,6 @@ function serveOnce( args: ServeOrcaAppArgs, userDataPath: string ): Promise { - // Why synchronous on the opt-out: it must spawn Electron exactly as before, without asking. if (process.env[SERVE_RUNTIME_ENV] === SERVE_RUNTIME_ELECTRON) { return serveWithElectron(executable, args, userDataPath) } @@ -187,8 +180,7 @@ async function serveWithSelectedRuntime( usesMacUpdateHandoff: args.recipeJson !== true && getMacAppBundlePath(executable) !== null }) if (selection.kind === 'orcad') { - // Why only supervised: orcad refuses a held profile as a configuration fault (exit 78), which - // the brake would count; exit 3 here lets the supervisor wait for the other Orca instead. + // Exit 3 waits for the profile owner; orcad’s exit 78 would count against the crash brake. const refusal = args.supervise ? await refuseIfProfileServed(args, userDataPath) : null if (refusal !== null) { return refusal @@ -235,8 +227,7 @@ function spawnElectronServe( ) const childArgs = pinned.args childArgs.push('--serve', ...serveOptionArgs(args, '--serve-')) - // Why: the isolated profile has no login keychain, and the probe that discovers this blocks the - // foreground process on the Keychain authorization prompt before it advertises readiness (#25453). + // Isolated profiles need keychain bypass to avoid blocking readiness on authorization (#25453). childArgs.push(...foregroundServeIsolatedProfileSwitches()) childArgs.push(...headlessServeOzonePlatformSwitches(process.platform)) const handoffPath = @@ -247,7 +238,6 @@ function spawnElectronServe( if (handoffPath) { childEnv[SERVE_UPDATE_HANDOFF_PATH_ENV] = handoffPath } - // Why process-host: it encodes a Windows `.cmd`/`.bat` executable's arguments instead of a raw shell. const spec: ProcessSpec = { program: executable, args: childArgs, @@ -264,8 +254,7 @@ function spawnElectronServe( const spawnServer: ServeSpawnChild = shouldRedactSupervisedStdout(args.supervise === true) ? redactingSpawn(spawnProcess) : spawnProcess - // Why in the spawn: every server start (first, resumed, after an install or a park) passes here, - // after its stop and park gates. Only our own executable: an update never deletes an override. + // Check before every spawn after stop/park gates; overrides are not replaced by app updates. const spawnServeChild: ServeSpawnChild = handoffPath && executable === process.execPath ? (childSpec) => { @@ -275,7 +264,7 @@ function spawnElectronServe( : spawnServer const interruptedHandoff = handoffPath ? readServeUpdateHandoffSync(handoffPath) : null if (interruptedHandoff?.phase === 'install-requested') { - // Why: the node-mode CLI is not an NSRunningApplication, so it can retain launchd ownership while ShipIt swaps the app. + // Node mode avoids NSRunningApplication registration, retaining launchd ownership during ShipIt swaps. return resumeInterruptedServeUpdate({ spec, spawnChild: spawnServeChild, @@ -299,7 +288,7 @@ function spawnElectronServe( handoffPath, expectedHandoff: null }).catch((error: unknown) => { - // Why only a prompt abort: the registration abort kills the child before any JS runs. + // Registration aborts happen before child JS runs; record only immediate startup failures. if (error instanceof MacServeStartupAbortError && Date.now() - spawnedAt < STARTUP_ABORT_MS) { recordElectronServeStartupAbort(userDataPath, error.message) } diff --git a/src/cli/runtime/mac-app-update-bundle.ts b/src/cli/runtime/mac-app-update-bundle.ts index 16f865715c8..7b9dafb91af 100644 --- a/src/cli/runtime/mac-app-update-bundle.ts +++ b/src/cli/runtime/mac-app-update-bundle.ts @@ -69,7 +69,7 @@ export async function waitForMacBundleVersion( signal?.addEventListener('abort', () => finish(false), { once: true }) poll = setInterval(check, 250) try { - // Why: ShipIt replaces the whole .app, so watch its stable parent rather than an inode inside the old bundle. + // ShipIt replaces the whole bundle; watch its stable parent. watcher = watch(dirname(appBundlePath), check) watcher.on('error', () => { watcher?.close() diff --git a/src/cli/runtime/metadata.ts b/src/cli/runtime/metadata.ts index 7a0d4f04ca9..9c686aec804 100644 --- a/src/cli/runtime/metadata.ts +++ b/src/cli/runtime/metadata.ts @@ -43,10 +43,6 @@ export function getDefaultUserDataPath( platform: NodeJS.Platform = process.platform, homeDir = homedir() ): string { - // Why: in dev mode (and for parallel Orca instances), the Electron app writes - // runtime metadata to a separate userData directory (e.g. `orca-dev`) to avoid - // clobbering the production app's metadata. The CLI needs to find the same - // metadata file, so this env var lets the CLI target a specific instance. if (process.env.ORCA_USER_DATA_PATH) { return process.env.ORCA_USER_DATA_PATH } @@ -71,8 +67,6 @@ export function getPlatformUserDataPath( } return join(appData, 'orca') } - // Why: the CLI must find the same metadata file Electron writes in packaged - // runs, so this mirrors Electron's default userData base instead of inventing - // a CLI-specific config path. + // Match Electron’s packaged userData path. return join(process.env.XDG_CONFIG_HOME || join(homeDir, '.config'), 'orca') } diff --git a/src/cli/runtime/orca-app-executable.ts b/src/cli/runtime/orca-app-executable.ts index 386b8ffb137..a165fefee25 100644 --- a/src/cli/runtime/orca-app-executable.ts +++ b/src/cli/runtime/orca-app-executable.ts @@ -5,8 +5,6 @@ import { serverOnlyServeMessage } from './serve-app-launcher' import { SERVE_SUPERVISOR_APP_ROOT_ENV } from './serve-supervisor-relocation' import { RuntimeClientError } from './types' -/** Which Orca app `orca` runs, from where, and with what environment. */ - export function getExecutableAppArgs(executable: string): string[] { const args = process.env.ORCA_APP_EXECUTABLE_NEEDS_APP_ROOT === '1' ? [resolveAppRoot()] : [] if (needsElectronNoSandbox(executable)) { @@ -16,10 +14,7 @@ export function getExecutableAppArgs(executable: string): string[] { } export function resolveAppRoot(): string { - // Why: dev-mode resource resolution in the Electron child may consult - // process.cwd(). Pin it to the app root so `orca serve` behaves the same - // regardless of the shell directory it was launched from. A supervisor re-run from its copy - // outside the app must still start the app's own files. + // Pin cwd to the app root; relocated supervisors must also load the app’s own files. return process.env[SERVE_SUPERVISOR_APP_ROOT_ENV] || resolve(__dirname, '../../..') } @@ -37,9 +32,9 @@ export function resolveForegroundOrcaExecutable(): string { export function stripElectronRunAsNode(env: NodeJS.ProcessEnv): NodeJS.ProcessEnv { const next = { ...env } delete next.ELECTRON_RUN_AS_NODE - // Why: the request is applied as a switch; the server's terminals must not inherit it. + // The switch applies only to app launch; terminals must not inherit the request. delete next[ELECTRON_NO_SANDBOX_REQUEST_ENV] - // Why: only the relocated supervisor itself may resolve its app root this way. + // Only the relocated supervisor may override its app root. delete next[SERVE_SUPERVISOR_APP_ROOT_ENV] return next } diff --git a/src/cli/runtime/orchestration-compatibility-envelope.ts b/src/cli/runtime/orchestration-compatibility-envelope.ts index 7d6c0dfda80..19fd88d157e 100644 --- a/src/cli/runtime/orchestration-compatibility-envelope.ts +++ b/src/cli/runtime/orchestration-compatibility-envelope.ts @@ -7,8 +7,7 @@ export function createOrchestrationCompatibilityEnvelope( env: NodeJS.ProcessEnv ): RuntimeOrchestrationEnvelope { const evidence = readOrchestrationCompatibilityEvidence(env) - // Read here, from this CLI's own environment only: the SSH paths build evidence from a remote - // shell's environment, where a session id could never name a session on this host. + // Only local environment session ids can identify a session on this host. const agentSessionId = readInjectedAgentSessionId(env) return { compatibilityInvocationId: randomUUID(), diff --git a/src/cli/runtime/runtime-access-denied.test.ts b/src/cli/runtime/runtime-access-denied.test.ts index fcd775fa4f8..d4f62231201 100644 --- a/src/cli/runtime/runtime-access-denied.test.ts +++ b/src/cli/runtime/runtime-access-denied.test.ts @@ -109,7 +109,7 @@ describe('runtime access denied', () => { } ) - // Why: a dead Orca leaves its socket behind, and the sandbox denies it before ECONNREFUSED. + // Sandbox denial can mask ECONNREFUSED on a dead runtime’s leftover socket. it('gives not-running advice when the denied endpoint belongs to a dead pid', async () => { mockKill('ESRCH') const human = formatCliError(await deniedRequest('EPERM')) diff --git a/src/cli/runtime/runtime-access-denied.ts b/src/cli/runtime/runtime-access-denied.ts index 720836fa05d..36393a88173 100644 --- a/src/cli/runtime/runtime-access-denied.ts +++ b/src/cli/runtime/runtime-access-denied.ts @@ -1,13 +1,13 @@ import { RuntimeClientError } from './types' import { isProcessRunning } from './runtime-pid-liveness' -// Why: the errno decides the classification; CODEX_SANDBOX only picks the wording. +// Errno determines denial; CODEX_SANDBOX only selects wording. export function runtimeAccessDeniedError( socketError: Error, pid: number ): RuntimeClientError | null { const systemCode = 'code' in socketError ? socketError.code : undefined - // Why: a sandbox still sees ESRCH, so a dead Orca's leftover socket gets not-running advice. + // ESRCH identifies a dead runtime even when its socket is access denied. if ((systemCode !== 'EPERM' && systemCode !== 'EACCES') || !isProcessRunning(pid)) { return null } diff --git a/src/cli/runtime/runtime-listener-probe.test.ts b/src/cli/runtime/runtime-listener-probe.test.ts index 068c1c791a2..e0018e059c8 100644 --- a/src/cli/runtime/runtime-listener-probe.test.ts +++ b/src/cli/runtime/runtime-listener-probe.test.ts @@ -33,8 +33,7 @@ function metadataFor(endpoint: string): RuntimeMetadata { pid: process.pid, transports: [ { kind: process.platform === 'win32' ? 'named-pipe' : 'unix', endpoint }, - // Why: the websocket entry must not be picked — it is reachable from - // anywhere on the network and proves nothing about this profile's owner. + // Network reachability does not prove ownership of this local profile. { kind: 'websocket', endpoint: 'ws://127.0.0.1:1' } ], authToken: 'token', @@ -51,8 +50,6 @@ function nextEndpoint(): string { describe('probeRuntimeListener', () => { it('accepts a runtime that is listening but silent', async () => { - // Why: this is the case the pid could never distinguish — the owner is alive - // and holding the profile, it just has not answered RPC yet. const endpoint = nextEndpoint() const server = createServer((socket) => { sockets.add(socket) @@ -65,16 +62,13 @@ describe('probeRuntimeListener', () => { }) it('frees a profile whose endpoint was never bound', async () => { - // Why: a runtime unlinks its socket on shutdown. Reading that as an owner is - // what makes a stale profile refuse serve forever. await expect(probeRuntimeListener(metadataFor(nextEndpoint()))).resolves.toBe('not-listening') }) it.skipIf(process.platform === 'win32')( 'frees a profile whose owner was killed with its socket file still on disk', async () => { - // Why: this is the STA-4336 crash shape — SIGKILL skips the unlink, so the path - // survives and only its refusal distinguishes it from a live owner. + // SIGKILL leaves the socket path behind; connection refusal proves absence. const endpoint = nextEndpoint() const child = spawn(process.execPath, [ '-e', @@ -91,9 +85,7 @@ describe('probeRuntimeListener', () => { it.skipIf(process.platform === 'win32')( 'refuses to call a profile free when the endpoint could not be reached at all', async () => { - // Why: only ENOENT/ECONNREFUSED disown an endpoint. Anything else — here an - // unreadable directory — leaves an owner possible, and guessing "free" is - // what spawns the second main that aborts pre-JS. + // Permission errors cannot prove the profile is free for another Electron main. const dir = join(mkdtempSync(join(tmpdir(), 'orca-probe-')), 'locked') mkdirSync(dir) chmodSync(dir, 0o000) @@ -106,9 +98,7 @@ describe('probeRuntimeListener', () => { ) it('refuses to call a profile free when a connect neither lands nor is refused', async () => { - // Why: the cap keeps this off the launch path's critical time, but a connect that - // ran out of time proved nothing. Fake timers fire the cap before the real connect - // can settle, which is the only way to reach that branch deterministically. + // Fake timers expire the probe before a real connection can settle. vi.useFakeTimers() try { const probe = probeRuntimeListener(metadataFor(nextEndpoint())) @@ -125,12 +115,9 @@ describe('probeRuntimeListener', () => { ['a blank endpoint', ' '], ['a numeric endpoint', 42] ])('frees a profile whose metadata carries %s instead of dialling it', async (_label, value) => { - // Why: the metadata file is unvalidated JSON. A missing or blank endpoint throws - // out of createConnection and would fail serve outright, and a number is read as a - // TCP port — 127.0.0.1:42 could be answered by something that is not Orca at all. + // Invalid endpoints can throw or dial an unrelated numeric TCP port. const metadata = metadataFor(nextEndpoint()) const transport = { ...metadata.transports[0]! } - // The file on disk is untyped JSON, so the probe must survive a non-string endpoint. Object.assign(transport, { endpoint: value }) await expect(probeRuntimeListener({ ...metadata, transports: [transport] })).resolves.toBe( diff --git a/src/cli/runtime/runtime-listener-probe.ts b/src/cli/runtime/runtime-listener-probe.ts index 482c353c53b..15601426b48 100644 --- a/src/cli/runtime/runtime-listener-probe.ts +++ b/src/cli/runtime/runtime-listener-probe.ts @@ -3,26 +3,20 @@ import { findTransport, type RuntimeMetadata } from '../../shared/runtime-bootst export const RUNTIME_LISTENER_PROBE_TIMEOUT_MS = 250 -/** What a connect to the published endpoint proved — `unproven` is not an absence. */ +/** An unproven connection does not establish absence. */ export type RuntimeListenerProbe = 'accepting' | 'not-listening' | 'unproven' -/** Why: measured on macOS — a stopped runtime unlinks its socket (ENOENT) and a SIGKILLed one leaves a path that refuses (ECONNREFUSED). Both land in <1ms, so these are the only outcomes that prove nobody is home. */ +/** Only a missing or refused endpoint proves no listener exists. */ const NOT_LISTENING_CODES = new Set(['ENOENT', 'ECONNREFUSED']) -/** - * Why: a recorded pid is weak evidence — the OS recycles pids. The runtime binds its - * endpoints before publishing metadata, so an accepted connect proves a live owner - * even when it is too busy for RPC; a crash leaves a path with nothing accepting. - */ +/** PIDs recycle; a listening endpoint proves a live owner even when RPC is unresponsive. */ export function probeRuntimeListener( metadata: RuntimeMetadata, timeoutMs: number = RUNTIME_LISTENER_PROBE_TIMEOUT_MS ): Promise { const transport = findTransport(metadata, 'unix', 'named-pipe') const endpoint = transport?.endpoint - // Why: metadata is unvalidated JSON from disk. A missing or blank endpoint makes - // createConnection throw out of serve, and a numeric one is read as a TCP port — - // it would dial 127.0.0.1 and could be answered by something that is not Orca. + // Invalid endpoints can throw or be interpreted as unrelated TCP ports. if (typeof endpoint !== 'string' || endpoint.trim().length === 0) { return Promise.resolve('not-listening') } @@ -38,12 +32,10 @@ export function probeRuntimeListener( socket.destroy() resolve(result) } - // Why: a connect that neither lands nor is refused means something still holds the - // endpoint, so it is no proof the profile is free. Capped because this is the launch path. + // A timeout cannot prove the profile is free; bound the wait on the launch path. const timer = setTimeout(() => settle('unproven'), timeoutMs) socket.once('connect', () => settle('accepting')) - // Why: `on`, not `once` — a socket destroyed mid-connect can emit again, and a - // second error with no listener left would take down the CLI. + // A socket destroyed mid-connect may emit another error; keep its listener installed. socket.on('error', (error: NodeJS.ErrnoException) => { settle(NOT_LISTENING_CODES.has(error.code ?? '') ? 'not-listening' : 'unproven') }) diff --git a/src/cli/runtime/runtime-pid-liveness.ts b/src/cli/runtime/runtime-pid-liveness.ts index f99f857507e..ceb9450b4a5 100644 --- a/src/cli/runtime/runtime-pid-liveness.ts +++ b/src/cli/runtime/runtime-pid-liveness.ts @@ -6,8 +6,7 @@ export function isProcessRunning(pid: number | null | undefined): boolean { process.kill(pid, 0) return true } catch (error) { - // Why: only ESRCH proves the pid is gone. EPERM means it exists under another uid, and - // reporting that as `stale_bootstrap` calls a live Orca dead. + // Only ESRCH proves absence; EPERM indicates a live PID under another uid. return !(error instanceof Error && 'code' in error && error.code === 'ESRCH') } } diff --git a/src/cli/runtime/runtime-source-fence.test.ts b/src/cli/runtime/runtime-source-fence.test.ts index 91db5aeb870..a3841e63290 100644 --- a/src/cli/runtime/runtime-source-fence.test.ts +++ b/src/cli/runtime/runtime-source-fence.test.ts @@ -42,7 +42,6 @@ afterEach(async () => { type FakeRuntime = { userDataPath: string; methods: string[]; expectations: unknown[] } -/** A live runtime on its own socket that answers every method and records what reached it. */ async function startRuntime(args: { runtimeId: string installationId?: string @@ -134,7 +133,7 @@ describe('RuntimeClient runtime-source fence', () => { await client.call('terminal.send', { terminal: 't1', text: 'hi' }) await client.call('worktree.list') expect(runtime.methods).toEqual(['status.get', 'terminal.send', 'worktree.list']) - // Why: the host refuses the send itself if it is no longer the runtime the probe verified. + // The host must recheck identity if it changes after the probe. const expected = { sourceId: SOURCE_A, runtimeId: 'runtime-a2' } expect(runtime.expectations).toEqual([undefined, expected, expected]) }) diff --git a/src/cli/runtime/runtime-source-fence.ts b/src/cli/runtime/runtime-source-fence.ts index 0210da945d7..5b173611448 100644 --- a/src/cli/runtime/runtime-source-fence.ts +++ b/src/cli/runtime/runtime-source-fence.ts @@ -9,10 +9,7 @@ import { RuntimeClientError, type RuntimeRpcResponse } from './types' type StatusProbe = (metadata: RuntimeMetadata) => Promise> -/** - * The stamp to enforce for a CLI dialing `userDataPath`. Explicitly choosing another profile (e.g. - * `orca-dev` from a packaged Orca's terminal) is the user's target, not a fallback, so it is not fenced. - */ +/** A different nondefault profile opts out of the inherited runtime fence. */ export function resolveFencedRuntimeSource( stamp: RuntimeSourceStamp | null, userDataPath: string, @@ -21,17 +18,13 @@ export function resolveFencedRuntimeSource( if (!stamp?.profilePath || isSameUserDataPath(userDataPath, stamp.profilePath)) { return stamp } - // Why: the platform default is what an unpinned terminal falls back to, which is the case to fence. + // Fence unpinned terminals falling back to the platform default. return platformDefaultPath !== null && isSameUserDataPath(userDataPath, platformDefaultPath) ? stamp : null } -/** - * Refuses, before the request is sent, a runtime other than the one that launched this process. - * A new incarnation of the same source (restart, update) is accepted so surviving panes keep working. - * Returns what the request must carry so the host itself refuses it if it is not that runtime. - */ +/** Fence requests to their launching source, allowing new incarnations after restarts and updates. */ export class RuntimeSourceFence { private readonly verifiedRuntimeIds = new Set() @@ -48,7 +41,7 @@ export class RuntimeSourceFence { metadata: RuntimeMetadata, method: string ): Promise { - // Why status.get is exempt: it is the read-only probe itself. + // status.get is the read-only probe itself. if (!this.expected || method === 'status.get') { return undefined } @@ -56,7 +49,7 @@ export class RuntimeSourceFence { if (this.verifiedRuntimeIds.has(metadata.runtimeId)) { return expectation } - // Why trust the answer: the transport rejects a response from any runtime but metadata's. + // The transport verifies that this response came from metadata’s runtime. const response = await this.probe(metadata) if (response.ok === false) { throw new RuntimeClientError( @@ -91,7 +84,7 @@ export function createRuntimeSourceFence( try { platformDefaultPath = getPlatformUserDataPath() } catch { - // Why: no resolvable default (Windows without APPDATA) means nothing can fall back to it. + // Without a platform default, no terminal can fall back to it. } return new RuntimeSourceFence( resolveFencedRuntimeSource(readRuntimeSourceStamp(env), userDataPath, platformDefaultPath), diff --git a/src/cli/runtime/serve-app-launcher.ts b/src/cli/runtime/serve-app-launcher.ts index 753851ec7ae..f2d28f13b46 100644 --- a/src/cli/runtime/serve-app-launcher.ts +++ b/src/cli/runtime/serve-app-launcher.ts @@ -3,11 +3,7 @@ import { delimiter, dirname, isAbsolute, join, relative } from 'node:path' import { RuntimeClientError } from './types' import { resolveLaunchUserDataPath } from './launch-user-data-path' -/** - * Only a CLI run by an installed Orca app's executable can start a server. The `orca` that an - * SSH-launched Orca server puts on a remote host runs on plain Node, belongs to that server's - * profile, and is rewritten with each server version. - */ +/** A remote server’s Node-based CLI cannot start a local app server. */ export function canStartOrcaServer(env: NodeJS.ProcessEnv): boolean { const override = env.ORCA_APP_EXECUTABLE return ( @@ -73,7 +69,7 @@ export function resolveServiceCommand( } const name = appCliName(deps.platform) for (const dir of (deps.env.PATH ?? '').split(delimiter).filter(isAbsolute)) { - // Why skip: launchers under the profile or a mounted AppImage point at one version's files. + // Profile and AppImage launchers point at files that disappear with an update. if (!isVersionScopedDir(dir, userData) && isExecutable(join(dir, name))) { return join(dir, name) } diff --git a/src/cli/runtime/serve-appimage-update.test.ts b/src/cli/runtime/serve-appimage-update.test.ts index ee28e3c2807..c9535d0c42a 100644 --- a/src/cli/runtime/serve-appimage-update.test.ts +++ b/src/cli/runtime/serve-appimage-update.test.ts @@ -27,7 +27,6 @@ async function backdate(path: string): Promise { await utimes(path, past, past) } -/** A service the stable launcher started from the copy unpacked from `Orca.AppImage`. */ async function makeRunningGeneration() { const root = await mkdtemp(join(tmpdir(), 'orca-serve-appimage-update-')) created.push(root) @@ -41,7 +40,7 @@ async function makeRunningGeneration() { return { root, appImagePath, executable: join(generationPath, 'orca-ide') } } -/** `mv C B`: a new inode at the same path. */ +/** Replace the inode at the same path, as an AppImage update does. */ async function swapIn(appImagePath: string, content: string): Promise { const replacement = `${appImagePath}.new` await writeFile(replacement, content, { mode: 0o755 }) @@ -154,7 +153,7 @@ describe('supervised serve and a replaced AppImage', () => { expect(stopping.execve).not.toHaveBeenCalled() }) - // Why: a build that dies before its own crash brake must not be exec'd on every service restart. + // Early build failures must not bypass the service’s durable crash brake. it('tries a new AppImage once, until a supervisor starts on it or --force', async () => { const { root, appImagePath, executable } = await makeRunningGeneration() const watch = resolveAppImageBuildWatch(executable, {}, 'linux')! @@ -183,7 +182,6 @@ describe('supervised serve and a replaced AppImage', () => { expect(make({ force: true }).inspect()).toBe('superseded') await make().restart(() => false) - // A supervisor running the attempted build clears the record. make({ runningKey: candidateKey! }) await expect(readFile(attemptPath, 'utf8')).rejects.toThrow() }) diff --git a/src/cli/runtime/serve-appimage-update.ts b/src/cli/runtime/serve-appimage-update.ts index 48037cb6459..b4638a5b5f2 100644 --- a/src/cli/runtime/serve-appimage-update.ts +++ b/src/cli/runtime/serve-appimage-update.ts @@ -1,10 +1,4 @@ -/** - * A supervised serve started from an AppImage keeps running the copy it unpacked (or the mount it - * started from), so replacing the AppImage file never reached it, not even across a full service - * restart. Before each start the supervisor checks that file; once it holds a new build, it execs - * the new AppImage with its own arguments (same pid, so the service manager keeps tracking it). - * That serve unpacks the new build and points the stable launcher at it, as any AppImage serve does. - */ +/** AppImage swaps leave running copies stale; re-exec at the same PID to preserve service tracking. */ import { readFileSync, rmSync, statSync, writeFileSync } from 'node:fs' import { dirname, isAbsolute, join, resolve } from 'node:path' import { runProcess } from '@orca/process-host' @@ -22,13 +16,11 @@ const APPIMAGE_PROBE_TIMEOUT_MS = 60_000 export type AppImageBuildWatch = { appImagePath: string - /** Identity of the AppImage file the running build came from. */ runningKey: string } export type AppImageBuildState = 'current' | 'settling' | 'superseded' -/** Null unless this supervisor runs a build unpacked from, or mounted from, a known AppImage. */ export function resolveAppImageBuildWatch( executable: string, env: NodeJS.ProcessEnv = process.env, @@ -74,7 +66,7 @@ export function inspectAppImageBuild( } export type AppImageRestartDeps = { - /** Checked before the exec: a stop that arrived while probing must not start a new server. */ + /** A stop received during the probe must prevent the exec. */ isStopped: () => boolean argv: readonly string[] env: NodeJS.ProcessEnv @@ -93,8 +85,7 @@ export async function restartFromNewAppImage( ): Promise { deps.log(`[serve] ${watch.appImagePath} holds a new Orca build; restarting from it.`) const env = appImageEnv(deps.env) - // Why probe first: a host without FUSE cannot mount it, and an exec that fails there would - // turn into a service restart loop instead of a server still running the old build. + // Probe FUSE support first to keep mount failures from causing a service restart loop. const refusal = await (deps.probe ?? probeAppImage)(watch.appImagePath, env) if (refusal) { return refusal @@ -148,7 +139,7 @@ function appImageEnv(env: NodeJS.ProcessEnv): NodeJS.ProcessEnv { ]) { delete next[name] } - // Why: node mode moved the user's NODE_OPTIONS aside; the new launch moves it aside again. + // Restore NODE_OPTIONS before the new launcher moves it aside again. if (env.ORCA_NODE_OPTIONS) { next.NODE_OPTIONS = env.ORCA_NODE_OPTIONS } @@ -158,11 +149,7 @@ function appImageEnv(env: NodeJS.ProcessEnv): NodeJS.ProcessEnv { return next } -/** - * The new AppImage this profile's supervisor last exec'd, kept until a supervisor starts on it. - * Why durable: a build that dies before its own crash brake would otherwise be exec'd on every - * service restart of the old one, until the service manager's start limit disables the service. - */ +/** Persist attempted builds to prevent service restart loops when they fail before their own brake. */ export function serveAppImageAttemptPath(userDataPath: string): string { return join(userDataPath, 'serve-supervisor-appimage-attempt.json') } @@ -193,7 +180,7 @@ export function createSupervisedAppImageUpdate( deps: SupervisedAppImageUpdateDeps ): SupervisedAppImageUpdate { const { watch, attemptPath } = deps - // A supervisor running the attempted build proves it started; so does --force, by intent. + // Clear on successful startup of the attempted build or an explicit --force retry. if (deps.force || readAttemptedKey(attemptPath) === watch.runningKey) { rmSync(attemptPath, { force: true }) } diff --git a/src/cli/runtime/serve-bundle-park.test.ts b/src/cli/runtime/serve-bundle-park.test.ts index 81070a59d2c..29e8aa34dc1 100644 --- a/src/cli/runtime/serve-bundle-park.test.ts +++ b/src/cli/runtime/serve-bundle-park.test.ts @@ -125,7 +125,6 @@ describe('openServeBundlePark', () => { return 0 } }) - // While parked the loop holds; release it so the next attempt runs. await vi.waitFor(async () => expect(await readdir(acksDir(dir))).toHaveLength(1)) await writeRecord({ phase: 'unpark' }) await expect(result).resolves.toBe(0) diff --git a/src/cli/runtime/serve-electron-preflight.test.ts b/src/cli/runtime/serve-electron-preflight.test.ts index 48eba25456e..06dd51ecadb 100644 --- a/src/cli/runtime/serve-electron-preflight.test.ts +++ b/src/cli/runtime/serve-electron-preflight.test.ts @@ -154,7 +154,6 @@ describe('preflightElectronServe', () => { expect(run({}, 2_000, () => 'Aqua|ssh=0|sandbox=0')).toBeNull() expect(run({}, 1_000 + 31 * 60_000)).toBeNull() expect(run({ force: true }, 2_000)).toBeNull() - // --force also clears it for the next run. expect(readServeAbortPark(userData)).toBeNull() }) diff --git a/src/cli/runtime/serve-electron-preflight.ts b/src/cli/runtime/serve-electron-preflight.ts index f973f71b486..99b944daa52 100644 --- a/src/cli/runtime/serve-electron-preflight.ts +++ b/src/cli/runtime/serve-electron-preflight.ts @@ -14,13 +14,12 @@ import { probeRuntimeListener, type RuntimeListenerProbe } from './runtime-liste import { getCliStatus } from './status' import { RuntimeClientError } from './types' -/** Who holds this profile, in the execution-boundary vocabulary: only `absent` lets serve start. */ +/** Only an absent owner permits a new serve process. */ export type ServeProfileVerdict = | { verdict: 'live'; evidence: 'rpc' | 'listening'; pid: number | null } | { verdict: 'unverifiable'; reason: 'endpoint-held' | 'access-denied'; pid: number | null } | { verdict: 'absent' } -/** The part of `orca status` the verdict reads. */ type ServeProfileStatus = { result: { app: { pid?: number | null }; runtime: { reachable: boolean } } } @@ -37,11 +36,7 @@ const DEFAULT_DEPS: PreflightDeps = { probeListener: probeRuntimeListener } -/** - * Why before the exec: macOS aborts a second Electron main inside `_RegisterApplication`, before - * any Orca JavaScript can apply the one-Orca-per-profile rule, so the CLI must decide first. - * Only a definitive refusal frees the profile; a recorded pid alone proves nothing (pids recycle). - */ +/** macOS can abort a second Electron before JS runs; only proven absence permits launch. */ export async function decideServeProfileVerdict( userDataPath: string, metadata: RuntimeMetadata, @@ -89,7 +84,7 @@ function refusalEnvelope(code: string, message: string, data: unknown): RuntimeR return { id: 'local', ok: false, error: { code, message, data }, _meta: { runtimeId: null } } } -/** Why: `--json` callers parse stdout; recipe stdout is a strict schema channel, so it gets stderr. */ +/** Recipe stdout has a strict schema; refusals belong on stderr. */ function writeRefusal( args: { json?: boolean; recipeJson?: boolean }, envelope: RuntimeRpcFailure @@ -101,7 +96,7 @@ function writeRefusal( } } -/** What separates "this environment" from another: launchd session type, SSH, app sandbox. */ +/** Separate launchd sessions, SSH environments, and app sandboxes. */ export function serveEnvironmentFingerprint(env: NodeJS.ProcessEnv = process.env): string { let manager = 'unknown' try { @@ -123,10 +118,7 @@ export type ElectronServePreflightArgs = { force?: boolean } -/** - * Null means start now. A number is the exit code of a refusal already reported. Synchronous - * when nothing has ever published this profile, so a fresh launch keeps its old timing. - */ +/** Null permits launch; refusal returns an exit code. Fresh profiles are checked synchronously. */ export function preflightElectronServe( args: ElectronServePreflightArgs, userDataPath: string, @@ -155,7 +147,7 @@ export function preflightElectronServe( return refuseIfProfileServed(args, userDataPath, options.deps) } -/** The runtime-neutral half: refuse with exit 3 while another Orca serves or may serve this profile. */ +/** Exit 3 means another runtime serves or may serve this profile. */ export function refuseIfProfileServed( args: ElectronServePreflightArgs, userDataPath: string, diff --git a/src/cli/runtime/serve-install-hold.ts b/src/cli/runtime/serve-install-hold.ts index c9cfc378487..1c1a909ce66 100644 --- a/src/cli/runtime/serve-install-hold.ts +++ b/src/cli/runtime/serve-install-hold.ts @@ -22,17 +22,14 @@ export const SERVE_INSTALL_HOLD_REASON_TEXT: Record< type ListProcesses = () => Promise -/** - * Whether a supervised serve may spawn from `executable` right now. Never spawn from a bundle an - * installer is swapping: the child would run half-old, half-new code or be killed mid-start. - */ +/** Installer swaps can expose mixed code or kill a child mid-start; hold spawns until they finish. */ export async function readServeInstallEvidence( executable: string, options: { platform?: NodeJS.Platform; listProcesses?: ListProcesses } = {} ): Promise { const platform = options.platform ?? process.platform const bundlePath = platform === 'darwin' ? macBundlePathOf(executable) : null - // Why ShipIt first: mid-swap the executable is briefly missing, and that must not look like removal. + // ShipIt temporarily removes the executable during swaps; do not treat that as uninstall. if (bundlePath) { const listing = await (options.listProcesses ?? listProcessCommands)() if (listing === null) { @@ -50,10 +47,7 @@ function macBundlePathOf(executable: string): string | null { return bundle.endsWith('.app') ? bundle : null } -/** - * Changes whenever the installed app does, so a new build starts with a clean restart count. The - * app archive is included because Linux releases on one Electron version share the launcher binary. - */ +/** Include the archive: Linux releases can share one Electron launcher binary. */ export function serveBuildIdentity(executable: string, appRoot?: string): string { return [executable, ...(appRoot ? [appRoot] : [])] .map((path) => { diff --git a/src/cli/runtime/serve-orcad-launch.test.ts b/src/cli/runtime/serve-orcad-launch.test.ts index 1bc1bf4b654..1d3de3b395a 100644 --- a/src/cli/runtime/serve-orcad-launch.test.ts +++ b/src/cli/runtime/serve-orcad-launch.test.ts @@ -102,7 +102,6 @@ describe('orca serve asking the app which host to run', () => { expect(orcadServeArgs({})).toEqual(['--bind', '0.0.0.0']) }) - // #30100: the slot carries no browser client, so without this orcad served no web page. it("hands orcad the app's browser client directory when the app has one", () => { expect( orcadServeArgs({ port: '6768' }, '/opt/Orca/resources/app.asar.unpacked/out/web') diff --git a/src/cli/runtime/serve-orcad-launch.ts b/src/cli/runtime/serve-orcad-launch.ts index 4276cd7e907..3d63f6fb3e6 100644 --- a/src/cli/runtime/serve-orcad-launch.ts +++ b/src/cli/runtime/serve-orcad-launch.ts @@ -1,7 +1,4 @@ -/** - * `orca serve` on this machine's orcad slot. Whether to (and the slot itself) is decided - * app-side by `src/main/orcad/orcad-local-serve-selection.ts`; the CLI only asks and runs. - */ +/** The app selects the local serve runtime; the CLI runs that selection. */ import { dirname, join } from 'node:path' import { runProcess, spawnProcess } from '@orca/process-host' import type { ProcessSpec } from '@orca/process-host/process-spec' @@ -21,7 +18,6 @@ import { SPEECH_RECOGNIZER_DIR_ENV } from '../../shared/speech-recognizer-packag /** A first run may download and verify the pinned Node; bound it well past that. */ const SELECTION_TIMEOUT_MS = 10 * 60_000 -/** Asks the app's own entry, run on the app's executable as plain Node, which host to serve on. */ export async function resolveLocalServeRuntime( options: { executable: string @@ -31,8 +27,7 @@ export async function resolveLocalServeRuntime( }, run: typeof runProcess = runProcess ): Promise { - // Why: only packaged macOS serve can take a remote app update, through Electron's updater and - // this CLI's supervisor; orcad has no updater, so switching would drop that. + // orcad has no updater; keep Electron for packaged macOS update handoffs. if (options.usesMacUpdateHandoff) { return { kind: 'electron', @@ -46,8 +41,7 @@ export async function resolveLocalServeRuntime( program: options.executable, args: [entry, FLAGS.userData, options.userDataPath, FLAGS.appRoot, options.appRoot], env: { ...process.env, ELECTRON_RUN_AS_NODE: '1' }, - // Why inherit stderr: a first run may download the pinned Node, and that progress is the - // only sign `orca serve` is not hung. + // Show pinned-Node download progress so first-run selection does not appear hung. stdio: ['ignore', 'pipe', 'inherit'], timeoutMs: SELECTION_TIMEOUT_MS }) @@ -74,7 +68,7 @@ export function serveWithOrcad( baseEnv: NodeJS.ProcessEnv, /** The app's browser client; the slot does not carry one, so orcad serves it from here. */ webClientRoot?: string, - /** The app's speech recognizer; the slot does not carry one either. */ + /** orcad’s slot has no speech recognizer; use the app’s copy. */ speechRecognizerDir?: string ): Promise { const spec: ProcessSpec = { diff --git a/src/cli/runtime/serve-pairing-redaction.ts b/src/cli/runtime/serve-pairing-redaction.ts index ae7c68c80b4..804979f47d4 100644 --- a/src/cli/runtime/serve-pairing-redaction.ts +++ b/src/cli/runtime/serve-pairing-redaction.ts @@ -5,10 +5,7 @@ import { createInterface } from 'node:readline' export const PAIRING_HIDDEN_LINE = 'Pairing URL: hidden because this supervised server is not writing to a terminal. Pair from the desktop app, or run `orca serve` in a terminal.' -/** - * Drops the human readiness block's pairing secrets: the web-client URL (it embeds the offer), - * the mobile QR, and the pairing URL itself. Stateful because the QR spans many lines. - */ +/** Remove readiness pairing secrets, retaining state for multiline QR output. */ export function createPairingRedactor(): (line: string) => string | null { let inQr = false return (line) => { @@ -39,16 +36,11 @@ function redactReadyJson(line: string): string { pairing: { available: false, reason: 'hidden', guidance: PAIRING_HIDDEN_LINE } }) } - } catch { - // Not the readiness payload after all; pass it through. - } + } catch {} return line } -/** - * Only for `--supervise` writing to a log: a service journal must never hold a live bearer - * offer. A plain `orca serve` keeps inheriting stdout, which scripts parse for `Pairing URL:`. - */ +/** Redact service logs; plain serve retains pairing stdout for scripts. */ export function shouldRedactSupervisedStdout( supervise: boolean, stdoutIsTty: boolean = process.stdout.isTTY === true @@ -60,7 +52,6 @@ let stdoutErrorsIgnored = false export type ServeSpawnChild = Parameters[0]['spawnChild'] -/** Pipes the child's stdout through the redactor; stdio slots other than stdout are kept. */ export function redactingSpawn(spawn: ServeSpawnChild): ServeSpawnChild { return (spec) => { const stdio: ProcessSpec['stdio'] = Array.isArray(spec.stdio) @@ -68,7 +59,7 @@ export function redactingSpawn(spawn: ServeSpawnChild): ServeSpawnChild { : ['inherit', 'pipe', 'inherit'] const child = spawn({ ...spec, stdio }) if (child.stdout) { - // Why: a closed log pipe must not crash the supervisor; the server keeps running. + // Closed log pipes must not crash the supervisor. child.stdout.on('error', () => undefined) if (!stdoutErrorsIgnored) { stdoutErrorsIgnored = true diff --git a/src/cli/runtime/serve-pid1-init-reaper.ts b/src/cli/runtime/serve-pid1-init-reaper.ts index 6adefa8b395..d1c58e4f0a1 100644 --- a/src/cli/runtime/serve-pid1-init-reaper.ts +++ b/src/cli/runtime/serve-pid1-init-reaper.ts @@ -3,17 +3,12 @@ import { isAbsolute, relative, resolve } from 'node:path' const BASH_CANDIDATES = ['/bin/bash', '/usr/bin/bash'] -/** - * Runs as PID 1: bash's wait reaps every exited child, including orphans the kernel reparents to - * it, which Node cannot do (#28147). PID 1 ignores untrapped signals, so each is forwarded. - * The served command runs in a forked subshell, so it is never PID 1 itself. - * Why the subshell: async commands in a non-interactive shell otherwise start with INT/QUIT ignored. - */ +/** Bash reaps PID 1’s orphans; a subshell restores INT/QUIT, and traps forward PID 1’s signals (#28147). */ export const SERVE_INIT_REAPER_SCRIPT = [ '(trap - INT QUIT; exec "$@") 0<&0 &', 'child=$!', 'for sig in TERM INT HUP QUIT USR1 USR2; do trap "kill -$sig $child 2>/dev/null" "$sig"; done', - // Why the final wait: a trap can interrupt one, and bash keeps the reaped child's own status. + // Traps can interrupt wait; the final wait retains the reaped child’s status. 'while kill -0 "$child" 2>/dev/null; do wait "$child"; done', 'wait "$child"' ].join('\n') @@ -46,18 +41,18 @@ function isInside(parent: string, child: string): boolean { function servedCommand(deps: ServeInitReaperDeps): ServedCommand | null { const evaluated = deps.execArgv.some((flag) => EVAL_FLAGS.has(flag)) - // Why: argv[1] is the CLI entry, except under `-e -- serve` (the legacy AppImage wrapper). + // Legacy AppImage wrappers evaluate the CLI with -e, shifting its argv offset. const cliArgs = deps.argv.slice(evaluated ? 1 : 2) const appDir = deps.env.APPDIR if (appDir && isAbsolute(appDir) && isInside(appDir, deps.execPath)) { - // Why: the exec would release this AppImage's mount under bash, so run the image afresh instead. + // Re-execing into bash releases the current AppImage mount; start the image afresh. const image = deps.env.APPIMAGE if (!image || !isAbsolute(image) || !deps.exists(image)) { return null } const env = { ...deps.env } delete env.ELECTRON_RUN_AS_NODE - // Why: the image's own CLI redirect moves these aside again; restore what the user set. + // Restore user options before the AppImage launcher moves them aside again. for (const name of ['NODE_OPTIONS', 'NODE_REPL_EXTERNAL_MODULE']) { const saved = env[`ORCA_${name}`] delete env[`ORCA_${name}`] @@ -71,7 +66,7 @@ function servedCommand(deps: ServeInitReaperDeps): ServedCommand | null { return { args: [deps.execPath, ...deps.execArgv, ...entry, ...cliArgs], env: deps.env } } -/** Replaces a PID 1 Linux serve with bash supervising the same command; returns only when it did not. */ +/** Replace Linux PID 1 serve with a Bash reaper; return only if no replacement occurred. */ export function reexecServeUnderInitReaper(deps: ServeInitReaperDeps): ServeInitReaperOutcome { if (deps.platform !== 'linux' || deps.pid !== 1) { return 'not-pid1' diff --git a/src/cli/runtime/serve-recipe-json.ts b/src/cli/runtime/serve-recipe-json.ts index 3fea331d86b..c2aae3397c3 100644 --- a/src/cli/runtime/serve-recipe-json.ts +++ b/src/cli/runtime/serve-recipe-json.ts @@ -8,7 +8,6 @@ import { RuntimeClientError } from './types' const IGNORED_NON_RECIPE_STDOUT = '[serve] ignored non-recipe stdout' -/** Relays the one recipe line a detached serve child prints, then lets the CLI exit. */ export function waitForRecipeJson(child: ChildProcessHandle): Promise { return new Promise((resolve, reject) => { let output = '' @@ -35,8 +34,7 @@ export function waitForRecipeJson(child: ChildProcessHandle): Promise { resolve(0) } const writeIgnoredRecipeStdout = (): void => { - // Why: non-readiness child stdout is untrusted and cannot be safely - // redacted, including schema-valid results with arbitrary user data. + // Arbitrary child output cannot be reliably redacted, even when it matches the schema. process.stderr.write(`${IGNORED_NON_RECIPE_STDOUT}\n`) } const processRecipeOutputLine = (line: string): void => { @@ -94,8 +92,7 @@ export function waitForRecipeJson(child: ChildProcessHandle): Promise { } child.stdout?.on('data', onData) child.once('error', onError) - // Why: `exit` can precede the final piped stdout data. `close` waits until - // stdio closes so a last recipe chunk is not mistaken for missing output. + // Wait for close: exit may precede the final recipe data on stdout. child.once('close', onClose) }) } diff --git a/src/cli/runtime/serve-replaced-app-restart.ts b/src/cli/runtime/serve-replaced-app-restart.ts index 2d3195577dc..fd69b99b897 100644 --- a/src/cli/runtime/serve-replaced-app-restart.ts +++ b/src/cli/runtime/serve-replaced-app-restart.ts @@ -1,9 +1,4 @@ -/** - * macOS charges the permissions of every process a launchd-started supervisor spawns to that - * supervisor, so once an update deletes the app it runs from, no server may start under it. Before - * each server starts, a supervisor whose executable was replaced re-runs itself in place (same pid) - * from the installed app. - */ +/** Re-exec replaced apps before spawning: macOS attributes child permissions to the launchd supervisor. */ import { accessSync, constants, existsSync, statSync } from 'node:fs' import { join } from 'node:path' import { @@ -13,7 +8,6 @@ import { type SupervisorRelocationDeps } from './serve-supervisor-relocation' -/** `dev:ino` of a file, which changes when an update replaces the app; null if unreadable. */ function fileIdentity(path: string): string | null { try { const stats = statSync(path) @@ -32,7 +26,7 @@ function isExecutable(path: string): boolean { } } -// Why process state: the first server may start long after an update wait or a park. +// A park or update wait can delay the first server until after bundle replacement. let startedFrom: string | null = null export function noteServeSupervisorStart(): void { @@ -47,16 +41,16 @@ export type ReplacedAppRestartDeps = Pick< /** Returns only when it did not restart. */ export function restartServeSupervisorIfReplaced(deps: ReplacedAppRestartDeps): void { const installed = fileIdentity(deps.execPath) - // Why: after a restart the start identity is the installed one, so this never loops. + // Re-exec resets the identity to the installed app, preventing a loop. if (!deps.startedFrom || !installed || installed === deps.startedFrom) { return } const env = { ...deps.env } const relocated = Boolean(env[SERVE_SUPERVISOR_APP_ROOT_ENV]) delete env[SERVE_SUPERVISOR_APP_ROOT_ENV] - // Why the app's CLI: a relocated supervisor must copy the new release aside, not rerun the old copy. + // Relocated supervisors must copy the new release rather than rerun their old copy. const cliEntry = relocated ? join(deps.appRoot, 'out', 'cli', 'index.js') : deps.argv[1] - // Why preflight: a failed process.execve aborts the process natively instead of throwing. + // A failed process.execve aborts natively; preflight the executable and entry point. if (!isExecutable(deps.execPath) || !existsSync(cliEntry)) { deps.log('[serve] Keeping the old executable; the installed app is not ready to run.') return diff --git a/src/cli/runtime/serve-service-unit.ts b/src/cli/runtime/serve-service-unit.ts index 232d5f17c9f..4688b41ff87 100644 --- a/src/cli/runtime/serve-service-unit.ts +++ b/src/cli/runtime/serve-service-unit.ts @@ -1,4 +1,3 @@ -/** Pure rendering of the per-user service units `orca serve service install` writes. */ import { createHash } from 'node:crypto' import { join } from 'node:path' import { SERVE_LAUNCHD_LABEL_PREFIX } from '../../shared/serve-bundle-coordination' @@ -27,7 +26,7 @@ export type ServeServiceInput = { pairingAddress?: string | null } -/** Why `--no-pairing`: a service's output lands in a log, which must never hold a live offer. */ +/** Service logs must never contain a live pairing offer. */ export function serveServiceArgs( input: Pick ): string[] { @@ -56,7 +55,7 @@ export function renderServeService(input: ServeServiceInput): ServeServiceSpec { throw new Error(`No service manager support on ${input.platform}`) } const suffix = profileSuffix(input) - // Why always pin: the manager's environment may resolve a different default profile. + // The manager’s environment may resolve a different default profile. const env: Record = { PATH: input.path, ORCA_USER_DATA_PATH: input.userDataPath } const argv = [input.command, ...serveServiceArgs(input)] if (manager === 'launchd') { @@ -99,8 +98,7 @@ function renderLaunchAgent( const envEntries = Object.entries(env) .map(([key, value]) => ` ${xml(key)}\n ${xml(value)}`) .join('\n') - // Why KeepAlive on failure only: the supervisor exits 0 after its crash-loop brake gives up, and - // launchd has no start-limit key, so a clean exit is the only way to make it stop. + // launchd has no start limit; a clean crash-brake exit must stop restarts. return ` @@ -147,8 +145,7 @@ function renderSystemdUnit(argv: string[], env: Record): string const environment = Object.entries(env) .map(([key, value]) => `Environment=${systemdQuote(`${key}=${value}`)}`) .join('\n') - // Why KillMode=process: stopping the service stops the supervisor, which stops its server; the - // terminal daemon is designed to outlive the server, as it outlives the desktop app. + // Kill only the supervisor: it stops its server while the terminal daemon survives. return `[Unit] Description=Orca server (orca serve --supervise) StartLimitIntervalSec=900 diff --git a/src/cli/runtime/serve-speech-recognizer.test.ts b/src/cli/runtime/serve-speech-recognizer.test.ts index 67e08eadc21..6bd7203fc72 100644 --- a/src/cli/runtime/serve-speech-recognizer.test.ts +++ b/src/cli/runtime/serve-speech-recognizer.test.ts @@ -11,7 +11,6 @@ afterEach(() => { } }) -/** A packaged app's `resources`, holding a recognizer package; the app root is app.asar.unpacked. */ function resourcesWith(packageName: string): string { const resources = mkdtempSync(join(tmpdir(), 'orca-serve-speech-')) roots.push(resources) diff --git a/src/cli/runtime/serve-supervised-service.test.ts b/src/cli/runtime/serve-supervised-service.test.ts index 8485a931326..3a16826a7f8 100644 --- a/src/cli/runtime/serve-supervised-service.test.ts +++ b/src/cli/runtime/serve-supervised-service.test.ts @@ -86,7 +86,6 @@ describe('superviseServeService', () => { const record = readServeSupervisorBrake(userDataPath) expect(record?.gaveUp?.reason).toContain('exited with code 1') - // A restart by the service manager must not start the loop again. let rerun = 0 const again = harness({ userDataPath, diff --git a/src/cli/runtime/serve-supervised-service.ts b/src/cli/runtime/serve-supervised-service.ts index 4fee03a5e38..8d59b578051 100644 --- a/src/cli/runtime/serve-supervised-service.ts +++ b/src/cli/runtime/serve-supervised-service.ts @@ -27,7 +27,7 @@ import type { AppImageBuildState } from './serve-appimage-update' export const SERVE_INSTALL_HOLD_POLL_MS = 2_000 /** A hold this long is reported as stuck; it still never times out into a spawn. */ export const SERVE_INSTALL_HOLD_STUCK_MS = 30 * 60_000 -/** A version-specific executable this long gone was removed by an update, not swapped by one. */ +/** After this delay, exit so the service manager can find the updated launcher. */ export const SERVE_EXECUTABLE_MISSING_EXIT_MS = 2 * 60_000 type StopListener = () => void @@ -112,10 +112,7 @@ async function settle(run: () => Promise): Promise { const now = options.now ?? Date.now const random = options.random ?? Math.random @@ -127,7 +124,7 @@ export async function superviseServeService(options: SupervisedServeOptions): Pr ((executable: string) => serveBuildIdentity(executable, options.appRoot)) const writeBrake = options.writeBrake ?? writeServeSupervisorBrake - // Why not keyed yet: mid-install the executable may be missing, which must not reset a give-up. + // Mid-install missing executables must not reset the crash brake. let record: ServeSupervisorBrake | null = (options.readBrake ?? readServeSupervisorBrake)( options.userDataPath ) @@ -139,7 +136,7 @@ export async function superviseServeService(options: SupervisedServeOptions): Pr log(`[serve] Not starting: ${gaveUp.reason} ${SERVE_GAVE_UP_ADVICE}`) return 0 } - // Why the AppImage check: a replaced AppImage is a new build, which a give-up must not block. + // A new AppImage deserves a retry even after the previous build hit the brake. if ( record?.gaveUp && record.build === identify(options.executable) && @@ -192,7 +189,6 @@ export async function superviseServeService(options: SupervisedServeOptions): Pr evidence.reason === 'bundle-missing' && now() - since >= SERVE_EXECUTABLE_MISSING_EXIT_MS ) { - // The outer loop sees the same hold and hands the restart to the service manager. return false } await sleep(SERVE_INSTALL_HOLD_POLL_MS) @@ -225,7 +221,7 @@ export async function superviseServeService(options: SupervisedServeOptions): Pr evidence.reason === 'bundle-missing' && now() - held.since >= SERVE_EXECUTABLE_MISSING_EXIT_MS ) { - // Why exit 1: the manager restarts the CLI through the stable launcher, which finds the installed build. + // Let the service manager find the updated app through its stable launcher. log( '[serve] The Orca executable this service started from is gone; exiting so the service manager starts the installed Orca.' ) @@ -242,7 +238,7 @@ export async function superviseServeService(options: SupervisedServeOptions): Pr hold.current = null const appImage = appImageUpdate?.inspect() ?? 'current' if (appImage === 'settling') { - // Why: the AppImage file is still being written; starting either build now would be wrong. + // Wait until the new AppImage finishes copying before choosing a build. await sleep(SERVE_INSTALL_HOLD_POLL_MS) continue } @@ -265,14 +261,14 @@ export async function superviseServeService(options: SupervisedServeOptions): Pr break } if (attempt.parked) { - // Why not a failure: the desktop stopped this server to install an update. + // Desktop update parks do not count against the crash brake. continue } let failure: string if (attempt.settled.status === 'fulfilled') { const code = attempt.settled.value if (code === SINGLE_INSTANCE_ALREADY_RUNNING_EXIT_CODE) { - // Why not a failure: another Orca serves this profile, so wait for it instead of racing it. + // An occupied profile warrants standby, not a crash-brake failure. const delayMs = standbyDelayMs(standbyAttempt++, random) log( `[serve] Another Orca serves this profile; checking again in ${Math.round(delayMs / 1000)}s.` @@ -283,7 +279,6 @@ export async function superviseServeService(options: SupervisedServeOptions): Pr failure = `exited with code ${code}` } else { const error: unknown = attempt.settled.reason - // Why: the child's own errors read "Orca serve exited via SIGKILL."; the log line names serve once. failure = (error instanceof Error ? error.message : String(error)) .replace(/^Orca serve /, '') .replace(/\.$/, '') diff --git a/src/cli/runtime/serve-supervised-stop.ts b/src/cli/runtime/serve-supervised-stop.ts index b3e767c2c54..41ef65cbfb0 100644 --- a/src/cli/runtime/serve-supervised-stop.ts +++ b/src/cli/runtime/serve-supervised-stop.ts @@ -1,4 +1,3 @@ -/** Process-wide stop and spawn gate for `orca serve --supervise`, shared with the update loop. */ let supervisedStopRequested = false let supervisedStop = new AbortController() let supervisedSpawnGate: (() => Promise) | null = null @@ -26,7 +25,7 @@ export function isSupervisedSpawnHeld(): boolean { return supervisedStopRequested || (supervisedSpawnHeld?.() ?? false) } -/** Why: update replacements spawn inside this loop, so the install hold must gate them too. */ +/** Update-loop replacements must honor the same install hold. */ export async function mayRespawn(): Promise { if (supervisedStopRequested) { return false @@ -41,7 +40,6 @@ export function supervisedStopSignal(): AbortSignal { type ServeChild = { pid: number | null; stop: () => void } let currentChild: ServeChild | null = null -/** The child a supervised serve is waiting on right now, so a desktop install can park it. */ export function currentServeChild(): ServeChild | null { return currentChild } diff --git a/src/cli/runtime/serve-supervisor-brake.ts b/src/cli/runtime/serve-supervisor-brake.ts index 5ccd562e6d8..6a1cada5aa4 100644 --- a/src/cli/runtime/serve-supervisor-brake.ts +++ b/src/cli/runtime/serve-supervisor-brake.ts @@ -1,7 +1,6 @@ import { mkdirSync, readFileSync, renameSync, rmSync, writeFileSync } from 'node:fs' import { join } from 'node:path' -/** Failures inside this window count toward giving up; older ones have aged out. */ export const SERVE_SUPERVISOR_FAILURE_WINDOW_MS = 15 * 60_000 /** This many failures inside the window and the supervisor gives up until a new build or --force. */ export const SERVE_SUPERVISOR_FAILURE_LIMIT = 6 @@ -9,17 +8,12 @@ export const SERVE_SUPERVISOR_BACKOFF_BASE_MS = 1_000 export const SERVE_SUPERVISOR_BACKOFF_MAX_MS = 30_000 export const SERVE_SUPERVISOR_STANDBY_BASE_MS = 5_000 export const SERVE_SUPERVISOR_STANDBY_MAX_MS = 60_000 -/** One piece of advice wherever a give-up is reported; reinstalling the service clears the brake. */ export const SERVE_GAVE_UP_ADVICE = 'Fix the cause, then run `orca serve service install` again (or, outside a service, `orca serve --supervise --force` once).' -/** - * Durable per profile, so the brake survives the service manager restarting the supervisor: - * launchd has no start-limit key and would otherwise loop forever. - */ +/** Persist across service-manager restarts; launchd has no start limit. */ export type ServeSupervisorBrake = { schemaVersion: 1 - /** Identity of the installed app build the failures were counted against. */ build: string failures: number[] gaveUp: { at: number; reason: string } | null @@ -37,7 +31,6 @@ export function emptyServeSupervisorBrake(build: string): ServeSupervisorBrake { return { schemaVersion: 1, build, failures: [], gaveUp: null } } -/** A record for another build starts over: a new install deserves a fresh try. */ export function brakeForBuild( record: ServeSupervisorBrake | null, build: string @@ -45,7 +38,6 @@ export function brakeForBuild( return record && record.build === build ? record : emptyServeSupervisorBrake(build) } -/** Pure: counts one unexpected exit and says how long to wait, or that it is time to stop. */ export function recordSupervisedFailure( record: ServeSupervisorBrake, input: { now: number; reason: string; random: () => number } @@ -87,7 +79,7 @@ export function standbyDelayMs(attempt: number, random: () => number): number { } function jitter(ms: number, random: () => number): number { - // Why ±20%: services on one machine that failed together should not retry in lockstep. + // Jitter prevents services that failed together from retrying in lockstep. return Math.round(ms * (0.8 + 0.4 * random())) } @@ -137,7 +129,7 @@ export function writeServeSupervisorBrake( const target = serveSupervisorBrakePath(userDataPath) const temporary = `${target}.${process.pid}.tmp` try { - // Why: a service's first run can fail before the server ever created its profile. + // The server may fail before creating its profile. mkdirSync(userDataPath, { recursive: true, mode: 0o700 }) writeFileSync(temporary, JSON.stringify(record), { mode: 0o600 }) renameSync(temporary, target) @@ -150,7 +142,5 @@ export function writeServeSupervisorBrake( export function clearServeSupervisorBrake(userDataPath: string): void { try { rmSync(serveSupervisorBrakePath(userDataPath), { force: true }) - } catch { - // Nothing recorded, or a read-only profile: nothing of ours to clear. - } + } catch {} } diff --git a/src/cli/runtime/serve-supervisor-policy.ts b/src/cli/runtime/serve-supervisor-policy.ts index c709fa6c76b..488f409dd16 100644 --- a/src/cli/runtime/serve-supervisor-policy.ts +++ b/src/cli/runtime/serve-supervisor-policy.ts @@ -27,14 +27,9 @@ export type ServeSupervisorDecision = /** This child asked to install an update: wait for the new bundle, then serve from it. */ | { kind: 'await-install'; handoff: InstallRequestedHandoff } -/** - * What the CLI supervisor does after its serve child exits. Pure, so the restart rules can be - * model-tested apart from process and file I/O; the loop in serve-update-supervisor only acts. - */ export function decideAfterServeChildExit(input: { exit: ServeChildExit expectedHandoff: InstallRequestedHandoff | null - /** The handoff on disk after the exit; null when there is none or no handoff path. */ handoff: ServeUpdateHandoffState | null hasHandoffPath: boolean }): ServeSupervisorDecision { diff --git a/src/cli/runtime/serve-supervisor-relocation.test.ts b/src/cli/runtime/serve-supervisor-relocation.test.ts index 66a9c17e6a8..c1d39a1ebfd 100644 --- a/src/cli/runtime/serve-supervisor-relocation.test.ts +++ b/src/cli/runtime/serve-supervisor-relocation.test.ts @@ -70,7 +70,6 @@ describe('relocateServeSupervisor', () => { const snapshotCli = String(args[1]) expect(snapshotCli.startsWith(serveSupervisorSnapshotRoot(userData))).toBe(true) expect(args).toEqual([d.execPath, snapshotCli, 'serve', '--supervise']) - // The copy keeps the Resources layout, so bare requires still find node_modules. const snapshot = join(snapshotCli, '..', '..', '..', '..') expect(existsSync(join(snapshot, 'node_modules', 'ws', 'index.js'))).toBe(true) expect(existsSync(join(snapshot, 'app.asar.unpacked', 'out', 'package.json'))).toBe(true) @@ -104,8 +103,7 @@ describe('relocateServeSupervisor', () => { } }) - // Why: re-exec from node mode closes the AppImage runtime's keepalive pipe, so the mount is - // torn down under the exec and the new image fails to load its libraries (real-host BUG-3). + // Re-exec closes the AppImage keepalive pipe and unmounts libraries needed by the new image. it('stays in place when running from an AppImage, whose files cannot change under it', async () => { const d = deps({ env: { @@ -137,7 +135,7 @@ describe('relocateServeSupervisor', () => { await expect(relocateServeSupervisor(failing)).resolves.toBe('in-place') }) - // Why skip: mode 000 blocks neither root nor Windows readers. + // Mode 000 does not block root or Windows readers. it.skipIf(process.platform === 'win32' || process.getuid?.() === 0)( 'removes a partial copy when copying fails', async () => { @@ -183,7 +181,7 @@ describe('relocateServeSupervisor', () => { expect(await readlink(join(copied, 'absolute'))).toBe(join(root, 'outside')) expect(await readlink(join(copied, 'ws-alias'))).toBe('ws') expect((await lstat(join(copied, 'ws-alias'))).isSymbolicLink()).toBe(true) - // Why: `cp -R` into an existing directory would nest the tree one level down. + // Copying into an existing directory would nest the tree one level too deep. expect(await readdir(snapshot)).toEqual( expect.arrayContaining(['app.asar.unpacked', 'node_modules']) ) @@ -255,7 +253,7 @@ describe('relocateServeSupervisor', () => { expect(restart({}, argv, { execPath: gone }).execve).not.toHaveBeenCalled() }) - // Why: a failed process.execve aborts natively, so nothing may reach it that cannot run. + // Failed execve aborts natively, so preflight must reject unusable files. it('keeps running and logs when the installed app cannot run yet', async () => { const argv = ['/ignored', appCli(), 'serve'] await chmod(deps().execPath, 0o644) diff --git a/src/cli/runtime/serve-supervisor-relocation.ts b/src/cli/runtime/serve-supervisor-relocation.ts index 1daba10cb29..995171c06a8 100644 --- a/src/cli/runtime/serve-supervisor-relocation.ts +++ b/src/cli/runtime/serve-supervisor-relocation.ts @@ -1,9 +1,4 @@ -/** - * `orca serve --supervise` re-runs itself from a copy of the CLI outside the app. The CLI loads - * modules lazily from the app's files, so a desktop update that swaps the app under a running - * supervisor would otherwise mix two releases' code inside one process. The served child still - * starts from the app, so it updates as before. - */ +/** Copy supervisors outside the app so updates cannot mix lazily loaded modules from two releases. */ import { createHash } from 'node:crypto' import { constants, readFileSync, rmSync, statSync } from 'node:fs' import { cp, mkdir, readdir, rename, rm, stat, writeFile } from 'node:fs/promises' @@ -11,7 +6,7 @@ import { basename, dirname, isAbsolute, join, relative, resolve } from 'node:pat import { cloneMacTree } from '../../main/macos-tree-clone' import { isProcessAlive } from '../../shared/serve-bundle-coordination' -/** Set on the re-run supervisor: the app root its own `__dirname` no longer points at. */ +/** Relocated supervisors can no longer derive the app root from __dirname. */ export const SERVE_SUPERVISOR_APP_ROOT_ENV = 'ORCA_SERVE_SUPERVISOR_APP_ROOT' const PACKAGED_APP_ROOT_NAME = 'app.asar.unpacked' @@ -60,15 +55,15 @@ async function exists(path: string): Promise { ) } -/** Who runs from each copy, kept beside the copies so it never blocks publishing one. */ +/** Keep PID claims outside snapshots so they cannot block publication. */ const usersDir = (root: string, id: string): string => join(root, USERS_DIR, id) async function copyTree(source: string, destination: string): Promise { - // Why not fs.cp on macOS: it clones file by file, several times slower than one `cp -c`. + // macOS cp -c clones a tree faster than fs.cp’s per-file cloning. if (process.platform === 'darwin') { return cloneMacTree(source, destination) } - // Why FICLONE: on reflink filesystems the copy shares blocks with the app. + // Share app blocks on reflink filesystems. await cp(source, destination, { recursive: true, verbatimSymlinks: true, @@ -82,7 +77,7 @@ async function buildSnapshot( id: string, pid: number ): Promise { - // Why mark before checking: another supervisor's cleanup keeps any copy with a live user. + // Claim before checking so concurrent cleanup preserves this snapshot. await mkdir(usersDir(root, id), { recursive: true }) await writeFile(join(usersDir(root, id), String(pid)), '') const snapshotDir = join(root, id) @@ -93,7 +88,7 @@ async function buildSnapshot( await rm(temporary, { recursive: true, force: true }) try { await mkdir(temporary) - // Why both: bare requires from out/ walk up into Resources/node_modules. + // Bare requires from out/ resolve through Resources/node_modules. for (const name of [PACKAGED_APP_ROOT_NAME, 'node_modules']) { if (await exists(join(resources, name))) { await copyTree(join(resources, name), join(temporary, name)) @@ -101,7 +96,7 @@ async function buildSnapshot( } await writeFile(join(temporary, COMPLETE_MARKER), '') } catch (error) { - // Why: a partial copy on a full disk would otherwise keep that space for the service's life. + // Failed copies must not retain disk space for the service’s lifetime. await rm(temporary, { recursive: true, force: true }).catch(() => undefined) throw error } @@ -142,11 +137,7 @@ async function removeUnusedSnapshots( } } -/** - * An AppImage's files are its own mount (or extracted copy) for the whole run, so no update can - * swap them. Re-exec from node mode would also close the runtime's keepalive pipe, which unmounts - * the AppImage under the exec and leaves the new image unable to load its libraries. - */ +/** AppImages keep stable files; re-exec would close their keepalive pipe and release the mount. */ function runsFromAppImage(appRoot: string, env: NodeJS.ProcessEnv): boolean { const appDir = env.APPDIR if (!appDir || !isAbsolute(appDir)) { @@ -156,17 +147,14 @@ function runsFromAppImage(appRoot: string, env: NodeJS.ProcessEnv): boolean { return inside !== '' && !inside.startsWith('..') && !isAbsolute(inside) } -/** - * Resolves only when the supervisor keeps running from where it is; a successful re-run replaces - * this process (same pid, so the service manager keeps tracking it) and never returns. - */ +/** A successful relocation replaces this process at the same PID and never returns. */ export async function relocateServeSupervisor( deps: SupervisorRelocationDeps ): Promise<'in-place' | 'already-relocated' | 'relocated'> { if (deps.env[SERVE_SUPERVISOR_APP_ROOT_ENV]) { return 'already-relocated' } - // Why packaged only: a dev or overridden executable has no app swapped under it by an update. + // Dev and overridden executables are not swapped by app updates. const override = deps.env.ORCA_APP_EXECUTABLE if ( deps.env.ELECTRON_RUN_AS_NODE !== '1' || diff --git a/src/cli/runtime/serve-update-supervisor.ts b/src/cli/runtime/serve-update-supervisor.ts index 1249c67540c..40924bd8023 100644 --- a/src/cli/runtime/serve-update-supervisor.ts +++ b/src/cli/runtime/serve-update-supervisor.ts @@ -255,7 +255,7 @@ function waitForForegroundChild( recordReplacementFailure(`Could not start the replacement process: ${String(error)}`) cleanup() child.off('exit', handleExit) - // Why: the LaunchAgent may restart this parent immediately, so durable failure must precede process rejection. + // Persist failure before rejection: launchd may immediately restart this parent. void stateWrite.then(() => reject(error)) }) child.once('exit', handleExit) diff --git a/src/cli/runtime/status-caller.test.ts b/src/cli/runtime/status-caller.test.ts index e83cbc9b1da..cf3d622f49d 100644 --- a/src/cli/runtime/status-caller.test.ts +++ b/src/cli/runtime/status-caller.test.ts @@ -20,10 +20,6 @@ type ReceivedRequest = RuntimeOrchestrationEnvelope & { type HostReply = { result: unknown } | { error: { code: string; message: string } } -/** - * The real `orca status` handler and CLI client over a real Unix socket. The host's side of - * `orchestration.callerShow` is covered against the real dispatcher in orchestration-caller-show. - */ describe.skipIf(process.platform === 'win32')( "orca status reports a session caller's Orca session ID", () => { @@ -124,7 +120,7 @@ describe.skipIf(process.platform === 'win32')( expect(await statusCaller()).toEqual(caller) const [request] = callerShowRequests() - // Nothing names the caller in params: the host reads the envelope, as every verb's entry does. + // Caller identity travels in the envelope, not caller-controlled params. expect(request?.params).toBeUndefined() expect(request?.orchestrationCompatibilityEvidence).toMatchObject({ agentSessionId: SESSION, @@ -144,7 +140,6 @@ describe.skipIf(process.platform === 'win32')( const text = await status(false) expect(printed.result).not.toHaveProperty('caller') - // The status text ends where main's does, with nothing about the caller. expect(text.split('\n').at(-1)).toMatch(/^graphState: /) expect(text).not.toMatch(/^(?:orcaSessionId|caller):/m) expect(callerShowRequests()).toHaveLength(0) diff --git a/src/cli/runtime/status-caller.ts b/src/cli/runtime/status-caller.ts index 17e199780f8..29c956824d8 100644 --- a/src/cli/runtime/status-caller.ts +++ b/src/cli/runtime/status-caller.ts @@ -14,15 +14,11 @@ const SESSION_REFUSAL_CODES = new Set( // Resolving a session may bring up the agent-session host, which the 1s status probe cannot cover. const CALLER_SHOW_TIMEOUT_MS = 10_000 -/** - * This process's Orca session ID, resolved by the host from the session id its environment carries. - * `undefined` when there is none to show: not a session, an older host, or a failed call. - */ +/** Resolve the injected session through its host; omit it when absent, unsupported, or unavailable. */ export async function resolveCliStatusCaller( client: Pick ): Promise { const sessionId = readInjectedAgentSessionId() - // Why: terminal agents have no Orca session ID yet, so their status stays exactly as it was. if (!sessionId) { return undefined } diff --git a/src/cli/runtime/status.test.ts b/src/cli/runtime/status.test.ts index 68d6f392a40..f137eab6267 100644 --- a/src/cli/runtime/status.test.ts +++ b/src/cli/runtime/status.test.ts @@ -27,8 +27,7 @@ afterEach(async () => { servers.clear() }) -// Why: legacy runtime metadata compatibility only applies to local Unix socket -// metadata; Windows uses named pipes and cannot run this fixture directly. +// This legacy fixture uses Unix sockets; Windows runtimes use named pipes. describe.skipIf(process.platform === 'win32')('CLI runtime status', () => { it('uses the legacy singular runtime transport when reporting status', async () => { const userDataPath = mkdtempSync(join(tmpdir(), 'orca-runtime-status-')) @@ -86,10 +85,7 @@ describe.skipIf(process.platform === 'win32')('CLI runtime status', () => { }) }) -// Why: `kill(pid, 0)` answers EPERM when the pid exists under another uid — an Orca the -// CLI was pointed at with ORCA_USER_DATA_PATH, or one started with sudo. Reading that -// refusal as absence reports a live app as a dead one -// (docs/reference/ssh-execution-boundary.md). +// EPERM proves a PID exists under another uid; it must not mark a live runtime dead. describe.skipIf(process.platform === 'win32')('CLI status pid fallback', () => { async function statusWithUnreachableRuntime( killError: NodeJS.ErrnoException @@ -147,8 +143,6 @@ describe('projectRemoteAppStatus', () => { } as RuntimeStatus } - // STA-4792 defect 4: the reported output was running:false alongside desktopWindowStatus - // 'available', while the target's GUI was demonstrably up. it('reports the target app as running when a renderer owns the graph there', () => { expect(projectRemoteAppStatus(remoteStatus({ desktopWindowStatus: 'available' }))).toEqual({ running: true, @@ -164,12 +158,10 @@ describe('projectRemoteAppStatus', () => { } ) - // A headless `serve` reports no window at all. it('reports not running when the target has no desktop window status', () => { expect(projectRemoteAppStatus(remoteStatus())).toEqual({ running: false, pid: null }) }) - // Why: old runtimes predate the explicit status but a positive window id still proves a window. it('honors the authoritativeWindowId fallback for older runtimes', () => { expect(projectRemoteAppStatus(remoteStatus({ authoritativeWindowId: 3 }))).toEqual({ running: true, @@ -178,8 +170,6 @@ describe('projectRemoteAppStatus', () => { }) }) - // Why: the SSH host-passthrough answers for the Orca host the caller reached, and used to - // claim running:true unconditionally. Both transports now share this projection. it('does not claim a desktop app for a headless serve on any transport', () => { expect(projectRemoteAppStatus(remoteStatus({ desktopWindowStatus: 'openable' }))).toEqual({ running: false, diff --git a/src/cli/runtime/status.ts b/src/cli/runtime/status.ts index f01ffe842df..b49cb7abc59 100644 --- a/src/cli/runtime/status.ts +++ b/src/cli/runtime/status.ts @@ -24,9 +24,7 @@ export async function getCliStatus( pid: null }, runtime: { - // Why: distinguishing "never started" from "was running but died" - // gives the user a better signal about what happened. If the metadata - // file exists, Orca was running at some point. + // Existing metadata proves the runtime started at least once. state: metadata ? 'stale_bootstrap' : 'not_running', reachable: false, runtimeId: null @@ -70,7 +68,7 @@ export async function getCliStatus( } }) } catch (error) { - // Why: a denied caller cannot tell a live Orca from a dead one, so report the denial, not a state. + // Access denial does not establish runtime liveness. if (error instanceof RuntimeClientError && error.code === 'runtime_access_denied') { throw error } diff --git a/src/cli/runtime/terminal-prompt-mutation-recovery.ts b/src/cli/runtime/terminal-prompt-mutation-recovery.ts index a1cbcc43c9a..31b735f5bfb 100644 --- a/src/cli/runtime/terminal-prompt-mutation-recovery.ts +++ b/src/cli/runtime/terminal-prompt-mutation-recovery.ts @@ -59,11 +59,7 @@ export function attachUnverifiedTerminalPromptRecovery(error: unknown): RuntimeC ) } -/** - * The prompt request ID survives a failed send unless a runtime other than the preflight's - * prompt-delivery host handled it; a transport failure alone means nobody else answered, and the - * attested host still holds the durable pending receipt that makes `--retry-request` idempotent. - */ +/** Transport failures retain the attested host’s retry receipt; a different responding runtime invalidates it. */ export function didAnotherRuntimeHandleTerminalPrompt( error: unknown, preflightRuntimeId: string | null, diff --git a/src/cli/runtime/transport.test.ts b/src/cli/runtime/transport.test.ts index 4d713c842a4..0d16c84ad2d 100644 --- a/src/cli/runtime/transport.test.ts +++ b/src/cli/runtime/transport.test.ts @@ -47,8 +47,7 @@ describe('runtime transport timeout validation', () => { ) }) -// Why: these tests create Unix domain socket servers in temp directories. -// Windows does not support Unix domain sockets in the same way. +// These fixtures create Unix domain sockets rather than Windows named pipes. describe.skipIf(process.platform === 'win32')('runtime transport', () => { it('refreshes the per-call timeout when the runtime sends keepalive frames', async () => { const userDataPath = mkdtempSync(join(tmpdir(), 'orca-runtime-transport-')) @@ -64,8 +63,7 @@ describe.skipIf(process.platform === 'win32')('runtime transport', () => { }) socket.once('data', (data) => { const request = JSON.parse(String(data).trim()) as { id: string } - // Why: full-suite load can delay short timers; keep the response past - // the client timeout while leaving enough margin between keepalives. + // Leave keepalive scheduling margin under load while responding after the original timeout. keepalive = setInterval(() => { socket.write('{"_keepalive":true}\n') }, 50) @@ -114,7 +112,6 @@ describe.skipIf(process.platform === 'win32')('runtime transport', () => { const server = createServer((socket) => { sockets.add(socket) socket.once('close', () => sockets.delete(socket)) - // Read the request, then close cleanly without writing a terminal frame. socket.once('data', () => socket.end()) }) servers.add(server) @@ -128,8 +125,7 @@ describe.skipIf(process.platform === 'win32')('runtime transport', () => { startedAt: 1 } - // A generous timeout: a passing fix rejects on close well before this; the - // pre-fix behavior would hang the full duration and trip vitest's own limit. + // Reject on close well before this timeout; waiting for it would exceed the test limit. const start = Date.now() await expect(sendRequest(metadata, 'status.get', undefined, 60000)).rejects.toMatchObject({ code: 'runtime_unavailable', diff --git a/src/cli/runtime/transport.ts b/src/cli/runtime/transport.ts index 594d8a752a4..f6b795eda92 100644 --- a/src/cli/runtime/transport.ts +++ b/src/cli/runtime/transport.ts @@ -16,7 +16,7 @@ export async function sendRequest( params: unknown, timeoutMs: number, envelope?: RuntimeOrchestrationEnvelope, - // Why separate from the envelope: federation forwards envelopes to other runtimes. + // Federation forwards envelopes; the runtime fence must stay local. expectedRuntimeSource?: ExpectedRuntimeSource ): Promise> { if (!isSafeTimerDelayMs(timeoutMs)) { @@ -77,7 +77,7 @@ export async function sendRequest( socket.once('error', (error) => { finish({ ok: false, - // Why: a sandbox denying the socket/pipe is not a dead app, so restart advice would mislead. + // Access denial does not establish that the app is dead. error: runtimeAccessDeniedError(error, metadata.pid) ?? new RuntimeClientError( @@ -86,10 +86,7 @@ export async function sendRequest( ) }) }) - // Why: a clean peer close (FIN, no 'error') before a terminal frame never - // settles the promise, so the call would otherwise hang until the full - // timeout fires. Reject promptly. finish() guards double-settle, so this - // no-ops on the normal success/error paths that already called socket.end(). + // A clean close without a terminal frame must reject rather than wait for the timeout. socket.once('close', () => { finish({ ok: false, @@ -100,11 +97,6 @@ export async function sendRequest( }) }) socket.on('data', (chunk: string) => { - // Why: the server may interleave `{"_keepalive":true}\n` frames with the - // final success/failure frame to keep both idle timers alive during a - // long-poll (see design doc §3.1). Read frames in a loop until we see a - // terminal frame. Each keepalive refreshes the client-side timer so a - // 10 min wait doesn't trip the 60 s default ceiling. let cursor = 0 while (cursor < chunk.length && !settled) { const newlineIndex = chunk.indexOf('\n', cursor) @@ -138,20 +130,13 @@ export async function sendRequest( return } - // Fast-path: ignore keepalives without running the full schema. - // setTimeout().refresh() is stable since Node 10 (Orca ships on - // Node 20+ via Electron and the standalone CLI targets the same - // major). See §7 risk #9. + // Keepalives refresh the idle timeout without a full envelope validation. if (isKeepaliveFrame(raw)) { timeout.refresh() continue } - // Why: validate the envelope shape (id, ok, result/error, _meta) at - // the decode boundary so version skew between the CLI and the Orca - // main runtime surfaces as a single invalid_runtime_response instead - // of a downstream mis-typed field access. `result` is left as - // unknown — the TResult generic is the caller's responsibility. + // Validate the envelope at the boundary; callers remain responsible for TResult’s shape. const parsed = RuntimeRpcEnvelopeSchema.safeParse(raw) if (!parsed.success) { finish({ @@ -164,8 +149,6 @@ export async function sendRequest( return } - // Narrow out keepalive (already filtered above) so TS can see a - // Success|Failure shape here. const frame = parsed.data if ('_keepalive' in frame) { timeout.refresh() diff --git a/src/cli/runtime/types.ts b/src/cli/runtime/types.ts index a15515acfdb..0e46ff1eebd 100644 --- a/src/cli/runtime/types.ts +++ b/src/cli/runtime/types.ts @@ -9,8 +9,7 @@ export type { export class RuntimeClientError extends Error { readonly code: string - // Why: optional structured recovery payload (e.g. did-you-mean suggestions, - // valid-flag enumeration) surfaced into both the human and --json error output. + // Structured recovery data is exposed in both human and JSON errors. readonly data?: unknown constructor(code: string, message: string, data?: unknown) { @@ -24,7 +23,6 @@ export class RuntimeRpcFailureError extends RuntimeClientError { readonly response: RuntimeRpcFailure constructor(response: RuntimeRpcFailure) { - // Why: all client errors expose recovery through the same inherited channel. super(response.error.code, response.error.message, response.error.data) this.response = { ...response, diff --git a/src/cli/specs/account.ts b/src/cli/specs/account.ts index 3da205e31eb..fd1400e1454 100644 --- a/src/cli/specs/account.ts +++ b/src/cli/specs/account.ts @@ -1,10 +1,6 @@ import { GLOBAL_FLAGS, type CommandSpec } from '../args' -// Why: the desktop "Add account" button is disabled when the UI drives a remote -// runtime (a headless server). These commands run the interactive agent login -// (`claude login` / `codex login`) in the caller's own terminal on the host and -// register the captured account with the local runtime, giving headless hosts a -// way to manage Claude and Codex accounts. +// Terminal login lets headless hosts manage accounts without the desktop login UI. export const ACCOUNT_COMMAND_SPECS: CommandSpec[] = [ { path: ['account', 'add'], diff --git a/src/cli/specs/browser-advanced.ts b/src/cli/specs/browser-advanced.ts index 695932fae97..0251705c45b 100644 --- a/src/cli/specs/browser-advanced.ts +++ b/src/cli/specs/browser-advanced.ts @@ -2,7 +2,6 @@ import type { CommandSpec } from '../args' import { GLOBAL_FLAGS } from '../args' export const BROWSER_ADVANCED_COMMAND_SPECS: CommandSpec[] = [ - // ── Cookie management ── { path: ['cookie', 'get'], summary: 'Get cookies for the active tab (optionally filter by URL)', @@ -35,7 +34,6 @@ export const BROWSER_ADVANCED_COMMAND_SPECS: CommandSpec[] = [ 'orca cookie delete --name [--domain ] [--url ] [--worktree ] [--json]', allowedFlags: [...GLOBAL_FLAGS, 'name', 'domain', 'url', 'worktree'] }, - // ── Viewport ── { path: ['viewport'], summary: 'Set browser viewport size', @@ -43,7 +41,6 @@ export const BROWSER_ADVANCED_COMMAND_SPECS: CommandSpec[] = [ 'orca viewport --width --height [--scale ] [--mobile] [--worktree ] [--json]', allowedFlags: [...GLOBAL_FLAGS, 'width', 'height', 'scale', 'mobile', 'worktree'] }, - // ── Geolocation ── { path: ['geolocation'], summary: 'Override browser geolocation', @@ -51,7 +48,6 @@ export const BROWSER_ADVANCED_COMMAND_SPECS: CommandSpec[] = [ 'orca geolocation --latitude --longitude [--accuracy ] [--worktree ] [--json]', allowedFlags: [...GLOBAL_FLAGS, 'latitude', 'longitude', 'accuracy', 'worktree'] }, - // ── Request interception ── { path: ['intercept', 'enable'], summary: 'Enable request interception (pause matching requests)', @@ -70,9 +66,6 @@ export const BROWSER_ADVANCED_COMMAND_SPECS: CommandSpec[] = [ usage: 'orca intercept list [--worktree ] [--json]', allowedFlags: [...GLOBAL_FLAGS, 'worktree'] }, - // TODO: add intercept continue/block once agent-browser supports per-request - // interception decisions (currently only supports URL-pattern-based route/unroute). - // ── Console/network capture ── { path: ['capture', 'start'], summary: 'Start capturing console and network events', @@ -97,7 +90,6 @@ export const BROWSER_ADVANCED_COMMAND_SPECS: CommandSpec[] = [ usage: 'orca network [--limit ] [--worktree ] [--json]', allowedFlags: [...GLOBAL_FLAGS, 'limit', 'worktree'] }, - // ── Additional core commands ── { path: ['dblclick'], summary: 'Double-click element by ref', @@ -128,14 +120,12 @@ export const BROWSER_ADVANCED_COMMAND_SPECS: CommandSpec[] = [ usage: 'orca is --what --element [--worktree ] [--json]', allowedFlags: [...GLOBAL_FLAGS, 'what', 'element', 'worktree'] }, - // ── Keyboard insert text ── { path: ['inserttext'], summary: 'Insert text without key events', usage: 'orca inserttext --text [--worktree ] [--json]', allowedFlags: [...GLOBAL_FLAGS, 'text', 'worktree'] }, - // ── Mouse commands ── { path: ['mouse', 'move'], summary: 'Move mouse to x,y coordinates', @@ -160,7 +150,6 @@ export const BROWSER_ADVANCED_COMMAND_SPECS: CommandSpec[] = [ usage: 'orca mouse wheel --dy [--dx ] [--worktree ] [--json]', allowedFlags: [...GLOBAL_FLAGS, 'dy', 'dx', 'worktree'] }, - // ── Find (semantic locators) ── { path: ['find'], summary: 'Find element by semantic locator and perform action', @@ -168,7 +157,6 @@ export const BROWSER_ADVANCED_COMMAND_SPECS: CommandSpec[] = [ 'orca find --locator --value --action [--text ] [--worktree ] [--json]', allowedFlags: [...GLOBAL_FLAGS, 'locator', 'value', 'action', 'text', 'worktree'] }, - // ── Set commands ── { path: ['set', 'device'], summary: 'Emulate a device', @@ -200,7 +188,6 @@ export const BROWSER_ADVANCED_COMMAND_SPECS: CommandSpec[] = [ 'orca set media [--color-scheme ] [--reduced-motion ] [--worktree ] [--json]', allowedFlags: [...GLOBAL_FLAGS, 'color-scheme', 'reduced-motion', 'worktree'] }, - // ── Clipboard commands ── { path: ['clipboard', 'read'], summary: 'Read clipboard contents', @@ -213,7 +200,6 @@ export const BROWSER_ADVANCED_COMMAND_SPECS: CommandSpec[] = [ usage: 'orca clipboard write --text [--worktree ] [--json]', allowedFlags: [...GLOBAL_FLAGS, 'text', 'worktree'] }, - // ── Dialog commands ── { path: ['dialog', 'accept'], summary: 'Accept a browser dialog', @@ -226,7 +212,6 @@ export const BROWSER_ADVANCED_COMMAND_SPECS: CommandSpec[] = [ usage: 'orca dialog dismiss [--worktree ] [--json]', allowedFlags: [...GLOBAL_FLAGS, 'worktree'] }, - // ── Storage commands ── { path: ['storage', 'local', 'get'], summary: 'Get a localStorage value by key', @@ -265,14 +250,12 @@ export const BROWSER_ADVANCED_COMMAND_SPECS: CommandSpec[] = [ usage: 'orca storage session clear [--worktree ] [--json]', allowedFlags: [...GLOBAL_FLAGS, 'worktree'] }, - // ── Download command ── { path: ['download'], summary: 'Download a file by clicking a selector', usage: 'orca download --selector --path [--worktree ] [--json]', allowedFlags: [...GLOBAL_FLAGS, 'selector', 'path', 'worktree'] }, - // ── Highlight command ── { path: ['highlight'], summary: 'Highlight an element by selector', diff --git a/src/cli/specs/bundled-guide-flags.test.ts b/src/cli/specs/bundled-guide-flags.test.ts index fe5f277a628..4250c726263 100644 --- a/src/cli/specs/bundled-guide-flags.test.ts +++ b/src/cli/specs/bundled-guide-flags.test.ts @@ -10,7 +10,6 @@ type GuideInvocation = { snippet: string } -// Why: guides write examples as `orca ...`, `orca-dev ...`, or the `ORCA` placeholder. const CLI_INVOCATION = /(?:^|[\s`(])(?:orca|orca-dev|orca-ide|ORCA)\s+([^\n`]*)/g // Longest path first so `orchestration worker-start` never resolves as `orchestration worker`. @@ -34,7 +33,7 @@ function collectGuideInvocations(): GuideInvocation[] { if (!spec) { continue } - // Why: a quoted flag value belongs to the nested program (`--command 'codex --model ...'`), not to orca. + // Quoted values contain nested commands whose flags do not belong to orca. const orcaArgs = invocation.replace(/'[^']*'|"[^"]*"/g, ' ') const flags = [...orcaArgs.matchAll(/(?:^|[\s[(])--([a-z][a-z0-9-]*)/g)].map( (flag) => flag[1] @@ -55,7 +54,6 @@ function collectGuideInvocations(): GuideInvocation[] { describe('bundled skill guides', () => { const invocations = collectGuideInvocations() - // Why: the guides ship inside the binary, so an example the parser rejects is a shipped defect. it('only documents flags the CLI parser accepts', () => { expect(invocations.length).toBeGreaterThan(0) const rejected = invocations.filter((invocation) => { diff --git a/src/cli/specs/core.ts b/src/cli/specs/core.ts index ab5480cf7ea..42e7a38f9a1 100644 --- a/src/cli/specs/core.ts +++ b/src/cli/specs/core.ts @@ -132,8 +132,6 @@ export const CORE_COMMAND_SPECS: CommandSpec[] = [ WORKTREE_SET_COMMAND_SPEC, { path: ['worktree', 'rm'], - // Why: agents reach for git's `remove`/`delete` verbs; accept them as - // aliases so a conventional guess resolves instead of dead-ending. aliases: [ ['worktree', 'remove'], ['worktree', 'delete'] @@ -236,8 +234,7 @@ export const CORE_COMMAND_SPECS: CommandSpec[] = [ }, { path: ['terminal', 'switch'], - // Why: `focus` is the legacy verb for this action; keep it working as an - // alias rather than a duplicate spec + handler registration. + // Preserve the legacy focus verb. aliases: [['terminal', 'focus']], summary: 'Switch to a terminal tab in the UI', usage: 'orca terminal switch [--terminal ] [--json]', diff --git a/src/cli/specs/search.ts b/src/cli/specs/search.ts index a356151ab23..314132a81cc 100644 --- a/src/cli/specs/search.ts +++ b/src/cli/specs/search.ts @@ -1,8 +1,6 @@ import { GLOBAL_FLAGS, type CommandSpec } from '../args' -// Why one command and not a `search status` subcommand: the query is a bare -// positional, so `orca search status` would be indistinguishable from searching -// for the word "status". The index report is a flag on the same command instead. +// Status is a flag because a subcommand would collide with searching for the word "status". export const SEARCH_COMMAND_SPECS: CommandSpec[] = [ { path: ['search'], diff --git a/src/cli/specs/worktree-listing-scope-notes.ts b/src/cli/specs/worktree-listing-scope-notes.ts index b9de89356d1..85345cd52e3 100644 --- a/src/cli/specs/worktree-listing-scope-notes.ts +++ b/src/cli/specs/worktree-listing-scope-notes.ts @@ -1,4 +1,3 @@ -/** Shared by `worktree list` and `worktree ps`, which report host coverage the same way. */ export const WORKTREE_LISTING_SCOPE_NOTES: readonly string[] = [ 'Each row carries the execution host that owns it (`host=`), and the trailing `scope:` line names every host the page covers plus the ones it does not.', 'A host named under `not covered` may still have workspaces; an empty answer for it is not evidence that it has none. A paired server is annotated with the `--environment` flag that reaches it; this machine and its SSH targets are reached through the same runtime, so no flag selects them.',