diff --git a/src/cli/handlers/account-list-format.ts b/src/cli/handlers/account-list-format.ts index 59df0c6b3a4..78d17dc08ed 100644 --- a/src/cli/handlers/account-list-format.ts +++ b/src/cli/handlers/account-list-format.ts @@ -1,7 +1,5 @@ import type { ManagedDataAccountsState } from '../../shared/managed-account-types' -// Why: Claude and Codex managed-account summaries both carry id+email+active id, -// so one formatter renders either provider's block. type AccountsBlock = { accounts: readonly { id: string; email: string; needsSignIn?: true }[] activeAccountId: string | null @@ -24,7 +22,6 @@ export function formatDataAccounts(label: string, state: ManagedDataAccountsStat .join('\n')}` } -/** Renders a provider's managed-account list as a human-readable block, marking the active account. */ export function formatAccountsBlock(label: string, block: AccountsBlock): string { if (block.accounts.length === 0) { return `No managed ${label} accounts.` diff --git a/src/cli/handlers/account.test.ts b/src/cli/handlers/account.test.ts index 9ace6891b7a..38ac1223e06 100644 --- a/src/cli/handlers/account.test.ts +++ b/src/cli/handlers/account.test.ts @@ -25,8 +25,7 @@ const { writeKeychainMock: vi.fn() })) -// Why: keep real temp-dir cleanup by default so leak assertions stay honest, -// while allowing deterministic Windows EBUSY coverage. +// Use real cleanup by default so leaks remain visible; override it only for Windows EBUSY cases. vi.mock('node:fs', async (importOriginal) => { const actual = await importOriginal() rmSyncMock.mockImplementation(actual.rmSync) @@ -43,8 +42,7 @@ vi.mock('../../main/claude-accounts/keychain', () => ({ readActiveClaudeKeychainCredentialsStrict: readKeychainMock, writeActiveClaudeKeychainCredentials: writeKeychainMock })) -// Why importOriginal: withCliRuntimeOnPath is a pure filesystem-probing helper, -// and the PATH assertions below are only meaningful against the real one. +// PATH assertions must exercise the real withCliRuntimeOnPath filesystem probes. vi.mock('../../shared/node-cli-command-resolution', async (importOriginal) => ({ ...(await importOriginal()), getVersionManagerBinPaths: getVersionManagerBinPathsMock, @@ -73,9 +71,7 @@ function successfulChild(): EventEmitter { return child } -// Why: identify the handler under test by set difference, not by position — -// `.at(-1)` picks up any listener a later registration appends (vitest installs -// its own once-wrapped SIGINT teardown), which made assertions flake. +// Diff listener sets: Vitest may append its own wrapped SIGINT listener after ours. function newSignalListener( signal: NodeJS.Signals, before: readonly unknown[] @@ -146,8 +142,7 @@ describe('account CLI handlers', () => { ) logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}) process.env.ELECTRON_RUN_AS_NODE = '1' - // Why: a test run inside a real distro carries WSL_DISTRO_NAME, which would - // leak WSL attribution into every faked-win32 add below. + // Clear real WSL attribution before simulating win32. delete process.env.WSL_DISTRO_NAME delete process.env.ORCA_CLI_WSL_DISTRO }) @@ -389,8 +384,7 @@ describe('account CLI handlers', () => { it.each(['linux', 'darwin'])( 'keeps host attribution when the CLI runs on %s', async (platform) => { - // Why: a Linux CLI talks to a Linux runtime whose accounts are host-lane; - // WSL_DISTRO_NAME there names the CLI's own environment, not a target lane. + // Linux runtimes use host-lane accounts even when the CLI itself runs in WSL. Object.defineProperty(process, 'platform', { configurable: true, value: platform }) process.env.WSL_DISTRO_NAME = 'Ubuntu-22.04' process.env.ORCA_CLI_WSL_DISTRO = 'Ubuntu-22.04' @@ -403,7 +397,7 @@ describe('account CLI handlers', () => { ) it('waits for physical child close before removing interrupted login credentials', async () => { - // Why: deleting first lets the still-live login recreate credentials afterward. + // A live login can recreate credentials after cleanup; wait for close before deleting them. Object.defineProperty(process, 'platform', { configurable: true, value: 'linux' }) const kill = vi.fn() const child = Object.assign(new EventEmitter(), { kill }) @@ -436,8 +430,7 @@ describe('account CLI handlers', () => { }) it('cleans up when an SSH hangup ends the login', async () => { - // Why: this flow targets headless/SSH hosts, where a dropped connection - // delivers SIGHUP — Node's default terminates without running cleanup. + // Node's default SIGHUP exit would bypass cleanup when an SSH connection drops. Object.defineProperty(process, 'platform', { configurable: true, value: 'linux' }) const child = Object.assign(new EventEmitter(), { kill: vi.fn() }) let codexHome = '' @@ -467,8 +460,7 @@ describe('account CLI handlers', () => { }) it('warns that the account may already be registered when interrupted mid-RPC', async () => { - // Why: the runtime finishes the add independently of this process, so an - // interrupt after sign-in cannot honestly be reported as "not added". + // Registration continues at the runtime after the CLI is interrupted. const child = Object.assign(new EventEmitter(), { kill: vi.fn() }) spawnMock.mockImplementation(() => { queueMicrotask(() => child.emit('exit', 0)) @@ -524,7 +516,6 @@ describe('account CLI handlers', () => { }) it('fails before the login when the runtime is unreachable', async () => { - // Why: discovering a dead runtime after sign-in wastes a full OAuth round trip. callMock.mockRejectedValue(new Error('runtime not running')) await expect(ACCOUNT_HANDLERS['account add'](context('codex'))).rejects.toThrow( @@ -571,8 +562,6 @@ describe('account CLI handlers', () => { it.each(['environment', 'pairing-code'])( 'rejects --%s instead of silently ignoring it', async (flag) => { - // Why: account commands are pinned to the local runtime, so honoring these - // silently would register the account on the wrong host. await expect( ACCOUNT_HANDLERS['account add']({ ...context('codex'), @@ -589,8 +578,6 @@ describe('account CLI handlers', () => { it.each(['environment', 'pairing-code'])( 'rejects --%s on `account list` instead of listing the local host', async (flag) => { - // Why: listing is read-only, but answering with the LOCAL machine's accounts - // when the user named a remote host is the specific wrong answer they'd act on. await expect( ACCOUNT_HANDLERS['account list']({ ...context('claude'), @@ -602,8 +589,6 @@ describe('account CLI handlers', () => { ) it('keeps the original add error when cleanup also fails', async () => { - // Why: cleanup runs in a `finally`, so an unguarded rejection there replaces - // the error that actually explains why the add failed. const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) rmSyncMock.mockImplementationOnce(() => { throw new Error('EBUSY: resource busy or locked') @@ -648,8 +633,6 @@ describe('account CLI handlers', () => { }) it('rejects `--agent` with no value instead of defaulting to Claude', async () => { - // Why: the parser turns a valueless flag into boolean true, so a silent - // default would run a full OAuth login for the wrong provider. await expect( ACCOUNT_HANDLERS['account add']({ ...context('claude'), flags: new Map([['agent', true]]) }) ).rejects.toThrow( @@ -705,8 +688,6 @@ describe('account CLI handlers', () => { }) it('lists accounts without forcing a provider usage refresh', async () => { - // Why: the forced lane bypasses the poll throttle and costs one serial - // round-trip per managed account, and this output shows no usage numbers. callMock.mockResolvedValue({ id: 'test', ok: true, diff --git a/src/cli/handlers/account.ts b/src/cli/handlers/account.ts index 70d2b35369b..c93edb63d5a 100644 --- a/src/cli/handlers/account.ts +++ b/src/cli/handlers/account.ts @@ -39,7 +39,6 @@ import { getWslAccountTarget } from './account-wsl-location' import { addDataAccount, listDataAccounts, mutateDataAccount } from './data-account-commands' import { formatAccountsBlock, formatDataAccounts } from './account-list-format' -// Why: add returns just that provider's state; list returns the full snapshot. type AccountsListSnapshot = { opencode?: ManagedDataAccountsState devin?: ManagedDataAccountsState @@ -61,11 +60,7 @@ function addAgentNodePaths(env: NodeJS.ProcessEnv): NodeJS.ProcessEnv { return env } -/** - * Runs the real agent login attached to the user's terminal so the OAuth - * URL/device-code prompt is visible and the code can be pasted back — the desktop - * GUI flow drives this via a browser Orca can't reach on a headless host. - */ +/** Attach login to the terminal so device-code authentication works on headless hosts. */ async function runAgentLoginInTerminal( command: string, args: string[], @@ -80,8 +75,6 @@ async function runAgentLoginInTerminal( try { ;({ spawnCmd, spawnArgs } = getSpawnArgsForWindows(resolvedCommand, args)) } catch (error) { - // Why: the bare sentinel message reaches the user verbatim otherwise, with - // nothing naming the path or the characters that made it unspawnable. rejectPromise( error instanceof UnsafeWindowsBatchArgumentsError ? new RuntimeClientError( @@ -94,9 +87,7 @@ async function runAgentLoginInTerminal( ) return } - // Why paired after the seed: addAgentNodePaths prepends the *newest* version - // manager bin, which is not necessarily where this CLI lives. Pairing last puts - // the CLI's own node in front of that seed (stablyai/orca#10932). + // Put the CLI's own Node ahead of newer version-manager bins to keep the command/runtime pair intact. const env = withCliRuntimeOnPath( resolvedCommand, addAgentNodePaths({ ...stripElectronRunAsNode(process.env), ...extraEnv }) @@ -105,8 +96,7 @@ async function runAgentLoginInTerminal( let child: ReturnType try { child = spawn(spawnCmd, spawnArgs, { - // Why: JSON mode reserves stdout for the response envelope while keeping - // the interactive login attached to the user's terminal via stderr. + // Reserve stdout for JSON; keep interactive login on the terminal via stderr. stdio: consoleStdio.stdio, env }) @@ -140,7 +130,6 @@ async function runAgentLoginInTerminal( }) } -/** Signs in to a new account folder the host created, then registers it (superset AddAccountDialog). */ async function addClaudeAccount({ client, cwd, json }: HandlerContext): Promise { const { result: signIn } = await client.call( 'accounts.beginClaudeSignIn', @@ -154,8 +143,7 @@ async function addClaudeAccount({ client, cwd, json }: HandlerContext): Promise< } const result = await withInteractiveLoginCleanup( session, - // Why always: the host keeps a folder that became an account, and deletes an abandoned one. - // Never thrown: an older running Orca lacks the method, and the add itself already finished. + // Cancel cleans abandoned folders and preserves registered accounts; older hosts may lack the method. async () => { const { accountId, runtime, wslDistro } = signIn await client @@ -192,7 +180,6 @@ async function addClaudeAccount({ client, cwd, json }: HandlerContext): Promise< printResult(result, json, (state) => formatAccountsBlock('Claude', state)) } -/** Logs into a Codex account in a temp CODEX_HOME, then registers it with the local runtime. */ async function addCodexAccount({ client, cwd, json }: HandlerContext): Promise { const codexHome = mkdtempSync(join(tmpdir(), 'orca-account-add-codex-')) const session: InteractiveLoginSession = { @@ -206,8 +193,7 @@ async function addCodexAccount({ client, cwd, json }: HandlerContext): Promise { - // Why: plain OAuth binds a loopback callback the user's browser cannot reach - // on a headless/SSH host; device auth is explicitly designed for this flow. + // Device auth works over SSH where the browser cannot reach the host's loopback OAuth callback. await runAgentLoginInTerminal( 'codex', ['login', '--device-auth'], @@ -225,13 +211,6 @@ async function addCodexAccount({ client, cwd, json }: HandlerContext): Promise formatAccountsBlock('Codex', state)) } -/** - * Rejects the runtime-selector flags instead of ignoring them. shouldIgnoreRemoteSelection - * pins account commands to the local runtime, so honoring `--environment homelab` - * silently would target the laptop rather than the host the user named — the exact - * mistake this feature exists to avoid. A `--help` note does not reach someone who - * already typed the flag. - */ function rejectAccountRemoteSelectionFlags(ctx: HandlerContext, command: string): void { rejectRemoteSelectionFlags( ctx.flags, @@ -254,12 +233,10 @@ async function assertAccountImportSupported( } } -/** CLI handlers for managed account enrollment and listing. */ export const ACCOUNT_HANDLERS: Record = { 'account add': async (ctx) => { const agentFlag = ctx.flags.get('agent') - // Why: a valueless `--agent` parses as boolean true; defaulting it to claude - // would silently run a full OAuth login for the provider the user did not ask for. + // A valueless flag is true; never silently default it to a different provider's OAuth flow. if (agentFlag !== undefined && typeof agentFlag !== 'string') { throw new RuntimeClientError( 'invalid_argument', @@ -278,7 +255,7 @@ export const ACCOUNT_HANDLERS: Record = { await addDataAccount(ctx, agent, runAgentLoginInTerminal) return } - // Why: fail on runtime version skew before burning a full OAuth round trip. + // Check runtime support before spending an OAuth round trip. await assertAccountImportSupported(ctx, agent) await ctx.client.call('accounts.list', { refreshUsage: false }) await (agent === 'claude' ? addClaudeAccount(ctx) : addCodexAccount(ctx)) @@ -291,8 +268,7 @@ export const ACCOUNT_HANDLERS: Record = { return } const { client, json } = ctx - // Why: this command renders no usage numbers, so skip the forced provider - // refresh — it is one serial network round-trip per managed account. + // Listing shows no usage, so avoid a serial provider refresh for every account. const result = await client.call('accounts.list', { refreshUsage: false }) diff --git a/src/cli/handlers/agent-hooks-prepare-codex-isolated-home.test.ts b/src/cli/handlers/agent-hooks-prepare-codex-isolated-home.test.ts index 01047cc79a1..5bdc65c18d5 100644 --- a/src/cli/handlers/agent-hooks-prepare-codex-isolated-home.test.ts +++ b/src/cli/handlers/agent-hooks-prepare-codex-isolated-home.test.ts @@ -42,7 +42,6 @@ let appHome: string let realHome: string let managedHome: string -// Why: stand in for a missing codex binary so no real app-server ever starts. function stubMissingCodexBinary(): never { throw Object.assign(new Error('spawn codex ENOENT'), { code: 'ENOENT' }) } @@ -73,7 +72,7 @@ beforeEach(() => { realHome = join(root, 'real-home') managedHome = join(appUserData, 'codex-runtime-home', 'home') mkdirSync(managedHome, { recursive: true }) - // The app, running with its own HOME, installed the pane's home at spawn. + // Simulate an app that installed the pane home under a different HOME at spawn. const appCommand = getManagedCommand(join(appHome, '.orca', 'agent-hooks', SCRIPT_NAME)) writeFileSync( join(managedHome, 'hooks.json'), @@ -102,8 +101,7 @@ afterEach(() => { rmSync(root, { recursive: true, force: true }) }) -// Why: the pane shell can run under the real HOME (login(1)) or the app's own; -// on a native host the pane step neither writes a Codex home nor asks the app to. +// Native pane preparation must leave Codex homes unchanged, whichever HOME its login shell uses. it.each([ ['the real HOME', 'real'], ["the app's HOME", 'app'] diff --git a/src/cli/handlers/agent-hooks.ts b/src/cli/handlers/agent-hooks.ts index 135c651e6a6..d4637605823 100644 --- a/src/cli/handlers/agent-hooks.ts +++ b/src/cli/handlers/agent-hooks.ts @@ -140,7 +140,7 @@ function localSuccess(result: TResult): RuntimeRpcSuccess { } function formatAgentHookCommandResult(result: AgentHookCommandResult): string { - // Why the path: it names the home a status reports on, which the CLI's process may not share with the app's panes. + // The CLI and app panes may use different homes; identify the one this status describes. const statusSummary = result.statuses .map( (status) => @@ -181,7 +181,7 @@ export const AGENT_HOOK_HANDLERS: Record = { 'agent hooks prepare-codex': async ({ client, flags }) => { rejectRemoteHookSelection(flags) if (!process.env.WSL_DISTRO_NAME?.trim()) { - // Why a no-op: native pane wrappers from builds up to v1.4.216 still call it; delete once no supported build's wrapper does. + // Native wrappers through v1.4.216 still call this; retain the no-op until they are unsupported. return } try { diff --git a/src/cli/handlers/artifacts.test.ts b/src/cli/handlers/artifacts.test.ts index 831bd95a5b7..deebc13daa1 100644 --- a/src/cli/handlers/artifacts.test.ts +++ b/src/cli/handlers/artifacts.test.ts @@ -106,7 +106,6 @@ describe('artifact CLI handlers', () => { json: false }) ).rejects.toThrow(/too large/) - // The capability preflight is the only permitted call; the oversized body never ships. expect(call).not.toHaveBeenCalledWith('artifacts.share', expect.anything()) }) diff --git a/src/cli/handlers/artifacts.ts b/src/cli/handlers/artifacts.ts index 8546d7997f3..e9c0b811063 100644 --- a/src/cli/handlers/artifacts.ts +++ b/src/cli/handlers/artifacts.ts @@ -78,12 +78,7 @@ async function readStdinWithinLimit(maxBytes: number): Promise { return Buffer.concat(chunks).toString('utf8') } -/** - * Publishing is off by default, so denial is the common outcome. A tiny `settings.get` read - * answers it before we load (or pipe in) up to the full RPC byte budget the host would reject. - * Only an explicit `false` denies: a host predating the capability omits the field entirely, - * and an unreachable host stays the publish RPC's problem, not the preflight's. - */ +// Check before reading a large body; only explicit false denies, preserving older-host compatibility. async function preflightPublishCapability(ctx: HandlerContext): Promise { let enabled: unknown try { diff --git a/src/cli/handlers/automation-destination-fencing.test.ts b/src/cli/handlers/automation-destination-fencing.test.ts index f89d829826b..c3cf84d65dd 100644 --- a/src/cli/handlers/automation-destination-fencing.test.ts +++ b/src/cli/handlers/automation-destination-fencing.test.ts @@ -13,19 +13,14 @@ function ok(result: unknown): unknown { return { id: 'request-1', ok: true, result, _meta: { runtimeId: 'runtime-1' } } } -/** - * Stands in for the storing authority: it answers the reads the CLI makes and - * enforces the destination exactly where persistence does — with the real - * assertions, and only when the client actually supplies one. - */ +/** Exercise the real destination assertions at write time, only when the client supplies a fence. */ function authority(options: { - /** What `ssh.listTargetSummaries` reports while the CLI captures its destination. */ registered: SshRegistration[] /** Generation the same target carries by the time the write lands. */ generationAtWrite?: number repoConnectionId?: string | null targetSummaryError?: Error - /** A host predating `ssh.listTargetSummaries`, which answers only `ssh.listTargets`. */ + /** Older hosts expose only ssh.listTargets. */ legacyHost?: boolean }): { call: ReturnType; writes: { method: string; params: unknown }[] } { const writes: { method: string; params: unknown }[] = [] @@ -82,10 +77,7 @@ const CREATE_FLAGS: [string, string][] = [ afterEach(() => vi.restoreAllMocks()) -// The expected owner only fences the host the record is leaving. Without the -// destination the arrival is never checked, so a move onto a host that is gone — -// or gone and re-registered as a different machine under the same id — is -// accepted and only surfaces later, at dispatch, as a dead automation. +// Source-owner checks cannot protect a destination removed or re-registered before the write. describe('CLI automation writes fence the host they land on', () => { it('refuses a selector-moving edit onto an SSH host the authority no longer registers', async () => { const { call, writes } = authority({ registered: [], repoConnectionId: 'box-1' }) @@ -208,8 +200,7 @@ describe('CLI automation writes fence the host they land on', () => { }) }) - // A host that predates SSH generations has no incarnation to capture, so the - // write must still land — failing closed there would break a host that is only old. + // Older hosts cannot provide generations; omit the fence to preserve compatibility. it('omits the destination when the authority registers the target without a generation', async () => { const { call, writes } = authority({ registered: [{ id: 'box-1', label: 'box 1' }], @@ -244,7 +235,6 @@ describe('CLI automation writes fence the host they land on', () => { expect(writes[0]?.params).toMatchObject({ destination: { selector: { kind: 'self' } } }) }) - // Nothing moves, so there is no arrival to fence and no reason to spend the reads. it('sends no destination when the edit names no project or workspace', async () => { const { call, writes } = authority({ registered: [] }) vi.spyOn(console, 'log').mockImplementation(() => undefined) diff --git a/src/cli/handlers/automation-extra-agent-args-flag.ts b/src/cli/handlers/automation-extra-agent-args-flag.ts index ec85733336e..11bf80e81fa 100644 --- a/src/cli/handlers/automation-extra-agent-args-flag.ts +++ b/src/cli/handlers/automation-extra-agent-args-flag.ts @@ -1,6 +1,6 @@ import { RuntimeClientError } from '../runtime-client' -/** Returns the text unchanged; an explicit empty value clears saved extras on edit. */ +/** An explicit empty value clears saved extras. */ export function getExtraAgentArgsFlag(flags: Map): string | undefined { if (!flags.has('extra-agent-args')) { return undefined diff --git a/src/cli/handlers/automation-owner-fencing.test.ts b/src/cli/handlers/automation-owner-fencing.test.ts index 0972c15a0ce..6ec3d9ee867 100644 --- a/src/cli/handlers/automation-owner-fencing.test.ts +++ b/src/cli/handlers/automation-owner-fencing.test.ts @@ -18,8 +18,7 @@ function clientAnsweringShow(owner: unknown): { call: ReturnType } afterEach(() => vi.restoreAllMocks()) -// Why: the fence refuses an unfenced mutation on a generation-bearing SSH record, so -// every CLI mutation has to read the owner first or it cannot touch its own host. +// Generation-bearing SSH records reject mutations that omit the owner precondition. describe('CLI automation mutations carry the owner they read', () => { it.each([ ['automations remove', 'automation.delete'], @@ -60,8 +59,7 @@ describe('CLI automation mutations carry the owner they read', () => { ) }) - // An older host has no fence to satisfy, so a missing owner must not become a - // fabricated one — and must not stop the mutation either. + // Older hosts supply no owner and require no precondition. it('omits the precondition when the host reports no owner', async () => { const { call } = clientAnsweringShow(undefined) vi.spyOn(console, 'log').mockImplementation(() => undefined) diff --git a/src/cli/handlers/automations.ts b/src/cli/handlers/automations.ts index 868ec3d730c..b16c81e2f34 100644 --- a/src/cli/handlers/automations.ts +++ b/src/cli/handlers/automations.ts @@ -137,13 +137,7 @@ async function getExplicitTarget( return { repo, workspace } } -/** - * Reads the record's current owner so the mutation that follows can name it. - * - * The CLI cannot project an owner itself — it has no SSH target registry — so it - * asks the authority that stores the record. A host too old to answer sends - * nothing, and that host has no fence to satisfy either. - */ +// Read the authoritative owner for fencing; older hosts omit it and impose no precondition. async function resolveExpectedOwner( client: Parameters[0]['client'], id: string @@ -169,7 +163,7 @@ function buildAutomationRunContextFromSetup(setup: ProjectHostSetup): WorkspaceR return runContext } -// Why: an older runtime strips the field and would run the automation without it. +// Older runtimes silently strip extraAgentArgs and would run without the requested arguments. async function assertExtraAgentArgsSupported( client: RuntimeClient, extraAgentArgs: string | undefined @@ -256,7 +250,7 @@ export const AUTOMATION_HANDLERS: Record = { } satisfies AutomationUpdateParams await assertExtraAgentArgsSupported(client, updates.extraAgentArgs) const expectedOwner = await resolveExpectedOwner(client, id) - // Why: expectedOwner only fences the host the record is leaving; an edit that moves it needs the arrival fenced too. + // Owner fencing covers the source host; moves must fence the destination too. const destination = await resolveAutomationDestination(client, target) const result = await client.call<{ automation: Automation }>('automation.update', { id, diff --git a/src/cli/handlers/browser-cookie.ts b/src/cli/handlers/browser-cookie.ts index 55d0f297cf7..5cb616c8c2b 100644 --- a/src/cli/handlers/browser-cookie.ts +++ b/src/cli/handlers/browser-cookie.ts @@ -45,7 +45,7 @@ export const BROWSER_COOKIE_HANDLERS: Record = { }, 'cookie set': async ({ flags, client, cwd, json }) => { const name = getRequiredStringFlag(flags, 'name') - // CookieSet.value accepts '' (clearing a cookie's value is a real operation). + // An empty cookie value clears its content without removing the cookie. const value = getRequiredStringFlagAllowingEmpty(flags, 'value') const params: Record = { name, value } const domain = getOptionalStringFlag(flags, 'domain') diff --git a/src/cli/handlers/browser-env.ts b/src/cli/handlers/browser-env.ts index f42600e79e7..1e07f44efd2 100644 --- a/src/cli/handlers/browser-env.ts +++ b/src/cli/handlers/browser-env.ts @@ -71,7 +71,7 @@ export const BROWSER_ENV_HANDLERS: Record = { }, 'set credentials': async ({ flags, client, cwd, json }) => { const user = getRequiredStringFlag(flags, 'user') - // SetCredentials.pass accepts '' (empty passwords are legal in HTTP basic auth). + // HTTP basic auth permits empty passwords. const pass = getRequiredStringFlagAllowingEmpty(flags, 'pass') const target = await getBrowserCommandTarget(flags, cwd, client) const result = await client.call('browser.setCredentials', { diff --git a/src/cli/handlers/browser-nav.ts b/src/cli/handlers/browser-nav.ts index e3a20d475d3..2ce1c2bde8e 100644 --- a/src/cli/handlers/browser-nav.ts +++ b/src/cli/handlers/browser-nav.ts @@ -19,10 +19,7 @@ import { import { RuntimeClientError } from '../runtime-client' import { getBrowserCommandTarget } from '../selectors' -// Why: selector/text/url waits can legitimately take longer than a normal RPC -// round-trip, even when Orca is healthy. Give browser.wait an explicit timeout -// budget so slow waits do not get mislabeled as "Orca is not running" by the -// generic client timeout path. +// Browser waits can outlast ordinary RPCs without indicating a disconnected runtime. const DEFAULT_BROWSER_WAIT_RPC_TIMEOUT_MS = 60_000 export const BROWSER_NAV_HANDLERS: Record = { diff --git a/src/cli/handlers/browser-profile.ts b/src/cli/handlers/browser-profile.ts index 4dc8ad731b3..5fae5245fc3 100644 --- a/src/cli/handlers/browser-profile.ts +++ b/src/cli/handlers/browser-profile.ts @@ -41,9 +41,7 @@ export const BROWSER_PROFILE_HANDLERS: Record = { scope }) if (result.result.profile === null) { - // Why: registry refuses non-isolated/imported scopes; we already validated - // the scope client-side, so a null here means a server-side rejection we - // shouldn't silently report as success. + // Client validation cannot guarantee the server accepted creation; null must not read as success. throw new RuntimeClientError( 'runtime_error', `Failed to create browser profile (label=${label}, scope=${scope})` diff --git a/src/cli/handlers/browser-storage.ts b/src/cli/handlers/browser-storage.ts index c2c98bb59fb..9275de041c0 100644 --- a/src/cli/handlers/browser-storage.ts +++ b/src/cli/handlers/browser-storage.ts @@ -12,7 +12,7 @@ export const BROWSER_STORAGE_HANDLERS: Record = { }, 'storage local set': async ({ flags, client, cwd, json }) => { const key = getRequiredStringFlag(flags, 'key') - // Why: the server accepts any string value; setItem(key, '') differs from an unset key. + // An empty value differs from an absent key. const value = getRequiredStringFlagAllowingEmpty(flags, 'value') const target = await getBrowserCommandTarget(flags, cwd, client) const result = await client.call('browser.storage.local.set', { diff --git a/src/cli/handlers/browser-tab.ts b/src/cli/handlers/browser-tab.ts index 5d19301ad4c..c4d519387a6 100644 --- a/src/cli/handlers/browser-tab.ts +++ b/src/cli/handlers/browser-tab.ts @@ -49,15 +49,9 @@ export const BROWSER_TAB_HANDLERS: Record = { if (index === undefined && !page) { throw new RuntimeClientError('invalid_argument', 'Missing required --index or --page') } - // Why: a stable browser page id is globally unique across Orca, so page- - // targeted tab switches should match the rest of the --page command model: - // global by default, with --worktree only acting as explicit validation. + // Page ids are globally unique; worktree is optional validation rather than an implicit filter. const target = await getBrowserCommandTarget(flags, cwd, client) - // Why: --focus is an opt-in side effect. The renderer's handler is - // worktree-scoped: it surfaces the browser pane only when the user is - // already on the targeted worktree, otherwise it pre-stages silently. - // Spread conditionally so the RPC payload stays shape-identical to the - // pre-flag form when --focus is absent. + // Focus surfaces the pane only in the current worktree; otherwise the renderer pre-stages it. const result = await client.call('browser.tabSwitch', { index, page, diff --git a/src/cli/handlers/bundled-skill-guide-table.ts b/src/cli/handlers/bundled-skill-guide-table.ts index efdf2ea003f..99dee6bb8d9 100644 --- a/src/cli/handlers/bundled-skill-guide-table.ts +++ b/src/cli/handlers/bundled-skill-guide-table.ts @@ -20,10 +20,7 @@ function canonicalGuides(guides: readonly BundledSkillGuide[]): BundledSkillGuid ) } -/** - * Load the embedded guide table in canonical order. Deferred because the table is - * large and unrelated CLI commands must not pay its module-load cost at startup. - */ +/** Defer the large guide table so unrelated commands avoid its startup cost. */ export async function loadCanonicalGuides(): Promise { const { BUNDLED_SKILL_GUIDES } = await import('../bundled-skill-guides.js') return canonicalGuides(BUNDLED_SKILL_GUIDES) @@ -41,8 +38,7 @@ export function requireTopic( `Missing skill topic. Available topics: ${availableTopics}` ) } - // Why: installed stubs may retain an old topic forever, so aliases and canonical - // names share one lookup table instead of being treated as transient CLI aliases. + // Installed stubs can retain old topics indefinitely, so aliases must remain valid lookup keys. const guideByTopic = new Map( guides.flatMap((guide) => [guide.name, ...guide.aliases].map((name) => [name, guide])) ) diff --git a/src/cli/handlers/bundled-skill-install-command.ts b/src/cli/handlers/bundled-skill-install-command.ts index a7b0047c3d5..4ad2740e1ef 100644 --- a/src/cli/handlers/bundled-skill-install-command.ts +++ b/src/cli/handlers/bundled-skill-install-command.ts @@ -34,7 +34,7 @@ export type SkillMutationVerb = 'install' | 'update' const SHARED_FOLDER_ONLY = 'universal' -// Why: earlier releases passed `--agent` straight to the skills CLI, so scripts still use its keys. +// Preserve --agent keys forwarded to the external skills CLI by earlier releases. const SKILLS_CLI_KEY_ALIASES = skillsCliAgentAliases(SKILLS_CLI_AGENT_KEY_BY_TUI_AGENT) /** `undefined` means detect; `[]` is `--agent universal`, the shared folder alone. */ @@ -48,7 +48,7 @@ function requestedAgents( if (typeof requested !== 'string') { return undefined } - // Why one comma-separated value: `agent` is a single-value flag on other commands. + // Other commands treat --agent as a single-value flag, so accept multiple providers with commas. const values = requested .split(',') .map((entry) => entry.trim()) @@ -156,8 +156,7 @@ function describePlan(plan: BundledSkillPlan): string[] { } async function workspaceDirectory(): Promise { - // Why the real path: the planner dedupes targets by real path, so the root it is given must - // be the same spelling every time this folder is planned. + // Match the real-path spelling used to deduplicate planner targets. return realpath(process.cwd()) } @@ -175,8 +174,7 @@ export function createSkillMutationHandler(verb: SkillMutationVerb): CommandHand return } - // Why before target resolution: the answer belongs to the other machine, and agents - // detected here would be the wrong host's. + // Local agent detection would target the wrong machine for a bridged invocation. if (process.env.ORCA_CLI_CWD) { throw new RuntimeClientError( 'invalid_environment', @@ -200,13 +198,12 @@ export function createSkillMutationHandler(verb: SkillMutationVerb): CommandHand selection: { names: skillNames, ...(flags.get('force') === true ? { force: 'all' as const } : {}), - // Why: update never adds a skill; install is the verb that does. installedOnly: verb === 'update' } }) const plan = await previewBundledSkillInstall(input) const planned = new Set(plan.skills.map((entry) => entry.name)) - // Why --all is exempt: it names every bundled skill, so skipping absent ones is expected. + // --all is allowed to skip bundled skills that are not installed. const notInstalled = verb === 'update' && flags.get('all') !== true ? skillNames.filter((name) => !planned.has(name)) diff --git a/src/cli/handlers/computer-action-validation.test.ts b/src/cli/handlers/computer-action-validation.test.ts index 621b0e41795..4c061bfde52 100644 --- a/src/cli/handlers/computer-action-validation.test.ts +++ b/src/cli/handlers/computer-action-validation.test.ts @@ -9,10 +9,7 @@ vi.mock('../runtime-client', async () => { openOrca = vi.fn() } - // Why: re-export the REAL error classes rather than redefining them. format.ts - // narrows with `instanceof` against ./runtime/types, so a look-alike class - // here would make every CLI error fall through to the generic `runtime_error` - // shape — mirroring the barrel keeps the mock faithful to production. + // Error formatting uses instanceof, so preserve the production error classes. const { RuntimeClientError, RuntimeRpcFailureError } = await import('../runtime/types.js') return { RuntimeClient, RuntimeClientError, RuntimeRpcFailureError } }) diff --git a/src/cli/handlers/computer.test.ts b/src/cli/handlers/computer.test.ts index 4fb36ae2362..1a681190ac0 100644 --- a/src/cli/handlers/computer.test.ts +++ b/src/cli/handlers/computer.test.ts @@ -9,9 +9,7 @@ vi.mock('../runtime-client', async () => { openOrca = vi.fn() } - // Why: re-export the REAL error classes; format.ts narrows with `instanceof` - // against ./runtime/types, so a look-alike would collapse every CLI error - // code into the generic `runtime_error` shape. + // Error formatting uses instanceof, so preserve the production error classes. const { RuntimeClientError, RuntimeRpcFailureError } = await import('../runtime/types.js') return { RuntimeClient, RuntimeClientError, RuntimeRpcFailureError } }) diff --git a/src/cli/handlers/core.test.ts b/src/cli/handlers/core.test.ts index ba2daf90c30..4d673a1c51c 100644 --- a/src/cli/handlers/core.test.ts +++ b/src/cli/handlers/core.test.ts @@ -2,12 +2,9 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' const { spawnMock } = vi.hoisted(() => ({ spawnMock: vi.fn() })) -// The claude-teams handler spawns `claude` via node:child_process; mock it so we -// can inspect the child env without launching a real process. vi.mock('node:child_process', () => ({ spawn: spawnMock })) -// Keep the socket runtime client out of the import graph; only the error type -// and serveOrcaApp binding are referenced by the module under test. +// Exclude the socket runtime graph; this handler needs only errors and serveOrcaApp. vi.mock('../runtime-client', () => ({ RuntimeClientError: class RuntimeClientError extends Error { readonly code: string @@ -25,8 +22,7 @@ import type { RuntimeClient } from '../runtime-client' type SpawnEnv = Record -// Minimal child stub: the handler only awaits `exit`, so resolve it on the next -// microtask to complete the spawned-process promise deterministically. +// Emit exit on the next microtask so the spawned-process promise settles deterministically. function mockClaudeChild(): { once: (event: string, cb: (...args: unknown[]) => void) => unknown } { const child = { once(event: string, cb: (...args: unknown[]) => void) { @@ -71,8 +67,7 @@ describe('orca claude-teams CLI handler', () => { previousRunAsNode = process.env.ELECTRON_RUN_AS_NODE previousPaneKey = process.env.ORCA_PANE_KEY previousExitCode = process.exitCode - // The `orca` launcher runs Orca's Electron binary as Node, so the CLI process - // itself carries ELECTRON_RUN_AS_NODE=1. Reproduce that inherited flag here. + // Reproduce the Electron-as-Node flag inherited from the orca launcher. process.env.ELECTRON_RUN_AS_NODE = '1' process.env.ORCA_PANE_KEY = 'tab-1:leaf-1' }) @@ -91,8 +86,7 @@ describe('orca claude-teams CLI handler', () => { process.exitCode = previousExitCode }) - // Guarded to non-Windows: the handler early-returns unsupported_platform on - // win32, so the leak path never runs there. + // Windows refuses the command before reaching child-process creation. it.skipIf(isWindows)( 'does not leak ELECTRON_RUN_AS_NODE into the spawned claude child', async () => { @@ -102,8 +96,6 @@ describe('orca claude-teams CLI handler', () => { const spawnEnv = spawnMock.mock.calls.at(-1)?.[2].env as SpawnEnv expect(spawnEnv.ELECTRON_RUN_AS_NODE).toBeUndefined() - // The prepareLaunch request env is built from the same helper, so it must - // be sanitized too. const prepareLaunchEnv = (callMock.mock.calls[0][1] as { env: SpawnEnv }).env expect(prepareLaunchEnv.ELECTRON_RUN_AS_NODE).toBeUndefined() } diff --git a/src/cli/handlers/core.ts b/src/cli/handlers/core.ts index dd999dc3142..33a764135ef 100644 --- a/src/cli/handlers/core.ts +++ b/src/cli/handlers/core.ts @@ -9,10 +9,7 @@ import { getOptionalServePort } from '../runtime/serve-port-flag' import { getServeOptionValidationError } from '../../shared/serve-option-validation' function envRecord(): Record { - // Why: the `orca` launcher runs Orca's Electron binary as Node, so this CLI - // process carries ELECTRON_RUN_AS_NODE=1. Strip it before it reaches the - // spawned `claude` (and any nested Electron it launches), which would - // otherwise be forced into headless plain-Node mode. + // Inherited ELECTRON_RUN_AS_NODE would force nested Electron children into plain Node mode. const env = stripElectronRunAsNode(process.env) return Object.fromEntries( Object.entries(env).filter((entry): entry is [string, string] => entry[1] !== undefined) diff --git a/src/cli/handlers/emulator.test.ts b/src/cli/handlers/emulator.test.ts index ff0dc5f141a..2dc08663d52 100644 --- a/src/cli/handlers/emulator.test.ts +++ b/src/cli/handlers/emulator.test.ts @@ -18,9 +18,7 @@ vi.mock('../runtime-client', async () => { } } - // Why: re-export the REAL error classes; format.ts narrows with `instanceof` - // against ./runtime/types, so a look-alike would collapse every CLI error - // code into the generic `runtime_error` shape. + // Error formatting uses instanceof, so preserve the production error classes. const { RuntimeClientError, RuntimeRpcFailureError } = await import('../runtime/types.js') return { RuntimeClient, RuntimeClientError, RuntimeRpcFailureError } }) diff --git a/src/cli/handlers/emulator.ts b/src/cli/handlers/emulator.ts index e41700ff4b5..ae5b9d220d5 100644 --- a/src/cli/handlers/emulator.ts +++ b/src/cli/handlers/emulator.ts @@ -128,8 +128,7 @@ export const EMULATOR_HANDLERS: Record = { const target = await getEmulatorCommandTarget(flags, cwd, client) const device = getOptionalStringFlag(flags, 'device') const focus = flags.get('focus') === true - // Why: attach may cold-boot or recycle a wedged simulator (shutdown + boot + - // helper restart), which can legitimately exceed the 60s default budget. + // Cold boots and wedged-simulator recovery can exceed the normal RPC timeout. const res = await client.call( 'emulator.attach', { device, worktree: target.worktree, focus }, diff --git a/src/cli/handlers/environment.ts b/src/cli/handlers/environment.ts index 8366763673d..7890bea7248 100644 --- a/src/cli/handlers/environment.ts +++ b/src/cli/handlers/environment.ts @@ -29,8 +29,7 @@ export const ENVIRONMENT_HANDLERS: Record = { throw new RuntimeClientError('invalid_argument', 'Missing value for --name') } if (typeof requestedName === 'string') { - // Why: an older runtime rejects the unknown settings field with a bare `invalid_params`; - // a runtime that does not publish a name cannot store one either, so say so plainly. + // Check name support before an older settings schema can reject the write as invalid_params. const current = await client.call('status.get') if (current.result.machineName === undefined) { throw new RuntimeClientError( @@ -40,8 +39,7 @@ export const ENVIRONMENT_HANDLERS: Record = { } await client.call('settings.update', { machineName: requestedName }) } - // Why: print what the runtime publishes after the write (a blank `--name` means the detected - // name), inside the runtime's own envelope so a routed answer is stamped with that runtime. + // A blank override restores the detected name; read it from the runtime after writing. const status = await client.call('status.get') printResult({ ...status, result: describeRuntimeHost(status.result) }, json, formatHostName) }, @@ -61,9 +59,6 @@ export const ENVIRONMENT_HANDLERS: Record = { `Saved environment ${result.environment.name} (${result.environment.id}).` ) }, - // Why: an agent told "run it on " had nowhere to look. `orca environment list` showed - // paired servers only, and nothing in the CLI listed SSH targets at all, so the wrong-axis - // guess was the only move available. This is the one place that answers both. 'host list': async ({ client, flags, json }) => { rejectLocalPairingStoreRetargeting( flags, @@ -79,8 +74,7 @@ export const ENVIRONMENT_HANDLERS: Record = { ...(target.connectionStatus ? { connectionStatus: target.connectionStatus } : {}), ...(target.remotePlatform ? { platform: target.remotePlatform } : {}) })) - // Why: this listing answers from the local pairing store; dialing each paired server would make - // it slow and offline-fragile. `orca host name --environment ` asks one server directly. + // Read the local pairing store without dialing every server, so listing works offline. const localStatus = await readLocalHostDescriptor(client) const environments = listEnvironments(getDefaultUserDataPath()).map((environment) => ({ kind: 'environment' as const, @@ -148,13 +142,7 @@ async function readLocalHostDescriptor(client: RuntimeClient): Promise, suffix: string, diff --git a/src/cli/handlers/file-absolute-paths.test.ts b/src/cli/handlers/file-absolute-paths.test.ts index f354e8151a6..56b579cc9db 100644 --- a/src/cli/handlers/file-absolute-paths.test.ts +++ b/src/cli/handlers/file-absolute-paths.test.ts @@ -39,8 +39,7 @@ describe('absolute file CLI paths', () => { vi.restoreAllMocks() callMock.mockReset() process.exitCode = undefined - // Why: a contributor running this suite inside WSL inherits a real - // WSL_DISTRO_NAME, which would flip the rewrite on for every case below. + // Prevent a real WSL_DISTRO_NAME from enabling the rewrite in unrelated cases. vi.stubEnv('WSL_DISTRO_NAME', '') vi.spyOn(console, 'log').mockImplementation(() => {}) vi.spyOn(console, 'error').mockImplementation(() => {}) @@ -142,8 +141,7 @@ describe('absolute file CLI paths', () => { expectOpenedWith('xxx/xxx.ts') }) - // Why: backslash is a legal Linux filename character, but reads as a separator - // once the path is UNC — rewriting `a\b.ts` would open `a/b.ts` instead. + // Backslashes are filename characters on Linux but separators in UNC paths. it('leaves a Linux path containing a backslash for the runtime guard', async () => { vi.stubEnv('WSL_DISTRO_NAME', 'Ubuntu') mockWorktreeShow(uncRoot) @@ -162,8 +160,7 @@ describe('absolute file CLI paths', () => { expectOpenedWith('/root/orca/workspaces/xxx/xxx/xxx.ts') }) - // Why: WSL_DISTRO_NAME is also set for a plain Linux CLI inside the distro, where - // roots are POSIX; prefixing there would strand every absolute path (#11393 regression). + // WSL_DISTRO_NAME also exists for Linux-native runtimes with POSIX roots. it('keeps relativizing POSIX roots while WSL_DISTRO_NAME is set', async () => { vi.stubEnv('WSL_DISTRO_NAME', 'Ubuntu') mockWorktreeShow('/root/orca/workspaces/xxx') @@ -182,7 +179,6 @@ describe('absolute file CLI paths', () => { expectOpenedWith('/root/orca/workspaces/xxx-2/xxx.ts') }) - // Why: Windows folds the distro segment, so only its case may differ from the root. it('matches when only the distro case differs', async () => { vi.stubEnv('WSL_DISTRO_NAME', 'ubuntu') mockWorktreeShow(uncRoot) @@ -210,8 +206,7 @@ describe('absolute file CLI paths', () => { expectOpenedWith('xxx/xxx.ts') }) - // Why: WSL_DISTRO_NAME only arrives if interop forwards it, but the launcher - // always sets ORCA_CLI_CWD, which reaches the handler as the invocation cwd. + // ORCA_CLI_CWD remains available when interop omits WSL_DISTRO_NAME. it('falls back to the distro named by a UNC cwd', async () => { mockWorktreeShow(uncRoot) diff --git a/src/cli/handlers/file-open-call.ts b/src/cli/handlers/file-open-call.ts index 0ccbe4ab3ec..d3eaea15743 100644 --- a/src/cli/handlers/file-open-call.ts +++ b/src/cli/handlers/file-open-call.ts @@ -13,11 +13,7 @@ function isRendererUnavailable(error: unknown): boolean { ) } -/** - * Why: a headless server (`orca serve`, orcad) has no window to put the tab in and answers with - * the bare `renderer_unavailable` token (#29822). The wire answer stays as is — the mobile app - * keys its preview fallback on it — so only the CLI turns it into something an agent can act on. - */ +/** Keep renderer_unavailable on the wire for mobile fallback; add CLI-specific recovery here. */ export async function callFileOpen( ctx: HandlerContext, method: 'files.open' | 'files.openDiff', diff --git a/src/cli/handlers/file.test.ts b/src/cli/handlers/file.test.ts index 5f306dd86e6..3a8fb008987 100644 --- a/src/cli/handlers/file.test.ts +++ b/src/cli/handlers/file.test.ts @@ -177,7 +177,6 @@ describe('orca file CLI handlers', () => { process.exitCode = undefined }) - // Why: an older host still answers a PDF with opened:false (STA-9113); that must not read as success. it('fails an unopened file open with ok:false in --json mode', async () => { queueFixtures( callMock, @@ -520,7 +519,7 @@ describe('orca file CLI handlers', () => { '/tmp/elsewhere' ) - // Why: an older host answers a binary edit open with opened:false, so focus carries to the next open. + // Declined binary opens must leave focus available for the next successful open. expect(callMock).toHaveBeenNthCalledWith(2, 'files.open', { worktree: 'id:wt-1', relativePath: 'assets/logo.png', diff --git a/src/cli/handlers/file.ts b/src/cli/handlers/file.ts index fac7a390cdc..a7f67af1d11 100644 --- a/src/cli/handlers/file.ts +++ b/src/cli/handlers/file.ts @@ -50,20 +50,9 @@ async function getFileWorktreeSelector({ flags, cwd, client }: HandlerContext): return await resolveCurrentWorktreeSelector(cwd, client) } -/** - * Why: a WSL workspace stores its worktree root as a Windows UNC path while the - * user types the Linux path they see inside the distro, so the two never match - * (#11393). Only the distro the caller is sitting in can name that Linux path, and - * only a UNC root wants the rewrite — a POSIX root already matches, so rewriting - * it would strand every absolute path. - * - * Backslash is a legal Linux filename character but a separator once the path - * reads as UNC, so `a\b.ts` would relativize to a different file, `a/b.ts`. - * Such a path has no Windows spelling; leave it to fail the match instead. - */ +// Only UNC roots need Linux-path translation; backslash filenames have no equivalent Windows spelling. function toWorktreeRootPathFlavor(rootPath: string, cwd: string, path: string): string { - // Why: WSL_DISTRO_NAME reaches this process only if interop forwards it, but the - // WSL launcher always sets ORCA_CLI_CWD, and its UNC form names the distro itself. + // Interop may omit WSL_DISTRO_NAME; the launcher's UNC cwd also identifies the caller's distro. const distro = process.env.WSL_DISTRO_NAME || parseWslUncPath(cwd)?.distro if ( !distro || @@ -85,7 +74,7 @@ async function resolveFilePath( if (!isRuntimePathAbsolute(path)) { return path } - // Why: only in-worktree absolute paths should be relativized here; outside paths must reach the runtime guard unchanged. + // Relativize only paths inside the worktree; leave others for the runtime guard. const result = await ctx.client.call<{ worktree: RuntimeWorktreeRecord }>('worktree.show', { worktree }) @@ -119,9 +108,7 @@ function getOpenChangedMode(flags: Map): OpenChangedMo throw new RuntimeClientError('invalid_argument', 'Invalid --mode. Use edit, diff, or both.') } -// Why: the CLI has no view of its own, so 'caller' moves nothing. --focus sends 'all', but unlike -// worktree create --activate a file open moves only the host's own window (intended; paired -// clients are not navigated). Hosts treat a missing field as the legacy switch. +// caller leaves views unchanged; --focus moves only the host window, preserving paired viewers. function getFileOpenNavigation(flags: Map): RuntimeNavigationTarget { return flags.get('focus') === true ? 'all' : 'caller' } @@ -193,7 +180,7 @@ function formatOpenChangedResult(result: FileOpenChangedResult): string { return lines.join('\n') } -// Why: opened:false (older hosts decline binaries) must fail a single-file command, not exit 0. +// A declined single-file open must exit unsuccessfully even when the RPC succeeded. function requireOpened(result: RuntimeFileOpenResult, target: string): void { if (!result.opened) { throw new RuntimeClientError( @@ -248,7 +235,7 @@ export const FILE_HANDLERS: Record = { const opened: FileOpenRecord[] = [] const skipped: FileOpenRecord[] = [] const openedEditPaths = new Set() - // Why: switch once — the first tab that actually opens carries --focus; later ones join the now-viewed worktree. + // Apply focus to the first successful open only. let navigation = getFileOpenNavigation(ctx.flags) const openWithFocusOnce = async ( open: () => Promise diff --git a/src/cli/handlers/interactive-login-interruption.ts b/src/cli/handlers/interactive-login-interruption.ts index 4ef1a06c2db..ef11786a1cf 100644 --- a/src/cli/handlers/interactive-login-interruption.ts +++ b/src/cli/handlers/interactive-login-interruption.ts @@ -61,10 +61,7 @@ export function terminateWindowsLoginProcessTree( }) } -/** - * Stops an interrupted interactive login before its credential directory is removed. - * The final close wait is bounded because a broken child handle must not hang Ctrl-C. - */ +/** Wait for close before credential cleanup, bounded so a broken child cannot hang Ctrl-C. */ export async function terminateInteractiveLoginProcess( child: ChildProcess, signal: NodeJS.Signals, @@ -112,9 +109,6 @@ export async function terminateInteractiveLoginProcess( } } -/** - * Runs an account add with signal-safe cleanup and primary-error precedence. - */ export async function withInteractiveLoginCleanup( session: InteractiveLoginSession, cleanup: () => Promise, @@ -147,7 +141,7 @@ export async function withInteractiveLoginCleanup( .catch(reportCleanupFailure) .finally(() => process.exit(INTERRUPT_EXIT_CODES[signal] ?? 1)) } - // Why: repeated signals must keep awaiting the same cleanup instead of restoring Node defaults. + // Repeated signals must await the same cleanup instead of restoring Node's termination default. for (const signal of INTERRUPT_SIGNALS) { process.on(signal, onSignal) } @@ -172,7 +166,6 @@ export async function withInteractiveLoginCleanup( } if (addFailed) { if (cleanupError !== undefined) { - // Why: retain the error that explains the failed account add. reportCleanupFailure(cleanupError) } throw addError diff --git a/src/cli/handlers/introspection.ts b/src/cli/handlers/introspection.ts index 92cef638e10..abc55372b80 100644 --- a/src/cli/handlers/introspection.ts +++ b/src/cli/handlers/introspection.ts @@ -3,8 +3,7 @@ import { COMMAND_SPECS } from '../specs' import { buildAgentContext, formatAgentContextSummary } from '../agent-context' export const INTROSPECTION_HANDLERS: Record = { - // Why: pure local command — reads the static spec registry and prints it, with - // no runtime RPC, so it works when the Orca app is not running (SSH/headless). + // Keep schema output runtime-free so it works offline and over SSH. 'agent-context': async ({ json }) => { const schema = buildAgentContext(COMMAND_SPECS) if (json) { diff --git a/src/cli/handlers/jira-write.test.ts b/src/cli/handlers/jira-write.test.ts index 192fae00a05..fdd71eb432d 100644 --- a/src/cli/handlers/jira-write.test.ts +++ b/src/cli/handlers/jira-write.test.ts @@ -31,7 +31,6 @@ async function clearAssigneeAfter(failure: Error): Promise { throw new Error('expected the write to fail') } -// Pins which write failures say "may have been applied" (round-2 S5), so a dropped code is deliberate. describe('Jira write outcome-unknown codes', () => { it.each([ 'runtime_timeout', diff --git a/src/cli/handlers/linear.test.ts b/src/cli/handlers/linear.test.ts index 98e369a739a..125461c7a94 100644 --- a/src/cli/handlers/linear.test.ts +++ b/src/cli/handlers/linear.test.ts @@ -19,9 +19,7 @@ vi.mock('../runtime-client', async () => { } } - // Why: re-export the REAL error classes; format.ts narrows with `instanceof` - // against ./runtime/types, so a look-alike would collapse every CLI error - // code into the generic `runtime_error` shape. + // Error formatting uses instanceof, so preserve the production error classes. const { RuntimeClientError, RuntimeRpcFailureError } = await import('../runtime/types.js') return { @@ -41,9 +39,7 @@ describe('orca linear CLI handlers', () => { vi.restoreAllMocks() callMock.mockReset() process.env = { ...originalEnv } - // Why: these tests can run inside an Orca-managed terminal, which exports - // real worktree/terminal/pairing env hints; clear them so handler context - // assertions stay deterministic. + // Clear real terminal/worktree/pairing hints inherited from the test runner. delete process.env.ORCA_WORKTREE_ID delete process.env.ORCA_TERMINAL_HANDLE delete process.env.ORCA_PAIRING_CODE diff --git a/src/cli/handlers/managed-server-reconnect.ts b/src/cli/handlers/managed-server-reconnect.ts index b9e31d82413..94ae2565130 100644 --- a/src/cli/handlers/managed-server-reconnect.ts +++ b/src/cli/handlers/managed-server-reconnect.ts @@ -1,8 +1,4 @@ -/** - * An update or rollback restarts the managed server, and a CLI talking to that runtime then sees - * its connection close mid-call. That is expected, not a failure: read the host's status once the - * runtime answers again and report what the action left behind. - */ +// Updates can close this connection mid-call; report the outcome from status after the runtime returns. import type { OrcadManagedRuntimeStatus } from '../../shared/orcad-managed-runtime' import type { HandlerContext } from '../dispatch' import { printResult } from '../format' diff --git a/src/cli/handlers/managed-server.test.ts b/src/cli/handlers/managed-server.test.ts index d20454f606e..2ff40a776e7 100644 --- a/src/cli/handlers/managed-server.test.ts +++ b/src/cli/handlers/managed-server.test.ts @@ -97,7 +97,7 @@ describe('managed server CLI verbs', () => { const log = vi.spyOn(console, 'log').mockImplementation(() => undefined) const confirmed = client(forgotten) await run('environment forget', confirmed, [['yes', true]]) - // Never managedServer.stop: an older desktop would drop a flag and decommission instead. + // Older desktops ignore flags; using stop for forget would decommission the server. expect(confirmed).toHaveBeenCalledWith( 'managedServer.forget', { selector: 'build-box' }, @@ -193,7 +193,6 @@ describe('managed server CLI verbs', () => { }) }) - // BUG-21: an update restarts the server, so the call's connection closes before it answers. it('reports what a restarting update left behind instead of failing on the closed connection', async () => { vi.useFakeTimers() const log = vi.spyOn(console, 'log').mockImplementation(() => {}) diff --git a/src/cli/handlers/managed-server.ts b/src/cli/handlers/managed-server.ts index 6005c19fec8..009b2562d4a 100644 --- a/src/cli/handlers/managed-server.ts +++ b/src/cli/handlers/managed-server.ts @@ -1,8 +1,3 @@ -/** - * `orca environment status|update|rollback|recover|stop|forget|cancel-stop`: the Managed servers - * actions over runtime RPC. Each call is gated on the runtime's managedServer.v1 capability, and - * an older runtime's method_not_found reads the same as a missing capability. - */ import type { OrcadManagedCancelStopResult, OrcadManagedDeployResult, @@ -25,8 +20,7 @@ import { reportAfterClosedConnection } from './managed-server-reconnect' const UNSUPPORTED_MESSAGE = 'This Orca runtime cannot manage servers over SSH. Run this on the computer whose Orca desktop app deployed the server, after updating Orca there.' -// Why 20 minutes: the desktop runs the whole action inline, and a Windows runtime promotion (5 min) -// plus a readiness wait (5 min) on a slow host already outlast the 60 s RPC default. +// Windows promotion and readiness waits can take ten minutes; allow headroom on slow hosts. export const MANAGED_SERVER_ACTION_TIMEOUT_MS = 20 * 60_000 const READ_ONLY_METHODS = new Set(['managedServer.status']) @@ -54,7 +48,7 @@ async function callManagedServer( if (error instanceof RuntimeClientError && error.code === 'method_not_found') { throw unsupported() } - // Why not a failure: the desktop keeps running the action after this client stops waiting. + // The runtime continues the action after this client times out. if (!readOnly && error instanceof RuntimeClientError && error.code === 'runtime_timeout') { throw new RuntimeClientError( 'managed_server_in_progress', @@ -88,10 +82,7 @@ function selectorOf(context: HandlerContext): { selector: string } { type Outcome = { outcome: string; code?: string; reason?: string } -/** - * Prints a result whose outcome is in `settled`; throws any other so scripts see a non-zero exit. - * Why an allow-list: an outcome a newer desktop adds must fail loudly, not print `undefined`. - */ +/** Reject unknown outcomes so newer-server responses cannot silently print undefined. */ function report( response: RuntimeRpcSuccess, json: boolean, diff --git a/src/cli/handlers/orchestration-caller-identity-cli.test.ts b/src/cli/handlers/orchestration-caller-identity-cli.test.ts index 6286ed680a8..c881b48494b 100644 --- a/src/cli/handlers/orchestration-caller-identity-cli.test.ts +++ b/src/cli/handlers/orchestration-caller-identity-cli.test.ts @@ -1,18 +1,9 @@ -/** - * How the orchestration CLI decides WHO is speaking. - * - * Split out of `orchestration.test.ts`, which sat exactly on the test-file line ceiling: these two - * suites are one subject — the coordinator and task-creator identity a command carries — and both - * exercise the env-handle validation and pane-remint chain rather than flag-to-param mapping. - */ - import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' const callMock = vi.fn() const getTerminalHandleMock = vi.hoisted(() => vi.fn()) const originalTerminalHandle = process.env.ORCA_TERMINAL_HANDLE const originalPaneKey = process.env.ORCA_PANE_KEY -// Why: isolate the handler's flag-to-param mapping; printResult only writes output. vi.mock('../format', () => ({ printResult: vi.fn() })) vi.mock('../selectors', () => ({ getTerminalHandle: getTerminalHandleMock })) @@ -24,8 +15,6 @@ function staleHandleError(): RuntimeClientError { return new RuntimeClientError('terminal_handle_stale', 'terminal_handle_stale') } -// Queues the stale-handle remint chain shared by coordinator commands: -// `terminal.resolveIdentity` answers not-live → resolvePane returns liveHandle → downstream RPC. function stubStaleHandleRemint(liveHandle: string, downstream: unknown): void { callMock .mockResolvedValueOnce(notLiveIdentity()) @@ -33,12 +22,10 @@ function stubStaleHandleRemint(liveHandle: string, downstream: unknown): void { .mockResolvedValueOnce(downstream) } -// Queues a not-live identity followed by a resolvePane remint that fails with `error`. function stubStaleHandleRemintFailure(error: RuntimeClientError): void { callMock.mockResolvedValueOnce(notLiveIdentity()).mockRejectedValueOnce(error) } -/** What the runtime answers for a handle whose leaf check reports `terminal_handle_stale`. */ function notLiveIdentity(): { result: { identity: { live: false } } } { return { result: { identity: { live: false } } } } diff --git a/src/cli/handlers/orchestration-gate-cli.test.ts b/src/cli/handlers/orchestration-gate-cli.test.ts index 7198ef5f4fd..83466b3278f 100644 --- a/src/cli/handlers/orchestration-gate-cli.test.ts +++ b/src/cli/handlers/orchestration-gate-cli.test.ts @@ -6,7 +6,7 @@ const { callMock, getTerminalHandleMock } = vi.hoisted(() => ({ })) vi.mock('../runtime-client', async () => { - // Why: re-export the REAL error classes so format.ts `instanceof` narrowing still matches. + // Error formatting uses instanceof, so preserve the production error classes. const { RuntimeClientError, RuntimeRpcFailureError } = await import('../runtime/types.js') class RuntimeClient { readonly isRemote = false @@ -34,8 +34,7 @@ import { okFixture, queueFixtures } from '../test-fixtures' const originalTerminalHandle = process.env.ORCA_TERMINAL_HANDLE const originalPaneKey = process.env.ORCA_PANE_KEY -// Why: a structured-session marker inherited from the runner diverts these cases to the -// structured refusal, so which branch they exercise would depend on who ran them. +// Isolate inherited session markers so the runner cannot change the refusal branch. const originalStructuredSession = process.env.ORCA_STRUCTURED_SESSION const originalCodexThreadId = process.env.CODEX_THREAD_ID @@ -167,7 +166,6 @@ describe('orchestration gate commands carry caller identity', () => { }) it('inspects a named Run without resolving a caller terminal', async () => { - // Why: read-only inspection must stay reachable from a pane with no bound Run. getTerminalHandleMock.mockRejectedValue( new RuntimeClientError('no_active_terminal', 'no active terminal') ) diff --git a/src/cli/handlers/orchestration-migration.test.ts b/src/cli/handlers/orchestration-migration.test.ts index 84c1038e3d7..369c9d13aa8 100644 --- a/src/cli/handlers/orchestration-migration.test.ts +++ b/src/cli/handlers/orchestration-migration.test.ts @@ -1,7 +1,7 @@ import { afterEach, describe, expect, it, vi } from 'vitest' import { ORCHESTRATION_HANDLERS } from './orchestration' -// worker_done carries a CLI-minted request id so its runtime_unavailable retries replay one mutation. +// Reuse one mutation identity when retrying worker_done after runtime_unavailable. const WORKER_DONE_REQUEST = { orchestrationRequestId: expect.any(String) } const originalPaneKey = process.env.ORCA_PANE_KEY diff --git a/src/cli/handlers/orchestration-request-show-cli.test.ts b/src/cli/handlers/orchestration-request-show-cli.test.ts index 6c7834f7e70..b33d41dc58d 100644 --- a/src/cli/handlers/orchestration-request-show-cli.test.ts +++ b/src/cli/handlers/orchestration-request-show-cli.test.ts @@ -1,5 +1,3 @@ -// `request-show` is the recovery path a lost mutation response sends you down, so it must -// stay read-only, render the honest reading, and name a version gap instead of a raw RPC error. import { beforeEach, describe, expect, it, vi } from 'vitest' const callMock = vi.fn() diff --git a/src/cli/handlers/orchestration-task-create-cli.test.ts b/src/cli/handlers/orchestration-task-create-cli.test.ts index 8b5957782d2..e633bdabcd7 100644 --- a/src/cli/handlers/orchestration-task-create-cli.test.ts +++ b/src/cli/handlers/orchestration-task-create-cli.test.ts @@ -4,7 +4,6 @@ const callMock = vi.hoisted(() => vi.fn()) const getTerminalHandleMock = vi.hoisted(() => vi.fn()) const originalTerminalHandle = process.env.ORCA_TERMINAL_HANDLE -// Why: isolate flag-to-RPC mapping; printResult only writes output. vi.mock('../format', () => ({ printResult: vi.fn() })) vi.mock('../selectors', () => ({ getTerminalHandle: getTerminalHandleMock })) diff --git a/src/cli/handlers/orchestration-task-list-brief.test.ts b/src/cli/handlers/orchestration-task-list-brief.test.ts index 968b0a979eb..090a56e3211 100644 --- a/src/cli/handlers/orchestration-task-list-brief.test.ts +++ b/src/cli/handlers/orchestration-task-list-brief.test.ts @@ -2,7 +2,6 @@ import { describe, expect, it, vi } from 'vitest' const callMock = vi.fn() -// Why: isolate the handler's flag-to-param mapping; printResult only writes output. vi.mock('../format', () => ({ printResult: vi.fn() })) import { ORCHESTRATION_HANDLERS } from './orchestration' @@ -26,7 +25,7 @@ describe('orchestration task-list brief output', () => { it('requests server-side brief and falls back client-side for older runtimes', async () => { callMock.mockReset().mockResolvedValue({ result: { - // No spec_truncated field — the pre-brief-runtime signature. + // Older runtimes omit spec_truncated. tasks: [{ id: 'task_1', spec: `First line\n${'detail '.repeat(40)}`, status: 'ready' }], count: 1 } @@ -50,8 +49,7 @@ describe('orchestration task-list brief output', () => { const response = await runTaskListBrief() - // Why: re-abbreviating a server-truncated spec would flip spec_truncated - // back to false (the truncated text fits the cap). + // Re-abbreviating would reset spec_truncated because the already-truncated text fits the cap. expect(response.result.tasks).toBe(serverTasks) }) }) diff --git a/src/cli/handlers/orchestration-worker-cli.test.ts b/src/cli/handlers/orchestration-worker-cli.test.ts index 24d1edf75a4..7da1746c1a7 100644 --- a/src/cli/handlers/orchestration-worker-cli.test.ts +++ b/src/cli/handlers/orchestration-worker-cli.test.ts @@ -572,7 +572,6 @@ describe('orchestration worker-start CLI contract', () => { } as never) ).rejects.toMatchObject({ code: 'incompatible_runtime' }) - // The extra call is the bound-Run lookup; the enumeration itself must not be retried. expect( callMock.mock.calls.filter(([method]) => method === 'orchestration.workerList') ).toHaveLength(1) diff --git a/src/cli/handlers/orchestration-worker-show-wait-cli.test.ts b/src/cli/handlers/orchestration-worker-show-wait-cli.test.ts index e42fea9850e..c8fef9cb2ee 100644 --- a/src/cli/handlers/orchestration-worker-show-wait-cli.test.ts +++ b/src/cli/handlers/orchestration-worker-show-wait-cli.test.ts @@ -1,4 +1,3 @@ -// `worker-show` text output must keep "nobody is waiting" apart from "nobody looked". import { beforeEach, describe, expect, it, vi } from 'vitest' const callMock = vi.fn() @@ -58,8 +57,6 @@ describe('orchestration worker-show interactive wait output', () => { }) it('says unknown when the field is absent, not none', async () => { - // An older host, or a worker whose identity could not be verified. Printing the same - // thing as an evaluated `null` is the false negative this field exists to remove. const line = await showWorker({ status: 'identity_changed', exactWorker: false }) expect(line).toContain('Interactive wait: unknown (not evaluated)') diff --git a/src/cli/handlers/orchestration.test.ts b/src/cli/handlers/orchestration.test.ts index a5b44d2163c..bd512ef665e 100644 --- a/src/cli/handlers/orchestration.test.ts +++ b/src/cli/handlers/orchestration.test.ts @@ -1,13 +1,12 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' const callMock = vi.fn() -// worker_done carries a CLI-minted request id so its runtime_unavailable retries replay one mutation. +// Reuse one mutation identity when retrying worker_done after runtime_unavailable. const WORKER_DONE_REQUEST = { orchestrationRequestId: expect.any(String) } const getTerminalHandleMock = vi.hoisted(() => vi.fn()) const originalTerminalHandle = process.env.ORCA_TERMINAL_HANDLE const originalPaneKey = process.env.ORCA_PANE_KEY -// Why: a structured-session marker inherited from the runner diverts these cases to the -// structured refusal, so which branch they exercise would depend on who ran them. +// Isolate inherited session markers so the runner cannot change the refusal branch. const originalStructuredSession = process.env.ORCA_STRUCTURED_SESSION const originalCliCommand = process.env.ORCA_CLI_COMMAND const originalExitCode = process.exitCode @@ -15,7 +14,6 @@ function lifecycleGroupRecipientError(type: 'worker_done' | 'heartbeat'): string return `${type} messages belong to one exact Dispatch and cannot target a group address.` } -// Why: isolate the handler's flag-to-param mapping; printResult only writes output. vi.mock('../format', () => ({ printResult: vi.fn() })) vi.mock('../selectors', () => ({ getTerminalHandle: getTerminalHandleMock })) @@ -274,10 +272,7 @@ describe('orchestration send structured payload flags', () => { ]) ) - // Why: pre-payload-authority runtimes only complete a worker_done whose - // sender equals the recorded (equally stale) assignee handle, and - // coordinator replies route to the sender row the worker's env-handle - // `check` actually reads — so lifecycle sends must stay env-verbatim. + // Older hosts match lifecycle senders to stale assignee handles, so preserve the env spelling. expect(callMock).toHaveBeenCalledTimes(1) expect(callMock).toHaveBeenCalledWith( 'orchestration.send', @@ -332,8 +327,7 @@ describe('orchestration send structured payload flags', () => { process.env.ORCA_STRUCTURED_SESSION = '1' getTerminalHandleMock.mockResolvedValue('term_sibling_pane') - // The refusal must not recommend --from: the explicit-flag branch returns before this guard, - // so the advice would succeed against a handle that necessarily belongs to another pane. + // --from bypasses this guard and could select another pane; never recommend it as recovery. await expect( invokeSend( new Map([ @@ -440,8 +434,7 @@ describe('orchestration timeout flag validation', () => { ]) ) - // Why: --peek rides with unread:false so pre-peek runtimes fall back to - // the non-consuming all mode instead of the destructive mark-read default. + // unread:false keeps --peek non-consuming when older runtimes ignore the peek field. expect(callMock).toHaveBeenCalledWith('orchestration.check', { terminal: 'term_worker', terminalPaneKey: undefined, @@ -479,8 +472,7 @@ describe('orchestration timeout flag validation', () => { } expect(response.result.messages.map((m) => m.id)).toEqual(['msg_new']) expect(response.result.count).toBe(1) - // Why: the pre-peek runtime built `formatted` from all rows, including - // the read one the filter just removed. + // Older formatted output includes read rows removed by the client filter. expect(response.result.formatted).toBeUndefined() }) diff --git a/src/cli/handlers/orchestration/check-keepalive.ts b/src/cli/handlers/orchestration/check-keepalive.ts index a87f6065428..ad867f5b529 100644 --- a/src/cli/handlers/orchestration/check-keepalive.ts +++ b/src/cli/handlers/orchestration/check-keepalive.ts @@ -1,6 +1,6 @@ const DEFAULT_KEEPALIVE_INTERVAL_MS = 15_000 -// Why: test-only escape hatch so subprocess tests avoid the full 15 s window; bogus values fall back to the default. +// Tests override the interval to avoid the full 15-second wait. function resolveKeepaliveIntervalMs(): number { const raw = process.env.ORCA_KEEPALIVE_INTERVAL_MS ?? process.env.ORCA_HEARTBEAT_INTERVAL_MS if (!raw) { @@ -18,7 +18,7 @@ export function startCheckKeepalive(deadlineMs: number | undefined): () => void const interval = setInterval(() => { const payload = { _keepalive: true, - // Why: retain the old marker for scripts still filtering it while callers migrate to _keepalive. + // Preserve the old marker for scripts migrating to _keepalive. _heartbeat: true, elapsedMs: Date.now() - startedAt, deadlineMs: deadlineMs ?? null diff --git a/src/cli/handlers/orchestration/dispatch-handlers.ts b/src/cli/handlers/orchestration/dispatch-handlers.ts index c8ade53e907..b1474eeea4d 100644 --- a/src/cli/handlers/orchestration/dispatch-handlers.ts +++ b/src/cli/handlers/orchestration/dispatch-handlers.ts @@ -13,7 +13,6 @@ export const ORCHESTRATION_DISPATCH_HANDLER: Record = { const from = await resolveCoordinatorTerminalHandle(flags, cwd, client) const dryRun = flags.has('dry-run') ? true : undefined const returnPreamble = flags.has('return-preamble') ? true : undefined - // Why: --to is only required for non-dry-run; the RPC handler re-enforces. const to = dryRun ? getOptionalStringFlag(flags, 'to') : getRequiredStringFlag(flags, 'to') const result = await callOrchestrationMutation<{ dispatch: { id: string; task_id: string; status: string } | null @@ -43,8 +42,7 @@ export const ORCHESTRATION_DISPATCH_HANDLER: Record = { export const ORCHESTRATION_DISPATCH_INSPECTION_HANDLERS: Record = { 'orchestration dispatch-show': async ({ flags, client, cwd, json }) => { const showPreamble = flags.has('preamble') ? true : undefined - // Why: a preview must embed the same real coordinator handle as an actual dispatch. Its --from - // only fills preview text and names no caller, so it passes through unfenced. + // --from fills preview text rather than asserting caller identity, so it is intentionally unfenced. const from = showPreamble ? (getOptionalStringFlag(flags, 'from') ?? injectedSessionAddress() ?? diff --git a/src/cli/handlers/orchestration/gate-handlers.ts b/src/cli/handlers/orchestration/gate-handlers.ts index 44c8f96d818..2fa44a1b61a 100644 --- a/src/cli/handlers/orchestration/gate-handlers.ts +++ b/src/cli/handlers/orchestration/gate-handlers.ts @@ -12,7 +12,6 @@ export const ORCHESTRATION_GATE_HANDLERS: Record = { task: getRequiredStringFlag(flags, 'task'), question: getRequiredStringFlag(flags, 'question'), options: getOptionalJsonFlag(flags, 'options'), - // Why: gates are Run-scoped, so the coordinator handle is the authorized caller identity. from: await resolveCoordinatorTerminalHandle(flags, cwd, client) }) printResult( @@ -36,7 +35,7 @@ export const ORCHESTRATION_GATE_HANDLERS: Record = { 'orchestration gate-list': async ({ flags, client, cwd, json }) => { const run = getOptionalStringFlag(flags, 'run') - // Why: named runs remain inspectable without a pane; only implicit runs resolve identity. + // Named runs can be inspected without resolving a pane identity. const from = run ? runScopedSessionCaller(flags) : await resolveCoordinatorTerminalHandle(flags, cwd, client) diff --git a/src/cli/handlers/orchestration/message-check-handler.ts b/src/cli/handlers/orchestration/message-check-handler.ts index 6ace43eac31..1a072b97fcf 100644 --- a/src/cli/handlers/orchestration/message-check-handler.ts +++ b/src/cli/handlers/orchestration/message-check-handler.ts @@ -31,7 +31,7 @@ export const ORCHESTRATION_CHECK_HANDLER: Record = { 'orchestration check': async ({ flags, client, cwd, json }) => { const wait = flags.has('wait') const peek = flags.has('peek') - // Why: older runtimes strip unknown peek and run --unread --peek as destructive mark-read. + // Older runtimes ignore peek, making --unread --peek destructive. if ([flags.has('unread'), peek, flags.has('all')].filter(Boolean).length > 1) { throw new RuntimeClientError( 'invalid_argument', @@ -41,7 +41,7 @@ export const ORCHESTRATION_CHECK_HANDLER: Record = { const timeoutMs = getOptionalPositiveIntegerValueFlag(flags, 'timeout-ms') const explicitTerminal = getOptionalStringFlag(flags, 'terminal') const terminal = await resolveOrchestrationTerminalHandle(flags, cwd, client, 'terminal') - // Why: a session names itself by its id alone; a pane key it inherited is not its identity. + // Sessions use their injected id; inherited pane keys belong to a different caller. const paneKey = explicitTerminal || terminal === undefined ? undefined : process.env.ORCA_PANE_KEY const callerLabel = orchestrationCallerLabel(terminal) @@ -51,7 +51,7 @@ export const ORCHESTRATION_CHECK_HANDLER: Record = { result = await callOrchestrationMutation(client, flags, 'orchestration.check', { terminal, terminalPaneKey: paneKey || undefined, - // Why: old runtimes degrade peek to non-consuming all mode instead of destructive mark-read. + // unread:false makes hosts that ignore peek fall back to non-consuming all mode. unread: flags.has('unread') ? true : peek ? false : undefined, peek: peek ? true : undefined, all: flags.has('all') ? true : undefined, @@ -99,7 +99,7 @@ function filterLegacyPeekResult( const rawRowCount = result.result.messages.length const unreadOnly = result.result.messages.filter((message) => message.read !== 1) const removedReadRows = unreadOnly.length !== rawRowCount - // Why: read rows prove a pre-peek runtime cannot honor wait, so fail instead of returning early. + // Read rows reveal an older runtime returning before the requested unread wait is satisfied. if (wait && removedReadRows && unreadOnly.length === 0) { throw new RuntimeClientError( 'peek_wait_unsupported', @@ -115,7 +115,7 @@ function filterLegacyPeekResult( ...result, result: { ...result.result, - // Why: a pre-peek runtime formats all rows, which no longer matches the filtered set. + // Filtered rows invalidate the older host's preformatted output. ...(removedReadRows ? { formatted: undefined } : {}), messages: unreadOnly, count: unreadOnly.length diff --git a/src/cli/handlers/orchestration/message-inbox-handlers.ts b/src/cli/handlers/orchestration/message-inbox-handlers.ts index c15c4edfafa..8623b5db08c 100644 --- a/src/cli/handlers/orchestration/message-inbox-handlers.ts +++ b/src/cli/handlers/orchestration/message-inbox-handlers.ts @@ -42,7 +42,6 @@ export const ORCHESTRATION_INBOX_HANDLERS: Record = { if (value.count === 0) { return 'No messages.' } - // Why: default output omits body/payload for at-a-glance sweeps; --full prints them for auditing. return value.messages .map((message) => { const head = `${message.id}${formatMessageReadOnlyTag(message)} ${message.from_handle} -> ${message.to_handle ?? '?'}: "${message.subject}"` diff --git a/src/cli/handlers/orchestration/message-payload.ts b/src/cli/handlers/orchestration/message-payload.ts index d847631dd9e..26213dd3b00 100644 --- a/src/cli/handlers/orchestration/message-payload.ts +++ b/src/cli/handlers/orchestration/message-payload.ts @@ -27,7 +27,7 @@ export function getOptionalStructuredMessagePayload( 'Use either --payload or structured payload flags, not both.' ) } - // Why: raw JSON args are fragile in Windows PowerShell; these flags avoid shell-specific quoting. + // Structured flags avoid fragile PowerShell JSON quoting. const payload: Record = {} if (taskId) { payload.taskId = taskId diff --git a/src/cli/handlers/orchestration/message-send-handler.ts b/src/cli/handlers/orchestration/message-send-handler.ts index 3b024de2cff..243f6894de0 100644 --- a/src/cli/handlers/orchestration/message-send-handler.ts +++ b/src/cli/handlers/orchestration/message-send-handler.ts @@ -81,11 +81,11 @@ export const ORCHESTRATION_SEND_HANDLER: Record = { !process.env.ORCA_TERMINAL_HANDLE && !readInjectedAgentSessionId() ) { - // Why: focus isn't lifecycle authority — an identity-less subprocess must fail closed rather than guess the worker. + // Focus cannot establish lifecycle ownership; never guess a worker for an identity-less subprocess. throwNoActiveSenderTerminal() } - // Why: lifecycle senders preserve ORCA_TERMINAL_HANDLE across restarts for older runtimes. + // Preserve the env handle across remints for older runtimes' lifecycle ownership checks. const from = await resolveOrchestrationTerminalHandle(flags, cwd, client, 'from') const sendParams = { from, @@ -97,8 +97,7 @@ export const ORCHESTRATION_SEND_HANDLER: Record = { priority: getOptionalStringFlag(flags, 'priority'), threadId: getOptionalStringFlag(flags, 'thread-id'), payload: getOptionalStructuredMessagePayload(flags), - // Why: pane key is the remint-stable sender identity the runtime verifies lifecycle ownership against; older runtimes strip it. - // A session names itself by its id alone. + // Pane keys survive remints; sessions instead authenticate by injected id, never inherited pane keys. senderPaneKey: from === undefined || readInjectedAgentSessionId() ? undefined @@ -113,7 +112,7 @@ export const ORCHESTRATION_SEND_HANDLER: Record = { 'orchestration.send', sendParams, dispatchCapability ? { orchestrationCapability: dispatchCapability } : undefined, - // Why: a worker reports once and ends its turn, so a brief app outage must delay worker_done, not drop it. + // Workers report once before ending their turn; retry brief outages so worker_done is not lost. type === 'worker_done' ? WORKER_DONE_UNAVAILABLE_RETRY_MS : 0 ) await requireWorkerDoneSettlement(client, type, sendParams.payload, result.result) diff --git a/src/cli/handlers/orchestration/mutation-request-show-handler.ts b/src/cli/handlers/orchestration/mutation-request-show-handler.ts index eb1251eea12..f0eddcd4480 100644 --- a/src/cli/handlers/orchestration/mutation-request-show-handler.ts +++ b/src/cli/handlers/orchestration/mutation-request-show-handler.ts @@ -10,8 +10,6 @@ export const ORCHESTRATION_REQUEST_SHOW_HANDLER: Record const result = await client .call('orchestration.requestShow', { request }) .catch((error: unknown) => { - // Why: an Orca server older than request-show answers method_not_found, which reads - // as a bug rather than a version gap on the very path a lost response sends you down. if (error instanceof RuntimeClientError && error.code === 'method_not_found') { throw new RuntimeClientError( 'incompatible_runtime', diff --git a/src/cli/handlers/orchestration/mutation-request.test.ts b/src/cli/handlers/orchestration/mutation-request.test.ts index 24d98e75357..7173b8e9b77 100644 --- a/src/cli/handlers/orchestration/mutation-request.test.ts +++ b/src/cli/handlers/orchestration/mutation-request.test.ts @@ -176,7 +176,7 @@ describe('callOrchestrationMutation runtime_unavailable retry', () => { }) }) - // Why: the fake runtime listens on a Unix socket path, which Windows does not accept. + // Windows cannot use this temporary Unix socket path. it.skipIf(process.platform === 'win32')( 'reaches a runtime that comes back after the first attempt found it down', async () => { @@ -191,7 +191,7 @@ describe('callOrchestrationMutation runtime_unavailable retry', () => { if (request.method === 'orchestration.send') { sendRequestIds.push(String(request.orchestrationRequestId)) if (sendRequestIds.length === 1) { - // A runtime that drops the connection mid-request leaves the outcome unknown. + // Dropping the connection mid-request leaves the mutation outcome unknown. socket.destroy() return } @@ -219,7 +219,7 @@ describe('callOrchestrationMutation runtime_unavailable retry', () => { undefined, RETRY_MS ) - // The runtime is down for the first attempt: no metadata, so even the contract probe fails. + // Initial attempts have no metadata, so the contract probe fails until this runtime starts. await new Promise((resolve) => server.listen(endpoint, resolve)) writeFileSync( join(userDataPath, 'orca-runtime.json'), diff --git a/src/cli/handlers/orchestration/mutation-request.ts b/src/cli/handlers/orchestration/mutation-request.ts index 2e9836c20ef..b81fa6aa618 100644 --- a/src/cli/handlers/orchestration/mutation-request.ts +++ b/src/cli/handlers/orchestration/mutation-request.ts @@ -13,7 +13,7 @@ export async function callOrchestrationMutation( options?: { timeoutMs?: number; orchestrationCapability?: string }, unavailableRetryMs = 0 ) { - // Why: every retry reuses one request id, so the host replays instead of applying the mutation twice. + // Reuse one request id so retries replay the mutation instead of applying it twice. const requestId = readRetryRequestFlag(flags) ?? (unavailableRetryMs > 0 ? randomUUID() : undefined) const deadline = Date.now() + unavailableRetryMs @@ -35,7 +35,7 @@ export async function callOrchestrationMutation( sentError = error } if (!unavailable || Date.now() + delayMs > deadline) { - // Why: a later attempt can fail before its request id is attached, though an earlier one may have landed. + // A later failure may lack the request id even though an earlier attempt landed. throw orchestrationMutationRecoveryError( carriesRequestId(error) ? error : (sentError ?? error) ) diff --git a/src/cli/handlers/orchestration/question-handler.ts b/src/cli/handlers/orchestration/question-handler.ts index f979fc79d15..d59d97c1ee8 100644 --- a/src/cli/handlers/orchestration/question-handler.ts +++ b/src/cli/handlers/orchestration/question-handler.ts @@ -69,13 +69,12 @@ export const ORCHESTRATION_QUESTION_HANDLER: Record = { compatibilityCliCommand: resolveCompatibilityCliCommand(), compatibilityWindowsCommand: resolvePackagedWindowsCompatibilityCommand() }, - // Why: extend past timeoutMs so transport does not abort before the runtime resolves its timeout. + // Outlive the server timeout so its result arrives before the transport aborts. { timeoutMs: resolveOrchestrationAskClientTimeoutMs(parsedTimeoutMs), orchestrationCapability: getOptionalStringFlag(flags, 'dispatch-capability') } ) - // Why: same {ok, result} envelope as every sibling verb; ask used to print a bare object. if (json) { printResult(result, true, () => '') } else if (result.result.legacyCompatibility?.resumeRequired) { @@ -99,7 +98,7 @@ export const ORCHESTRATION_QUESTION_HANDLER: Record = { } if (result.result.timedOut) { if (!json) { - // Why: report the server's clamped effective budget rather than overstating the wait. + // Report the server's effective budget, which may be clamped below the requested timeout. const waitedMs = result.result.timeoutMs ?? timeoutMs const messageId = result.result.messageId const dispatchCapability = getOptionalStringFlag(flags, 'dispatch-capability') diff --git a/src/cli/handlers/orchestration/task-handlers.ts b/src/cli/handlers/orchestration/task-handlers.ts index d6201087792..72c6ba929e7 100644 --- a/src/cli/handlers/orchestration/task-handlers.ts +++ b/src/cli/handlers/orchestration/task-handlers.ts @@ -62,7 +62,7 @@ export const ORCHESTRATION_TASK_HANDLERS: Record = { run, callerTerminalHandle }) - // Why: only older runtimes (no spec_truncated) skip server-side abbreviation and need this client-side fallback. + // Only hosts that omit spec_truncated need client-side abbreviation. const needsClientAbbreviation = brief && result.result.tasks.some((task) => task.spec_truncated === undefined) const output = needsClientAbbreviation diff --git a/src/cli/handlers/orchestration/terminal-identity.ts b/src/cli/handlers/orchestration/terminal-identity.ts index c3fefe71027..a96fb20d6ed 100644 --- a/src/cli/handlers/orchestration/terminal-identity.ts +++ b/src/cli/handlers/orchestration/terminal-identity.ts @@ -9,11 +9,7 @@ import { } from '../../../shared/agent-session-caller-env' import { sessionAddressForHost } from '../../session-caller-flags' -/** - * The caller's terminal handle, or `undefined` when an injected agent session id names the caller: - * the orchestration envelope carries that id and the host binds the caller param to it, so nothing - * is resolved or guessed here, except a session address only the host can place. - */ +/** undefined delegates caller identity to the injected session; the host resolves /clear addresses. */ export async function resolveOrchestrationTerminalHandle( flags: Map, cwd: string, @@ -21,7 +17,7 @@ export async function resolveOrchestrationTerminalHandle( flagName: 'from' | 'terminal', options: { validateEnvHandle?: boolean } = {} ): Promise { - // A caller flag naming anyone else was already refused at the CLI entry, from the command's spec. + // CLI entry already fences caller flags using the command spec. const explicit = getOptionalStringFlag(flags, flagName) const sessionId = readInjectedAgentSessionId() if (sessionId) { @@ -33,7 +29,7 @@ export async function resolveOrchestrationTerminalHandle( const envHandle = process.env.ORCA_TERMINAL_HANDLE if (envHandle && envHandle.length > 0) { if (flagName === 'from' && options.validateEnvHandle) { - // Why: long-lived shells can retain a stale ORCA_TERMINAL_HANDLE after remint; don't bake it into coordinator preambles. + // Shells can retain stale handles after remint; validate before embedding them in coordinator preambles. const live = await isLiveTerminalHandle(envHandle, client) if (!live) { const reminted = await resolveOrchestrationPaneTerminalHandle(client) @@ -45,11 +41,7 @@ export async function resolveOrchestrationTerminalHandle( } return envHandle } - // Past this point every remaining route GUESSES an implicit terminal, and a structured session - // has no pane for the guess to land on — so it lands on a sibling. `check` is destructive by - // default, so that guess consumed another pane's oldest unread batch and marked it read, and the - // rightful worker never saw its mail. Refusing is the only honest answer: this child genuinely - // cannot infer its own identity. + // Implicit guesses can select a sibling pane and consume its mail; identity-less sessions must refuse. if (hasStructuredSessionMarker()) { throw structuredSessionRefusal(flagName) } @@ -59,14 +51,7 @@ export async function resolveOrchestrationTerminalHandle( return await getTerminalHandle(flags, cwd, client) } -/** - * Whether the handle this process was born with still names a live identity. - * - * `terminal.resolveIdentity`, never `terminal.show`: `show` is a PTY verb, so it missed for a - * structured worker and reported `terminal_handle_stale` for a handle that was perfectly live — - * which then failed every coordinator verb, because the pane remint below needs an `ORCA_PANE_KEY` - * a structured child deliberately does not carry. - */ +// terminal.show requires a PTY; resolveIdentity also recognizes structured workers without panes. async function isLiveTerminalHandle(handle: string, client: RuntimeClient): Promise { try { const response = await client.call<{ identity?: { live?: boolean } }>( @@ -81,9 +66,7 @@ async function isLiveTerminalHandle(handle: string, client: RuntimeClient): Prom return false } if (getClientErrorCode(err) === 'method_not_found') { - // Clients and remote hosts update independently, so a host that predates the identity probe - // is the normal mixed-version state. Fall back to what it does have — which is correct for - // that host, because a host without the probe also has no structured workers to miss. + // Hosts without the identity probe also predate structured workers, so terminal.show is sufficient. return await showResolvesTerminalHandle(handle, client) } throw err @@ -128,7 +111,7 @@ async function resolveOrchestrationPaneTerminalHandle( return undefined } try { - // Why: pane-key reminting preserves caller identity; focus-based active-terminal fallback can point at a different pane. + // Remint by pane key; focus-based fallback can select another caller. const response = await client.call<{ terminal: { handle: string } }>('terminal.resolvePane', { paneKey }) @@ -172,15 +155,11 @@ function getClientErrorMessage(err: unknown): string | undefined { return typeof message === 'string' ? message : undefined } -/** How check output names its caller: the handle, or the session's address. */ export function orchestrationCallerLabel(handle: string | undefined): string { return handle ?? injectedSessionAddress() ?? 'unknown' } -/** - * The caller for a read that `--run` scopes: no terminal is resolved, but a session's declared - * address still goes to the host, which places a `/clear` root and refuses anyone else. - */ +/** Named-run reads need no terminal lookup, but the host must still verify declared session lineage. */ export function runScopedSessionCaller(flags: Map): string | undefined { const declared = getOptionalStringFlag(flags, 'from') const sessionId = readInjectedAgentSessionId() @@ -203,8 +182,7 @@ async function resolveImplicitOrchestrationSender( client: RuntimeClient ): Promise { try { - // Unambiguous: naming the sender is an identity claim, so an arbitrary pick would let this - // command speak as a sibling worker. + // An arbitrary active-terminal choice would let this command speak as a sibling worker. return await getTerminalHandle(flags, cwd, client, { requireUnambiguous: true }) } catch (err) { if (!isNoActiveTerminalError(err)) { @@ -214,11 +192,7 @@ async function resolveImplicitOrchestrationSender( } } -/** - * Why no flag is suggested: every caller reaches a refusal only after the explicit-flag branch has - * already returned, so `--from` advice would succeed — against a handle that necessarily belongs to - * another pane, whose unread mail the next `check` consumes. - */ +// --from bypasses implicit resolution and could consume another pane's mailbox; never suggest it here. function structuredSessionRefusal(flagName: 'from' | 'terminal'): RuntimeClientError { return new RuntimeClientError( 'no_active_sender_terminal', @@ -230,14 +204,11 @@ function structuredSessionRefusal(flagName: 'from' | 'terminal'): RuntimeClientE } export function throwNoActiveSenderTerminal(): never { - // Lifecycle sends refuse here before the structured guard above ever runs, so this is the only - // place left that would tell an identity-less session to pass a handle it does not have. A stale - // ORCA_TERMINAL_HANDLE is a different case — that caller HAS an identity, so it keeps the advice - // to re-run under a live one. + // Lifecycle sends can refuse before the structured guard; distinguish missing identity from stale handles. if (hasStructuredSessionMarker() && !process.env.ORCA_TERMINAL_HANDLE) { throw structuredSessionRefusal('from') } - // Why: Codex sets CODEX_THREAD_ID after its env filter, which is what drops Orca's (#25775). + // Codex sets CODEX_THREAD_ID after the env filter that can drop Orca identity (#25775). if (process.env.CODEX_THREAD_ID && !process.env.ORCA_TERMINAL_HANDLE) { throw new RuntimeClientError( 'no_active_sender_terminal', diff --git a/src/cli/handlers/orchestration/worker-list-run-scope.test.ts b/src/cli/handlers/orchestration/worker-list-run-scope.test.ts index 0dbc41a38a7..6a75ad2c074 100644 --- a/src/cli/handlers/orchestration/worker-list-run-scope.test.ts +++ b/src/cli/handlers/orchestration/worker-list-run-scope.test.ts @@ -3,9 +3,6 @@ import { ORCHESTRATION_HANDLERS } from '../orchestration' type Call = { name: string; params: Record } -/** The runtime half of this seam (`runCurrent` from a coordinator handle, `workerList` filtered by - * `run`) is proven in `rpc/methods/orchestration/worker/worker-list-run-scope-rpc.test.ts`; this - * half proves the handler asks exactly those two questions and reports what it decided. */ describe('orchestration worker-list Run scope (CLI handler)', () => { const originalTerminalHandle = process.env.ORCA_TERMINAL_HANDLE let calls: Call[] diff --git a/src/cli/handlers/orchestration/worker-list-run-scope.ts b/src/cli/handlers/orchestration/worker-list-run-scope.ts index 13037616153..39eeb8e1b7c 100644 --- a/src/cli/handlers/orchestration/worker-list-run-scope.ts +++ b/src/cli/handlers/orchestration/worker-list-run-scope.ts @@ -2,15 +2,9 @@ import { getOptionalStringFlag } from '../../flags' import type { RuntimeClient } from '../../runtime-client' import { resolveOrchestrationTerminalHandle } from './terminal-identity' -/** Which Run `worker-list` enumerated, and why. Additive: old readers ignore it. */ export type WorkerListRunScope = { run?: string; source: 'flag' | 'bound' | 'all' } -/** - * Unscoped `worker-list` returned every Dispatch the database has ever held. The bound Run is - * the same Run `check` reads from the calling terminal, so it is the default and `--run` - * overrides it. With no binding to read, listing everything stays the answer and the receipt - * says which of the three it was. - */ +// Default to the caller's bound Run; an explicit --run overrides it and unbound callers see all Runs. export async function resolveWorkerListRunScope( flags: Map, cwd: string, @@ -27,8 +21,7 @@ export async function resolveWorkerListRunScope( }) return current.result.run ? { run: current.result.run.id, source: 'bound' } : { source: 'all' } } catch { - // No live terminal, no stable pane, or a runtime that predates runCurrent: the caller asked - // for an inventory, so answer with the whole one rather than failing the enumeration. + // A missing caller binding or older runtime must not prevent a full inventory. return { source: 'all' } } } diff --git a/src/cli/handlers/orchestration/worker-observation-handlers.ts b/src/cli/handlers/orchestration/worker-observation-handlers.ts index 80e6bfdd944..8e2c315bf93 100644 --- a/src/cli/handlers/orchestration/worker-observation-handlers.ts +++ b/src/cli/handlers/orchestration/worker-observation-handlers.ts @@ -26,15 +26,14 @@ export const ORCHESTRATION_WORKER_OBSERVATION_HANDLERS: Record { - // The replay case this row is durable for: SQLite-backed, re-sent on every - // reconnect, and read here by a client that draws no roster block, runs no - // reconciliation, and cannot re-check whether those children still exist. A - // sentence frozen mid-flight outlives the process that wrote it, so it must - // not keep asserting a liveness only that process could have observed — - // `docs/reference/ssh-execution-boundary.md` calls that loss of contact - // reported as a live state. + // Replayed journal rows cannot prove current child liveness. it('replays a mid-flight roster row without claiming a child is still working', () => { const midFlight: readonly NativeChatSubagentEntry[] = [ { id: 'child-1', label: 'read', state: 'working' }, @@ -354,10 +348,7 @@ describe('formatWorkerRead', () => { expect(output).not.toMatch(/\bworking\b/) }) - // The body `codexSubagentGroupBody` actually writes: the plain-text twin, then - // the block it stands in for. The twin exists for clients that cannot draw the - // block, so a client printing the block must not print the twin beside it — - // the renderer drops the twin for the same reason, from the other side. + // Producers emit a text twin for clients that cannot render roster blocks; print only one copy. it('prints the roster sentence once for the two-block row the producer writes', () => { const sentence = subagentGroupFallbackText(ROSTER) const output = formatWorkerRead( @@ -374,9 +365,6 @@ describe('formatWorkerRead', () => { expect(occurrences(output, sentence)).toBe(1) }) - // Suppression is per twin, not per message. One twin beside two roster blocks - // silenced BOTH groups and printed one sentence, so the second roster vanished - // with no marker — the same silent drop the missing-twin case above avoids. it('stands in for the second roster block when only one twin accompanies two', () => { const other: readonly NativeChatSubagentEntry[] = [ { id: 'child-3', label: 'plan', state: 'completed' } @@ -396,10 +384,6 @@ describe('formatWorkerRead', () => { expect(output).toContain(`[subagents] ${subagentGroupFallbackText(other)}`) }) - // Which group a lone twin belongs to is decided by its TEXT, not its position. - // Claiming positionally silenced whichever group came first, so a twin - // belonging to a LATER group erased the earlier group's roster and printed the - // later one's sentence twice — the same silent drop, one permutation over. it('claims a lone twin for the group it names, not the first group in the message', () => { const other: readonly NativeChatSubagentEntry[] = [ { id: 'child-3', label: 'plan', state: 'completed' } @@ -420,8 +404,6 @@ describe('formatWorkerRead', () => { expect(output).toContain(`[subagents] ${subagentGroupFallbackText(ROSTER)}`) }) - // The same claim, with the twin written after both blocks: nothing about the - // ORDER of a twin and its group is guaranteed by the block schema. it('claims a trailing twin for the group it names', () => { const other: readonly NativeChatSubagentEntry[] = [ { id: 'child-3', label: 'plan', state: 'completed' } @@ -442,9 +424,6 @@ describe('formatWorkerRead', () => { expect(output).toContain(`[subagents] ${subagentGroupFallbackText(ROSTER)}`) }) - // A group with no twin beside it is a shape the block schema admits and no - // producer writes. Dropping it would lose the roster entirely, so the block - // itself carries the sentence when nothing else does. it('stands in for a roster block that arrived without its twin', () => { const output = formatWorkerRead( transcriptRead([{ type: 'subagent-group', groupId: 'thread:turn-1', agents: [...ROSTER] }]) @@ -453,11 +432,7 @@ describe('formatWorkerRead', () => { expect(output).toContain(`[assistant] [subagents] ${subagentGroupFallbackText(ROSTER)}`) }) - // A roster from a newer build holds a state this build does not know, which - // `summarizeSubagentGroup` reads as `unverifiable`. Recomputing the sentence - // to compare it against the frozen twin therefore produced a DIFFERENT string, - // and the CLI printed the roster twice: the twin's own wording plus a - // `[subagents]` line contradicting it. + // Unknown roster states can change recomputed text; preserve the producer's twin without duplicating it. it('prints the roster once when the twin names a state this build cannot reproduce', () => { const frozenTwin = 'Ran 2 subagents (1 cancelled)' const output = formatWorkerRead( @@ -482,10 +457,7 @@ describe('formatWorkerRead', () => { expect(output).not.toContain('unverifiable') }) - // The journal admits block types this build does not know, and `client.call` - // casts the RPC result rather than validating it — so a newer remote host's - // block reaches this formatter as-is. Reading fields off it threw a TypeError - // and took down the whole `worker read`. + // RPC results are not schema-validated here, so newer hosts' unknown block types reach the formatter. it('degrades an unknown block type from a newer host instead of throwing', () => { const output = formatWorkerRead( transcriptRead([ diff --git a/src/cli/handlers/orchestration/worker-output.ts b/src/cli/handlers/orchestration/worker-output.ts index 9d6d1949911..7de970fabe6 100644 --- a/src/cli/handlers/orchestration/worker-output.ts +++ b/src/cli/handlers/orchestration/worker-output.ts @@ -22,8 +22,7 @@ export type WorkerStartReceipt = { export function formatWorkerStart(value: WorkerStartReceipt): string { const lines = [`Worker ${value.dispatchId} [${value.state}] for ${value.taskId}`] - // Settings-driven rather than requested, so the human line always names the mode that ran: a - // fallback from the user's structured default is never silent. + // Show the mode that actually ran so a fallback from the user's default is visible. if (value.mode) { lines.push(value.mode.detail) } @@ -74,12 +73,11 @@ function formatWorkerReadDetails(value: OrchestrationWorkerReadResult): string { ? `Source: transcript (provider=${value.provider})` : 'Source: terminal' const lines = [source] - // A released archive read otherwise prints identically to a live one. if (value.status?.worker) { lines.push(`Worker: ${value.status.worker}`) } lines.push(`Archived: ${value.archived === true}`) - // Two different verdicts: status.liveness is the PTY's, the fleet projection is the agent's. + // PTY liveness and agent liveness are separate verdicts. if (value.status?.liveness) { lines.push(`Terminal liveness: ${value.status.liveness}`) } diff --git a/src/cli/handlers/orchestration/worker-terminal-handlers.ts b/src/cli/handlers/orchestration/worker-terminal-handlers.ts index 7abda2464f8..12a471db48c 100644 --- a/src/cli/handlers/orchestration/worker-terminal-handlers.ts +++ b/src/cli/handlers/orchestration/worker-terminal-handlers.ts @@ -67,7 +67,7 @@ export const ORCHESTRATION_WORKER_TERMINAL_HANDLERS: Record now routes these -// commands to that server, a client can reach an older host without meaning to. The desktop -// already names this case; match it instead of surfacing the raw dispatcher error. async function callProjectHostSetup( client: HandlerContext['client'], method: string, params?: unknown ): Promise> { try { - // Why: forward the exact arity the caller used; passing an explicit undefined would change - // the request shape for the no-params methods. + // Preserve absent params; explicitly passing undefined changes the request shape. return params === undefined ? await client.call(method) : await client.call(method, params) @@ -71,9 +66,7 @@ async function getResolvedHostId( return host.id } -// Why: the setup paths keep the unresolved id on purpose. The runtime rejects every `ssh:` host -// for those operations regardless of whether it exists, so resolving first would answer "no such -// target" and imply the command would have worked with the right id. +// The runtime rejects SSH setup operations regardless of target existence; preserve that refusal. async function getRequiredHostId( flags: Map, client: HandlerContext['client'] @@ -85,8 +78,7 @@ async function getRequiredHostId( if (host.kind !== 'alias') { return host.id } - // A bare alias has no unresolved spelling to forward — naming the machine is the lookup — so - // this one case resolves even on the paths that otherwise pass the id through untouched. + // Bare aliases must resolve to an id before they can be forwarded. const resolved = await resolveHostFlagTarget(flags, client) if (!resolved) { throw new RuntimeClientError('invalid_argument', 'Missing required --host') @@ -94,8 +86,7 @@ async function getRequiredHostId( return resolved.id } -// Why: a project on two hosts has two setups with the same id; `--host` picks one. An older -// host strips the field and acts on the first twin, so refuse rather than let it guess. +// Older hosts drop hostId and pick the first setup twin; refuse unsupported host targeting. async function getOptionalSetupHostId( flags: Map, client: HandlerContext['client'] @@ -149,8 +140,7 @@ export const PROJECT_HANDLERS: Record = { 'project setup-existing-folder': async ({ flags, client, cwd, json }) => { const rawPath = getRequiredStringFlag(flags, 'path') const hostId = await getRequiredHostId(flags, client) - // An SSH host's filesystem is not the CLI's, so resolving a relative path against the client - // cwd would register a path that names the wrong machine. + // SSH paths belong to the target filesystem, even when the runtime connection itself is local. const pathIsOffClient = client.isRemote || getSshTargetIdForExecutionHost(hostId) !== null const args: ProjectHostSetupExistingFolderArgs = { hostId, @@ -191,11 +181,7 @@ export const PROJECT_HANDLERS: Record = { 'project setup-create': async ({ flags, client, cwd, json }) => { const path = getOptionalStringFlag(flags, 'path') const args: ProjectHostSetupCreateArgs = { - // Why: unlike the setup paths below, the runtime does not reject `ssh:` here — this records - // independent metadata — so an unknown target would persist a row pointing at a machine - // that does not exist. Resolving catches that. `local` and `runtime:` pass through - // untouched, because this is also the provisioning path and a runtime host legitimately - // may not exist yet when its metadata is written. + // Resolve SSH targets to reject nonexistent hosts; runtime metadata may precede provisioning. hostId: await getResolvedHostId(flags, client), setupId: getOptionalStringFlag(flags, 'setup-id'), path: diff --git a/src/cli/handlers/reference.ts b/src/cli/handlers/reference.ts index a363f3d8ea9..23c395129e0 100644 --- a/src/cli/handlers/reference.ts +++ b/src/cli/handlers/reference.ts @@ -24,8 +24,7 @@ import { } from '../reference-input' import { RuntimeClientError, type RuntimeRpcSuccess } from '../runtime-client' -// Why: hosts from before reference.* still advertise the linked-items delta capability, so the -// raw method_not_found would read as an Orca bug rather than a version gap. +// Older hosts may advertise linked-item deltas without supporting reference.*. async function callReference( client: HandlerContext['client'], method: 'reference.list' | 'reference.find', @@ -116,7 +115,7 @@ async function mutateReferences(context: HandlerContext, operation: 'add' | 'rem : `id:${workspace.id}` const stored = before.result.references.map(({ key, selected: _selected, ...item }) => ({ item, - // Why: --key values come from the host's `reference list`; the CLI may be a different build. + // Reference keys come from the host, which may run a different build. keys: new Set([key, getWorkspaceReferenceIdentity(item)]) })) const base = stored.map(({ item }) => item) @@ -132,9 +131,7 @@ async function mutateReferences(context: HandlerContext, operation: 'add' | 'rem } }) if (changes.some((change) => change.changed)) { - // Why: storage may hold same-URL twins with different source contexts; derive from `base` - // so the host's delta merge never sees an unrequested twin as removed. - // Input is already deduped by identity. + // Preserve same-URL twins with distinct source contexts when preparing the host's delta merge. const linkedItems = operation === 'add' ? [...base, ...input.filter((item) => !isLinked(getWorkspaceReferenceIdentity(item)))] diff --git a/src/cli/handlers/search.test.ts b/src/cli/handlers/search.test.ts index 2db6b9785fc..cdb60acf91e 100644 --- a/src/cli/handlers/search.test.ts +++ b/src/cli/handlers/search.test.ts @@ -90,7 +90,6 @@ async function runSearch( type CliFlags = [string, string | boolean][] -/** The printed envelope, narrowed by shape rather than asserted. */ function printedEnvelope(output: string): { keys: string[]; result: unknown } { const parsed: unknown = JSON.parse(output) if (typeof parsed !== 'object' || parsed === null || !('result' in parsed)) { @@ -99,7 +98,6 @@ function printedEnvelope(output: string): { keys: string[]; result: unknown } { return { keys: Object.keys(parsed), result: parsed.result } } -/** Re-reads the printed result through the contract, so the shape is checked, not claimed. */ function printedResults(output: string): Extract { const parsed = AiVaultSearchResponseSchema.parse(printedEnvelope(output).result) if (parsed.kind !== 'results') { diff --git a/src/cli/handlers/search.ts b/src/cli/handlers/search.ts index c70be386cdf..d28f4cec55d 100644 --- a/src/cli/handlers/search.ts +++ b/src/cli/handlers/search.ts @@ -13,12 +13,7 @@ import { parseSearchCommand } from '../search-command-arguments' export const MALFORMED_CURSOR_MESSAGE = 'The host did not recognise that --cursor value. Cursors belong to one query on one host; re-run the search without --cursor.' -/** - * The shared contract client over the CLI's runtime RPC. Reusing it is what makes - * an old host's unknown-method refusal an `unavailable/no-service` answer instead - * of a raw JSON-RPC error, and it applies the same exposure policy the host did: - * a paired runtime is a relay caller, a local one is not. - */ +// Reuse contract compatibility and exposure policy: paired runtimes are relay callers. function createCliSessionSearch(client: RuntimeClient) { let lastEnvelope: RuntimeRpcSuccess | undefined const search = createSessionSearchClient( @@ -28,8 +23,7 @@ function createCliSessionSearch(client: RuntimeClient) { }, client.isRemote ? 'relay' : 'runtime' ) - // Why `client`: an answer synthesised from a refusal had no successful call, so - // no runtime produced it and none of its identifiers may be claimed here. + // A synthesized refusal has no runtime envelope; do not claim an unobserved runtime identity. const envelope = (result: TResult): RuntimeRpcSuccess => lastEnvelope ? { ...lastEnvelope, result } @@ -37,7 +31,6 @@ function createCliSessionSearch(client: RuntimeClient) { return { search, envelope } } -/** `orca search` over `aiVault.searchSessions` / `aiVault.searchStatus` on one host. */ export const SEARCH_HANDLERS: Record = { search: async ({ client, flags, json }) => { const command = parseSearchCommand(flags) diff --git a/src/cli/handlers/serve-service.test.ts b/src/cli/handlers/serve-service.test.ts index 5a8784ff6f7..ab2a84a3dac 100644 --- a/src/cli/handlers/serve-service.test.ts +++ b/src/cli/handlers/serve-service.test.ts @@ -152,7 +152,7 @@ describe('orca serve service', () => { ).toThrow(/PATH/) }) it('names the command to run when only a server-only orca exists here', async () => { - // The `orca` an SSH-launched Orca server puts on a remote host runs on plain Node. + // SSH server launchers run on plain Node, without an installed Electron desktop. const serverCli = (platform: NodeJS.Platform, path: string): ServeServiceDeps => { const base = deps(platform) const { ELECTRON_RUN_AS_NODE: _electron, ...env } = base.env @@ -169,7 +169,6 @@ describe('orca serve service', () => { installServeService({ command: null, dryRun: true }, serverCli('darwin', scoped)) ).rejects.toThrow(/`orca serve service install`/) expect(calls).toEqual([]) - // An installed app's launcher on PATH (or named by --command) still works from that terminal. await expect( installServeService({ command: null, dryRun: true }, serverCli('linux', `${scoped}:${bin}`)) ).resolves.toMatchObject({ dryRun: true }) diff --git a/src/cli/handlers/serve-service.ts b/src/cli/handlers/serve-service.ts index 74f26d0c9e9..fdcf7f9d500 100644 --- a/src/cli/handlers/serve-service.ts +++ b/src/cli/handlers/serve-service.ts @@ -116,7 +116,7 @@ export async function installServeService( if (spec.logPath) { mkdirSync(dirname(spec.logPath), { recursive: true, mode: 0o700 }) } - // Why: installing is the documented way to retry after the crash-loop brake gave up. + // Reinstalling retries a supervisor stopped by the crash-loop brake. clearServeSupervisorBrake(resolveLaunchUserDataPath(deps.env)) if (spec.manager === 'launchd') { await deps.run('launchctl', ['bootout', `gui/${deps.uid}/${spec.name}`]) @@ -170,7 +170,7 @@ export async function readServeServiceStatus( if (result.code === null) { return { ...base, verdict: 'unverifiable', pid: null, detail: 'launchctl did not answer' } } - // Why only 113: other failures (no GUI session over SSH, a refused request) prove nothing. + // Other launchctl failures cannot establish an exit, e.g. a missing GUI session over SSH. if (result.code === 113) { return { ...base, verdict: 'exited', pid: null, detail: 'not loaded' } } @@ -228,7 +228,6 @@ export async function uninstallServeService( return { removed: existed, manager: spec.manager, name: spec.name, unitPath: spec.unitPath } } -/** Local, runtime-free output: the same envelope shape `--json` callers get elsewhere. */ function emit(result: T, json: boolean, format: (value: T) => string): void { console.log(json ? JSON.stringify({ ok: true, result }, null, 2) : format(result)) } @@ -241,7 +240,7 @@ export const SERVE_SERVICE_HANDLERS: Record = { 'serve service install': async ({ flags, json }) => { const result = await installServeService({ command: optionalString(flags.get('command')), - // Why validate here: a bad value fails before the supervisor's brake and launchd would loop. + // Validate before launchd can loop on a bad value and trip the supervisor brake. port: getOptionalServePort(flags), pairingAddress: optionalString(flags.get('pairing-address')), dryRun: flags.get('dry-run') === true diff --git a/src/cli/handlers/skill-guide-get.ts b/src/cli/handlers/skill-guide-get.ts index 8854f6072be..ab2006f6039 100644 --- a/src/cli/handlers/skill-guide-get.ts +++ b/src/cli/handlers/skill-guide-get.ts @@ -10,8 +10,7 @@ import { type GuideSelection = { full: boolean; reference: string | null; listReferences: boolean } -// Why: the kernel's gate table names each document as `references/.md`, so that -// exact string must resolve as well as the bare name an agent is likely to retype. +// Accept references/.md exactly as published in the kernel's gate table. function normalizeReferenceSelector(value: string): string { return value .trim() diff --git a/src/cli/handlers/skills.ts b/src/cli/handlers/skills.ts index e1ee86a8f24..6ee52f43c1e 100644 --- a/src/cli/handlers/skills.ts +++ b/src/cli/handlers/skills.ts @@ -6,8 +6,7 @@ import { createSkillMutationHandler } from './bundled-skill-install-command' export const SKILL_HANDLERS: Record = { 'skills list': async ({ json }) => { - // Why: generated registry order is not a user-facing contract, while stable - // canonical sorting keeps agent-visible output reproducible across builds. + // Canonical sorting keeps output stable across generated-registry changes. const topics = (await loadCanonicalGuides()).map((guide) => ({ name: guide.name, description: guide.description.replace(/\s+/g, ' ').trim() diff --git a/src/cli/handlers/speech.test.ts b/src/cli/handlers/speech.test.ts index fee33ee91ca..5f56d158866 100644 --- a/src/cli/handlers/speech.test.ts +++ b/src/cli/handlers/speech.test.ts @@ -17,7 +17,6 @@ describe('orca speech openai-key set', () => { await expect(readOpenAiKeyFromStdin(piped('sk-test-', 'abc\n'))).resolves.toBe('sk-test-abc') }) - // Why: a key typed as an argument would sit in shell history and process listings. it('refuses an interactive terminal instead of waiting on it', async () => { const terminal = Object.assign(chunks(), { isTTY: true }) await expect(readOpenAiKeyFromStdin(terminal)).rejects.toThrow('Pipe the key on stdin') diff --git a/src/cli/handlers/speech.ts b/src/cli/handlers/speech.ts index 89d698aeae0..c7ca57652b7 100644 --- a/src/cli/handlers/speech.ts +++ b/src/cli/handlers/speech.ts @@ -14,7 +14,7 @@ function formatKeyStatus(status: OpenAiSpeechKeyStatus): string { : 'OpenAI key saved on this host.' } -// Why stdin and never a flag: argv shows up in shell history and process listings. +// Keep secrets out of argv, shell history, and process listings. export async function readOpenAiKeyFromStdin( stdin: AsyncIterable & { isTTY?: boolean } = process.stdin ): Promise { @@ -44,7 +44,6 @@ export async function readOpenAiKeyFromStdin( return key } -// Why: `--host runtime:` can reach an older server, whose raw `method_not_found` reads as a bug. export async function callOpenAiKeyMethod( client: Pick, method: string, diff --git a/src/cli/handlers/terminal-close.ts b/src/cli/handlers/terminal-close.ts index 9e65b2f5f47..2b2e5e7895a 100644 --- a/src/cli/handlers/terminal-close.ts +++ b/src/cli/handlers/terminal-close.ts @@ -21,7 +21,6 @@ function terminalCloseFailure(close: RuntimeTerminalClose): RuntimeClientError | { close } ) } - // Why: the close itself committed; only the process stop is unproven, so say which. return new RuntimeClientError( 'terminal_stop_unverifiable', `Closed terminal ${close.handle}. ${describeUnconfirmedCloseStop(close)}`, @@ -91,12 +90,10 @@ export const terminalCloseHandler: CommandHandler = async ({ flags, client, cwd, const result = await client.call<{ close: RuntimeTerminalClose }>(method, { terminal: await getTerminalHandle(flags, cwd, client) }) - // Why: a transport-level success must not hide a live or unverifiable PTY. Keep the receipt in - // error.data so JSON callers retain the host's exact evidence while receiving a failing outcome. + // A successful RPC can leave a live or unverified PTY; retain the host receipt in error.data. const failure = terminalCloseFailure(result.result.close) if (failure) { - // Keep the established human receipt (including its liveness warning); JSON needs the - // standard failure envelope so callers do not mistake transport success for a stopped PTY. + // Keep human liveness warnings; JSON needs a failure envelope to distinguish an unconfirmed stop. if (json) { reportCliError(failure, true) } else { diff --git a/src/cli/handlers/terminal-daemon.ts b/src/cli/handlers/terminal-daemon.ts index e01f0a1d053..12cfa55aacb 100644 --- a/src/cli/handlers/terminal-daemon.ts +++ b/src/cli/handlers/terminal-daemon.ts @@ -57,7 +57,7 @@ function formatDaemonLiveness(daemon: DaemonLiveness | undefined): string[] { export function formatSeveredDaemonStatus(result: SeveredStatusResult): string { const severed = result.severed const liveness = formatDaemonLiveness(result.daemon) - // Why: a severed daemon that has since died hosts nothing; the liveness line says so. + // An exited daemon hosts nothing; do not print stale severed-state details. if (!severed || result.daemon?.verdict === 'exited') { return liveness.join('\n') } diff --git a/src/cli/handlers/terminal-send.ts b/src/cli/handlers/terminal-send.ts index 2a92bfa5ea6..ee8a9ce51b1 100644 --- a/src/cli/handlers/terminal-send.ts +++ b/src/cli/handlers/terminal-send.ts @@ -100,7 +100,6 @@ export const terminalSendHandler: CommandHandler = async ({ flags, client, cwd, baselineWorkingSequence: 0 } } - // Why: the delivery warnings only existed in the text formatter, so --json callers never saw them. const warnings = terminalSendWarnings(result.result.send) printResult( warnings.length > 0 ? { ...result, result: { ...result.result, warnings } } : result, diff --git a/src/cli/handlers/terminal.test.ts b/src/cli/handlers/terminal.test.ts index a2db47bad9b..0dc140c2b44 100644 --- a/src/cli/handlers/terminal.test.ts +++ b/src/cli/handlers/terminal.test.ts @@ -785,8 +785,7 @@ describe('terminal create --shell', () => { expect(call).not.toHaveBeenCalled() }) - // An older host strips the unknown param and answers with its default shell, which reads as a - // successful create. Creating the wrong shell silently is worse than refusing. + // Older hosts ignore shell and report a successful create using their default shell. it('refuses rather than creating a default-shell terminal on a host without the capability', async () => { const call = vi.fn() @@ -796,8 +795,7 @@ describe('terminal create --shell', () => { expect(call).not.toHaveBeenCalled() }) - // A status probe that fails or times out reports no capabilities either; blaming the host - // version would send the caller to update a host that may already be current. + // Missing capabilities on an unreachable runtime do not establish a version gap. it('reports an unreachable host as unavailable rather than incompatible', async () => { const call = vi.fn() diff --git a/src/cli/handlers/terminal.ts b/src/cli/handlers/terminal.ts index de8e6cc8cfe..1e0e7544149 100644 --- a/src/cli/handlers/terminal.ts +++ b/src/cli/handlers/terminal.ts @@ -52,9 +52,7 @@ import { revealPairedTerminalOnLocalDesktop } from '../paired-terminal-desktop-r import { terminalCloseHandler } from './terminal-close' import { terminalSendHandler } from './terminal-send' -// Why: terminal wait legitimately needs to outlive the CLI's default RPC -// timeout. Even without an explicit server timeout, the client must allow -// long waits instead of failing at the generic 15s transport cap. +// Long terminal waits must outlive the ordinary transport timeout. const DEFAULT_TERMINAL_WAIT_RPC_TIMEOUT_MS = 5 * 60 * 1000 const terminalFocusHandler: CommandHandler = async ({ flags, client, cwd, json }) => { @@ -76,7 +74,7 @@ export const TERMINAL_HANDLERS: Record = { { worktree: await getOptionalWorktreeSelector(flags, 'worktree', cwd, client), limit: getOptionalPositiveIntegerFlag(flags, 'limit'), - // Why: agent JSON calls dominate; topology stays available through an explicit opt-in. + // Omit topology from JSON unless requested to keep agent responses small. includeVisualLayouts: !json || flags.has('include-visual-layouts') } ) @@ -99,8 +97,7 @@ export const TERMINAL_HANDLERS: Record = { throw new RuntimeClientError('invalid_argument', '--cursor must be a non-negative integer') } const screen = flags.get('screen') === true - // Why: a cursor pages through accumulated output. A screen read is the current frame and has - // nothing behind it to page, so accepting both would imply history that is not there. + // A screen is one current frame; a cursor would falsely imply pageable history. if (screen && cursorFlag !== undefined) { throw new RuntimeClientError( 'invalid_argument', @@ -113,9 +110,7 @@ export const TERMINAL_HANDLERS: Record = { ...(screen ? { screen: true } : {}), limit: getOptionalPositiveIntegerFlag(flags, 'limit') }) - // Why: an older host drops the unknown `screen` param and answers with its ordinary stream - // read, which carries no source. Returning that silently is the exact failure this flag - // exists to prevent, so refuse rather than hand back the other question's answer. + // Older hosts ignore screen and return a stream without source; never present that as a screen. if (screen && result.result.terminal.source === undefined) { throw new RuntimeClientError( 'incompatible_runtime', @@ -140,8 +135,7 @@ export const TERMINAL_HANDLERS: Record = { ) printResult(result, json, formatTerminalWait) if (result.result.wait.satisfied === false) { - // Why: callers commonly chain `terminal wait && terminal send`; a - // structured blocked result is still an unsatisfied wait condition. + // Chained commands such as wait && send need a failing exit for an unsatisfied condition. process.exitCode = 1 } }, @@ -185,9 +179,7 @@ export const TERMINAL_HANDLERS: Record = { `--shell must be one of: ${listSupportedWindowsShellOverrides().join(', ')}` ) } - // Why refused rather than sent hopefully: an older host strips the unknown param and hands - // back a healthy terminal running its DEFAULT shell. Nothing in that reply says the shell - // was ignored, so a caller that wanted cmd would drive a PowerShell session believing it won. + // Older hosts ignore shell and successfully create the wrong default shell; check support first. const status = await client.getCliStatus() // An unreachable host reports no capabilities at all; that is not evidence it lacks --shell. if (!status.result.runtime.reachable) { @@ -214,14 +206,11 @@ export const TERMINAL_HANDLERS: Record = { title: getOptionalStringFlag(flags, 'title'), focus, ...(focus ? { presentation: 'focused' } : {}), - // Compat: hosts before v1.4.211 (#22384) leave a background-started agent's startup queries - // unanswered, so they need this to start it in the window; newer hosts ignore it. Delete - // once the minimum supported host version is v1.4.211 or later. + // Compat: hosts before v1.4.211 (#22384) need renderer startup replies; remove once unsupported. ...(useRendererBackedInteractiveTerminal ? { rendererBacked: true, activate: focus } : {}) }) printResult(result, json, formatTerminalCreate) }, - // `focus` resolves to this canonical path via CommandSpec.aliases before dispatch. 'terminal switch': terminalFocusHandler, 'terminal close': terminalCloseHandler, 'terminal split': async ({ flags, client, cwd, json }) => { diff --git a/src/cli/handlers/vm.ts b/src/cli/handlers/vm.ts index 7afb01fe2b5..7419fcbce95 100644 --- a/src/cli/handlers/vm.ts +++ b/src/cli/handlers/vm.ts @@ -12,8 +12,7 @@ import { getEphemeralVmRecipeResultWarnings, redactEphemeralVmRecipeDiagnosticText } from '../../shared/ephemeral-vm-recipe-diagnostics' -// Why: import directly from the doctor module (not the barrel) — it uses Node -// fs/path and must stay out of the browser bundle that imports the barrel. +// This module uses Node fs/path; exporting it through the shared barrel would break browser bundles. import { doctorEphemeralVmRecipe } from '../../shared/ephemeral-vm-recipe-doctor' import { runEphemeralVmRecipeCleanup, @@ -85,9 +84,6 @@ function readTextFile(path: string): string { return readFileSync(path, 'utf8') } -// Why: give the agent the full create/destroy output so it can self-diagnose a -// failed provision instead of relaying logs through the user. Each stream is -// redacted and capped (head+tail) so a huge log stays readable but complete enough. type ProvisionStageTranscript = { exitCode: number | null signal: NodeJS.Signals | null @@ -107,8 +103,7 @@ type DoctorResult = EphemeralVmRecipeDoctorResult & { const MAX_TRANSCRIPT_STREAM_CHARS = 16_000 -// Why: keep both ends of a long log — the script's setup context (head) and the -// failure itself (tail) — rather than only the last 500 chars. +// Keep setup context and final diagnostics while bounding and redacting each stream. function capTranscriptStream(value: string): string { const redacted = redactEphemeralVmRecipeDiagnosticText(value) if (redacted.length <= MAX_TRANSCRIPT_STREAM_CHARS) { @@ -158,8 +153,6 @@ async function doctorRecipeWithProvision( remediation: buildProvisionFailureRemediation(start.stderr, start.stdout) } ], - // Why: the create script failed — hand the agent the complete output (not a - // tail) so it can see what the script printed and why parsing/exit failed. provisionTranscript: { provision: { exitCode: start.exitCode, @@ -222,8 +215,7 @@ async function doctorRecipeWithProvision( }) } - // Why: include both stages' full output even on success — the agent can confirm - // pairing/teardown looked right, and diagnose a destroy failure without re-running. + // Retain both stages for diagnosis even when creation succeeds and teardown fails. const provisionTranscript: ProvisionTranscript = { provision: { exitCode: 0, diff --git a/src/cli/handlers/worktree-create-agent-launch.ts b/src/cli/handlers/worktree-create-agent-launch.ts index 88787199ce6..1a5d630cc36 100644 --- a/src/cli/handlers/worktree-create-agent-launch.ts +++ b/src/cli/handlers/worktree-create-agent-launch.ts @@ -1,8 +1,4 @@ -/** - * `worktree create --agent`: the worktree and its agent in one `agent.launch`, so the agent follows - * `--surface`, else the user's Chat UI setting, like every other launch. A host older than - * `agent.launch` gets the create's own startup terminal, which cannot take a model, effort or surface. - */ +// Use agent.launch for surface/model/effort; older hosts fall back to the create startup terminal. import type { AgentLaunchResult } from '../../shared/agent-launch-intent' import { diff --git a/src/cli/handlers/worktree-create-parent-selector.ts b/src/cli/handlers/worktree-create-parent-selector.ts index 6c66a53509f..4b75d649683 100644 --- a/src/cli/handlers/worktree-create-parent-selector.ts +++ b/src/cli/handlers/worktree-create-parent-selector.ts @@ -43,8 +43,7 @@ export async function resolveCreateParentSelector( const parentWorkspace = getWorkspaceKeyParentSelector(rawParentWorktree) if (parentWorkspace) { - // Why: create exposes one public parent flag, while the runtime still needs - // workspace keys to preserve folder/worktree lineage accurately. + // The public parent flag can name either kind; runtime workspace keys preserve folder lineage. return { parentWorkspace } } @@ -53,7 +52,7 @@ export async function resolveCreateParentSelector( ? getWorkspaceKeyParentSelector(parentWorktree) : undefined if (resolvedParentWorkspace) { - // Why: active/current may resolve to a folder workspace pseudo-worktree id. + // active/current can resolve to a folder workspace pseudo-worktree id. return { parentWorkspace: resolvedParentWorkspace } } diff --git a/src/cli/handlers/worktree-gitlab-link.test.ts b/src/cli/handlers/worktree-gitlab-link.test.ts index 6b718ebab89..5b0f620ee38 100644 --- a/src/cli/handlers/worktree-gitlab-link.test.ts +++ b/src/cli/handlers/worktree-gitlab-link.test.ts @@ -44,8 +44,6 @@ describe('getOptionalGitLabLinkFlag', () => { ).toBe(77) }) - // Issues and merge requests are separate namespaces on GitLab, so a reference - // to one must never be taken as the other's number. it('refuses a merge request reference in the issue flag', () => { expect(() => getOptionalGitLabLinkFlag(flags({ 'gitlab-issue': '!42' }), 'issue')).toThrow( /GitLab issue number/ @@ -92,8 +90,7 @@ describe('getOptionalGitLabLinkFlag', () => { } ) - // A URL without GitLab's `/-/` separator is not a GitLab link, and a project - // path needs a group segment — neither may fall through to a number. + // GitLab URLs need /-/ and a grouped project path; malformed URLs must not fall back to numbers. it.each(['https://gitlab.com/group/project/issues/42', 'https://gitlab.com/project/-/issues/42'])( 'refuses the non-GitLab URL shape %s', (input) => { @@ -101,8 +98,7 @@ describe('getOptionalGitLabLinkFlag', () => { } ) - // `/^\d+$/` accepts 400 digits, which parseInt turns into Infinity — persisting - // that writes `null` over the link it meant to set. + // parseInt turns huge digit strings into Infinity, which JSON would persist as null. it('refuses a number too large to be an integer', () => { expect(() => getOptionalGitLabLinkFlag(flags({ 'gitlab-issue': '9'.repeat(400) }), 'issue') diff --git a/src/cli/handlers/worktree-linear-issue-link.ts b/src/cli/handlers/worktree-linear-issue-link.ts index f9f10e3359e..a866c16ef8c 100644 --- a/src/cli/handlers/worktree-linear-issue-link.ts +++ b/src/cli/handlers/worktree-linear-issue-link.ts @@ -23,10 +23,7 @@ export function getOptionalLinearIssueLinkFlag( return { ...LINEAR_ISSUE_LINK_CLEARED } } - // Why: the shared builder treats empty input as "clear", which is right for a - // text field the user can blank but wrong for a flag — `--linear-issue " "` - // is a mistyped argument, not a request to unlink. Only the literal `null` - // clears, and that is handled above. + // Only literal null unlinks; blank flag values must not inherit the text-field builder's clear behavior. const updates = value.trim() === '' ? null : buildLinearIssueLinkUpdates(value) if (!updates) { throw new RuntimeClientError( diff --git a/src/cli/handlers/worktree-removal-warnings.ts b/src/cli/handlers/worktree-removal-warnings.ts index 08079c51a0d..6c8cd929450 100644 --- a/src/cli/handlers/worktree-removal-warnings.ts +++ b/src/cli/handlers/worktree-removal-warnings.ts @@ -21,8 +21,7 @@ export function printHookWarning(result: HookWarningResult, json: boolean): void if (result.warning) { console.error(`warning: ${result.warning}`) } - // Why (#19334): a waived archive-hook failure is the one case where Orca deleted a checkout - // whose archive step did not succeed. It has to stay visible in human output. + // A waived archive failure must remain visible after the checkout is deleted. if (result.archiveHookOverride) { console.error(`warning: ${formatArchiveHookOverride(result.archiveHookOverride)}`) } diff --git a/src/cli/handlers/worktree-review-link-flags.ts b/src/cli/handlers/worktree-review-link-flags.ts index 80a9f8df386..928e5b3ed3a 100644 --- a/src/cli/handlers/worktree-review-link-flags.ts +++ b/src/cli/handlers/worktree-review-link-flags.ts @@ -10,7 +10,7 @@ type ReviewTargetLinks = { linkedGitLabMR?: number | null } -// Why: only `set` may clear a link, so `create` parses the same flags non-nullable. +// Only set may clear a link; create uses non-nullable flags. export function getReviewTargetLinkFlags( flags: Map, options: { nullable?: boolean } = {} diff --git a/src/cli/handlers/worktree-setup-decision-recovery.ts b/src/cli/handlers/worktree-setup-decision-recovery.ts index dbf292fc57b..e03ce1df770 100644 --- a/src/cli/handlers/worktree-setup-decision-recovery.ts +++ b/src/cli/handlers/worktree-setup-decision-recovery.ts @@ -1,13 +1,11 @@ import { RuntimeClientError, RuntimeRpcFailureError } from '../runtime-client' -// Why match the text: every host version refuses an undecided `ask` create with exactly this -// message and the generic runtime_error code, so it is the only stable signal. +// Hosts use generic runtime_error here; this message is the stable discriminator. const SETUP_DECISION_REQUIRED = 'Setup decision required for this repository' const SETUP_DECISION_NEXT_STEP = 'Pass --setup run to run the setup script, or --setup skip to create without it.' -/** Why here, not on the host: desktop and phone choose setup in their own UI, so the host's - * shared refusal names no flag; only the CLI user answers it with --setup. */ +/** Only the CLI answers setup prompts with --setup; desktop and mobile have their own UI. */ export async function withSetupDecisionRecovery( create: Promise, replayingLaunch = false diff --git a/src/cli/handlers/worktree-unread-flag.ts b/src/cli/handlers/worktree-unread-flag.ts index e77967fcfc9..6f28cd8a0ec 100644 --- a/src/cli/handlers/worktree-unread-flag.ts +++ b/src/cli/handlers/worktree-unread-flag.ts @@ -1,12 +1,11 @@ import { RuntimeClientError } from '../runtime-client' -/** `--unread` / `--read` as `worktree.set`'s `isUnread`; absent leaves it unchanged. */ export function getOptionalWorktreeUnreadFlag( flags: Map ): boolean | undefined { const unread = flags.get('unread') const read = flags.get('read') - // Why: `--unread=x` and `--read x` parse as values and would otherwise be silently dropped. + // Reject valued booleans instead of silently ignoring --unread=x or --read x. for (const [name, value] of [ ['unread', unread], ['read', read] diff --git a/src/cli/handlers/worktree.ts b/src/cli/handlers/worktree.ts index f47b0d5e8f2..0343ba2467a 100644 --- a/src/cli/handlers/worktree.ts +++ b/src/cli/handlers/worktree.ts @@ -167,9 +167,7 @@ export const WORKTREE_HANDLERS: Record = { needsCwdRepoInference ) { try { - // Why: agent shells can lose ORCA_TERMINAL_HANDLE while still running - // inside an Orca worktree. Cwd keeps CLI-created children nestable and - // lets create infer the repo for the common current-workspace case. + // Cwd preserves parent/repo inference when agent shells lose ORCA_TERMINAL_HANDLE. cwdParentWorktree = await resolveCurrentWorktreeSelection(cwd, client) } catch { cwdParentWorktree = undefined @@ -208,8 +206,7 @@ export const WORKTREE_HANDLERS: Record = { ...(cwdParentWorktree ? { cwdParentWorktree: cwdParentWorktree.selector } : {}), noParent, callerTerminalHandle, - // Why: marks the workspace as CLI-created so the sidebar can badge and - // filter it. Sent on every `worktree create` — hand-typed or agent-run. + // Always request CLI provenance, including hand-typed creates without a terminal handle. cliProvenanceRequest: callerTerminalHandle ? { callerTerminalHandle } : {} } if (agentLaunch) { @@ -265,8 +262,6 @@ export const WORKTREE_HANDLERS: Record = { 'Orca cannot tell which host owns this workspace. Refresh projects and try again.' ) } - // Why (#19334): the waiver only ever applies to a hook that ran, so without --run-hooks it - // silently does nothing. Rejecting it beats letting someone believe they waived something. if (flags.get('allow-failed-archive-hook') === true && flags.get('run-hooks') !== true) { throw new RuntimeClientError( 'invalid_argument', @@ -277,11 +272,10 @@ export const WORKTREE_HANDLERS: Record = { worktree, hostId, force: flags.get('force') === true, - // Why (#11960): --force is explicit here, so it may also waive PTY-stop proof. + // --force also waives PTY-stop proof. allowUnverifiedPtyStop: flags.get('force') === true, runHooks: flags.get('run-hooks') === true, - // Why (#19334): deliberately NOT coupled to --force, which above already waives PTY-stop - // proof. Waiving a failed archive hook is a separate decision about the user's data. + // Archive failure waivers protect user data separately from --force's PTY-stop waiver. allowFailedArchiveHook: flags.get('allow-failed-archive-hook') === true }) printHookWarning(result.result, json)