diff --git a/src/cli/agent-context.ts b/src/cli/agent-context.ts index f307040b3a4..224eec63b5a 100644 --- a/src/cli/agent-context.ts +++ b/src/cli/agent-context.ts @@ -1,9 +1,6 @@ import type { CommandSpec } from './args' import { effectiveAllowedFlags } from './args' -// Why: serialize the live spec table so agent discovery cannot drift from the -// command surface it describes. - const SCHEMA_VERSION = 1 export type AgentContextCommand = { @@ -29,7 +26,7 @@ export type AgentContextSchema = { export function buildAgentContext(specs: CommandSpec[]): AgentContextSchema { const commands = specs - // Why: hidden specs dispatch but stay off every discovery surface, including this one. + // Hidden commands dispatch but stay off discovery surfaces. .filter((spec) => spec.hidden !== true) .map((spec) => ({ command: spec.path.join(' '), @@ -38,8 +35,7 @@ export function buildAgentContext(specs: CommandSpec[]): AgentContextSchema { argumentMode: spec.argumentMode ?? 'parsed', summary: spec.summary, usage: spec.usage, - // Why: the effective accepted set (globals + conditional --page), not just - // allowedFlags — otherwise agents treat --json/--help as unsupported. + // Include globals and conditional flags so discovery matches validation. flags: effectiveAllowedFlags(spec), positionalArgs: spec.positionalArgs ?? [], ...(spec.variadicPositional ? { variadicPositional: true } : {}), @@ -47,7 +43,6 @@ export function buildAgentContext(specs: CommandSpec[]): AgentContextSchema { examples: spec.examples ?? [], notes: spec.notes ?? [] })) - // Why: deterministic ordering so the JSON diffs cleanly across runs. .sort((a, b) => a.command.localeCompare(b.command)) return { schemaVersion: SCHEMA_VERSION, @@ -57,8 +52,6 @@ export function buildAgentContext(specs: CommandSpec[]): AgentContextSchema { } export function formatAgentContextSummary(schema: AgentContextSchema): string { - // Why: keep the default (human) output bounded — the full surface is large, so - // point the reader at --json rather than dumping every command. return [ `${schema.commandCount} commands (schema v${schema.schemaVersion}).`, 'Run `orca agent-context --json` for the full machine-readable command schema.' diff --git a/src/cli/agent-launch-request.ts b/src/cli/agent-launch-request.ts index e2fed0f9d3c..a5151b3e441 100644 --- a/src/cli/agent-launch-request.ts +++ b/src/cli/agent-launch-request.ts @@ -1,8 +1,4 @@ -/** - * How the CLI starts an agent: through `agent.launch`, the host's one launch point that the desktop - * and phone use too. `--surface chat|terminal` picks a chat or a terminal for this launch; without - * it the user's Chat UI setting does. Either way the receipt says which ran and why. - */ +// agent.launch shares desktop/mobile policy; absent --surface uses the host's Chat UI setting. import { randomUUID } from 'node:crypto' import type { AgentLaunchPrompt, AgentLaunchResult } from '../shared/agent-launch-intent' @@ -31,7 +27,7 @@ import { withCliAgentLaunchRecovery } from './agent-launch-recovery' type Flags = Map -/** `create` is the `worktree.create` request the CLI already builds; the host strips agent fields. */ +/** The host strips agent fields from the worktree.create payload. */ export type CliAgentLaunchTarget = | { kind: 'existing'; worktree: string } | { kind: 'create-worktree'; create: Record } @@ -63,8 +59,6 @@ export function parseCliAgent(value: string): TuiAgent { return value } -/** The agent, prompt, model and effort a CLI start asks for; refuses a model or effort the launch - * could not apply, by the same rule as `orchestration worker-start`. */ export function readCliAgentLaunch( flags: Flags, agent: TuiAgent, @@ -94,7 +88,6 @@ export function readCliAgentLaunch( agent, ...(surface ? { surface } : {}), ...(operationId ? { operationId } : {}), - // A blank prompt is no prompt: the agent starts with nothing sent. ...(typeof prompt === 'string' && prompt.trim() ? { prompt: { text: prompt, delivery: 'submit' as const } } : {}), diff --git a/src/cli/agent-launch-test-fixtures.ts b/src/cli/agent-launch-test-fixtures.ts index faa6539554d..07a53cf4f8a 100644 --- a/src/cli/agent-launch-test-fixtures.ts +++ b/src/cli/agent-launch-test-fixtures.ts @@ -1,12 +1,10 @@ import { AGENT_LAUNCH_RUNTIME_CAPABILITIES } from '../shared/agent-launch-runtime-capability' import { okFixture } from './test-fixtures' -/** A host that runs `agent.launch`, as `status.get` reports it. */ export function agentLaunchHostFixture() { return okFixture('req_status', { capabilities: [...AGENT_LAUNCH_RUNTIME_CAPABILITIES] }) } -/** What `agent.launch` answers when the user's Chat UI setting opened a chat. */ export function agentLaunchFixture(worktreeId: string, warning?: string) { return okFixture('req_launch', { outcome: { kind: 'structured', sessionId: 'codex_1', handle: 'agent_1' }, diff --git a/src/cli/agent-session-search-format.ts b/src/cli/agent-session-search-format.ts index ebefb6ff7af..d1953c6de34 100644 --- a/src/cli/agent-session-search-format.ts +++ b/src/cli/agent-session-search-format.ts @@ -16,12 +16,7 @@ export type PrintableSessionSearchResponse = Exclude< { kind: 'malformed-cursor' } > -/** - * Transcript text reaches the terminal verbatim, so an OSC 52 or cursor sequence - * inside a tool log would otherwise run on the reader's terminal. The `[[` `]]` - * match marks are left as the engine wrote them: this CLI emits no ANSI anywhere, - * so a colour scheme invented here would be the only one in the surface. - */ +// Strip terminal controls from untrusted transcripts, including OSC 52 clipboard writes. export function terminalSafe(value: string): string { return stripAnsiEscapeSequences(value).replace(TERMINAL_CONTROL_CHARACTER_PATTERN, '') } @@ -85,8 +80,7 @@ function formatUnavailable( if (reason === 'unsupported-agent') { return 'This host does not support history search for the selected agent. Update Orca on that host or select another agent.' } - // The CLI scopes with --path, never with an identity, so this only reaches a - // caller that built a request by hand. + // Only hand-built requests can select an unknown identity; the CLI scopes by path. if (reason === 'scope-unknown') { return 'This host does not know the workspace or project that search was scoped to.' } diff --git a/src/cli/args.ts b/src/cli/args.ts index 6ac8dd52ce2..6c2304a83ba 100644 --- a/src/cli/args.ts +++ b/src/cli/args.ts @@ -38,7 +38,7 @@ function setFlagValue( flags.set(name, value) } -/** The most specific spec whose path prefixes `path`, so a group never shadows a leaf. */ +// Prefer the longest path so command groups cannot shadow leaf commands. function specForPathPrefix( specs: readonly CommandSpec[], path: readonly string[] @@ -75,9 +75,7 @@ export function parseArgs( } const assignment = token.slice(2) - // Why: `--flag=value` is the only unambiguous way to pass a value that - // itself starts with `--` (e.g. `--text=--help`); the space-separated form - // treats a `--`-leading next token as a new flag, so it can't express one. + // --flag=value permits values starting with -- without treating them as another flag. const equalsIndex = assignment.indexOf('=') if (equalsIndex !== -1) { flagEntries.push([assignment.slice(0, equalsIndex), assignment.slice(equalsIndex + 1)]) @@ -89,7 +87,7 @@ export function parseArgs( flagEntries.push([flag, true]) continue } - // Why: a pre-command flag must not consume a registry-resolvable command path. + // Pre-command flags must not consume a registered command path. if (commandPath.length === 0 && i + 1 === commandIndex) { flagEntries.push([flag, true]) continue @@ -176,7 +174,6 @@ export function supportsBrowserPageFlag(commandPath: string[]): boolean { ].includes(joined) } -// Why: validation and agent discovery must expose the same effective flag set. export function effectiveAllowedFlags(spec: CommandSpec): string[] { if (spec.argumentMode === 'passthrough') { return [] @@ -208,14 +205,12 @@ export function isCommandGroup(specs: CommandSpec[], commandPath: string[]): boo export function normalizeCommandPositionals(specs: CommandSpec[], parsed: ParsedArgs): ParsedArgs { for (const spec of specs) { const positionalArgs = spec.positionalArgs ?? [] - // Why: aliased paths still need canonicalization when there are no positionals. + // Alias-only paths still need canonicalization without positional arguments. if (positionalArgs.length === 0 && !spec.aliases) { continue } - // Why: canonicalize aliases before validation and dispatch so both use one key. for (const base of specPaths(spec)) { - // Why: `< 0` (not `<= 0`) so an exact base match with zero positionals - // still canonicalizes an aliased path; upper bound guards over-consumption. + // Zero positionals still permit an exact alias match. const positionalCount = parsed.commandPath.length - base.length if ( positionalCount < 0 || @@ -228,8 +223,7 @@ export function normalizeCommandPositionals(specs: CommandSpec[], parsed: Parsed } const flags = new Map(parsed.flags) const values = parsed.commandPath.slice(base.length) - // Why: validation runs inside main's error-reporting path, so normalization - // records ambiguity instead of throwing before CLI errors can be formatted. + // Record ambiguity here; validation reports it through the CLI error formatter. const providedPositionals = values.map( (_, index) => positionalArgs[Math.min(index, positionalArgs.length - 1)] ) diff --git a/src/cli/automation-destination.ts b/src/cli/automation-destination.ts index 0428a5b334e..ac5ef81b553 100644 --- a/src/cli/automation-destination.ts +++ b/src/cli/automation-destination.ts @@ -1,14 +1,4 @@ -/** - * Where a CLI create — or an edit that moves the record's project/workspace — - * wants the automation to land. - * - * The CLI cannot project a host itself, so it asks the same authority that will - * perform the write to resolve the selector it was given and to name its - * current SSH registrations. That captured incarnation rides along on the - * request, and the authority re-checks it inside the write, so a target that - * disappears in between fails the write instead of silently repointing the - * record at a dead or re-registered host. - */ +// Capture the authority's SSH generation so it can reject removed or re-registered targets at write time. import { AUTOMATION_OWNER_CONFLICT_CODES, @@ -21,7 +11,7 @@ import type { Repo } from '../shared/repo-types' import type { RuntimeClient } from './runtime-client' import { fetchSshTargetSummaries } from './host-selector-alternatives' -/** The project/workspace selectors the same request carries; both empty means the host cannot change. */ +/** Empty selectors mean the execution host cannot change. */ export type AutomationDestinationTarget = { repo?: string; workspace?: string } async function resolveTargetRepo( @@ -40,12 +30,7 @@ async function resolveTargetRepo( .repo } -/** - * `undefined` means the authority registers the target but assigns no - * generation, which is a host with nothing to fence — not a host to fail - * against. A target the authority does not register at all is positive - * evidence that the destination is gone. - */ +// A registered target without a generation needs no fence; an unregistered target is gone. async function sshTargetGeneration( client: RuntimeClient, targetId: string diff --git a/src/cli/automation-format.test.ts b/src/cli/automation-format.test.ts index 875307930df..e5b550aab56 100644 --- a/src/cli/automation-format.test.ts +++ b/src/cli/automation-format.test.ts @@ -31,10 +31,6 @@ function automation(overrides: Partial = {}): Automation { } } -// `orca automations run` sends users to `orca automations show` to find out where -// an automation lives. The stored `executionTarget*` fields survive their host's -// removal untouched, so printing only those renders a dead automation byte-for-byte -// like a healthy one and the named recovery step cannot be completed. describe('formatAutomationShow reports the host the authority projects', () => { it('distinguishes a live SSH host from a replaced one by its incarnation', () => { const live = formatAutomationShow({ @@ -58,7 +54,7 @@ describe('formatAutomationShow reports the host the authority projects', () => { }) expect(output).toContain('host: orphan (no host can run this automation)') - // The stored pin stays visible; it is the record's history, not its current owner. + // Keep the stored target as history, separately from current host ownership. expect(output).toContain('target: ssh:box-1') }) @@ -71,8 +67,6 @@ describe('formatAutomationShow reports the host the authority projects', () => { expect(output).toContain('host: self') }) - // An older authority projects no owner; inventing one from the stored fields would - // report a removed host as healthy. it('omits the host line when the authority reports no owner', () => { const output = formatAutomationShow({ automation: automation() }) diff --git a/src/cli/automation-format.ts b/src/cli/automation-format.ts index d7d355a736c..ec81da2d055 100644 --- a/src/cli/automation-format.ts +++ b/src/cli/automation-format.ts @@ -1,12 +1,4 @@ -/** - * Human-readable automation output for the CLI. - * - * The host lines come from the authority's own projection, never from the - * stored `executionTarget*` fields: a target that was removed — or removed and - * re-registered under the same id — leaves those fields untouched, so printing - * them alone would render a dead host identically to a healthy one and leave - * `orca automations show` unable to complete the recovery it is named in. - */ +// Use authority-projected owners: stored target fields survive host removal and re-registration. import type { Automation, AutomationRun } from '../shared/automations-types' import type { AutomationListItem } from '../shared/automation-list-scope' @@ -95,7 +87,6 @@ export function formatAutomationShow(result: AutomationShowPayload): string { `baseBranch: ${automation.baseBranch ?? 'null'}`, `reuseSession: ${automation.reuseSession}`, `target: ${automation.executionTargetType}:${automation.executionTargetId}`, - // Omitted rather than guessed: a host that reports no owner has none to report. ...(result.owner ? [`host: ${formatOwnerSelector(result.owner.selector)}`] : []), `prompt: ${automation.prompt}` ].join('\n') diff --git a/src/cli/automation-owner-conflict-recovery.test.ts b/src/cli/automation-owner-conflict-recovery.test.ts index 738632945f8..379ff1367ba 100644 --- a/src/cli/automation-owner-conflict-recovery.test.ts +++ b/src/cli/automation-owner-conflict-recovery.test.ts @@ -25,8 +25,6 @@ describe('automation owner conflicts read as CLI outcomes', () => { expect(output).toContain('Next step: Run the command again') }) - // A removed SSH host fails identically forever, so proposing a retry would send - // the user in a loop instead of to the two things that actually resolve it. it('never proposes a retry for a removed target', () => { const output = formatCliError( conflict(AUTOMATION_OWNER_CONFLICT_CODES.targetRemoved, 'Host removed.') @@ -58,9 +56,6 @@ describe('automation owner conflicts read as CLI outcomes', () => { expect(output).toBe('Something else: runtime_error') }) - // The trailing token is the convention precisely because a hop can flatten the - // error class. Classifying on `.code` alone reads only the hop that preserved it, - // and the user loses the recovery on every other one. it('recovers the conflict from the message when a hop flattened the code', () => { const flattened = new RuntimeRpcFailureError({ id: 'request-1', @@ -100,7 +95,6 @@ describe('automation owner conflicts read as CLI outcomes', () => { expect(payload.error.data.nextSteps[0]).toContain('no longer registered') }) - // A local throw never reaches the envelope, so the token is all the CLI has. it('classifies a locally thrown conflict the same way', () => { const output = formatCliError( new Error( diff --git a/src/cli/automation-owner-conflict-recovery.ts b/src/cli/automation-owner-conflict-recovery.ts index 8a900fcb17c..d785a39977f 100644 --- a/src/cli/automation-owner-conflict-recovery.ts +++ b/src/cli/automation-owner-conflict-recovery.ts @@ -1,11 +1,4 @@ -/** - * What a user should do about each automation owner conflict, phrased for the - * CLI rather than for a window that can re-render itself. - * - * `automation_target_removed` deliberately offers no retry: the SSH host the - * record is pinned to is gone, so every retry fails identically until the user - * re-adds that host or deletes the automation. - */ +// Removed SSH targets cannot recover through retry; users must re-add the host or delete the automation. import { AUTOMATION_OWNER_CONFLICT_CODES, diff --git a/src/cli/automation-runs-paging.test.ts b/src/cli/automation-runs-paging.test.ts index 1fd88730dcb..048c7558391 100644 --- a/src/cli/automation-runs-paging.test.ts +++ b/src/cli/automation-runs-paging.test.ts @@ -19,7 +19,6 @@ function run(index: number, snapshotBytes = 0): AutomationRun { } as unknown as AutomationRun } -/** A paired host: pages when asked, and refuses any reply a remote connection cannot carry. */ function pairedHost(runs: AutomationRun[], options: { pages: boolean } = { pages: true }) { return vi.fn(async (params: { automationId?: string; limit?: number; cursor?: string }) => { const result = @@ -35,7 +34,6 @@ function pairedHost(runs: AutomationRun[], options: { pages: boolean } = { pages describe('readAllAutomationRuns', () => { it('reads a history larger than one remote reply, in order and without gaps', async () => { - // 430 runs of ~23 KB is ~10 MB, which a single unpaged reply could not carry. const runs = Array.from({ length: 430 }, (_, index) => run(index, 23_000)) const host = pairedHost(runs) const result = await readAllAutomationRuns(host, undefined) diff --git a/src/cli/automation-runs-paging.ts b/src/cli/automation-runs-paging.ts index 25f31033798..264a6c5e266 100644 --- a/src/cli/automation-runs-paging.ts +++ b/src/cli/automation-runs-paging.ts @@ -2,7 +2,7 @@ import type { AutomationRun } from '../shared/automations-types' import type { RuntimeRpcSuccess } from '../shared/runtime-rpc-envelope' import { RuntimeClientError, type RuntimeClient } from './runtime-client' -// Why 25: a snapshot can reach ~200 KB, so 25 runs stay under a remote reply's 4 MB in practice. +// Keep typical snapshot pages below the remote reply limit; retry smaller when needed. export const AUTOMATION_RUNS_PAGE_SIZE = 25 const REPLY_TOO_LARGE = 'reply_too_large' @@ -10,7 +10,6 @@ type RunsReply = { runs: AutomationRun[]; nextCursor?: string | null } type RunsQuery = { automationId?: string; limit: number; cursor?: string } -/** Sends one `automation.runs` request. */ export type AutomationRunsRpc = ( params: Partial ) => Promise> @@ -44,7 +43,6 @@ async function readShrinkingPage( } } -/** One page, halving the page size while the remote host refuses the reply as too large. */ export async function readAutomationRunsPage( rpc: AutomationRunsRpc, query: RunsQuery @@ -52,10 +50,7 @@ export async function readAutomationRunsPage( return (await readShrinkingPage(rpc, query)).page } -/** - * Every run, read page by page so no single reply has to carry the whole history (#28254). - * A host older than paging ignores `limit` and returns everything with no cursor, ending the loop. - */ +/** Older hosts ignore paging and return all runs without a cursor, ending the loop. */ export async function readAllAutomationRuns( rpc: AutomationRunsRpc, automationId: string | undefined @@ -65,7 +60,7 @@ export async function readAllAutomationRuns( let cursor: string | undefined let limit = AUTOMATION_RUNS_PAGE_SIZE for (;;) { - // Why carry the limit: the next page is likely as heavy as the one that had to shrink. + // Later pages may need the same reduced limit. const read = await readShrinkingPage(rpc, { automationId, limit, cursor }) const page = read.page limit = read.limit diff --git a/src/cli/browser-empty-values.test.ts b/src/cli/browser-empty-values.test.ts index 23b110d46cd..5928f8c0200 100644 --- a/src/cli/browser-empty-values.test.ts +++ b/src/cli/browser-empty-values.test.ts @@ -37,7 +37,6 @@ vi.mock('./runtime-client', () => { import { main } from './index' import { okFixture, queueFixtures } from './test-fixtures' -// Why: StorageKeyValue requires a non-empty key but accepts any string value, empty included. describe('orca cli storage set preserves an empty value', () => { beforeEach(() => { callMock.mockReset() @@ -82,7 +81,7 @@ describe('orca cli storage set preserves an empty value', () => { }) }) - // Why: PowerShell 5.1 drops empty native-exe args, so `--value=` is the Windows escape hatch. + // PowerShell 5.1 drops empty native arguments; equals form preserves them. it('passes an empty --value= (equals form) through to browser.storage.local.set', async () => { queueFixtures(callMock, okFixture('req_storage_local_set', { success: true })) vi.spyOn(console, 'log').mockImplementation(() => {}) @@ -133,8 +132,6 @@ describe('orca cli storage set preserves an empty value', () => { }) }) -// Why: CookieSet.value and SetCredentials.pass accept any string, empty included, -// while name and user require a non-empty one. describe('orca cli cookie set and set credentials preserve an empty value', () => { beforeEach(() => { callMock.mockReset() diff --git a/src/cli/browser-handler-groups.ts b/src/cli/browser-handler-groups.ts index 87cf8850380..8d55d88af1b 100644 --- a/src/cli/browser-handler-groups.ts +++ b/src/cli/browser-handler-groups.ts @@ -1,7 +1,5 @@ import type { HandlerGroup } from './handler-group-manifest' -// Why split out: the browser command surface is a third of the CLI's groups and -// changes as a unit, so it keeps handler-group-manifest.ts readable at a glance. export const BROWSER_HANDLER_GROUPS: readonly HandlerGroup[] = [ { name: 'browser-identity', diff --git a/src/cli/caller-host-worktree-selection.ts b/src/cli/caller-host-worktree-selection.ts index 931e6d0555e..b2edfb0b2e1 100644 --- a/src/cli/caller-host-worktree-selection.ts +++ b/src/cli/caller-host-worktree-selection.ts @@ -14,7 +14,7 @@ type CatalogRow = Pick & { identity?: { key: string } } -/** The execution host this CLI process runs on: the SSH target its relay stamped, else this machine. */ +/** SSH relays stamp the caller host; otherwise use this machine. */ export function getCallerExecutionHostId( env: Readonly> = process.env ): ExecutionHostId { @@ -22,9 +22,7 @@ export function getCallerExecutionHostId( return host?.kind === 'ssh' ? toSshExecutionHostId(host.targetId) : LOCAL_EXECUTION_HOST_ID } -// Why: the catalog lists SSH hosts' worktrees too and two hosts can hold the same path, so an SSH -// caller may only take its own host's rows and a local caller none of an SSH host's. `runtime:*` -// rows in this runtime's catalog are checkouts it holds itself, and unstamped rows predate stamping. +// Paths may exist on multiple hosts; runtime:* and unstamped rows belong to this runtime. function isOnCallerHost(row: CatalogRow, callerHostId: ExecutionHostId): boolean { const rowHostId = row.hostId ?? LOCAL_EXECUTION_HOST_ID return parseExecutionHostId(callerHostId)?.kind === 'ssh' @@ -32,17 +30,15 @@ function isOnCallerHost(row: CatalogRow, callerHostId: ExecutionHostId): boolean : parseExecutionHostId(rowHostId)?.kind !== 'ssh' } -// Why: `id:` keeps repo inference and the runtime's scoped fast path; only a bare id that another -// host's row shares needs the exact identity, or the runtime would refuse it as ambiguous. +// Prefer id: for repo inference; shared IDs need an exact host-qualified identity. function toHostScopedSelector(row: CatalogRow, catalog: readonly CatalogRow[]): string { const sharedId = catalog.some((other) => other !== row && other.id === row.id) return sharedId && row.identity?.key ? `identity:${row.identity.key}` : `id:${row.id}` } -/** A host-scoped selector plus the worktree id it names, which an `identity:` selector hides. */ export type HostScopedWorktreeSelection = { selector: string; worktreeId: string } -/** The caller host's innermost worktree enclosing `cwd`; undefined when none does. */ +/** Prefer the innermost enclosing worktree on the caller's host. */ export function selectEnclosingWorktreeSelectionOnHost( catalog: readonly CatalogRow[], cwd: string, @@ -67,7 +63,6 @@ export function selectEnclosingWorktreeSelectionOnHost( : undefined } -/** The caller host's innermost worktree enclosing `cwd`, as a selector; undefined when none does. */ export function selectEnclosingWorktreeOnHost( catalog: readonly CatalogRow[], cwd: string, @@ -76,7 +71,6 @@ export function selectEnclosingWorktreeOnHost( return selectEnclosingWorktreeSelectionOnHost(catalog, cwd, hostId)?.selector } -/** Scopes a pane's forwarded bare worktree id to the caller's host; undefined when it has none. */ export function selectWorktreeIdOnHost( catalog: readonly CatalogRow[], worktreeId: string, @@ -86,7 +80,6 @@ export function selectWorktreeIdOnHost( return match ? toHostScopedSelector(match, catalog) : undefined } -/** Scopes a pane's forwarded bare `ORCA_WORKTREE_ID` to the caller's host before it is used. */ export async function resolveCallerPaneWorktreeSelector( worktreeId: string, client: RuntimeClient diff --git a/src/cli/cli-error.ts b/src/cli/cli-error.ts index 15491801247..d4bb81ed14d 100644 --- a/src/cli/cli-error.ts +++ b/src/cli/cli-error.ts @@ -13,7 +13,7 @@ export type CliErrorContext = { commandPath?: readonly string[] /** The `--worktree` value this invocation sent; the runtime's error never echoes it. */ worktreeSelector?: string - /** The `--repo` value this invocation sent, for the same reason. */ + /** The runtime error omits the original --repo selector. */ repoSelector?: string /** True when --environment, --pairing-code, --host runtime:, or their env vars chose a server. */ remoteServerSelected?: boolean @@ -38,7 +38,7 @@ function contextRecovery( remoteServerSelected: context.remoteServerSelected }) } - // Why: a paired server's handle sent to this machine reads as a dead terminal, not a missing flag. + // A remote handle sent locally looks stale rather than missing a flag. if ( code === 'terminal_handle_stale' && context.commandPath?.[0] === 'terminal' && @@ -78,8 +78,7 @@ export function formatCliError(error: unknown, context: CliErrorContext = {}): s } return `${message}\nOrca is not running. Run 'orca open' first.` } - // Why: error-specific recovery must win over the generic computer fallback. - // Classified from the whole error, not just `.code`: a hop that flattens the class leaves only the token. + // Match before generic recovery; flattened errors may retain only the conflict token. const conflict = automationOwnerConflictRecovery(matchAutomationOwnerConflict(error)) if (conflict) { return formatMessageWithNextSteps(stripAutomationOwnerConflictCode(message), conflict.nextSteps) @@ -167,7 +166,6 @@ export function reportCliError(error: unknown, json: boolean, context: CliErrorC } } -/** Machine-readable half of the same recovery the human message carries. */ function withAutomationOwnerConflictRecovery(response: RuntimeRpcFailure): RuntimeRpcFailure { const code = matchAutomationOwnerConflict(response) const conflict = automationOwnerConflictRecovery(code) @@ -178,7 +176,7 @@ function withAutomationOwnerConflictRecovery(response: RuntimeRpcFailure): Runti ...response, error: { ...response.error, - // Restores the classification a flattening hop dropped, so --json consumers read the conflict, not the transport. + // Restore conflict classification lost during transport flattening. code, message: stripAutomationOwnerConflictCode(response.error.message), data: response.error.data ?? conflict @@ -193,7 +191,7 @@ function formatMessageWithNextSteps(message: string, nextSteps: readonly string[ return `${message}\n${nextSteps.map((step) => `Next step: ${step}`).join('\n')}` } -/** Why merge: a mutation error already carries its request id, and `??` dropped the selector grammar. */ +// Preserve both mutation request IDs and selector recovery hints. function mergeContextRecovery( data: unknown, selector: ReturnType @@ -224,7 +222,7 @@ function nextStepsFromData(data: unknown): string[] { return [] } -/** Read here, not from the error's data: the servers that send this predate the hint (#31777). */ +// Older servers do not include this recovery hint in error data. function computerSidecarExitRecovery(error: unknown, context: CliErrorContext) { if ( context.commandPath?.[0] !== 'computer' || @@ -243,7 +241,7 @@ function localCliErrorData(error: unknown, context: CliErrorContext): unknown { return sidecarExit } const selector = contextRecovery(errorCode(error), context) - // Why: error-specific recovery must win over the generic computer fallback. + // Error-specific recovery takes precedence over the generic fallback. if (error instanceof RuntimeClientError && error.data !== undefined) { return mergeContextRecovery(error.data, selector) } diff --git a/src/cli/codex-command-classification.ts b/src/cli/codex-command-classification.ts index b68efdcb60d..3c07ceb5424 100644 --- a/src/cli/codex-command-classification.ts +++ b/src/cli/codex-command-classification.ts @@ -231,8 +231,7 @@ function isNonInteractiveCodexSubcommand(tokens: string[]): boolean { const normalizedSubcommand = subcommand.value.toLowerCase() if (normalizedSubcommand === 'login') { - // Why: bare `codex login` displays an auth flow; only explicit status/help - // or stdin-fed token modes are safe to leave in a background PTY. + // Bare codex login opens an auth flow; only status/help or stdin-token modes are background-safe. const loginStartIndex = subcommand.index + 1 const loginSubcommand = findCodexSubcommand( tokens, @@ -250,8 +249,7 @@ function isNonInteractiveCodexSubcommand(tokens: string[]): boolean { ) } if (normalizedSubcommand === 'cloud') { - // Why: bare `codex cloud` opens the interactive cloud browser, while its - // named child commands are plain one-shot commands. + // Bare codex cloud is interactive; named subcommands are one-shot commands. const cloudStartIndex = subcommand.index + 1 const cloudSubcommand = findCodexSubcommand( tokens, diff --git a/src/cli/command-scoped-flag-help.ts b/src/cli/command-scoped-flag-help.ts index b3e85334382..e9a09bf61ca 100644 --- a/src/cli/command-scoped-flag-help.ts +++ b/src/cli/command-scoped-flag-help.ts @@ -1,8 +1,7 @@ -// Why: the shared --focus line describes terminal create's terminal session. +// File focus differs from terminal create's shared --focus help. const FILE_OPEN_FOCUS_HELP = "--focus Bring the user to the file (switches Orca's window to its worktree)" -/** Per-command flag help, kept out of the shared help chain it would crowd. */ const COMMAND_SCOPED_FLAG_HELP: Record> = { 'reference add': { url: '--url Full review/issue URL or Jira key; use positionals or repeated --url', diff --git a/src/cli/command-spec.ts b/src/cli/command-spec.ts index 3df33960e07..8d328f69a29 100644 --- a/src/cli/command-spec.ts +++ b/src/cli/command-spec.ts @@ -1,23 +1,20 @@ export type CommandSpec = { path: string[] - // Why: conventional alternate verbs should resolve without duplicating specs or handlers. aliases?: string[][] argumentMode?: 'parsed' | 'passthrough' - // Why: typo recovery must never steer a benign mistake into destructive state changes. + // Prevent typo recovery from suggesting destructive actions for unrelated commands. destructive?: boolean hidden?: boolean summary: string usage: string allowedFlags: string[] - // Why: repeatability is per-command vocabulary. `--agent` repeats for `search` - // and is single-valued for `worktree create`, which one global set cannot say. + // Flag repeatability varies by command, even for the same flag name. repeatableFlags?: string[] positionalArgs?: string[] variadicPositional?: boolean examples?: string[] notes?: string[] - // Why: `--from`/`--terminal` names either the acting caller or a target, and only the spec can - // say which. An agent session refuses a caller flag naming anyone else before dispatch. + // Caller flags must match the acting agent session; target flags may name another session. identityFlagRoles?: Partial> } diff --git a/src/cli/command-suggestion.test.ts b/src/cli/command-suggestion.test.ts index 2f8a5ce71ca..8f2b6056d8b 100644 --- a/src/cli/command-suggestion.test.ts +++ b/src/cli/command-suggestion.test.ts @@ -28,8 +28,6 @@ const specs: CommandSpec[] = [ allowedFlags: [] }, { - // A destructive command outside the delete-family, to prove the guard keys - // off the spec flag rather than a hardcoded verb list. path: ['emulator', 'kill'], destructive: true, summary: 'Kill the emulator', @@ -64,8 +62,6 @@ describe('suggestCommands', () => { }) it('never suggests a destructive command for a benign non-destructive typo', () => { - // `worktree move` sits distance 2 from `worktree remove`; without the - // guard it would sole-suggest an irreversible delete on blind retry. #6303 const result = suggestCommands(specs, ['worktree', 'move']) expect(result).not.toContain('worktree remove') expect(result).not.toContain('worktree rm') @@ -83,10 +79,7 @@ describe('suggestCommands', () => { }) it('guards destructive commands outside the delete-family via the spec flag', () => { - // `emulator ball` is a benign token, distance 2 from the flagged `emulator - // kill` — close enough to otherwise rank, so the guard must exclude it. expect(suggestCommands(specs, ['emulator', 'ball'])).not.toContain('emulator kill') - // A genuine near-miss of the destructive verb still recovers. expect(suggestCommands(specs, ['emulator', 'kil'])).toContain('emulator kill') }) diff --git a/src/cli/command-suggestion.ts b/src/cli/command-suggestion.ts index 47c78d74119..192bfe8b552 100644 --- a/src/cli/command-suggestion.ts +++ b/src/cli/command-suggestion.ts @@ -1,21 +1,17 @@ import { specPaths, type CommandSpec } from './command-spec' import { levenshtein } from '../shared/edit-distance' -// Why: rank the live registry so typo recovery cannot drift from accepted paths. - const SUGGESTION_THRESHOLD = 3 const MAX_SUGGESTIONS = 3 -// Why: a close typo of a destructive verb (`remov`→`remove`) still signals that -// intent, but `move` (distance 2 from `remove`) does not — keep this at 1 so -// genuine recovery works while unrelated verbs stay locked out. #6303 +// Distance 1 recovers remov→remove without suggesting deletion for move. const DESTRUCTIVE_INTENT_THRESHOLD = 1 function finalToken(path: string[]): string { return path.at(-1) ?? '' } -// Why: declared aliases connect equivalent destructive verbs without unlocking unrelated actions. +// Aliases join destructive verb families without admitting unrelated verbs. function destructiveVerbFamilies(specs: CommandSpec[]): Map> { const families = new Map>() for (const spec of specs) { @@ -48,7 +44,6 @@ export type CommandErrorData = { nextSteps: string[] } -// Why: one bounded near-match ranking keeps command and flag recovery consistent. function rankByDistance(scored: { label: string; distance: number }[]): string[] { return scored .filter((entry) => entry.distance > 0 && entry.distance <= SUGGESTION_THRESHOLD) @@ -57,7 +52,7 @@ function rankByDistance(scored: { label: string; distance: number }[]): string[] .map((entry) => entry.label) } -// Why: same-depth matching avoids suggesting parent groups or unrelated commands. +// Match depth to avoid suggesting parent groups or unrelated commands. export function suggestCommands(specs: CommandSpec[], commandPath: string[]): string[] { const input = commandPath.join(' ') const families = destructiveVerbFamilies(specs) @@ -107,10 +102,7 @@ export type FlagErrorData = { nextSteps: string[] } -// Why: edit distance cannot recover a rename. `orchestration check` is the one verb -// that identifies its caller with `--terminal` while every sibling uses `--from`, so -// the near-miss ranking answered `--json`/`--run` and left the caller stuck (#16904). -// A synonym only fires where the typed flag is rejected and its partner is accepted. +// Recover renamed flags only when the typed flag is rejected and its synonym is accepted. const FLAG_SYNONYMS: Readonly> = { from: 'terminal' } function suggestFlags(flag: string, validFlags: string[]): string[] { @@ -127,7 +119,6 @@ function suggestFlags(flag: string, validFlags: string[]): string[] { : ranked } -// Why: include the accepted set so agents can recover without another help call. export function unknownFlagData(flag: string, validFlags: string[]): FlagErrorData { const sortedValid = [...validFlags].sort((a, b) => a.localeCompare(b)) const suggestions = suggestFlags(flag, sortedValid) diff --git a/src/cli/computer-format.ts b/src/cli/computer-format.ts index 3ec34c9cfbc..a3b4eef6e55 100644 --- a/src/cli/computer-format.ts +++ b/src/cli/computer-format.ts @@ -95,8 +95,7 @@ export function prepareComputerCliJsonResult( } } as RuntimeRpcSuccess } catch { - // Why: temp-file export is an ergonomics optimization; keep inline screenshot - // data when disk, permissions, or path validation would otherwise fail --json. + // Preserve inline screenshot data if the optional temp-file export fails. return response } } @@ -124,8 +123,7 @@ function cleanupComputerScreenshots(outputDir: string): void { const now = Date.now() const markerPath = join(outputDir, COMPUTER_SCREENSHOT_CLEANUP_MARKER) try { - // Why: agents can call computer-use CLI commands in loops; a marker keeps - // temp cleanup from becoming a synchronous directory scan per screenshot. + // Throttle synchronous cleanup scans across repeated screenshots. if (statSync(markerPath).mtimeMs > now - COMPUTER_SCREENSHOT_CLEANUP_INTERVAL_MS) { return } @@ -144,13 +142,13 @@ function cleanupComputerScreenshots(outputDir: string): void { rmSync(path, { force: true }) } } catch { - // Best-effort cleanup only; formatting should not fail because a temp file raced. + // A raced temp file must not fail screenshot formatting. } } try { writeFileSync(markerPath, `${now}\n`, { mode: 0o600 }) } catch { - // Best-effort marker only; stale cleanup state should not hide a screenshot. + // A failed cleanup marker must not hide the screenshot. } } diff --git a/src/cli/dispatch.ts b/src/cli/dispatch.ts index 7ad882b1e11..b6fb322420b 100644 --- a/src/cli/dispatch.ts +++ b/src/cli/dispatch.ts @@ -12,8 +12,7 @@ export type HandlerContext = { export type CommandHandler = (ctx: HandlerContext) => Promise -// Why: routing only needs key→group, so every CLI invocation can skip the -// transitive module graph of the 24 groups it does not dispatch into. +// Load only the selected handler group to avoid importing unrelated command dependencies. function buildRoutes(groups: readonly HandlerGroup[]): Map { const table = new Map() for (const group of groups) { @@ -32,7 +31,6 @@ function buildRoutes(groups: readonly HandlerGroup[]): Map const ROUTES = buildRoutes(HANDLER_GROUPS) -// Why: exposes only the canonical command keys, not the handler internals. export const HANDLER_COMMAND_KEYS: ReadonlySet = new Set(ROUTES.keys()) export async function dispatch(commandPath: string[], ctx: HandlerContext): Promise { @@ -42,8 +40,7 @@ export async function dispatch(commandPath: string[], ctx: HandlerContext): Prom throw new RuntimeClientError('invalid_argument', `Unknown command: ${key}`) } const handler = (await group.load())[key] - // Why: the manifest key list is verified against the real exports in CI, so a - // miss here means the group changed without the manifest — fail loudly. + // Missing handlers indicate drift from the CI-verified manifest. if (!handler) { throw new RuntimeClientError( 'invalid_argument', diff --git a/src/cli/execution-host-flag.test.ts b/src/cli/execution-host-flag.test.ts index 4cb9d9b9a36..21d6551c954 100644 --- a/src/cli/execution-host-flag.test.ts +++ b/src/cli/execution-host-flag.test.ts @@ -54,8 +54,6 @@ describe('parseHostFlag', () => { expect(() => parseHostFlag(flags({ host: true }))).toThrow('Missing value for --host') }) - // A value that claims a kind but does not parse is malformed, not a name someone could have - // meant — only an unprefixed value can be a host alias. it.each(['runtime:', 'ssh:', 'ssh:a|b', ' '])('rejects the unparseable host id %s', (value) => { expect(() => parseHostFlag(flags({ host: value }))).toThrow(`Invalid --host value: ${value}`) }) @@ -70,8 +68,6 @@ describe('parseHostFlag', () => { }) }) - // Why: `orca host list` prints the alias, so the alias is what gets typed. Rejecting it sent - // people hunting for a machine-generated id the listing showed in a different column. it('carries an unprefixed host name through as an alias', () => { expect(parseHostFlag(flags({ host: 'trader-local' }))).toEqual({ kind: 'alias', @@ -111,8 +107,6 @@ describe('resolveHostFlagEnvironmentId', () => { ).rejects.toThrow('no paired Orca server is named or has id env-missing') }) - // Why: the name is what a person or agent actually knows; requiring the raw uuid made the - // obvious spelling fail against a server that is sitting right there. it('accepts the environment name as well as its id', async () => { listEnvironmentsMock.mockReturnValue([environment('env-1', 'gpu')]) @@ -213,8 +207,6 @@ describe('resolveHostFlagEnvironmentId', () => { }) }) -// Why: the reported failure. `orca host list` shows `trader-local`, so `--host trader-local` is -// what gets typed, and only the machine-generated `ssh:ssh-1783599077486-fyasux` was accepted. describe('unprefixed --host aliases', () => { function clientWithTargets(targets: { id: string; label: string }[]): RuntimeClient { // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: SSH target resolution reads only client.call. @@ -232,7 +224,6 @@ describe('unprefixed --host aliases', () => { ) }) - // The SSH axis lives on the connected runtime, so a miss here is not yet a failure. it('leaves an alias that names no paired server to the SSH axis', async () => { listEnvironmentsMock.mockReturnValue([environment('env-1', 'gpu')]) @@ -260,8 +251,6 @@ describe('unprefixed --host aliases', () => { ).rejects.toThrow('Unknown host in --host trader-local') }) - // Why: a paired server and an SSH target are different machines. Picking either would run the - // command somewhere the caller never named. it('refuses an alias that names a machine on both axes', async () => { listEnvironmentsMock.mockReturnValue([environment('env-1', 'awin')]) listSshTargetsMock.mockResolvedValue([{ id: 'ssh-1-a', label: 'awin' }]) @@ -279,9 +268,6 @@ describe('unprefixed --host aliases', () => { }) }) - // Why: findSshTargetByName returns undefined for a duplicated label, which reads the same as - // "no SSH target by that name" — so this case used to look conflict-free and resolve to the - // paired server, the one direction that still guessed. it('refuses an alias naming a server and two SSH targets sharing a label', async () => { listEnvironmentsMock.mockReturnValue([environment('env-1', 'awin')]) listSshTargetsMock.mockResolvedValue([ @@ -329,7 +315,6 @@ describe('unprefixed --host aliases', () => { }) }) - // Why: `orca host list` prints `this machine` as the local row's name, with a space. it('resolves the local row name to --host local', async () => { listEnvironmentsMock.mockReturnValue([environment('env-1', 'gpu')]) await expect( @@ -375,7 +360,7 @@ describe('unprefixed --host aliases', () => { }) }) - // Why: cmd.exe does not strip single quotes, so `--host 'gpu'` arrives with them. + // cmd.exe passes wrapping single quotes through literally. it('ignores one pair of wrapping quotes around a host name', async () => { listEnvironmentsMock.mockReturnValue([environment('env-1', 'gpu')]) listSshTargetsMock.mockResolvedValue([]) @@ -385,8 +370,6 @@ describe('unprefixed --host aliases', () => { expect(parseHostFlag(flags({ host: '"local"' }))).toEqual({ kind: 'local', id: 'local' }) }) - // Why: with --environment the command runs on that server, so a bare name is a machine it - // reaches; a locally paired server sharing the name, even the routed one, is not a host there. it('leaves an alias to the routed server SSH targets when --environment picks the server', async () => { listEnvironmentsMock.mockReturnValue([ environment('env-1', 'openclaw'), @@ -482,8 +465,7 @@ describe('resolveEnvironmentSelectorForStore', () => { expect(listSshTargetsMock).not.toHaveBeenCalled() }) - // Why: the store matches names exactly, so a loosely matched name has to leave here as the id; - // an exact one stays as typed, which `orca status` shows as the target. + // The store matches names exactly; canonicalize loose matches to IDs. it('hands back the id for a name typed in another case or quoted', async () => { listEnvironmentsMock.mockReturnValue([environment('env-1', 'Build')]) @@ -495,8 +477,6 @@ describe('resolveEnvironmentSelectorForStore', () => { ) }) - // Why: the inverse of the --host case, and the one the report actually hit — a name that is - // an SSH target dead-ended with a bare "unknown environment". it('names the SSH target when the selector is one', async () => { listEnvironmentsMock.mockReturnValue([environment('env-1', 'awin')]) listSshTargetsMock.mockResolvedValue([{ id: 'ssh-1-a', label: 'openclaw' }]) diff --git a/src/cli/execution-host-flag.ts b/src/cli/execution-host-flag.ts index ba4cd93287d..57351723ec7 100644 --- a/src/cli/execution-host-flag.ts +++ b/src/cli/execution-host-flag.ts @@ -23,21 +23,16 @@ import type { RuntimeClient } from './runtime-client' import { RuntimeClientError } from './runtime/types' export type HostFlagRoutingSelection = { - // Why: SSH targets live in the running Orca host, not on disk, so enumerating them needs a - // client. Injected as a thunk so the lookup only happens on the error path we are explaining. + // Enumerating SSH targets needs the running host; defer lookup until error recovery. listSshTargets: () => Promise - // Why: unlike the suggestion listing, this throws when the targets cannot be read. A bare name - // may only route to a paired server once no SSH target is known to share it. + // Fail on unreadable targets: routing a bare name requires ruling out SSH collisions. verifySshTargets: () => Promise pairingCode: string | null - // Why: an ambient ORCA_ENVIRONMENT counts as a selection too, so carry the label to - // name the real source in the conflict message. + // Preserve the selection source, including ORCA_ENVIRONMENT, for conflict messages. environmentSelector: { value: string; label: string } | null } -// Why: `orca host list` prints the alias a caller actually knows (`trader-local`), so that is what -// they type — but which machine it names needs the host listings, which only the async resolvers -// below can read. Carry the alias unresolved rather than rejecting the obvious spelling. +// Host labels need async catalog lookup before they can resolve to a machine. export type HostFlagAlias = { kind: 'alias'; alias: string } export type HostFlagSelection = ParsedExecutionHost | HostFlagAlias @@ -59,8 +54,7 @@ export function parseHostFlag(flags: Map): HostFlagSel return parsed } const alias = value.trim() - // A value that claims a kind but does not parse is malformed, not an alias: `ssh:` names no - // target, and a `|` would rebind the id downstream. Only an unprefixed name can be a label. + // Prefixed values must parse as IDs, never aliases; malformed IDs could rebind downstream. if (!alias || alias.startsWith('ssh:') || alias.startsWith('runtime:')) { throw new RuntimeClientError( 'invalid_argument', @@ -70,10 +64,7 @@ export function parseHostFlag(flags: Map): HostFlagSel return { kind: 'alias', alias } } -// Why: `runtime:` ids are minted by this machine's pairing store, so they -// only name a machine relative to it. Filtering or mutating the locally connected runtime -// with one answers for the wrong host, and an id that matches nothing is indistinguishable -// from a real host with no rows — so resolve the id here and send the command to it. +// Pairing IDs are local to this machine; route to the paired host before applying its filters. export async function resolveHostFlagEnvironmentId( flags: Map, selection: HostFlagRoutingSelection @@ -93,25 +84,18 @@ export async function resolveHostFlagEnvironmentId( })) const wanted = host.kind === 'runtime' ? host.environmentId : host.alias const flagSpelling = host.kind === 'runtime' ? `--host ${host.id}` : `--host ${host.alias}` - // Why: with --environment or --pairing-code the command already runs on a chosen server, so a - // bare name is this machine or an SSH target that server reaches, never a second server. + // An explicit server selection scopes bare aliases to that server's local/SSH hosts. if (host.kind === 'alias' && (selection.environmentSelector || selection.pairingCode)) { return null } - // Why: --environment has always taken a name or an id, and the name is what people and agents - // actually know. Requiring the raw uuid here made the obvious spelling fail; accept either and - // canonicalize to the id so stored host ids still compare correctly downstream. + // Accept environment names but canonicalize them to IDs for stored-host comparisons. const environment = findEnvironmentByName(environments, wanted) if (!environment) { assertEnvironmentNameUnambiguous(environments, wanted, flagSpelling) - // A bare alias has axes left to try: this machine, and SSH targets on the connected runtime, - // which resolveHostFlagTarget answers once a client exists. Missing here is not a failure. + // A missing server alias may still name the local machine or one of its SSH targets. if (host.kind === 'alias') { return null } - // Why: `runtime:` is a host id that also appears in stored rows, so it resolves by id - // only — unlike --environment, which also accepts a name. Say so, and hand back the ids an - // agent can retry with instead of making it scrape the sentence. const sshTargets = await selection.listSshTargets() throw new RuntimeClientError( 'invalid_argument', @@ -163,9 +147,7 @@ export async function resolveHostFlagEnvironmentId( return environment.id } -// Why: a paired server and an SSH target are different machines, and one alias can name both. -// Picking either would send the command to a machine the caller did not choose — the failure the -// prefixed spellings exist to prevent — so hand back both flags and let them say which. +// An alias naming both a paired server and an SSH target must not choose either silently. async function assertAliasNamesOneHost( alias: string, environment: EnvironmentSummary, @@ -186,9 +168,7 @@ async function assertAliasNamesOneHost( } ) } - // Why: findSshTargetByName collapses a duplicated label to undefined, which reads here as "no - // SSH target by that name" — so two colliding targets would have looked like no conflict at all - // and resolved to the server. Duplicates are more ambiguity, not less. + // Duplicate SSH labels are ambiguous, not evidence that no target matches. const matched = findSshTargetByName(sshTargets, alias) const conflicts = matched ? [matched] : ambiguousSshTargets(sshTargets, alias) if (conflicts.length === 0) { @@ -212,9 +192,7 @@ async function assertAliasNamesOneHost( ) } -// Why: a runtime stamps its own setups `local` when they were made on the box and -// `runtime:` when a paired client made them. Once --host routed the command to that -// runtime both spellings mean the same machine, so a host filter has to accept both. +// After routing, local and runtime: setups both belong to the selected server. export function hostFilterMatchesHostId( filter: ParsedExecutionHost, candidateHostId: string | null | undefined @@ -226,10 +204,7 @@ export function hostFilterMatchesHostId( return filter.kind === 'runtime' && candidate === LOCAL_EXECUTION_HOST_ID } -// Why: `ssh:` reaches a machine the connected runtime owns, so it can only be checked against -// that runtime — unlike `runtime:`, which is resolved from this machine's pairing store before a -// client exists. Callers that act on a --host value run this so an unknown target fails loudly -// instead of quietly filtering to nothing. +// Validate SSH IDs against the connected runtime, not this machine's pairing store. export async function resolveHostFlagTarget( flags: Map, client: RuntimeClient @@ -249,8 +224,7 @@ export async function resolveHostFlagTarget( if (host.kind === 'alias' && isLocalHostListName(host.alias)) { return resolveLocalHostListName(host.alias, client) } - // A bare alias reaches here only after resolveHostFlagEnvironmentId found no paired server by - // that name, so SSH is the one axis left and an unknown name is a real failure. + // Server lookup already missed; SSH is the remaining alias scope. const wanted = host.kind === 'ssh' ? host.targetId : host.alias const flagSpelling = host.kind === 'ssh' ? `--host ${host.id}` : `--host ${host.alias}` const targetId = await resolveSshHostTargetId( @@ -289,9 +263,7 @@ async function resolveLocalHostListName( return { kind: 'local', id: LOCAL_EXECUTION_HOST_ID } } -// Why: the store refuses an ambiguous environment name rather than guessing which server was -// meant. Resolving one here would put the guess back, in the flag whose whole purpose is to stop -// a command reaching a machine the caller did not choose. +// Preserve the store's refusal to guess between duplicate environment names. function assertEnvironmentNameUnambiguous( environments: readonly { id: string; name: string }[], name: string, @@ -313,11 +285,7 @@ function assertEnvironmentNameUnambiguous( ) } -// Why: the inverse of the --host case. `--environment openclaw` failed with a bare "Unknown -// environment", when openclaw is very often an SSH target — a different axis, not a typo. The -// store's own error cannot carry the hint (translateStoreError forwards code and message only, -// dropping data), so resolve the selector here where the payload survives. Returns what the store -// resolves to the same server: the store matches names exactly, so a loose match leaves as the id. +// Resolve here so cross-host suggestions survive store errors that discard structured data. export async function resolveEnvironmentSelectorForStore( selector: string, listSshTargetsForSuggestion: () => Promise diff --git a/src/cli/flag-help-text.ts b/src/cli/flag-help-text.ts index ae5745998ff..cefbfe52e36 100644 --- a/src/cli/flag-help-text.ts +++ b/src/cli/flag-help-text.ts @@ -1,4 +1,3 @@ -/** One-line flag descriptions shared by every command's help output. */ export const FLAG_HELP_TEXT: Record = { 'external-worktree-visibility': '--external-worktree-visibility show|hide|inherit Override or inherit non-Orca worktree visibility', @@ -113,7 +112,6 @@ export const FLAG_HELP_TEXT: Record = { 'value-stdin': '--value-stdin Read set-value payload from stdin', 'window-id': '--window-id Target a window id from list-windows', 'window-index': '--window-index Target a window index from list-windows', - // Browser automation flags element: '--element Element ref from snapshot (e.g. e3)', url: '--url URL to navigate to', value: '--value Value to fill or select', diff --git a/src/cli/flags.ts b/src/cli/flags.ts index 358217d3aa6..30fc0a72628 100644 --- a/src/cli/flags.ts +++ b/src/cli/flags.ts @@ -32,11 +32,7 @@ export function getOptionalStringFlag( return typeof value === 'string' && value.length > 0 ? value : undefined } -/** - * A valued flag whose value the shell (or a missing variable) ate parses as `true`. Dropping it - * silently mints a fresh mutation identity and can deliver a prompt twice (#15180), so every - * valued-flag accessor refuses the damaged shape by name. - */ +// A missing valued flag must fail: silently regenerating mutation IDs can submit prompts twice. export function rejectValuelessFlag(value: string | boolean | undefined, name: string): void { if (value === true) { throw new RuntimeClientError( @@ -46,11 +42,7 @@ export function rejectValuelessFlag(value: string | boolean | undefined, name: s } } -/** - * A JSON-valued flag, rejected up front when a native argv boundary stripped its quotes so the - * error names the shell instead of the user's value (#16706). The value itself is still parsed - * downstream; this only catches the damaged shape. - */ +/** Detect shell-stripped JSON quotes before downstream parsing so the error names the argv boundary. */ export function getOptionalJsonFlag( flags: Map, name: string diff --git a/src/cli/format-recovery.test.ts b/src/cli/format-recovery.test.ts index a448f0f074c..c78e62a9bcd 100644 --- a/src/cli/format-recovery.test.ts +++ b/src/cli/format-recovery.test.ts @@ -120,7 +120,7 @@ describe('worktree selector recovery', () => { error: { code: 'selector_not_found', message: 'selector_not_found', - // The mutation-recovery layer already attached its request id. + // Preserve the request ID already attached by mutation recovery. data: { orchestrationRequestId: 'req_abc', nextSteps: ['Run request-show first.'] } }, _meta: { runtimeId: 'runtime_local' } @@ -218,7 +218,7 @@ describe('CLI error recovery', () => { }) it('tells an older server to reprovision when its computer sidecar is missing', () => { - // An older orcad answers with the generic provider steps, which never name the server install. + // Older servers return generic steps without naming the server installation. const error = new RuntimeRpcFailureError({ id: 'req_sidecar_exit', ok: false, diff --git a/src/cli/format.ts b/src/cli/format.ts index 6cb79f3e985..f5ebdf11c40 100644 --- a/src/cli/format.ts +++ b/src/cli/format.ts @@ -89,8 +89,7 @@ export type HostListEntry = { connectionStatus?: string } -// Why: the selector column is the point of this command — the name alone is what callers already -// had, and passing it on the wrong axis is the mistake this output exists to prevent. +// Explicit host selectors prevent callers from using a display name on the wrong host scope. export function formatHostList(result: { hosts: HostListEntry[] }): string { const kindLabel: Record = { local: 'local',