diff --git a/src/cli/handler-group-manifest.test.ts b/src/cli/handler-group-manifest.test.ts index ec543c562a5..d5eb85caf13 100644 --- a/src/cli/handler-group-manifest.test.ts +++ b/src/cli/handler-group-manifest.test.ts @@ -5,16 +5,9 @@ import { describe, expect, it } from 'vitest' import { buildHandlerRoutes, dispatch, type HandlerContext } from './dispatch' import { HANDLER_GROUPS, type HandlerGroup } from './handler-group-manifest' -// Why: dispatch trusts the manifest's eager key lists to route without loading a -// group. These tests are the only thing standing between that trust and a -// silently unreachable command, so they load every group for real. - -// Why: __dirname works under both Vitest and the CommonJS tsc emit that -// build:cli type-checks this file against; import.meta.dirname does not. +// import.meta.dirname is unavailable in the CommonJS build:cli emit. const HANDLERS_DIR = join(__dirname, 'handlers') -// Why: both the plural records and the single-command `*_HANDLER` ones that -// nested modules export get spread into a group, so both must route. const HANDLER_RECORD_EXPORT = /_HANDLERS?$/ function listHandlerModules(dir: string): string[] { @@ -49,10 +42,7 @@ describe('handler group manifest', () => { expect(notCallable).toEqual([]) }) - // Why: dropping a group from the manifest silently unregisters its commands — - // walk the tree so a new or forgotten handler file fails here, not in prod. - // Nested modules are spread into a parent group rather than registered under - // their own name, so routability, not file name, is the invariant that holds. + // Nested modules are spread into parent groups; verify routability rather than group names. it('routes every command exported by a handler module', async () => { const routes = buildHandlerRoutes(HANDLER_GROUPS) const unroutable: string[] = [] diff --git a/src/cli/handler-group-manifest.ts b/src/cli/handler-group-manifest.ts index 88df2cf0694..f9455dfc56d 100644 --- a/src/cli/handler-group-manifest.ts +++ b/src/cli/handler-group-manifest.ts @@ -6,14 +6,12 @@ import { REFERENCE_HANDLER_GROUP } from './reference-handler-group' export type HandlerGroup = { name: string - // Why: eager string keys let dispatch build (and duplicate-check) the whole - // command table without loading any group's transitive module graph. + // Eager keys allow routing and duplicate checks without importing each handler graph. keys: readonly string[] load: () => Promise> } -// Why: `keys` mirrors each group's exported record and is verified against the -// real exports by handler-group-manifest.test.ts, so drift fails CI, not dispatch. +// handler-group-manifest.test.ts checks keys against the real exports to prevent drift. export const HANDLER_GROUPS: readonly HandlerGroup[] = [ JIRA_HANDLER_GROUP, LINEAR_HANDLER_GROUP, diff --git a/src/cli/help.ts b/src/cli/help.ts index b28103373cc..13c4ff5c46c 100644 --- a/src/cli/help.ts +++ b/src/cli/help.ts @@ -88,7 +88,6 @@ function formatCommandFlagHelp(flag: string, commandPath: string[]): string { if (command === 'terminal close' && flag === 'tab') { return '--tab Close the whole tab and wait for durable persistence' } - // Why: the shared --title help says "terminal tab" — correct for `rename`, wrong for one pane. if (command === 'terminal set-pane-title' && flag === 'title') { return '--title Custom title for one pane (pass "" to reset)' } @@ -182,8 +181,6 @@ function formatCommandFlagHelp(flag: string, commandPath: string[]): string { if (command === 'orchestration task-create' && flag === 'display-name') { return '--display-name UI label shown for dispatched worker rows' } - // Why: the shared --agent help describes launching a TUI agent in a terminal, - // which is the wrong meaning here — this selects the account provider. if (command === 'account add' && flag === 'agent') { return '--agent Account provider: claude, codex, opencode, or devin (default claude)' } diff --git a/src/cli/host-selector-alternatives.test.ts b/src/cli/host-selector-alternatives.test.ts index 9460931a083..e849a7b6938 100644 --- a/src/cli/host-selector-alternatives.test.ts +++ b/src/cli/host-selector-alternatives.test.ts @@ -20,8 +20,6 @@ function clientReturning(targets: { id: string; label: string }[]): RuntimeClien } describe('findSshTargetByName', () => { - // Why: ids are machine-generated `ssh--`, so the label is the only name a - // person or agent ever has. Matching ids alone reports "not found" for a target that exists. it('matches the human label as well as the generated id', () => { expect(findSshTargetByName(SSH_TARGETS, 'openclaw')?.id).toBe('ssh-1755000000000-a1b2c3') expect(findSshTargetByName(SSH_TARGETS, 'ssh-1755000000000-a1b2c3')?.label).toBe('openclaw') @@ -61,7 +59,6 @@ describe('crossKindNextSteps', () => { expect(crossKindNextSteps('nowhere', alternatives, 'ssh')).toEqual([]) }) - // Why: suggesting the axis the caller already used would be noise, not help. it('does not suggest the axis that was already requested', () => { expect(crossKindNextSteps('openclaw', alternatives, 'ssh')).toEqual([]) expect(crossKindNextSteps('awin', alternatives, 'environment')).toEqual([]) @@ -75,8 +72,6 @@ describe('resolveSshHostTargetId', () => { ).resolves.toBe('ssh-1755000000000-a1b2c3') }) - // Why: this used to answer ok:true with an empty list — the same silent wrong-machine result - // that unknown runtime ids gave before they were rejected. it('rejects an unknown target instead of letting it filter to nothing', async () => { await expect( resolveSshHostTargetId(clientReturning(SSH_TARGETS), 'nowhere', ENVIRONMENTS) @@ -103,8 +98,6 @@ describe('resolveSshHostTargetId', () => { }) describe('listSshTargets', () => { - // Why: an older host answers listTargets but not listTargetSummaries. Treating that as "no - // targets" would reject an ssh id that is valid on that host. it('falls back to the older listing when the newer method is absent', async () => { const { RuntimeClientError } = await import('./runtime/types.js') const call = vi.fn(async (method: string) => { @@ -136,8 +129,6 @@ describe('listSshTargets', () => { expect(call).toHaveBeenCalledWith('ssh.getState', { targetId: SSH_TARGETS[0].id }) }) - // Why: this only ever runs to enrich an error we are already reporting; a failure here must - // not replace that error with a confusing one about SSH enumeration. it('returns nothing rather than masking the error it was enriching', async () => { const client = { call: vi.fn(async () => { @@ -159,8 +150,6 @@ describe('ambiguous names never resolve silently', () => { { id: 'env-2', name: 'awin' } ] - // Why: picking the first would choose a machine on the caller's behalf — the exact failure the - // whole selector path exists to prevent. it('refuses to guess between two ssh targets sharing a label', () => { expect(findSshTargetByName(twoOpenclaw, 'openclaw')).toBeUndefined() expect(ambiguousSshTargets(twoOpenclaw, 'openclaw')).toHaveLength(2) @@ -171,7 +160,6 @@ describe('ambiguous names never resolve silently', () => { expect(ambiguousEnvironments(twoAwin, 'awin')).toHaveLength(2) }) - // An exact id is never ambiguous, even when labels collide. it('still resolves an exact id past a colliding label', () => { expect(findSshTargetByName(twoOpenclaw, 'ssh-2-b')?.id).toBe('ssh-2-b') expect(findEnvironmentByName(twoAwin, 'env-2')?.id).toBe('env-2') diff --git a/src/cli/host-selector-alternatives.ts b/src/cli/host-selector-alternatives.ts index f9e785c5b82..9e4db8d4508 100644 --- a/src/cli/host-selector-alternatives.ts +++ b/src/cli/host-selector-alternatives.ts @@ -10,7 +10,6 @@ export type SshTargetSummary = { } export type EnvironmentSummary = { id: string; name: string } -/** The name `orca host list` prints for its own `--host local` row. */ export const LOCAL_HOST_LIST_NAME = 'this machine' export type HostAlternatives = { @@ -18,9 +17,6 @@ export type HostAlternatives = { environments: readonly EnvironmentSummary[] } -// Why: SSH target ids are machine-generated (`ssh--`), so the name a person -// or an agent actually knows is the label. Looking a name up as an id alone reports "not found" -// for a target that is sitting right there. export function findSshTargetByName( targets: readonly SshTargetSummary[], name: string @@ -30,14 +26,9 @@ export function findSshTargetByName( return byId } const byLabel = matchesByLabel(targets, name) - // Why: two targets can share a label. Picking the first would silently choose a machine for - // the caller — the failure this whole selector path exists to prevent — so an ambiguous name - // resolves to nothing and the caller is told to use an id. return byLabel.length === 1 ? byLabel[0] : undefined } -// Why: cmd.exe passes single quotes through literally, so `--host 'my box'` from a POSIX habit -// arrives quoted; a host or project name never legitimately starts and ends with the same quote. export { normalizeTypedName, stripWrappingQuotes } function matchesByLabel(targets: readonly SshTargetSummary[], name: string): SshTargetSummary[] { @@ -62,8 +53,6 @@ export function findEnvironmentByName( return byId } const byName = matchesByEnvironmentName(environments, name) - // Why: the environment store itself refuses an ambiguous name rather than guessing; resolving - // one here would quietly reintroduce the guess it exists to prevent. return byName.length === 1 ? byName[0] : undefined } @@ -83,9 +72,6 @@ export function ambiguousEnvironments( return byName.length > 1 ? byName : [] } -// Why: a paired Orca server and an SSH target are different machines reached different ways, but -// a caller only knows "the machine called X". When X misses on one axis, the useful answer is -// almost always that it exists on the other — so say which, and give the exact flag. export function crossKindNextSteps( name: string, alternatives: HostAlternatives, @@ -107,11 +93,7 @@ export function crossKindNextSteps( return steps } -/** - * The host's SSH target summaries; throws what the host reports. Why the fallback: hosts - * predating listTargetSummaries still answer listTargets from the same summariser, and without it - * an old host looks like one with no SSH targets, rejecting a target id that is valid there. - */ +/** Older hosts expose summaries through listTargets; propagate errors other than method_not_found. */ export async function fetchSshTargetSummaries( client: RuntimeClient ): Promise<{ targets: T[]; legacy: boolean }> { @@ -127,8 +109,7 @@ export async function fetchSshTargetSummaries( } } -// Why: only display identity crosses this boundary — the RPC deliberately withholds addresses -// and credentials — and an enumeration failure must never mask the error we are explaining. +// Best-effort discovery avoids replacing selector errors with SSH enumeration failures. export async function listSshTargets(client: RuntimeClient): Promise { try { const { targets, legacy } = await fetchSshTargetSummaries(client) @@ -166,19 +147,11 @@ async function enrichLegacySshTargetStates( ) } -// Why: `--host ssh:` was never validated, so an unknown target answered ok:true with an -// empty list — the same silent wrong-machine answer that `runtime:` ids used to give. And since -// target ids are machine-generated (`ssh--`), the label a caller actually -// knows never matches one, so this fires on the common spelling rather than a rare typo. -// Resolving the label and naming the alternative is what makes the failure recoverable. -// `flagSpelling` names the flag the way the caller typed it, so a bare `--host trader-local` -// is not echoed back as `--host ssh:trader-local` — a spelling they did not use. export async function resolveSshHostTargetId( client: RuntimeClient, targetId: string, environments: readonly EnvironmentSummary[], flagSpelling = `--host ssh:${targetId}`, - // A bare name was also tried as this machine and as a paired server, so say "host". bareName = false ): Promise { const targets = await listSshTargets(client) diff --git a/src/cli/index-file-open-headless.test.ts b/src/cli/index-file-open-headless.test.ts index 909dffa0bba..c69d323aa09 100644 --- a/src/cli/index-file-open-headless.test.ts +++ b/src/cli/index-file-open-headless.test.ts @@ -15,7 +15,6 @@ import { main } from './index' import { okFixture } from './test-fixtures' import { RuntimeRpcFailureError } from './runtime/types' -// What a headless server (`orca serve`, orcad) answers today: the bare token, flattened (#29822). function headlessRefusal(): RuntimeRpcFailureError { return new RuntimeRpcFailureError({ id: 'req_open', diff --git a/src/cli/index-local-command-routing-flags.test.ts b/src/cli/index-local-command-routing-flags.test.ts index aecd984668f..adfbceb9794 100644 --- a/src/cli/index-local-command-routing-flags.test.ts +++ b/src/cli/index-local-command-routing-flags.test.ts @@ -50,7 +50,6 @@ import { pairRuntimeEnvironment, useWorktreeAwarenessEnvironment } from './index const SSH_TARGET = { id: 'ssh-1777360569033-yvz2mp', label: 'openclaw', remotePlatform: 'win32' } -/** Every SSH-target lookup answers with the one target only this machine's runtime knows about. */ function queueSshTargetLookups(count: number): void { queueFixtures( callMock, @@ -58,11 +57,7 @@ function queueSshTargetLookups(count: number): void { ) } -/** - * A runtime that answers `status.get` with the name it currently publishes: the stored override - * when one is set, else the detected name. `settings.update` replies with the real `{ settings }` - * envelope so a handler reading a bare `machineName` off it prints `undefined`. - */ +/** Mirror the settings.update envelope and publish the effective name through status.get. */ function fakeMachineNameRuntime(detectedName: string): { updates: unknown[] } { const updates: unknown[] = [] let override = '' @@ -114,7 +109,6 @@ describe('runtime-selector flags on locally pinned CLI commands', () => { expect( printed.result.hosts.find((host: { id: string }) => host.id === SSH_TARGET.id).platform ).toBe('win32') - // The tell: `runtimeId: local` is only honest if no routed client was ever built. expect(runtimeClientConstructorMock).toHaveBeenCalledWith(null, null) }) @@ -131,10 +125,8 @@ describe('runtime-selector flags on locally pinned CLI commands', () => { .slice(0, 3) .map((call) => JSON.parse(String(call[0]))) expect(first.result).toEqual({ machineName: 'm4airs-Air', platform: 'darwin' }) - // The write reply is `{ settings }`; the printed name must be what the runtime publishes now. expect(second.result).toEqual({ machineName: 'build-server', platform: 'darwin' }) expect(second._meta.runtimeId).toBe('runtime-1') - // A blank `--name` returns to the detected name — and prints it, not an empty string. expect(third.result.machineName).toBe('m4airs-Air') expect(logSpy.mock.calls[3]?.[0]).toBe('m4airs-Air (darwin)') expect(runtime.updates).toEqual([{ machineName: ' build-server ' }, { machineName: '' }]) @@ -156,8 +148,6 @@ describe('runtime-selector flags on locally pinned CLI commands', () => { expect(runtimeClientConstructorMock).toHaveBeenCalledWith(undefined, 'm4air') }) - // Why: the pre-check matches names loosely but the connection lookup is exact, so `build` for a - // server saved as `Build` passed the check and then failed with "Unknown environment: build". it('connects to the server an --environment name in another case resolved to', async () => { pairRuntimeEnvironment(listEnvironmentsMock, 'env-m4air', 'M4Air') fakeMachineNameRuntime('M4 Air') @@ -169,7 +159,6 @@ describe('runtime-selector flags on locally pinned CLI commands', () => { expect(runtimeClientConstructorMock).toHaveBeenCalledWith(undefined, 'env-m4air') }) - /** The selectors an agent's environment carries: set empty, which the CLI reads as unset. */ function agentSelectors(): void { process.env.ORCA_ENVIRONMENT = '' process.env.ORCA_PAIRING_CODE = '' @@ -203,8 +192,6 @@ describe('runtime-selector flags on locally pinned CLI commands', () => { }) it('routes `host name` through an ambient ORCA_ENVIRONMENT, unlike the pinned `host list`', async () => { - // Why: the pin used to cover the whole `host` family, which silently answered for this machine - // when the shell was pointed at another one. `host name` describes one runtime, so it routes. process.env.ORCA_ENVIRONMENT = 'm4air' pairRuntimeEnvironment(listEnvironmentsMock, 'env-m4air', 'm4air') fakeMachineNameRuntime('M4 Air') @@ -217,14 +204,12 @@ describe('runtime-selector flags on locally pinned CLI commands', () => { expect(printed.ok).toBe(true) expect(printed.result.machineName).toBe('M4 Air') expect(printed._meta.runtimeId).toBe('runtime-1') - // The ambient selector is left for the client to honour; a `null` here would pin it local. expect(runtimeClientConstructorMock).toHaveBeenCalledWith(undefined, undefined) expect(runtimeClientConstructorMock).not.toHaveBeenCalledWith(null, null) }) it('refuses to rename a runtime that does not publish a machine name', async () => { - // Why: an older runtime's strict settings schema answers the write with a bare `invalid_params`; - // the field's absence from status is the tell, so the CLI refuses before writing anything. + // Missing status support lets the CLI reject the rename before an older settings schema rejects it. pairRuntimeEnvironment(listEnvironmentsMock, 'env-m4air', 'm4air') callMock.mockImplementation(async (method: string) => { if (method === 'status.get') { @@ -263,9 +248,6 @@ describe('runtime-selector flags on locally pinned CLI commands', () => { }) it('rejects `host list --environment` instead of answering with a half-routed listing', async () => { - // Why: pre-fix this routed the SSH lookup to m4air while reading paired servers from this - // machine, dropped the openclaw row, and still stamped `_meta.runtimeId: "local"` — one - // listing describing two hosts, which reads as "m4air has no SSH targets". pairRuntimeEnvironment(listEnvironmentsMock, 'env-m4air', 'm4air') const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}) runtimeClientConstructorMock.mockClear() @@ -317,8 +299,6 @@ describe('runtime-selector flags on locally pinned CLI commands', () => { }) it('keeps `host list` local when ORCA_ENVIRONMENT is set ambiently', async () => { - // Why: the ambient variable produced the same two-machine listing as the explicit flag, with - // no flag to reject. Pinning the family is what makes `runtimeId: local` true in both cases. process.env.ORCA_ENVIRONMENT = 'm4air' pairRuntimeEnvironment(listEnvironmentsMock, 'env-m4air', 'm4air') queueSshTargetLookups(1) @@ -336,7 +316,6 @@ describe('runtime-selector flags on locally pinned CLI commands', () => { }) it('still treats --environment as the selector argument on `environment show` and `rm`', async () => { - // Why: the guard must not fire where the flag names the row to act on rather than a route. const environment = { id: 'env-m4air', name: 'm4air', diff --git a/src/cli/index-memory-diagnostics.test.ts b/src/cli/index-memory-diagnostics.test.ts index 9896c40b0aa..40c600c734c 100644 --- a/src/cli/index-memory-diagnostics.test.ts +++ b/src/cli/index-memory-diagnostics.test.ts @@ -113,7 +113,6 @@ describe('orca cli worktree awareness', () => { expect(output).toContain('hostAvailable: 2.0 MB (free-memory)') expect(output).toContain('app: 1.0 MB') expect(output).toContain('- feature 1.0 MB 2.5% 1 session') - // Back-compat: a host that never heard of committed bytes prints none. expect(output).not.toContain('totalPrivateMemory') expect(output).not.toContain('processCommitMetric') }) diff --git a/src/cli/index-omitted-host-scope-selectors.test.ts b/src/cli/index-omitted-host-scope-selectors.test.ts index 794fe327a82..69426ac9195 100644 --- a/src/cli/index-omitted-host-scope-selectors.test.ts +++ b/src/cli/index-omitted-host-scope-selectors.test.ts @@ -72,9 +72,6 @@ describe('omittedHostIds selector annotation', () => { }) it('marks a stale runtime host that no caller can select', async () => { - // Why: `omittedHostIds` is built from the runtime's own bookkeeping, so it names `runtime:` - // ids for servers that are no longer paired. An agent looping over the list to complete a - // partial listing hard-errors on those — 6 of 9 in the recorded QA run. pairRuntimeEnvironment(listEnvironmentsMock, 'env-paired', 'm4air') queueFixtures( callMock, @@ -133,8 +130,6 @@ describe('omittedHostIds selector annotation', () => { ] as const)( 'only suggests flags that `%s %s` accepts', async (group, action, requestId, rows) => { - // Why: `--host local` / `--host ssh:` were suggested here, and the listing rejected - // them as unknown flags, breaking the agent loop the annotation exists for (#29901). pairRuntimeEnvironment(listEnvironmentsMock, 'env-paired', 'm4air') queueFixtures( callMock, @@ -166,7 +161,7 @@ describe('omittedHostIds selector annotation', () => { expect(spec?.allowedFlags).toContain(selector.split(' ')[0].replace(/^--/, '')) } } - // An SSH host is reached through the answering runtime, so no target lookup is needed. + // SSH hosts are reached through the answering runtime, without another target lookup. expect(callMock).toHaveBeenCalledTimes(1) } ) @@ -192,8 +187,6 @@ describe('omittedHostIds selector annotation', () => { }) it('never keeps a host id out of omittedHostIds', async () => { - // Why: filtering the unreachable ones would shrink what the listing admits it did not cover. - // The gap is real whether or not this machine can name the host that owns it. listEnvironmentsMock.mockReturnValue([]) queueFixtures( callMock, diff --git a/src/cli/index-orchestration.test.ts b/src/cli/index-orchestration.test.ts index 3c0329f73bb..0668c321fb6 100644 --- a/src/cli/index-orchestration.test.ts +++ b/src/cli/index-orchestration.test.ts @@ -133,7 +133,6 @@ describe('orca cli worktree awareness', () => { }) it("refuses an agent session's caller flag naming another caller before any request", async () => { - // One chokepoint for every verb: the spec says which flag names the caller. process.env.ORCA_AGENT_SESSION_ID = 'f7a1c0de-1111-4222-8333-444455556666' const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}) try { @@ -213,7 +212,6 @@ describe('orca cli worktree awareness', () => { expect(callMock).not.toHaveBeenCalled() expect(process.exitCode).toBe(1) - // Reset exitCode so subsequent tests don't inherit the failure. process.exitCode = priorExitCode errSpy.mockRestore() }) diff --git a/src/cli/index-project-name-selectors.test.ts b/src/cli/index-project-name-selectors.test.ts index 9036cfb6b1d..b965877331c 100644 --- a/src/cli/index-project-name-selectors.test.ts +++ b/src/cli/index-project-name-selectors.test.ts @@ -54,8 +54,6 @@ const ORCA_PROJECT = { } const PROJECT_LIST_FIXTURE = okFixture('req_project_list', { projects: [ORCA_PROJECT] }) -// Why: #30367 — the names `orca project list` and `orca host list` print were rejected by the -// commands that take --project and --host; only the generated ids worked. describe('orca cli project and host names', () => { useWorktreeAwarenessEnvironment({ callMock, @@ -66,8 +64,6 @@ describe('orca cli project and host names', () => { spawnMock }) - // Why: the issue's own repro — the names `orca project list` and `orca host list` print were - // rejected, and only the generated ids worked. it('filters project setups by the display name and host name the list commands print', async () => { queueFixtures( callMock, @@ -162,8 +158,6 @@ describe('orca cli project and host names', () => { ) }) - // Why: a project whose only setup is not ready must not fall through to another project whose - // display name happens to equal the id that was typed. it('keeps an exact project id even when another project is named that id', async () => { queueFixtures( callMock, diff --git a/src/cli/index-project-setup.test.ts b/src/cli/index-project-setup.test.ts index 1bf8b8895c3..b94569c412f 100644 --- a/src/cli/index-project-setup.test.ts +++ b/src/cli/index-project-setup.test.ts @@ -202,8 +202,6 @@ describe('orca cli worktree awareness', () => { expect(logSpy.mock.calls[0]?.[0]).not.toContain('setup-by-client') }) - // Why: --host runtime: routes to a paired server, so an older one is reachable without the - // caller meaning to. A raw method_not_found reads as an Orca bug rather than a version gap. it('names the version gap when the server predates project host setup', async () => { pairRuntimeEnvironment(listEnvironmentsMock, 'old-server') const { RuntimeClientError } = await import('./runtime/types.js') @@ -231,11 +229,9 @@ describe('orca cli worktree awareness', () => { await main(['project', 'setups', '--host', 'runtime:not-a-real-env', '--json'], '/tmp/repo') - // The command itself never reached a runtime; only the suggestion lookup did. expect(callMock).not.toHaveBeenCalledWith('projectHostSetup.list') const printed = [...logSpy.mock.calls, ...errSpy.mock.calls].flat().join('\n') expect(printed).toContain('no paired Orca server is named or has id not-a-real-env') - // An agent reads the code and the retry candidates, not the prose. expect(JSON.parse(printed).error.code).toBe('invalid_argument') expect(JSON.parse(printed).error.data.knownEnvironments).toEqual([]) expect(process.exitCode).toBe(1) @@ -243,8 +239,6 @@ describe('orca cli worktree awareness', () => { process.exitCode = priorExitCode }) - // Why: ssh: was never validated, so an unknown target answered ok:true with an empty list — - // the same silent wrong-machine answer unknown runtime ids used to give. it('rejects an unknown ssh host instead of answering empty', async () => { queueFixtures(callMock, okFixture('req_ssh_targets', { targets: [] })) const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}) @@ -307,9 +301,6 @@ describe('orca cli worktree awareness', () => { expect(logSpy.mock.calls[0]?.[0]).toContain('setup-openclaw') }) - // Why: `runtime:` is a persisted token — it lands in ProjectHostSetup.hostId and is - // embedded in generated setup ids. Accepting a name is only safe because it is canonicalized - // to the id before anything downstream sees it; this pins that. it('never lets an environment name reach a persisted host id', async () => { pairRuntimeEnvironment(listEnvironmentsMock, 'env-uuid-1', 'awin') queueFixtures( @@ -495,8 +486,6 @@ describe('orca cli worktree awareness', () => { }) it('rejects SSH project setup relative paths, which name the client filesystem', async () => { - // A local CLI reaching an `ssh:*` host is still off-client: resolving `./orca` against the - // CLI cwd would register a path that exists on the wrong machine. const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}) const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) const priorExitCode = process.exitCode @@ -593,9 +582,6 @@ describe('orca cli worktree awareness', () => { } ) - // Why: STA-4792 defect 2. `--host runtime:` used to leave the client local, so a Windows - // destination fell into resolve(cwd, ...) and became a literal directory next to the caller. - // Routing makes the client remote, which is what sends the path through untouched. it('sends a windows destination to the routed host instead of joining it to the local cwd', async () => { pairRuntimeEnvironment(listEnvironmentsMock, 'awin') queueFixtures( @@ -794,7 +780,6 @@ describe('orca cli worktree awareness', () => { it.each([ { flags: [], force: false, hostId: undefined }, { flags: ['--force'], force: true, hostId: undefined }, - // Why: a project on two hosts has two setups with one id; --host names the twin. { flags: ['--host', 'local'], force: false, hostId: 'local' } ])('deletes project host setup metadata with $flags', async ({ flags, force, hostId }) => { queueFixtures( diff --git a/src/cli/index-serve-restore.test.ts b/src/cli/index-serve-restore.test.ts index c1caf174054..89cbe79d81e 100644 --- a/src/cli/index-serve-restore.test.ts +++ b/src/cli/index-serve-restore.test.ts @@ -73,7 +73,6 @@ describe('orca serve restore resume', () => { expect(serveOrcaAppMock).not.toHaveBeenCalled() expect(callMock).toHaveBeenCalledWith('runtime.automaticWork.resume') - // Why null: `serve` commands always target this machine, never an ambient paired host. expect(runtimeClientConstructorMock).toHaveBeenLastCalledWith(null, null) expect(logSpy.mock.calls[0]?.[0]).toContain('Automatic restores resumed') expect(logSpy.mock.calls[0]?.[0]).toContain('stopped unexpectedly') diff --git a/src/cli/index-terminal-list-host-scope.test.ts b/src/cli/index-terminal-list-host-scope.test.ts index 083e3b4f5fd..bbaa3249c3f 100644 --- a/src/cli/index-terminal-list-host-scope.test.ts +++ b/src/cli/index-terminal-list-host-scope.test.ts @@ -89,8 +89,7 @@ describe('orca terminal list host scope', () => { expect(printed.result.hostScope).toEqual({ hostIds: ['ssh:box-1'], omittedHostIds: ['local'], - // `terminal list` takes no --host, so no flag reaches the answering runtime's own hosts - // (#29901); see index-omitted-host-scope-selectors.test.ts. + // These hosts are reached through the answering runtime; listing takes no --host flag. omittedHostSelectors: [{ hostId: 'local', selector: null }] }) }) diff --git a/src/cli/index-test-harness.ts b/src/cli/index-test-harness.ts index bb8c2d61366..c0f1df88d9a 100644 --- a/src/cli/index-test-harness.ts +++ b/src/cli/index-test-harness.ts @@ -17,10 +17,7 @@ export type WorktreeAwarenessMocks = { } export async function createRuntimeClientModuleMock(mocks: RuntimeClientModuleMocks) { - // 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 the mock must preserve the production classes. const { RuntimeClientError, RuntimeRpcFailureError } = await import('./runtime/types.js') class RuntimeClient { @@ -83,7 +80,6 @@ export async function createChildProcessModuleMock(spawnMock: Mock) { } } -/** Registers a paired environment so `--host runtime:` routes there instead of being rejected. */ export function pairRuntimeEnvironment(listEnvironmentsMock: Mock, id: string, name = id): void { listEnvironmentsMock.mockReturnValue([ { @@ -108,7 +104,6 @@ export function pairRuntimeEnvironment(listEnvironmentsMock: Mock, id: string, n ]) } -/** Installs the env-var save/restore + mock-reset hooks shared by the CLI worktree-awareness suites. */ export function useWorktreeAwarenessEnvironment(mocks: WorktreeAwarenessMocks): void { const originalTerminalHandle = process.env.ORCA_TERMINAL_HANDLE const originalUserDataPath = process.env.ORCA_USER_DATA_PATH @@ -126,8 +121,7 @@ export function useWorktreeAwarenessEnvironment(mocks: WorktreeAwarenessMocks): delete process.env.ORCA_DEV_CLI_INVOCATION delete process.env.ORCA_WORKSPACE_ID delete process.env.ORCA_WORKTREE_ID - // Isolate the pane key so claude-teams tests that set it don't leak a - // senderPaneKey into later orchestration.send assertions. + // Prevent claude-teams tests from leaking senderPaneKey into later assertions. delete process.env.ORCA_PANE_KEY mocks.serveOrcaAppMock.mockReset() mocks.getDefaultUserDataPathMock.mockClear() diff --git a/src/cli/index-vm-recipe-doctor.test.ts b/src/cli/index-vm-recipe-doctor.test.ts index 1e8bd99e15f..f3c2c62d99a 100644 --- a/src/cli/index-vm-recipe-doctor.test.ts +++ b/src/cli/index-vm-recipe-doctor.test.ts @@ -251,7 +251,6 @@ describe('orca cli worktree awareness', () => { expect.objectContaining({ id: 'recipe.destroy.run', status: 'pass' }) ]) ) - // The transcript carries both stages so the agent can self-diagnose. expect(output.provisionTranscript?.provision.exitCode).toBe(0) expect(output.provisionTranscript?.destroy?.exitCode).toBe(0) const cleanupPayload = JSON.parse( @@ -288,7 +287,6 @@ describe('orca cli worktree awareness', () => { stdin: { write: vi.fn(), end: vi.fn() }, kill: vi.fn() }) - // create emits a non-JSON line to stdout + a real diagnostic to stderr, then exits 0 spawnMock.mockImplementationOnce(() => { process.nextTick(() => { startChild.stdout.emit('data', Buffer.from('Provisioning sandbox...\n')) @@ -330,7 +328,6 @@ describe('orca cli worktree awareness', () => { expect.objectContaining({ id: 'recipe.provision', status: 'fail' }) ]) ) - // The agent gets the full create output, not a 500-char tail. expect(output.provisionTranscript?.provision.stdout).toContain('Provisioning sandbox...') expect(output.provisionTranscript?.provision.stderr).toContain('missing scope') expect(output.provisionTranscript?.provision.parseError).toBeTruthy() diff --git a/src/cli/index-worktree-create-agent.test.ts b/src/cli/index-worktree-create-agent.test.ts index a556bf63491..36258a8091d 100644 --- a/src/cli/index-worktree-create-agent.test.ts +++ b/src/cli/index-worktree-create-agent.test.ts @@ -80,7 +80,6 @@ describe('orca cli worktree awareness', () => { comment: undefined, runHooks: true, activate: true, - // CLI activation targets the host desktop. navigation: 'host', parentWorktree: undefined, cwdParentWorktree: 'id:repo-1::/tmp/repo', diff --git a/src/cli/index-worktree-create-linear.test.ts b/src/cli/index-worktree-create-linear.test.ts index 3c36f9239bc..2a9c6d1b2b3 100644 --- a/src/cli/index-worktree-create-linear.test.ts +++ b/src/cli/index-worktree-create-linear.test.ts @@ -145,9 +145,7 @@ describe('orca cli worktree awareness', () => { linkedIssue: undefined, linkedLinearIssue: 'STA-335', linkedLinearIssueWorkspaceId: null, - // Why: a bare identifier carries no org, and the two scoping fields - // describe the issue being replaced. Inheriting a stale org key would - // pin the lookup to a workspace this issue does not live in. + // A bare identifier supplies no org; inheriting the replaced issue's org would misroute lookup. linkedLinearIssueOrganizationUrlKey: null, comment: undefined, runHooks: false, diff --git a/src/cli/index-worktree-create-repo-host.test.ts b/src/cli/index-worktree-create-repo-host.test.ts index 0fbcda05b9b..8cab190e8f4 100644 --- a/src/cli/index-worktree-create-repo-host.test.ts +++ b/src/cli/index-worktree-create-repo-host.test.ts @@ -56,7 +56,7 @@ function buildSetup(overrides: { id: string; hostId: string; repoId: string; pat } } -// Why one repo id on two setups: a project copied between hosts keeps its id (Codex review 6 #6). +// Copies on different hosts retain the same repo id. const TWIN_SETUPS = [ buildSetup({ id: 'setup-1', hostId: 'local', repoId: 'repo-1', path: '/tmp/orca' }), buildSetup({ id: 'setup-1', hostId: 'runtime:gpu', repoId: 'repo-1', path: '/srv/orca' }) diff --git a/src/cli/index-worktree-create-target.test.ts b/src/cli/index-worktree-create-target.test.ts index 9752a29defd..638a39f2a63 100644 --- a/src/cli/index-worktree-create-target.test.ts +++ b/src/cli/index-worktree-create-target.test.ts @@ -233,9 +233,6 @@ describe('orca cli worktree awareness', () => { ) }) - // Why: swallowing every project.list failure made a network blip or a permission error - // indistinguishable from "no project by that name", and the caller was then told their project - // was not set up on any host. it('surfaces a project.list failure instead of reporting the project as not set up', async () => { const { RuntimeClientError } = await import('./runtime/types.js') const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}) @@ -265,8 +262,6 @@ describe('orca cli worktree awareness', () => { process.exitCode = priorExitCode }) - // Why: the legacy fallback matches a repo folder name, and two projects can hold repos with the - // same folder name. Selecting either would target a project the caller never named. it('refuses a legacy display-name match that spans two projects', async () => { const { RuntimeClientError } = await import('./runtime/types.js') const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}) @@ -312,7 +307,6 @@ describe('orca cli worktree awareness', () => { process.exitCode = priorExitCode }) - // Only a version gap earns the weaker match against the setup's own display name. it('falls back to the setup display name when the server has no project.list', async () => { const { RuntimeClientError } = await import('./runtime/types.js') queueFixtures( @@ -354,9 +348,6 @@ describe('orca cli worktree awareness', () => { ) }) - // Why: the reported failure. `orca project list` shows the display name, so that is what gets - // typed; matching the provider-scoped projectId alone reported "not set up" for a project that - // was sitting right there, and blamed the missing --host for it. it('resolves --project by display name and infers the only host it is set up on', async () => { queueFixtures( callMock, @@ -397,7 +388,6 @@ describe('orca cli worktree awareness', () => { ) }) - // Why: a project that matched nothing used to be reported as a host problem. it('reports an unknown project as a project miss, not a host one', async () => { queueFixtures( callMock, @@ -420,8 +410,6 @@ describe('orca cli worktree awareness', () => { process.exitCode = priorExitCode }) - // Why: one host is not a choice. Two are, and picking one would create the worktree on a - // machine the caller never named. it('asks which host only when the project is set up on more than one', async () => { const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}) const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) @@ -470,8 +458,6 @@ describe('orca cli worktree awareness', () => { process.exitCode = priorExitCode }) - // Why: `orca host list` prints `trader-local`, so that is what gets typed. Only the generated - // `ssh:ssh--` spelling used to be accepted. it('accepts the host alias that `orca host list` prints', async () => { queueFixtures( callMock, @@ -589,8 +575,6 @@ describe('orca cli worktree awareness', () => { }) it('marks every worktree.create as CLI-created even from an external shell', async () => { - // Why: the sidebar badge/filter must catch hand-typed creates too, so the - // provenance request is sent with no terminal handle rather than omitted. delete process.env.ORCA_TERMINAL_HANDLE queueFixtures( callMock, diff --git a/src/cli/index-worktree-selector-resolution.test.ts b/src/cli/index-worktree-selector-resolution.test.ts index c3f3d7dbd60..a38823be94a 100644 --- a/src/cli/index-worktree-selector-resolution.test.ts +++ b/src/cli/index-worktree-selector-resolution.test.ts @@ -100,9 +100,7 @@ describe('orca cli worktree awareness', () => { }) it('resolves the invocation cwd from ORCA_CLI_CWD when no cwd is passed', async () => { - // Why: the SSH relay bridge runs the CLI on the Orca host with the remote - // shell's cwd carried in ORCA_CLI_CWD (#7716); cwd-based selectors must - // resolve against it, not the host process cwd. + // ORCA_CLI_CWD carries the remote shell's cwd when the bridge executes the CLI on the host. process.env.ORCA_CLI_CWD = '/tmp/repo/feature/src' try { queueFixtures( diff --git a/src/cli/index-worktree-set.test.ts b/src/cli/index-worktree-set.test.ts index 167fb27a905..36ed857f87f 100644 --- a/src/cli/index-worktree-set.test.ts +++ b/src/cli/index-worktree-set.test.ts @@ -449,7 +449,7 @@ describe('orca cli worktree awareness', () => { const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) const priorExitCode = process.exitCode - // `active` resolves through worktree.list, so any RPC here means validation ran too late. + // Resolving active makes an RPC, so no calls proves validation ran first. await main(['worktree', 'set', '--worktree', 'active', ...flags, '--json'], '/tmp/repo') expect(callMock).not.toHaveBeenCalled() diff --git a/src/cli/index.test.ts b/src/cli/index.test.ts index 1687cc215ad..97e0f4c5ac8 100644 --- a/src/cli/index.test.ts +++ b/src/cli/index.test.ts @@ -46,7 +46,7 @@ import { okFixture, queueFixtures } from './test-fixtures' describe('COMMAND_SPECS collision check', () => { it('has no duplicate command or alias paths', () => { - // Why: first-match resolution would silently shadow duplicate aliases. + // First-match resolution would silently shadow duplicate aliases. const seen = new Set() for (const spec of COMMAND_SPECS) { for (const path of specPaths(spec)) { @@ -82,8 +82,7 @@ describe('command aliases dispatch to the canonical handler', () => { afterEach(() => { callMock.mockReset() - // Why: restore console.log so a downstream describe's vi.spyOn starts from a - // clean spy — otherwise this block's --json output leaks into its calls[0]. + // Restore the spy so later describe blocks cannot inherit these calls. logSpy.mockRestore() }) @@ -148,8 +147,6 @@ describe('command aliases dispatch to the canonical handler', () => { } }) - // #19334: a failed archive hook blocks removal, so the CLI must exit non-zero rather than - // report a delete that did not happen — and the waiver must ride its own flag, never --force. it('exits non-zero when worktree removal is refused by a failed archive hook', async () => { queueFixtures(callMock, okFixture('req_show', { worktree: { hostId: 'local' } })) callMock.mockRejectedValueOnce( @@ -179,7 +176,6 @@ describe('command aliases dispatch to the canonical handler', () => { } }) - // #19334 S4: the waiver only applies to a hook that ran, so alone it silently does nothing. it('rejects the archive-hook waiver without --run-hooks instead of ignoring it', async () => { queueFixtures(callMock, okFixture('req_show', { worktree: { hostId: 'local' } })) const priorExitCode = process.exitCode @@ -191,7 +187,6 @@ describe('command aliases dispatch to the canonical handler', () => { ) expect(process.exitCode).toBe(1) - // The removal must never have been attempted. expect(callMock).not.toHaveBeenCalledWith('worktree.rm', expect.anything()) } finally { process.exitCode = priorExitCode @@ -240,7 +235,6 @@ describe('command aliases dispatch to the canonical handler', () => { runtimeClientConstructorMock.mockClear() await main(['agent-context', '--json'], '/tmp/repo') - // Why: pure local read — proves the SSH/offline property (no RPC). expect(runtimeClientConstructorMock).not.toHaveBeenCalled() expect(callMock).not.toHaveBeenCalled() const schema = JSON.parse(String(logSpy.mock.calls.at(-1)?.[0])) diff --git a/src/cli/index.ts b/src/cli/index.ts index 663a29b3fc9..93251af013e 100644 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -32,9 +32,7 @@ function shouldIgnoreRemoteSelection(commandPath: string[]): boolean { commandPath[0] === 'account' || commandPath[0] === 'artifacts' || commandPath[0] === 'environment' || - // Why: `host list` answers "what can this machine target, and with what flag". Half of that - // answer (paired servers) is read from this machine's own pairing store and cannot be routed, - // so routing the other half produced one listing describing two machines at once. + // Paired servers come from the local store; routing SSH lookup would mix two machines in one list. commandPath.join(' ') === 'host list' || commandPath[0] === 'serve' || (commandPath[0] === 'agent' && commandPath[1] !== 'launch') || @@ -44,19 +42,12 @@ function shouldIgnoreRemoteSelection(commandPath: string[]): boolean { ) } -// Why: the RuntimeClient graph is 153 of the CLI's 199 eager modules (zod via -// shared/pairing, ws + tweetnacl via websocket-transport). Loading it here -// rather than at module scope means --help, `help `, and command/flag -// errors — which all return before this call — never pay for it. Awaited -// before dispatch so `ctx.client` stays a synchronous getter. +// Defer the runtime graph until after help and validation; await it to keep ctx.client synchronous. async function loadRuntimeClientClass(): Promise { return (await import('./runtime-client.js')).RuntimeClient } -// Why: the SSH relay bridge executes this CLI on the Orca host while the -// caller's shell cwd lives on the remote machine (which cannot be chdir'd -// into). ORCA_CLI_CWD carries that remote cwd so cwd-based selectors like -// `--worktree active` resolve against the caller's directory. +// The SSH bridge runs here, but ORCA_CLI_CWD carries the remote caller's directory. function resolveInvocationCwd(): string { const override = process.env.ORCA_CLI_CWD return typeof override === 'string' && override.length > 0 ? override : process.cwd() @@ -108,9 +99,7 @@ export async function main( const json = parsed.flags.has('json') try { - // Why: CLI syntax and flag errors should be reported before any runtime - // lookup so users do not get misleading "Orca is not running" failures for - // simple command typos or unsupported flags. + // Report invalid commands before runtime lookup can mask them with connection errors. validateCommandAndFlags(COMMAND_SPECS, parsed) const commandSpec = findCommandSpec(COMMAND_SPECS, parsed.commandPath) refuseConflictingSessionCallerFlags(commandSpec, parsed.flags) @@ -118,27 +107,18 @@ export async function main( const ignoreRemoteSelection = shouldIgnoreRemoteSelection(parsed.commandPath) const pairingCode = ignoreRemoteSelection ? null : parsed.flags.get('pairing-code') const environmentSelector = ignoreRemoteSelection ? null : parsed.flags.get('environment') - // Why: only the explicit flag is asserted eagerly. An ambient ORCA_ENVIRONMENT is background - // config, and failing local-only commands because of a stale one would be a regression; the - // explicit flag means the caller named that machine, so a bad name should fail immediately - // with the cross-kind hint rather than a bare store error at first use. + // Eagerly resolve explicit --environment; stale ambient config must not break local-only commands. const listSshTargetsForSuggestion = async (): Promise<{ id: string; label: string }[]> => listSshTargets(new RuntimeClientClass(undefined, undefined, null, null)) const storeEnvironmentSelector = typeof environmentSelector === 'string' ? await resolveEnvironmentSelectorForStore(environmentSelector, listSshTargetsForSuggestion) : null - // Why: --host runtime: names a paired server, not a filter over this - // runtime's rows, so it has to pick the connection before the client exists. - // An ambient ORCA_ENVIRONMENT is checked for disagreement too — silently - // retargeting a mutation to another server is the bug this flag already had. - // An ambient pairing code cannot be resolved to an id to compare, so the - // explicit flag simply wins there. + // Select the paired connection before constructing the client; reject conflicting environments. const hostEnvironmentId = ignoreRemoteSelection ? null : await resolveHostFlagEnvironmentId(parsed.flags, { - // Why: only consulted when the name missed, and against this machine's own runtime — - // SSH targets are registered there, not in the paired server we failed to find. + // SSH targets belong to this machine, even when the missing name was a paired server. listSshTargets: listSshTargetsForSuggestion, verifySshTargets: async () => ( @@ -154,18 +134,13 @@ export async function main( ? { value: process.env.ORCA_ENVIRONMENT, label: 'ORCA_ENVIRONMENT' } : null }) - // Why: --host runtime: is canonicalized to the environment's id so downstream host-id - // comparisons against stored rows still match; rewrite the flag once, here, rather than - // resolving the name again at every consumer. + // Stored host identities use ids, so canonicalize names before any downstream comparison. if (hostEnvironmentId !== null) { parsed.flags.set('host', `runtime:${hostEnvironmentId}`) } - // Why: pass `null` (not `undefined`) when remote selection is suppressed - // so the RuntimeClient default parameter does not re-activate the - // ORCA_PAIRING_CODE / ORCA_ENVIRONMENT env-var fallback for commands - // that must run locally (environment / serve). + // null suppresses RuntimeClient's ORCA_PAIRING_CODE / ORCA_ENVIRONMENT default parameters. const suppressed = ignoreRemoteSelection ? null : undefined - // An explicit --host runtime: outranks an ambient pairing code or environment. + // An explicit runtime host overrides an ambient pairing code. const remotePairingCode = hostEnvironmentId !== null ? null : typeof pairingCode === 'string' ? pairingCode : suppressed const remoteEnvironment = hostEnvironmentId ?? storeEnvironmentSelector ?? suppressed @@ -208,8 +183,7 @@ export async function main( async function runClaudeTeams(argv: string[], cwd: string): Promise { try { - // Why: everything after `orca claude-teams` belongs to Claude Code, not - // Orca's own flag parser, so new Claude flags work without Orca changes. + // Forward Claude Code arguments verbatim so new flags work without Orca changes. const client = new (await loadRuntimeClientClass())(undefined, undefined, null, null) await dispatch(['claude-teams'], { flags: new Map(), diff --git a/src/cli/jira-format.ts b/src/cli/jira-format.ts index 4a300ed14da..0f2f7dced82 100644 --- a/src/cli/jira-format.ts +++ b/src/cli/jira-format.ts @@ -99,7 +99,6 @@ export function formatJiraCommentList(comments: JiraComment[]): string { .join('\n\n') } -// Why: the handler throws on ok:false, so only successful creates reach here. export function formatJiraCreate(result: Extract): string { return [`Created ${result.key}`, `URL: ${result.url}`].join('\n') } diff --git a/src/cli/omitted-host-scope-selectors.ts b/src/cli/omitted-host-scope-selectors.ts index 965eb11ee72..3c5cb89cea0 100644 --- a/src/cli/omitted-host-scope-selectors.ts +++ b/src/cli/omitted-host-scope-selectors.ts @@ -8,11 +8,11 @@ import { findEnvironmentByName } from './host-selector-alternatives' export type OmittedHostScopeSelector = { hostId: ExecutionHostId - /** The flag that routes a follow-up query to this host, or null when it names nothing here. */ + /** null means this machine has no selector for the omitted host. */ selector: string | null } -/** A host scope annotated on this machine. The runtime never sends `omittedHostSelectors`. */ +/** omittedHostSelectors is added locally, never sent by the runtime. */ export type ListingHostScopeWithSelectors = RuntimeListingHostScope & { omittedHostSelectors?: OmittedHostScopeSelector[] } @@ -21,23 +21,7 @@ export type WithAnnotatedHostScope = Omit & { hostScope?: ListingHostScopeWithSelectors } -/** - * Resolves how to reach each host a listing did not cover. - * - * `hostScope` is the documented way to complete a partial listing, but `omittedHostIds` is built - * from the runtime's own bookkeeping — repos, folder workspaces, and workspace sessions — so it - * names `runtime:` ids for servers that are no longer paired. An agent looping over the list to - * finish the job hard-errors on those. - * - * The ids are kept rather than filtered: dropping one would shrink what the listing admits it did - * not cover, and `docs/reference/ssh-execution-boundary.md` requires a listing to name its gaps. - * A `null` selector marks the ones this machine cannot name, which is the part a caller needs. - * Only the local pairing store is consulted, so this answers "can I select it", never "is it up" — - * no host is claimed live or exited on this path. - * - * `local` and `ssh:` hosts get no selector: the listing commands take no `--host` (#29901), and - * the runtime that answered is the one that reaches them, so no flag would route elsewhere. - */ +// Preserve unreachable hosts as coverage gaps; pairing proves selectability, not liveness. export async function resolveOmittedHostScopeSelectors( omittedHostIds: readonly ExecutionHostId[] ): Promise { @@ -73,10 +57,11 @@ function resolveSelector( const environment = findEnvironmentByName(environments, host.environmentId) return environment ? `--environment ${environment.name}` : null } + // Listings take no --host flag; local and SSH hosts are reached through the answering runtime. return null } -/** Renders a scope line; an absent scope means the host never reported one, not full coverage. */ +/** An absent scope means coverage is unknown. */ export function formatListingHostScope(scope: ListingHostScopeWithSelectors | undefined): string { if (!scope) { return 'scope: unverifiable — this host does not report which hosts it lists' @@ -104,7 +89,6 @@ export function formatListingHostScope(scope: ListingHostScopeWithSelectors | un return `scope: ${covered} — not covered: ${omitted.join(', ')}` } -/** Attaches the resolved selectors in place; a listing with no omitted hosts pays nothing. */ export async function annotateOmittedHostScope(result: { hostScope?: ListingHostScopeWithSelectors }): Promise { diff --git a/src/cli/orchestration-dispatch-refusal-format.test.ts b/src/cli/orchestration-dispatch-refusal-format.test.ts index 0a4461f25f1..3b6fd63a36d 100644 --- a/src/cli/orchestration-dispatch-refusal-format.test.ts +++ b/src/cli/orchestration-dispatch-refusal-format.test.ts @@ -12,9 +12,7 @@ afterEach(() => { vi.restoreAllMocks() }) -// Why: these are the exact envelopes the RPC dispatcher test proved the runtime emits. This -// checkout's formatter never enumerates codes (verified below with a code no build has defined), -// which is what lets a client that predates a new code still print its message and nextSteps. +// Runtime envelopes may contain codes introduced after this client; formatting must preserve them. describe('orchestration dispatch refusals through the CLI error boundary', () => { it.each([ { @@ -49,8 +47,6 @@ describe('orchestration dispatch refusals through the CLI error boundary', () => }) }) -// Why: a code this build has never defined stands in for a future host's new code; if the -// formatter ever starts gating on known codes, this is the assertion that catches it. it('prints an unknown code with its message and nextSteps unchanged', () => { const failure: RuntimeRpcFailure = { id: 'rpc_1', diff --git a/src/cli/orchestration-mutation-recovery.test.ts b/src/cli/orchestration-mutation-recovery.test.ts index 92de56f5698..505df0a0746 100644 --- a/src/cli/orchestration-mutation-recovery.test.ts +++ b/src/cli/orchestration-mutation-recovery.test.ts @@ -315,7 +315,6 @@ describe('orchestration mutation recovery', () => { expect((result.data as { recovery: object }).recovery).not.toHaveProperty('retryCommand') }) - // Pins the outcome-unknown codes (round-2 S5): only these get recovery guidance. it.each([ ['runtime_unavailable', true], ['remote_runtime_unavailable', true], diff --git a/src/cli/orchestration-mutation-recovery.ts b/src/cli/orchestration-mutation-recovery.ts index d5d80326c49..f79d2094cd0 100644 --- a/src/cli/orchestration-mutation-recovery.ts +++ b/src/cli/orchestration-mutation-recovery.ts @@ -29,8 +29,7 @@ export function orchestrationMutationRecoveryError(error: unknown): unknown { ? [...originalCommand, '--retry-request', requestId] : undefined const executable = originalCommand?.[0] ?? resolveOrchestrationCliExecutable() - // Why: a lost response usually carries no dispatch id, and the old code then emitted - // no read-only step at all — leaving retry as the only way to learn what happened. + // Without a dispatch id, query by request id to inspect the outcome before retrying a mutation. const queryCommand = dispatchId ? [executable, 'orchestration', 'worker-show', '--dispatch', dispatchId, '--json'] : [executable, 'orchestration', 'request-show', '--request', requestId, '--json'] @@ -65,8 +64,7 @@ export function orchestrationMutationRecoveryError(error: unknown): unknown { recovery, nextSteps } - // Preserve the RPC failure envelope so --json callers retain the request id and - // runtime metadata while receiving the structured recovery guidance. + // Preserve request id and runtime metadata for --json recovery. if (error instanceof RuntimeRpcFailureError) { return new RuntimeRpcFailureError({ ...error.response, @@ -195,8 +193,7 @@ function resolveRecoveryShell( function quoteRecoveryArgument(value: string, shell: AgentStartupShell): string { if (shell === 'cmd') { - // cmd has no single-quote syntax and expands %VAR% inside double quotes; - // use the argv encoder that keeps quote parity and escapes percent pairs. + // cmd expands %VAR% inside double quotes; its argv encoder must escape percent pairs too. return quoteWindowsCmdArgument(value) } if (shell === 'powershell') { diff --git a/src/cli/orchestration-session-caller-cli.test.ts b/src/cli/orchestration-session-caller-cli.test.ts index 035108604f2..1fbcaf48205 100644 --- a/src/cli/orchestration-session-caller-cli.test.ts +++ b/src/cli/orchestration-session-caller-cli.test.ts @@ -1,16 +1,4 @@ -/** - * A command that runs inside a structured agent session is that session: the injected - * `ORCA_AGENT_SESSION_ID` names the caller, and nothing resolves or guesses a terminal for it. - * - * One rule for every verb that names a caller: a caller flag may restate the session, but a flag - * naming anyone else is refused — never silently dropped, never allowed to win. A session address - * the CLI cannot place is left to the host, whose `/clear` lineage decides. - * The #21097 accident was a chat that named a sibling's terminal and consumed that sibling's mail. - * - * The session env here is the hardest case, a chat that inherited a pane's `ORCA_TERMINAL_HANDLE` - * and `ORCA_PANE_KEY` (an Orca launched from an Orca terminal), and the implicit-terminal guess has - * a sibling to find. - */ +// Inherited pane identity and an available sibling must never override the injected session caller. import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' @@ -30,7 +18,7 @@ import { formatCliError, reportCliError } from './cli-error' import { RuntimeRpcFailureError } from './runtime/types' const SESSION = 'f7a1c0de-1111-4222-8333-444455556666' -/** The session a `/clear` continued as SESSION: the chat's orchestration address stays this one's. */ +/** SESSION continues ROOT after /clear, retaining ROOT's orchestration address. */ const ROOT = '0b5e2d7c-9a41-4c3e-8f62-7d1a3e5b9c08' const IDENTITY_ENV = [ 'ORCA_AGENT_SESSION_ID', @@ -40,7 +28,6 @@ const IDENTITY_ENV = [ ] as const const originalEnv = Object.fromEntries(IDENTITY_ENV.map((name) => [name, process.env[name]])) -/** Enough of every receipt shape that each handler finishes after its RPC. */ const RESULT = { result: { run: { id: 'run_1', objective: 'o', consumer_generation: 1 }, @@ -73,14 +60,12 @@ const RESULT = { type Verb = { command: string flags: Record - /** The flag that names the caller, when the verb has one. */ callerFlag?: 'from' | 'terminal' method: string callerParam: 'from' | 'terminal' | 'callerTerminalHandle' } -/** Every verb whose request names its caller: the host's caller-param map, from the CLI side. - * `dispatch-show` is not one: its --from only fills preview text, so it is pinned on its own. */ +/** dispatch-show is excluded: its --from fills preview text rather than naming the caller. */ const CALLER_VERBS: Verb[] = [ { command: 'run-create', @@ -175,11 +160,9 @@ const CALLER_VERBS: Verb[] = [ method: 'workerStart', callerParam: 'from' }, - // No caller flag: its spec takes none. It asks runCurrent for the caller's Run. { command: 'worker-list', flags: {}, method: 'runCurrent', callerParam: 'from' } ] -/** Enough flags for any verb to get past its own validation to identity resolution. */ const EVERY_REQUIRED_FLAG = { objective: 'o', id: 'id_1', @@ -199,7 +182,6 @@ function flagMap(flags: Record): Map, @@ -243,7 +225,6 @@ function setEnv(env: Partial>): vo } } -/** A chat with its id, plus a pane identity it inherited from the Orca that launched it. */ function asSessionWithInheritedPane(): void { setEnv({ ORCA_AGENT_SESSION_ID: SESSION, @@ -274,7 +255,6 @@ describe.each(CALLER_VERBS)('orchestration $command run as an agent session', (v const [params] = callsTo(verb.method) expect(params, 'the verb reached its method').toBeDefined() expect(params?.[verb.callerParam]).toBeUndefined() - // An inherited pane is not the session's identity. expect(params?.terminalPaneKey).toBeUndefined() expect(params?.senderPaneKey).toBeUndefined() expect(getTerminalHandleMock).not.toHaveBeenCalled() @@ -325,7 +305,6 @@ describe.each(CALLER_VERBS.filter((verb) => verb.callerFlag !== undefined))( beforeEach(asSessionWithInheritedPane) it('sends the address it had before the clear for the host to place, instead of refusing it', async () => { - // The chat's address is its lineage root's, which only the host's session records know. const flags = flagMap({ ...verb.flags, [verb.callerFlag ?? 'from']: `orca_session_id:${ROOT}` @@ -367,8 +346,7 @@ describe.each([ it.each([`orca_session_id:${ROOT}`, 'orca_session_id:9d4c1b2a-3e5f-4a6b-8c7d-0e1f2a3b4c5d'])( 'sends a session address it cannot place (%s) for the host to check, never dropping it', async (declared) => { - // A /clear-ed chat's root is accepted by the host; anyone else is refused there, before any - // effect. Dropped here, a conflicting caller would pass unchecked. + // Forward session addresses for host lineage checks; dropping them would bypass caller fencing. await invoke(command, flagMap({ run: 'run_1', from: declared })) expect(callsTo(method)[0]).toMatchObject({ run: 'run_1', [callerParam]: declared }) } @@ -445,7 +423,6 @@ describe('the identity a session presents', () => { } asSessionWithInheritedPane() expect(await preview({})).toBe(`orca_session_id:${SESSION}`) - // Not a caller flag: it names the text to preview, so it is never fenced. expect(await preview({ from: 'term_sibling' })).toBe('term_sibling') setEnv({ ORCA_AGENT_SESSION_ID: SESSION, ORCA_TERMINAL_HANDLE: 'structworker_self' }) expect(await preview({})).toBe('structworker_self') @@ -528,9 +505,7 @@ describe('the orchestration envelope', () => { }) it('binds whichever current Orca CLI the agent reached, not only the one the session names', () => { - // A login shell can put another install's `orca` first, and the packaged Windows launcher - // rewrites ORCA_CLI_COMMAND in its own process. Neither matters: the id rides the envelope - // and the pinned instance is the one dialed. + // Another CLI install may rewrite ORCA_CLI_COMMAND; the envelope must still pin the session instance. vi.stubEnv('ORCA_USER_DATA_PATH', '/data/session-orca') try { const envelope = createOrchestrationCompatibilityEnvelope({ @@ -573,8 +548,6 @@ describe('which flag names the caller, declared on every spec', () => { const ORCHESTRATION_SPECS = COMMAND_SPECS.filter((spec) => spec.path[0] === 'orchestration') it('classifies every --from and --terminal an orchestration verb accepts', () => { - // A new verb cannot take either flag without saying whether it names the caller, so the entry - // check covers it by construction instead of each handler remembering to refuse. const unclassified = ORCHESTRATION_SPECS.flatMap((spec) => (['from', 'terminal'] as const) .filter((flag) => spec.allowedFlags.includes(flag) && !spec.identityFlagRoles?.[flag]) @@ -625,7 +598,6 @@ describe('which flag names the caller, declared on every spec', () => { }) describe('every orchestration verb, enumerated', () => { - /** Runs every verb once; returns the ones that guessed an implicit terminal. */ async function verbsThatGuess(): Promise { const guessed: string[] = [] for (const command of Object.keys(ORCHESTRATION_HANDLERS)) { @@ -641,8 +613,7 @@ describe('every orchestration verb, enumerated', () => { } it('guesses a terminal for no verb when the session id is present', async () => { - // Positive control first: with no identity at all the same harness sees the guess, so an empty - // result below is about the id, not a harness that cannot observe a guess. + // Prove the harness observes guesses without an injected identity before testing their absence. setEnv({}) const population = await verbsThatGuess() expect(population).toEqual([ diff --git a/src/cli/orchestration-structured-sender-identity.test.ts b/src/cli/orchestration-structured-sender-identity.test.ts index 9b85125d99e..7d44ed8514f 100644 --- a/src/cli/orchestration-structured-sender-identity.test.ts +++ b/src/cli/orchestration-structured-sender-identity.test.ts @@ -1,13 +1,3 @@ -/** - * The env-handle path, with NO `--from`. - * - * Every other orchestration CLI test passes `--from term_coord` explicitly, so the resolver a real - * worker actually goes through — `ORCA_TERMINAL_HANDLE` plus `validateEnvHandle` — was never - * exercised. That is why twelve coordinator verbs could fail for a structured worker while the - * whole suite stayed green, and why the worker's own preamble (which tells it to run these with no - * `--from`) failed on its first line. - */ - import { describe, expect, it, vi } from 'vitest' const { @@ -55,13 +45,7 @@ import { useWorktreeAwarenessEnvironment } from './index-test-harness' const STRUCTURED_HANDLE = 'structworker_a1b2c3d4-e5f6-4a7b-8c9d-0e1f2a3b4c5d' -/** - * Every command below is one of the twelve that resolve their sender through - * `resolveCoordinatorTerminalHandle`. All twelve share ONE resolver and one liveness probe, so the - * set is sufficient if it covers the distinct shapes that reach it: a read, a mutation, a - * run-scoped verb, a dispatch verb and a worker-start. A per-verb sweep would pin the argv parsing - * of twelve handlers and still tell us nothing more about the seam that actually broke. - */ +// Cover distinct handler shapes that share sender resolution and liveness checks. const SENDER_VERBS: { argv: string[]; method: string }[] = [ { argv: ['orchestration', 'run-current'], method: 'orchestration.runCurrent' }, { argv: ['orchestration', 'run-create', '--objective', 'x'], method: 'orchestration.runCreate' }, @@ -97,18 +81,13 @@ describe('a structured worker running orchestration commands as itself', () => { it.each(SENDER_VERBS)( 'resolves its own identity for $method with no --from', async ({ argv, method }) => { - // The defect this pins: the sender resolver validated the env handle with `terminal.show`, a - // PTY verb that misses for a structured worker and answers `terminal_handle_stale`. The pane - // remint that would have recovered it needs `ORCA_PANE_KEY`, which a structured child - // deliberately does not carry, so the command died on `no_active_sender_terminal`. process.env.ORCA_TERMINAL_HANDLE = STRUCTURED_HANDLE answerCalls() vi.spyOn(console, 'log').mockImplementation(() => {}) await expect(main(argv)).resolves.not.toThrow() const called = callMock.mock.calls.map((call) => call[0] as string) expect(called).toContain(method) - // Never through `terminal.show`: teaching that verb structured handles would hand every - // public terminal verb something that looks writable and is not. + // Structured workers lack writable PTYs; terminal.show must not validate them. expect(called).not.toContain('terminal.show') } ) @@ -123,8 +102,6 @@ describe('a structured worker running orchestration commands as itself', () => { }) it('still refuses a handle the runtime reports dead, with no pane key to remint from', async () => { - // The invariant the fix must not break: a stale `ORCA_TERMINAL_HANDLE` in a long-lived shell - // must keep failing rather than being baked into a coordinator preamble. process.env.ORCA_TERMINAL_HANDLE = 'term_stale' callMock.mockImplementation(async (method: string) => { if (method === 'terminal.resolveIdentity') { diff --git a/src/cli/orchestration-structured-session-no-identity.test.ts b/src/cli/orchestration-structured-session-no-identity.test.ts index d7c76f2b5e0..c5a46872187 100644 --- a/src/cli/orchestration-structured-session-no-identity.test.ts +++ b/src/cli/orchestration-structured-session-no-identity.test.ts @@ -1,14 +1,3 @@ -/** - * A structured chat session with NO orchestration identity must refuse, not guess. - * - * Non-worker structured sessions get no `ORCA_TERMINAL_HANDLE`, so `orchestration check` fell - * through to the active-terminal guess — and `check` is destructive by default, so it consumed - * another pane's oldest unread batch and marked it read. The rightful worker never saw that mail. - * - * The case pinned here is ONE terminal pane in the worktree, because that is the case - * `requireUnambiguous` misses: with a single candidate the guess still resolves, to a sibling. - */ - import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { ORCA_STRUCTURED_SESSION_ENV } from '../shared/structured-session-marker' @@ -38,8 +27,7 @@ describe('a structured chat session with no orchestration identity', () => { getTerminalHandleMock.mockReset() delete process.env.ORCA_TERMINAL_HANDLE process.env[ORCA_STRUCTURED_SESSION_ENV] = '1' - // Exactly ONE terminal pane in the worktree: the single-candidate case, where - // `requireUnambiguous` still resolves and would hand this session a sibling's handle. + // Even one candidate can belong to a sibling, so requireUnambiguous cannot protect this session. getTerminalHandleMock.mockResolvedValue('term_sibling') }) @@ -61,7 +49,6 @@ describe('a structured chat session with no orchestration identity', () => { code: 'no_active_sender_terminal', message: expect.stringContaining('--terminal') }) - // Neither guessed nor sent: a destructive read must not reach the runtime at all. expect(getTerminalHandleMock).not.toHaveBeenCalled() expect(callMock).not.toHaveBeenCalled() }) diff --git a/src/cli/paired-terminal-desktop-reveal.test.ts b/src/cli/paired-terminal-desktop-reveal.test.ts index 15ea4912e7a..a2c7d8f61f2 100644 --- a/src/cli/paired-terminal-desktop-reveal.test.ts +++ b/src/cli/paired-terminal-desktop-reveal.test.ts @@ -58,7 +58,7 @@ describe('revealPairedTerminalOnLocalDesktop', () => { expect(mocks.resolveEnvironment).toHaveBeenCalledWith('/profile', 'winpc') expect(serverCall).toHaveBeenCalledWith('terminal.show', { terminal: HANDLE }) - // Why null selectors: the reveal must reach this machine's runtime, not the environment again. + // null bypasses ambient remote selection so reveal reaches the local desktop. expect(mocks.desktopClient).toHaveBeenCalledWith('/profile', undefined, null, null) expect(mocks.desktopCall).toHaveBeenCalledWith('terminal.reveal', { worktreeId: shownTerminal.worktreeId, diff --git a/src/cli/paired-terminal-desktop-reveal.ts b/src/cli/paired-terminal-desktop-reveal.ts index 7a91d848388..2eb159b9f05 100644 --- a/src/cli/paired-terminal-desktop-reveal.ts +++ b/src/cli/paired-terminal-desktop-reveal.ts @@ -4,11 +4,7 @@ import type { RuntimeTerminalReveal } from '../shared/terminal-pane-reveal' import { RuntimeClient } from './runtime-client' import { resolveEnvironment } from './runtime/environments' -/** - * A paired server has no window, so `terminal.focus` there never navigates (#24688). The desktop - * app on this machine shows that server's panes under the same ids and can reveal one. - * Resolves false when no local desktop shows the environment; the server's answer then stands. - */ +/** Paired servers have no window; reveal their pane through a local desktop if one is available. */ export async function revealPairedTerminalOnLocalDesktop( client: RuntimeClient, handle: string @@ -30,7 +26,6 @@ export async function revealPairedTerminalOnLocalDesktop( }) return revealed.result.reveal.navigated } catch { - // Why: no desktop here, one without terminal.reveal, a headless one, or a handle gone since. return false } } diff --git a/src/cli/project-selector.ts b/src/cli/project-selector.ts index deccd568a6a..9f5602c2dd9 100644 --- a/src/cli/project-selector.ts +++ b/src/cli/project-selector.ts @@ -12,9 +12,6 @@ export function setupHostId(setup: ProjectHostSetup): string { return normalizeExecutionHostId(setup.hostId) ?? setup.hostId } -// Why: `orca project list` shows display names, and the stored projectId is a provider-scoped -// string ("github:stablyai/orca") nobody keeps in their head — so the name is what gets typed. -// Matching ids alone reported "not set up" for a project sitting right there in the listing. export async function findProjectSetups( client: RuntimeClient, setups: readonly ProjectHostSetup[], @@ -28,7 +25,7 @@ export async function findProjectSetups( if (projects === null) { return legacyNameMatchedSetups(setups, projectSelector) } - // Why: an exact id with no setups names that project, not another one whose name equals it. + // An exact id must not fall through to another project whose name equals it. if (projects.some((project) => project.id === projectSelector)) { return [] } @@ -40,10 +37,7 @@ export async function findProjectSetups( : setups.filter((candidate) => candidate.projectId === matchedId) } -/** - * The project id a `--project` value names, for commands that send the id itself. An id or an - * unknown value passes through unchanged so the server still answers for it, as it always did. - */ +/** Unknown selectors pass through for the server to resolve. */ export async function resolveProjectIdSelector( client: RuntimeClient, projectSelector: string @@ -52,8 +46,7 @@ export async function resolveProjectIdSelector( try { projects = (await client.call<{ projects: Project[] }>('project.list')).result.projects } catch { - // Why: falling back sends the value exactly as before this lookup existed, so a failed or - // missing project.list costs only the name convenience, never a working id. + // Failed name lookup must not prevent a valid id from reaching the server. return projectSelector } if (projects.some((project) => project.id === projectSelector)) { @@ -78,17 +71,14 @@ function assertProjectNameUnambiguous(named: readonly Project[], projectSelector ) } -// Why: a server predating project.list still answers projectHostSetup.list, and those rows carry -// a display name of their own. It is the repo's name rather than the project's, so this is the -// weaker match — reachable only on that older server. +// Older servers expose only setup display names, which name repos rather than projects. function legacyNameMatchedSetups( setups: readonly ProjectHostSetup[], projectSelector: string ): ProjectHostSetup[] { const matched = setups.filter((candidate) => matchesName(candidate.displayName, projectSelector)) const projectIds = [...new Set(matched.map((candidate) => candidate.projectId))] - // Why: two projects can hold repos with the same folder name, and this match is by folder name. - // Selecting either would target a project the caller did not name. + // Repo folder names can collide across projects; never guess between them. if (projectIds.length <= 1) { return matched } @@ -107,9 +97,7 @@ async function listProjectsUnlessUnsupported(client: RuntimeClient): Promise('project.list')).result.projects } catch (error) { - // Why: only a version gap justifies the weaker fallback. Swallowing everything reported a - // network blip or a permission error as "no project by that name", which then surfaced as - // "Project is not set up on the selected host" for a project that exists and is set up. + // Only a version gap permits the weaker name match; propagate network and permission failures. if (error instanceof RuntimeClientError && error.code === 'method_not_found') { return null } diff --git a/src/cli/quote-stripped-json-flag.test.ts b/src/cli/quote-stripped-json-flag.test.ts index f19f95c004e..e3cfad1048c 100644 --- a/src/cli/quote-stripped-json-flag.test.ts +++ b/src/cli/quote-stripped-json-flag.test.ts @@ -4,8 +4,7 @@ import { describeQuoteStrippedJsonFlag, looksQuoteStripped } from './quote-strip describe('quote-stripped JSON flag detection', () => { it('flags the exact shape measured from Windows PowerShell 5.1', () => { - // Measured on a real Windows host: ConvertTo-Json emitted - // ["task_b2a580db74d8","task_c3b691ec85e9"] and argv received the value below. + // PowerShell 5.1 passed this shape after ConvertTo-Json emitted quoted task ids. expect(looksQuoteStripped('[task_b2a580db74d8,task_c3b691ec85e9]')).toBe(true) expect(looksQuoteStripped('[a,b]')).toBe(true) expect(looksQuoteStripped('{a:b}')).toBe(true) @@ -26,7 +25,6 @@ describe('quote-stripped JSON flag detection', () => { }) it('requires a key:value pair per entry before calling an object stripped', () => { - // Quoting these cannot produce a valid object, so they are ordinary invalid JSON. expect(looksQuoteStripped('{a,b}')).toBe(false) expect(looksQuoteStripped('{a:b,c}')).toBe(false) expect(looksQuoteStripped('{:b}')).toBe(false) @@ -40,14 +38,12 @@ describe('quote-stripped JSON flag detection', () => { expect(message).toContain('--payload is not valid JSON') expect(message).toContain('PowerShell 5.1') expect(message).toContain('$v') - // A payload can carry secrets, and this reaches --json output. expect(message).not.toContain('hunter2') expect(message).not.toContain('secret') expect(describeQuoteStrippedJsonFlag('options', '["a","b"]')).toBeNull() }) it('hedges the shell attribution, since it inspects only the value shape', () => { - // The same shape occurs when a macOS/Linux user simply forgets to quote. expect(describeQuoteStrippedJsonFlag('options', '[a,b]')).toContain('If you ran this from') }) }) diff --git a/src/cli/quote-stripped-json-flag.ts b/src/cli/quote-stripped-json-flag.ts index 84283912b87..2de3377eee7 100644 --- a/src/cli/quote-stripped-json-flag.ts +++ b/src/cli/quote-stripped-json-flag.ts @@ -1,14 +1,4 @@ -/** - * Windows PowerShell 5.1 does not escape inner quotes when it builds a native command line, so - * `--payload '{"a":"b"}'` reaches the exe as `--payload {a:b}` (#16706). The value is correct when - * printed and damaged by the time argv is parsed, which makes the resulting "invalid JSON" error - * point at the user's input rather than at the shell that mangled it. - * - * Detection only — recovery is deliberately not attempted. Un-quoting is lossy for anything but a - * strict grammar: `["1","2"]` and `[1,2]` arrive identically, so a general repair would silently - * turn strings into numbers. `--deps` is recoverable only because generated task IDs have a fixed - * shape (see `task-deps-flag.ts`). - */ +// PowerShell 5.1 can strip JSON quotes at the native argv boundary; repair would lose type information. const BARE_TOKEN = /^[A-Za-z0-9_.:+-]+$/ const BARE_OBJECT_ENTRY = /^([A-Za-z0-9_.+-]+):([A-Za-z0-9_.+-]+)$/ @@ -17,7 +7,6 @@ function splitEntries(body: string): string[] { return body.split(',').map((entry) => entry.trim()) } -/** Whether `raw` looks like JSON whose quotes a native argv boundary stripped. */ export function looksQuoteStripped(raw: string): boolean { const trimmed = raw.trim() if (trimmed.includes('"')) { @@ -36,26 +25,19 @@ export function looksQuoteStripped(raw: string): boolean { if (body.length === 0) { return false } - // Why: only claim mangling when re-quoting every entry would actually produce valid JSON. - // An object needs a `key:value` pair per entry; `{a,b}` is not a stripped object. + // Re-quoting must yield valid JSON; {a,b} is not a stripped object. return isObject ? splitEntries(body).every((entry) => BARE_OBJECT_ENTRY.test(entry)) : splitEntries(body).every((entry) => BARE_TOKEN.test(entry)) } } -/** - * Error text for a JSON flag whose quotes were stripped. Returns null when `raw` is not that shape. - * - * The value itself is never echoed: a `--payload` can carry secrets, and this message reaches both - * human and `--json` output. The shape is already described by the flag name and the guidance. - */ +/** Never echo the value: JSON payloads can contain secrets. */ export function describeQuoteStrippedJsonFlag(flagName: string, raw: string): string | null { if (!looksQuoteStripped(raw)) { return null } - // Why conditional wording: this inspects only the value's shape, so it also fires when someone - // types an unquoted `[a,b]` on macOS or Linux, where PowerShell is not involved. + // Shape alone cannot prove PowerShell was involved; other shells can receive the same invalid input. return ( `--${flagName} is not valid JSON: its quotes are missing.\n` + 'If you ran this from Windows PowerShell 5.1, it strips inner quotes when building a native ' + diff --git a/src/cli/remote-selection-flag-rejection.ts b/src/cli/remote-selection-flag-rejection.ts index e2609fa604a..86b560ad111 100644 --- a/src/cli/remote-selection-flag-rejection.ts +++ b/src/cli/remote-selection-flag-rejection.ts @@ -1,17 +1,9 @@ import { RuntimeClientError } from './runtime/types' -/** - * The flags that pick which runtime answers a command. `shouldIgnoreRemoteSelection` - * in `src/cli/index.ts` pins some command families to the local runtime, which drops - * these silently — so every pinned family pairs the pin with this rejection instead. - */ +// Local-only command families reject explicit runtime selectors rather than silently ignoring them. export const REMOTE_SELECTION_FLAGS = ['environment', 'pairing-code'] as const -/** - * Fails a pinned command that was given a runtime selector, rather than answering - * for a machine the caller did not name. `suffix` completes "`--` does not - * retarget …" and should say what the command answers for and where to run it. - */ +/** suffix completes "-- does not retarget …" with the command's actual scope. */ export function rejectRemoteSelectionFlags( flags: ReadonlyMap, suffix: string, diff --git a/src/cli/repo-path-arguments.ts b/src/cli/repo-path-arguments.ts index 3223eb73dca..f574eb68179 100644 --- a/src/cli/repo-path-arguments.ts +++ b/src/cli/repo-path-arguments.ts @@ -19,8 +19,7 @@ export function resolveRepoPathArgument( if (!isRemote) { return resolvePath(cwd, inputPath) } - // Why: the local CLI cwd is unrelated to a paired runtime's filesystem. - // Relative remote paths would silently target the wrong machine. + // The client cwd belongs to a different filesystem from a paired runtime's relative paths. if (!isAbsoluteServerPath(inputPath)) { throw new RuntimeClientError( 'invalid_argument', diff --git a/src/cli/retry-request-flag.ts b/src/cli/retry-request-flag.ts index bb84dd0e9cd..6d676bcc3cc 100644 --- a/src/cli/retry-request-flag.ts +++ b/src/cli/retry-request-flag.ts @@ -5,11 +5,7 @@ import { RETRY_REQUEST_ID_GUIDANCE } from '../shared/orchestration-retry-request-id' -/** - * `--retry-request` carries the mutation identity that makes a replay idempotent. A damaged value - * must never fall through to `undefined`, because the client would then mint a fresh identity and - * re-apply a mutation that may already have taken effect (#15180). - */ +/** Reject damaged identities: falling back to a fresh id could replay an already-applied mutation. */ export function readRetryRequestFlag(flags: Map): string | undefined { const value = flags.get('retry-request') rejectValuelessFlag(value, 'retry-request') diff --git a/src/cli/runtime-client-deferral.test.ts b/src/cli/runtime-client-deferral.test.ts index c5cadcfcec7..fd14eee1598 100644 --- a/src/cli/runtime-client-deferral.test.ts +++ b/src/cli/runtime-client-deferral.test.ts @@ -17,11 +17,7 @@ const { testUserDataPathRef: { current: '' } })) -// Why: `main` reaches RuntimeClient through `await import('./runtime-client.js')` -// now. Mocking the same specifier the eager import used proves the dynamic -// import still resolves to the module the 10 existing vi.mock suites target. -// Why: --environment is now resolved against the paired-environment store before the client is -// constructed, so a forwarding assertion needs an environment that actually resolves. +// Explicit environment selectors must resolve before the client is constructed. vi.mock('./runtime/environments', async (importOriginal) => { const actual = await importOriginal>() return { @@ -30,14 +26,7 @@ vi.mock('./runtime/environments', async (importOriginal) => { } }) -// Why: this suite runs the REAL `main()`, and `agent hooks off` below reaches the production -// handler, which calls removeManagedAgentHooks() against the developer's OWN ~/.claude and -// ~/.cursor — a green test run silently deleted every Orca-managed hook on the machine, so agent -// status stopped reporting until the next Orca restart (STA-5679). The byte-for-byte equivalence -// twin already refuses these tokens for exactly this reason -// (config/scripts/cli-runtime-client-deferral-equivalence.mjs); this is the same guard for vitest. -// Stubbed, not dropped: the row is the only case that reads ctx.client, so it carries the -// null-vs-undefined coverage the rest of the table cannot. +// Stub hook writes: this suite runs real handlers that would otherwise edit the developer's agent configs. vi.mock('../main/agent-hooks/managed-agent-hook-controls', () => ({ applyAgentStatusHooksEnabled: applyAgentStatusHooksEnabledMock, getManagedAgentHookStatuses: vi.fn(() => []) @@ -92,18 +81,7 @@ describe('RuntimeClient module-graph deferral', () => { expect(constructorArgsMock).not.toHaveBeenCalled() }) - // Why: `agent hooks on|off` are the only commands that both sit in a - // suppressed group and touch ctx.client, and they rewrite the real ~/.claude - // hook config — so the byte-for-byte equivalence script cannot invoke them. - // Assert the constructor arguments directly instead: `null` (not `undefined`) - // is what stops the ORCA_* env fallback re-activating for local-only groups. - // - // `constructs` is declared per case and asserted BEFORE the args, because - // only `agent hooks off` reads ctx.client. Looping over `mock.calls` alone - // would pass vacuously for the other four, and would keep passing if the one - // case that carries the null-vs-undefined coverage stopped constructing at - // all. The zero rows are not filler: they assert the deferral itself — a - // local-only group must reach its handler without building a client. + // Assert construction counts before arguments; otherwise an unused getter would pass vacuously. const SUPPRESSED_GROUPS: [name: string, argv: string[], constructs: number][] = [ ['agent', ['agent', 'hooks', 'off'], 1], ['environment', ['environment', 'list'], 0], @@ -146,10 +124,7 @@ describe('RuntimeClient module-graph deferral', () => { } ) - // Why: four of the five groups above never read ctx.client, so they can only - // assert that no client is built — not that suppression forwards `null`. - // Stub dispatch and read the getter directly so every group asserts the - // constructor arguments unconditionally, exactly once. + // Probe the getter directly: handlers that never use it cannot verify null selector forwarding. it.each(SUPPRESSED_GROUPS.map(([name, argv]) => [name, argv] as const))( 'forwards null remote selection to the client %s would build', async (_name, argv) => { @@ -172,9 +147,6 @@ describe('RuntimeClient module-graph deferral', () => { } ) - // Why: the mirror — the same stubbed-dispatch probe must show `undefined` - // (not `null`) for a non-suppressed group, or the assertion above would pass - // for a build that suppressed EVERY command's env fallback. it('forwards undefined remote selection for a non-suppressed group', async () => { vi.stubEnv('ORCA_PAIRING_CODE', 'pairing-code') const dispatchSpy = vi.spyOn(dispatchModule, 'dispatch').mockResolvedValue(undefined) @@ -191,9 +163,6 @@ describe('RuntimeClient module-graph deferral', () => { } }) - // Why: the mirror of the above — for every other command the env fallback - // MUST stay live, which the RuntimeClient default parameters implement. That - // only works if `undefined` is forwarded. it('forwards undefined for non-suppressed commands so the env fallback applies', async () => { callMock.mockResolvedValue({ result: { worktrees: [] } }) @@ -218,8 +187,6 @@ describe('RuntimeClient module-graph deferral', () => { expect(constructorArgsMock.mock.calls[0][3]).toBe('env-1') }) - // Why: the getter is memoised; a dynamic import inside it would have made it - // async and changed every handler signature. it('reuses one client instance across repeated ctx.client reads', async () => { callMock.mockResolvedValue({ result: { worktrees: [] } }) diff --git a/src/cli/runtime-client.test.ts b/src/cli/runtime-client.test.ts index 625a82aff24..541655b8012 100644 --- a/src/cli/runtime-client.test.ts +++ b/src/cli/runtime-client.test.ts @@ -62,9 +62,7 @@ function writeMetadata( } function findUnusedPid(seed = 200_000): number { - // Why: the stale-bootstrap test must point metadata at a definitely-dead - // process. Hard-coding a small PID is host-dependent and flakes when that - // PID happens to be alive on the machine running the suite. + // Find a dead PID at runtime; a fixed PID may belong to a live process on this host. let pid = Math.max(seed, process.pid + 10_000) while (pid < 2_000_000) { try { @@ -77,9 +75,7 @@ function findUnusedPid(seed = 200_000): number { return 2_000_000 } -// Why: these tests create Unix domain socket servers in temp directories. -// Windows does not support Unix domain sockets in the same way, causing -// EACCES errors on listen(), so the suite is skipped on that platform. +// Temp-path Unix sockets fail with EACCES on Windows. describe.skipIf(process.platform === 'win32')('RuntimeClient', () => { it('adds an opaque durable request ID only to orchestration mutations', async () => { const userDataPath = makeRuntimeClientFixture() @@ -398,7 +394,6 @@ describe.skipIf(process.platform === 'win32')('RuntimeClient', () => { await expect(client.openOrca(100)).rejects.toMatchObject({ code: 'desktop_activation_blocked' }) - // A blocked runtime can't promote, so we bail before spawning the app. expect(launchOrcaApp).not.toHaveBeenCalled() }) @@ -408,8 +403,7 @@ describe.skipIf(process.platform === 'win32')('RuntimeClient', () => { const server = createServer((socket) => { sockets.add(socket) socket.once('close', () => sockets.delete(socket)) - // Why: keep the socket open without replying so the client timeout path - // is exercised against a real hung runtime connection. + // Leave the connection open without a reply to exercise the timeout against a real socket. }) servers.add(server) await new Promise((resolve) => server.listen(endpoint, resolve)) diff --git a/src/cli/runtime-client.ts b/src/cli/runtime-client.ts index 981759a005a..08d774955bf 100644 --- a/src/cli/runtime-client.ts +++ b/src/cli/runtime-client.ts @@ -1,4 +1,3 @@ -// Why: one import surface (and one vi.mock target) for CLI handlers over ./runtime/*. export { RuntimeClient } from './runtime/client' export { serveOrcaApp } from './runtime/launch' export { getDefaultUserDataPath } from './runtime/metadata' diff --git a/src/cli/search-command-arguments.test.ts b/src/cli/search-command-arguments.test.ts index 4e9a7fa8805..3cc51bf12d1 100644 --- a/src/cli/search-command-arguments.test.ts +++ b/src/cli/search-command-arguments.test.ts @@ -11,7 +11,6 @@ import { COMMAND_SPECS } from './specs' const COMMAND_PATHS = COMMAND_SPECS.flatMap((spec) => specPaths(spec)) -/** The real registry pipeline, so these assertions cover the shipped spec too. */ function parseCli(argv: string[]): ParsedArgs { const parsed = normalizeCommandPositionals( COMMAND_SPECS, diff --git a/src/cli/search-command-arguments.ts b/src/cli/search-command-arguments.ts index c3b09184d61..d5cddaed499 100644 --- a/src/cli/search-command-arguments.ts +++ b/src/cli/search-command-arguments.ts @@ -16,8 +16,7 @@ export type SearchCommand = | { kind: 'index-status' } | { kind: 'search'; request: AiVaultSearchRequest } -// Why enumerated: --index-status calls a different RPC that reads none of these, -// so ignoring one would answer a question the caller did not ask. +// --index-status uses a different RPC; query flags would otherwise be silently ignored. const QUERY_ONLY_FLAGS = [ 'query', 'scope', @@ -40,7 +39,6 @@ function readEnum( if (value === undefined) { return undefined } - // Why find and not includes: the match carries the narrow type, so nothing is asserted. const matched = allowed.find((candidate) => candidate === value) if (matched === undefined) { throw new RuntimeClientError( @@ -86,8 +84,7 @@ function readScopePaths(flags: Map): string[] | undefi return paths } -// Why the contract's own schema: the offset requirement lives there, and a -// second copy here would drift from what the host accepts. +// Reuse the host schema so timestamp offset requirements cannot drift. function readSince(flags: Map): string | undefined { const since = getOptionalStringFlag(flags, 'since') if (since === undefined) { @@ -129,7 +126,6 @@ function readFilters( return Object.keys(filters).length > 0 ? filters : undefined } -/** Maps `orca search` flags onto the session-search contract; nothing it does not have. */ export function parseSearchCommand(flags: Map): SearchCommand { if (flags.has('index-status')) { const conflicting = QUERY_ONLY_FLAGS.filter((flag) => flags.has(flag)) diff --git a/src/cli/selector-ambiguity-recovery.ts b/src/cli/selector-ambiguity-recovery.ts index 113b585142f..eb255779384 100644 --- a/src/cli/selector-ambiguity-recovery.ts +++ b/src/cli/selector-ambiguity-recovery.ts @@ -1,6 +1,4 @@ -// Why: the runtime answers a selector that matches several rows (two projects both named `app`) -// with a bare `selector_ambiguous`, naming neither the value nor a way out. Only the CLI still -// knows which flag the caller typed, so it says which selector was ambiguous and what is exact. +// The runtime omits the typed selector from ambiguity errors; only the CLI can supply it. export type SelectorAmbiguityRecovery = { ambiguousSelectors: { flag: string; selector: string }[] @@ -9,14 +7,12 @@ export type SelectorAmbiguityRecovery = { const EXACT_FORMS = { repo: { - // Why the --project route: a local and an SSH project can share both repo id and path. forms: 'id: or path:; if two hosts share that id or path, use --project --host where the command takes them', list: 'orca repo list --json', noun: 'project' }, worktree: { - // Why identity first: a local and an SSH checkout can share both repo id and path. forms: 'identity: (host-qualified), or id::: when only one host has that checkout', list: 'orca worktree list --json', @@ -27,7 +23,7 @@ const EXACT_FORMS = { export function selectorAmbiguityRecovery(selectors: { repo?: string worktree?: string - /** The command chose a paired server, so the listing must ask that server, not this machine. */ + /** Route recovery listings to the selected server. */ remoteServerSelected?: boolean }): SelectorAmbiguityRecovery | undefined { const entries = (['worktree', 'repo'] as const).flatMap((flag) => { diff --git a/src/cli/selectors.ts b/src/cli/selectors.ts index a3062134b6c..57c17d7a656 100644 --- a/src/cli/selectors.ts +++ b/src/cli/selectors.ts @@ -37,21 +37,13 @@ export function normalizeWorktreeSelector(selector: string, cwd: string): string return selector } -/** - * Rewrites the Linux path a WSL shell prints into the UNC path the runtime stored (#16628). - * - * Why here and not in the runtime: only the CLI sits in the distro, so only it can - * prove which distro the typed path belongs to. Translating once at this chokepoint - * covers `worktree show`, `terminal list --worktree` and `worktree rm --worktree`. - */ +/** Resolve Linux selectors to stored UNC paths here, where the caller's distro is known. */ export async function resolveCallerDistroPathSelector( selector: string, cwd: string, client: RuntimeClient ): Promise { - // Why not WSL_DISTRO_NAME: it is also set for a Linux-native CLI whose runtime stores - // POSIX paths, so it would name a distro for a caller that has none. ORCA_CLI_CWD, which - // the WSL launcher always sets and which arrives here as the invocation cwd, proves it. + // A UNC cwd proves Windows/WSL routing; WSL_DISTRO_NAME is also set for Linux-native runtimes. const callerDistro = parseWslUncPath(cwd)?.distro const linuxPath = selector.startsWith('path:') ? selector.slice(5) : '' const isLinuxMountedPath = /^\/mnt\/[A-Za-z](?:\/|$)/.test(linuxPath) @@ -73,8 +65,7 @@ export async function resolveCallerDistroPathSelector( ? isWslUncPathForLinuxMountedPath(worktree.path, linuxPath) : isWslUncPathForCallerLinuxPath(worktree.path, linuxPath, callerDistro!) ) - // Why the stored spelling rather than a synthesized UNC path: an unmatched selector must - // reach the runtime verbatim and fail as the caller typed it, never as a guessed distro. + // Preserve unmatched input verbatim; synthesizing a UNC path could guess the wrong distro. return match ? `path:${match.path}` : selector } @@ -94,8 +85,7 @@ function assertLocalCwdWorktreeSelector(selector: string, client: RuntimeClient) if (!client.isRemote) { return } - // Why: a paired CLI's cwd belongs to the client machine, not the runtime - // server, so cwd-derived worktree selectors are only valid locally. + // A paired CLI's cwd belongs to the client filesystem, so it cannot identify server worktrees. throw new RuntimeClientError( 'invalid_argument', `${selector} is a local cwd shortcut and cannot be resolved against a remote runtime. Pass an explicit server-side worktree selector such as identity:, id:::, name:, branch:, issue:, or path:.` @@ -118,10 +108,7 @@ export async function resolveCurrentWorktreeSelection( const worktrees = await client.call('worktree.list', { limit: 10_000 }) - // Why: users expect "active/current" to mean the enclosing managed worktree - // even from nested subdirectories. Resolve to the concrete runtime row here: - // duplicate repo registrations can expose the same Git worktree path, and a - // path selector would throw selector_ambiguous after losing the repo id. + // Preserve the repo id: duplicate registrations can make a bare path selector ambiguous. const selection = selectEnclosingWorktreeSelectionOnHost( worktrees.result.worktrees, cwd, @@ -167,8 +154,7 @@ export async function getRequiredWorktreeSelector( return await normalizeWorktreeSelectorForCaller(value, cwd, client) } -// Why: local browser commands default to the current worktree by auto-resolving -// from cwd. Remote commands omit worktree so the runtime uses server-side focus. +// Remote commands leave worktree unset to use server-side focus. export async function getBrowserWorktreeSelector( flags: Map, cwd: string, @@ -188,18 +174,13 @@ export async function getBrowserWorktreeSelector( if (client.isRemote) { return undefined } - // Default: auto-resolve from cwd try { return await resolveCurrentWorktreeSelector(cwd, client) } catch { - // Not inside a managed worktree — no filter return undefined } } -// Why: mirrors browser's implicit active-tab targeting. When --terminal is -// omitted, resolve the active terminal in the current worktree so commands -// like `orca terminal send --text "hello" --enter` Just Work. export async function getTerminalHandle( flags: Map, cwd: string, @@ -270,7 +251,6 @@ export async function getComputerCommandTarget( } } -// Match browser targeting: workspace by default, explicit device/emulator/worktree overrides. export type EmulatorCliTarget = { worktree?: string device?: string diff --git a/src/cli/serve-electron-flag-parity.test.ts b/src/cli/serve-electron-flag-parity.test.ts index 3aa54cf8bf4..d6d70b0a414 100644 --- a/src/cli/serve-electron-flag-parity.test.ts +++ b/src/cli/serve-electron-flag-parity.test.ts @@ -3,15 +3,9 @@ import { normalizeServeModeArgv } from '../main/startup/serve-mode-argv' import { BOOLEAN_FLAGS } from './args' import { SERVE_COMMAND_SPECS } from './specs/serve' -// Why this test lives under src/cli: the Electron rewrite duplicates the CLI's serve flag list because -// the main tsconfig cannot import the CLI project, so the parity check has to run from the CLI side. -// It fails when `orca serve` grows a flag that a direct ` serve …` launch would silently drop (#12677). +// The main tsconfig cannot import CLI specs, so test the duplicated serve flag lists from here. -// Global flags with no `--serve-*` counterpart: -// - help: a help launch is refused outright, not translated (see serve-mode-argv's HELP_FLAGS). -// - pairing-code / environment: nothing in src/main reads them, so they ride through untouched. -// - force: overrides the CLI's own pre-exec check; a direct ` serve` launch has none to override. -// - supervise: the CLI is the supervisor; a direct ` serve` launch has no parent to restart it. +// Help is refused; pairing selectors pass through; force and supervise apply only to the CLI parent. const UNTRANSLATED_GLOBAL_FLAGS = new Set([ 'help', 'pairing-code', @@ -41,20 +35,16 @@ describe('serve flag parity between the CLI spec and the Electron argv rewrite', expect(normalizeServeModeArgv(argv)).toEqual(expected) if (takesValue) { - // The equals form is the other shape `orca serve` accepts; normalize it to the internal shape. expect(normalizeServeModeArgv(['/AppRun', 'serve', `--${flag}=value`])).toEqual(expected) } else { - // A boolean with an attached value is not a truthy assertion: the CLI reads these as - // `flags.get(name) === true`, so `--no-pairing=false` must not disable pairing here either. - // (`--json` is the one flag the CLI reads with `.has()`; erring toward the literal value is - // the safe direction and only changes output format.) + // Keep attached boolean values literal, as the CLI does for flags such as --no-pairing. expect(normalizeServeModeArgv(['/AppRun', 'serve', `--${flag}=false`])).toEqual([ '/AppRun', '--serve', `--${flag}=false` ]) } - // Idempotent: `orca serve` spawns the app already in this shape, and the rewrite runs over it too. + // The app also normalizes argv already rewritten by the CLI launcher. expect(normalizeServeModeArgv(expected)).toEqual(expected) }) }) diff --git a/src/cli/session-caller-flags.ts b/src/cli/session-caller-flags.ts index 80078ffda7b..4cc75bd690e 100644 --- a/src/cli/session-caller-flags.ts +++ b/src/cli/session-caller-flags.ts @@ -1,15 +1,4 @@ -/** - * An agent session is its own orchestration caller. A flag that names the caller may restate that - * session, but one naming anyone else is refused before any effect — never dropped, never allowed - * to win. Against a host that predates session callers this is the only guard: such a host - * would honor the flag as the caller, which is how a chat consumed a sibling's mail (#21097). - * - * Whether `--from`/`--terminal` names the caller or a target is declared on the command's spec - * (`identityFlagRoles`) and checked once, here, at the CLI entry, so a handler cannot forget it. - * - * A `/clear`ed chat keeps its lineage root's address, which only the host's records can place, so - * a session address that is not this session's own spelling goes to the host's canonical-id check. - */ +// Fence caller flags before dispatch: older hosts would let a session act as another caller. import type { CommandSpec, IdentityFlag } from './command-spec' import { RuntimeClientError } from './runtime/types' @@ -48,10 +37,7 @@ export function refuseConflictingSessionCallerFlags( const IDENTITY_FLAGS: readonly IdentityFlag[] = ['from', 'terminal'] -/** - * The session's own spellings, plus the handle a structured worker session was minted. Plain - * strings: this runs at the CLI entry for every command, before the address codec's module graph. - */ +// Avoid loading the address codec graph at CLI entry for every command. function namesInjectedSession(value: string, sessionId: string, env: NodeJS.ProcessEnv): boolean { return ( value === sessionId || @@ -60,7 +46,7 @@ function namesInjectedSession(value: string, sessionId: string, env: NodeJS.Proc ) } -/** A session address only the host can place: this session's `/clear` root, or someone else. */ +/** Only the host can distinguish this session's /clear lineage root from another caller. */ export function sessionAddressForHost( value: string, sessionId: string, diff --git a/src/cli/skills-install.test.ts b/src/cli/skills-install.test.ts index 7f8a1c40582..584f6fe848a 100644 --- a/src/cli/skills-install.test.ts +++ b/src/cli/skills-install.test.ts @@ -23,8 +23,7 @@ const sha = (text: string) => createHash('sha256').update(text).digest('hex') const { detectAgentsMock } = vi.hoisted(() => ({ detectAgentsMock: vi.fn() })) -// Why: the real installer runs against a temp home; only the shipped bytes and the -// host's agent probe are pinned so the test does not depend on this machine. +// Use the real installer in a temporary home; pin bundled bytes and agent detection for portability. vi.mock('../main/skills/bundled-skill-install/bundled-skill-sources', () => ({ BUNDLED_SKILL_SOURCES: [ { name: NAME, text: NEW_TEXT, releasedDigests: [sha(OLD_TEXT), sha(NEW_TEXT)] } @@ -33,7 +32,7 @@ vi.mock('../main/skills/bundled-skill-install/bundled-skill-sources', () => ({ vi.mock('../main/preflight/installed-agent-detection', () => ({ detectInstalledAgentsWithShellPathHydration: detectAgentsMock })) -// Why: a held lock should fail fast here instead of waiting out the real 5s default. +// Avoid the production five-second lock wait in tests. vi.mock('../main/skills/bundled-skill-install/local-host', async (importOriginal) => { const actual = await importOriginal() return { @@ -130,7 +129,7 @@ describe('orca skills install/update', () => { const text = await run(['update', '--skill', NAME, '--agent', 'claude', '--dry-run']) - // Why no Claude line: update never adds the skill for an agent that does not have it. + // Updating must not install skills for an agent that lacks them. expect(text).toContain(`${NAME}\n ${paths.shared}: Update\n\n`) expect(text).toMatch(/Plan digest: [0-9a-f]{64}\nRerun without --dry-run to update now\.\n$/u) expect(await skillText(paths.shared)).toBe(OLD_TEXT) @@ -204,7 +203,6 @@ describe('orca skills install/update', () => { it('exits 1 when the run cannot apply the plan', async () => { await writeCopy(paths.shared, OLD_TEXT) - // Why: another install holding the target's lock is a run that wrote nothing. const release = await acquireSkillInstallLock({ path: skillInstallLockPath(join(paths.userData, 'skill-installs'), paths.shared) }) diff --git a/src/cli/skills.test.ts b/src/cli/skills.test.ts index 4867c7009eb..9bceb3d6b69 100644 --- a/src/cli/skills.test.ts +++ b/src/cli/skills.test.ts @@ -6,7 +6,7 @@ const { guideModuleLoadMock, previewMock, runtimeClientConstructorMock } = vi.ho runtimeClientConstructorMock: vi.fn() })) -// Why: these cases stop before planning; the installer itself is covered in skills-install.test.ts. +// Installer behavior is covered separately in skills-install.test.ts. vi.mock('../main/skills/bundled-skill-install/cli-entry', () => ({ localBundledSkillInput: previewMock, previewBundledSkillInstall: previewMock, @@ -45,10 +45,7 @@ vi.mock('./bundled-skill-guides.js', () => { }) vi.mock('./runtime-client', async () => { - // 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 the mock must preserve the production classes. const { RuntimeClientError, RuntimeRpcFailureError } = await import('./runtime/types.js') class RuntimeClient { diff --git a/src/cli/stdout-line.ts b/src/cli/stdout-line.ts index ddafe075a33..dc3675576e2 100644 --- a/src/cli/stdout-line.ts +++ b/src/cli/stdout-line.ts @@ -1,4 +1,3 @@ -/** Write one newline-terminated payload to stdout without doubling an existing newline. */ export function writeStdoutLine(value: string): void { process.stdout.write(value.endsWith('\n') ? value : `${value}\n`) } diff --git a/src/cli/terminal-format.test.ts b/src/cli/terminal-format.test.ts index 58904929a55..2e8bd3f89a8 100644 --- a/src/cli/terminal-format.test.ts +++ b/src/cli/terminal-format.test.ts @@ -200,8 +200,7 @@ describe('formatTerminalSend', () => { }) }) -// Why: an older host still publishes the codex-* tokens for dialogs its matcher never proved were -// Codex's, so a Gemini/Cursor/Antigravity user reads a Codex label unless the CLI names the neutral one. +// Older hosts can mislabel other providers' dialogs with codex-* tokens; include the neutral names. describe('blocked-reason rendering against a mixed-version host', () => { function showResult(reason?: RuntimeTerminalWaitBlockedReason): { terminal: RuntimeTerminalShow @@ -242,16 +241,13 @@ describe('blocked-reason rendering against a mixed-version host', () => { } } - // Why one assertion over every reason: a test that only asserts the *absence* of an alias suffix - // passes when the aliasing code is deleted, so each case is paired with a legacy token that must - // gain one. + // Pair alias additions with unchanged tokens so missing alias logic cannot pass on absence alone. it.each([ ['codex-trust-workspace', 'codex-trust-workspace (agent-trust-workspace)'], ['codex-update-prompt', 'codex-update-prompt (agent-update-prompt)'], ['codex-cwd-prompt', 'codex-cwd-prompt (agent-cwd-prompt)'], ['codex-hooks-review-prompt', 'codex-hooks-review-prompt (agent-hooks-review-prompt)'], ['codex-interactive-prompt', 'codex-interactive-prompt (agent-interactive-prompt)'], - // This build published these itself, so there is nothing to reinterpret. ['agent-trust-workspace', 'agent-trust-workspace'], ['codex-model-migration-prompt', 'codex-model-migration-prompt'] ] as const)('renders %s as %s on both wait and show', (reason, rendered) => { diff --git a/src/cli/terminal-format.ts b/src/cli/terminal-format.ts index 40ff6b9f181..92f31a2b00f 100644 --- a/src/cli/terminal-format.ts +++ b/src/cli/terminal-format.ts @@ -106,8 +106,6 @@ export function formatTerminalShow(result: { terminal: RuntimeTerminalShow }): s `ptyId: ${terminal.ptyId ?? 'none'}`, `connected: ${terminal.connected}`, `writable: ${terminal.writable}`, - // Why listed above the preview: the preview is where a reader would otherwise have to - // spot the prompt by eye, which is the work this line exists to remove. `agentWait: ${formatAgentWait(terminal.agentWait)}`, `preview: ${terminal.preview || ''}` ].join('\n') @@ -143,8 +141,6 @@ export function formatTerminalRead(result: { terminal: RuntimeTerminalRead }): s ...latestCursor, ...(terminal.truncated ? ['warning: older output is no longer retained'] : []), ...(limitedWarning ? [limitedWarning] : []), - // Why: the caller asked for the rendered screen; say plainly that this is not it rather - // than let repaint fragments be read as what the terminal displayed. ...(terminal.source === 'screen-unavailable' ? [ 'warning: no rendered screen was available, so this is accumulated output; repainted lines may appear as stacked fragments' @@ -208,8 +204,7 @@ function promptObservationWarning( if (prompt.observation === 'incarnation_replaced') { return 'delivery was not observed because the terminal process was replaced. Inspect the current terminal before sending a new prompt; do not retry with this request ID.' } - // Ordered before the unsupported arm: an agent provider that never reached turn_started - // needs the swallowed-Enter recovery even if this host could not observe the submit. + // Providers need swallowed-Enter recovery without turn_started, even if submit observation failed. if (prompt.provider !== 'unsupported' && prompt.provider !== 'old-host') { return prompt.stages.includes('turn_started') ? null diff --git a/src/cli/terminal-read-screen.test.ts b/src/cli/terminal-read-screen.test.ts index b3aba57c6bf..f87c8c59481 100644 --- a/src/cli/terminal-read-screen.test.ts +++ b/src/cli/terminal-read-screen.test.ts @@ -100,8 +100,6 @@ describe('orca terminal read --screen', () => { expect(String(logSpy.mock.calls[0]?.[0])).toContain('source: screen') }) - // Why: the whole defect is a stream being read as if it were the screen. A fallback has to - // announce itself rather than look like a successful screen read. it('warns when a screen was asked for but only the stream was available', async () => { queueFixtures( callMock, @@ -116,8 +114,7 @@ describe('orca terminal read --screen', () => { expect(printed).toContain('stacked fragments') }) - // Why: zod strips the unknown `screen` key on an older host, so it answers with an ordinary - // stream read. Handing that back would be the original defect wearing the new flag's name. + // Older hosts strip the unknown screen key and return a stream read. it("refuses to pass an older host's stream off as a screen read", async () => { queueFixtures(callMock, readFixture({ tail: ['cclclecleaclear'] })) const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}) diff --git a/src/cli/test-fixtures.ts b/src/cli/test-fixtures.ts index 4296a00c263..101b4b4a4c7 100644 --- a/src/cli/test-fixtures.ts +++ b/src/cli/test-fixtures.ts @@ -63,12 +63,11 @@ export function okFixture(id: string, result: unknown): OkFixture { return { id, ok: true, result, _meta: { runtimeId: 'runtime-1' } } } -/** The project read a CLI automation write makes to fence the host it lands on. */ +/** Repo lookup fences automation writes to the destination host. */ export function localRepoDestinationFixtures(repoId = 'repo-1'): OkFixture[] { return [okFixture('req_repo_show', { repo: { id: repoId, connectionId: null } })] } -/** A workspace selector resolves through its workspace before its project. */ export function workspaceDestinationFixtures( repoId = 'repo-1', worktreePath = '/tmp/repo/feature' diff --git a/src/cli/vocabulary-policy.ts b/src/cli/vocabulary-policy.ts index 311d22c8c1a..6728068a2ef 100644 --- a/src/cli/vocabulary-policy.ts +++ b/src/cli/vocabulary-policy.ts @@ -1,7 +1,6 @@ import type { CommandSpec } from './args' -// Why: predictable verbs prevent failed agent guesses; existing divergences stay -// grandfathered because renaming them would break compatibility. +// Grandfather existing verb divergences to avoid breaking compatibility. type VerbFamily = { name: string @@ -61,8 +60,6 @@ export function findVocabularyViolations(specs: CommandSpec[]): VocabularyViolat if (family.allowlist.has(command)) { continue } - // Why: an alias on the canonical verb makes the command reachable the - // canonical way, which satisfies the policy without a rename. if (hasCanonicalAlias(spec, family.canonical)) { continue } diff --git a/src/cli/workspace-format.ts b/src/cli/workspace-format.ts index e9ac6ffed09..c8f144a1ccc 100644 --- a/src/cli/workspace-format.ts +++ b/src/cli/workspace-format.ts @@ -63,8 +63,7 @@ function formatWorktreeMemoryLine(worktree: WorktreeMemory): string { ].join(' ') } -// Why omitted rather than zeroed: a host that predates the field, or cannot read -// commit at all, must not be printed as agents committing nothing. +// Missing commit fields mean unknown usage, not zero. function formatCommitLines(snapshot: MemorySnapshot): string[] { if (typeof snapshot.totalPrivateMemory !== 'number') { return [] diff --git a/src/cli/worktree-project-target.ts b/src/cli/worktree-project-target.ts index 66413289be5..198d8ac7242 100644 --- a/src/cli/worktree-project-target.ts +++ b/src/cli/worktree-project-target.ts @@ -7,7 +7,7 @@ import { RuntimeClientError } from './runtime-client' export type ProjectCreateTarget = { repoSelector: string - /** The selected setup's host; a repo id alone is ambiguous once it is set up on two hosts. */ + /** Repo ids can be shared across hosts. */ repoHostId: string setup: ProjectHostSetup } @@ -46,8 +46,7 @@ export function assertWorkspaceTargetFlagsCompatible(flags: Map` row and a `local` row for the -// same project; the exact stamp is the one the caller named, so it wins the selection. +// Prefer the exact runtime stamp over its local alias when both setups exist. function selectSetupOnHost( candidates: readonly ProjectHostSetup[], host: ParsedExecutionHost | undefined, @@ -59,8 +58,7 @@ function selectSetupOnHost( candidates.find((candidate) => hostFilterMatchesHostId(host, candidate.hostId)) ) } - // Why: one host is not a choice, so demanding --host for it was ceremony. Two are a choice, and - // picking one would run the command on a machine the caller never named. + // Infer a sole host, but never choose between multiple hosts without an explicit selector. const hostIds = new Set(candidates.map(setupHostId)) if (hostIds.size <= 1) { return candidates[0] @@ -102,8 +100,6 @@ export async function resolveProjectCreateTarget( try { result = await client.call<{ setups: ProjectHostSetup[] }>('projectHostSetup.list') } catch (error) { - // Why: --host runtime: routes here, so an older server is reachable without the caller - // meaning to; name the version gap rather than surfacing a raw method_not_found. if (error instanceof RuntimeClientError && error.code === 'method_not_found') { throw new RuntimeClientError( 'incompatible_runtime', @@ -120,7 +116,7 @@ export async function resolveProjectCreateTarget( : (await findProjectSetups(client, result.result.setups, projectId)).filter( (candidate) => candidate.setupState === 'ready' ) - // Why --host applies to both: setup ids are per host, so the same id can name two setups. + // Setup ids are scoped to a host, so --host must disambiguate them too. const setup = projectHostSetupId ? selectSetupOnHost( ready.filter((candidate) => candidate.id === projectHostSetupId), @@ -135,7 +131,6 @@ export async function resolveProjectCreateTarget( `Project host setup is not ready or was not found: ${projectHostSetupId}${host ? ` on ${host.id}` : ''}` ) } - // Why: blaming the host when no project matched sent callers to debug the wrong flag. if (candidates.length === 0) { throw new RuntimeClientError( 'invalid_argument', @@ -150,8 +145,6 @@ export async function resolveProjectCreateTarget( throw new RuntimeClientError( 'invalid_argument', `Project is not set up on the selected host: ${projectId}${host ? ` on ${host.id}` : ''}`, - // Why: when the project is set up somewhere just not here, the recoverable answer is which - // hosts do have it — otherwise the caller has to go re-derive the listing by hand. { knownProjectHostSetups: candidates.map(summarizeSetup), nextSteps: candidates.map( @@ -176,10 +169,7 @@ function getRepoSelectorFromWorktreeId(worktreeId: string | undefined): string | return `id:${worktreeId.slice(0, separatorIndex)}` } -/** - * The repo a create names, as RPC params: spread it whole into every call that looks the repo up, - * so the host a project target picked is never dropped. `--repo` stays selector-only. - */ +/** Forward repoHostId with the repo in every lookup; the same repo can exist on multiple hosts. */ export type CliRepoTarget = { repo: string; repoHostId?: string } export async function getCreateRepoTarget( diff --git a/src/cli/worktree-ps-program-status.ts b/src/cli/worktree-ps-program-status.ts index 52d0df0a03f..6a9a1798b2c 100644 --- a/src/cli/worktree-ps-program-status.ts +++ b/src/cli/worktree-ps-program-status.ts @@ -4,7 +4,7 @@ import { resolveProgramStatusPresentation } from '../shared/program-status/presentation' -/** Compact OSC 7501 suffix for a `worktree ps` line: a block beats progress; '' when neither. */ +/** A block takes precedence over progress. */ export function formatWorktreePsProgramStatus( agents: readonly RuntimeWorktreeAgentRow[] | undefined ): string { diff --git a/src/cli/worktree-ps-terminal-verdict.ts b/src/cli/worktree-ps-terminal-verdict.ts index 2d79796cec4..80487ed8d1f 100644 --- a/src/cli/worktree-ps-terminal-verdict.ts +++ b/src/cli/worktree-ps-terminal-verdict.ts @@ -19,7 +19,6 @@ function terminalVerdict(row: TerminalCounts): TerminalVerdict { return 'none' } -/** `live:` and `pty:` words for one row; lost contact never reads as zero or no. */ export function formatWorktreePsTerminalFields(row: TerminalCounts): string { if (terminalVerdict(row) === 'unverifiable') { return 'live:unverifiable pty:unverifiable' @@ -31,10 +30,7 @@ export function formatWorktreePsTerminalFields(row: TerminalCounts): string { return `live:${row.liveTerminalCount}+${unverifiable} unverifiable pty:${row.hasAttachedPty ? 'yes' : 'unverifiable'}` } -/** - * JSON counterpart. The count fields keep their number/boolean types for existing scripts, so - * `terminalVerdict` is what says a 0/false came from a host that could not be asked. - */ +/** Preserve numeric/boolean fields for existing scripts; terminalVerdict distinguishes unknown values. */ export function projectWorktreePsTerminalVerdict( row: TRow ): TRow & { terminalVerdict: TerminalVerdict } { diff --git a/src/cli/worktree-selector-caller-host.test.ts b/src/cli/worktree-selector-caller-host.test.ts index 941d8841166..d68e1d0b544 100644 --- a/src/cli/worktree-selector-caller-host.test.ts +++ b/src/cli/worktree-selector-caller-host.test.ts @@ -1,7 +1,3 @@ -/** - * #21210: several SSH hosts hold a worktree of one repo at the same path. A CLI on host D must - * resolve `current` (and a pane's forwarded worktree id) to host D's row, never another host's twin. - */ import { describe, expect, it } from 'vitest' import { getCallerExecutionHostId, @@ -37,7 +33,6 @@ describe('implicit worktree selection is scoped to the caller host (#21210)', () it('reads the caller host from the relay stamp, encoding the SSH target', () => { expect(getCallerExecutionHostId(SSH_CALLER_D)).toBe('ssh:host%20d') expect(getCallerExecutionHostId({})).toBe('local') - // A WSL stamp is still this machine. expect( getCallerExecutionHostId({ ORCA_ORCHESTRATION_COMPATIBILITY_HOST_KIND: 'wsl', @@ -57,7 +52,6 @@ describe('implicit worktree selection is scoped to the caller host (#21210)', () expect(selectEnclosingWorktreeOnHost(TWINS, PATH, 'local')).toBe(`id:repo-l::${PATH}`) }) - // A runtime-stamped row in this runtime's own catalog is a checkout it holds locally. it('keeps a runtime-stamped local checkout for a local caller', () => { const rows = [ TWINS[0]!, diff --git a/src/cli/worktree-selector-recovery.ts b/src/cli/worktree-selector-recovery.ts index ac597fc6c8c..09719db1e1b 100644 --- a/src/cli/worktree-selector-recovery.ts +++ b/src/cli/worktree-selector-recovery.ts @@ -1,8 +1,4 @@ -// Why: the runtime answers an unresolvable `--worktree` with a bare -// `selector_not_found` — no offending value and no grammar — so a caller who passed -// a repo id where a worktree id belongs cannot tell what was wrong (#16904). The CLI -// is the only layer that still knows what the caller typed, so it shapes the recovery -// here, in the same validFlags/suggestions/nextSteps shape as an unknown-flag error. +// The runtime omits the typed selector from not-found errors; add it and its grammar at the CLI boundary. export const WORKTREE_SELECTOR_FORMS = [ 'id:::', diff --git a/src/cli/worktree-selector-wsl-posix-path.test.ts b/src/cli/worktree-selector-wsl-posix-path.test.ts index 78ef1779c82..8429793f586 100644 --- a/src/cli/worktree-selector-wsl-posix-path.test.ts +++ b/src/cli/worktree-selector-wsl-posix-path.test.ts @@ -1,8 +1,4 @@ -/** - * The CLI half of #16628: inside WSL the user types `/home/neil/qa-repo`, the runtime stored the - * UNC path Windows sees. Only this process sits in the distro, so only it may translate — and the - * same selector reaches `worktree rm`, so an unprovable distro must leave the path alone. - */ +// These selectors also reach worktree rm, so translate only with proof of the caller's distro. import { afterEach, describe, expect, it, vi } from 'vitest' import type { RuntimeWorktreeRecord } from '../shared/runtime-types' import type { RuntimeClient } from './runtime-client' @@ -65,7 +61,7 @@ describe('normalizeWorktreeSelectorForCaller in a WSL shell (#16628)', () => { it('leaves the path alone when only another distro spells it', async () => { const { client } = makeClient([uncPath(UBUNTU, LINUX_PATH)]) - // Interop forwards WSL_DISTRO_NAME, so it must not outvote the cwd that proves the distro. + // Interop can forward WSL_DISTRO_NAME from a different distro than the cwd proves. vi.stubEnv('WSL_DISTRO_NAME', UBUNTU) await expect( @@ -103,7 +99,7 @@ describe('normalizeWorktreeSelectorForCaller in a WSL shell (#16628)', () => { it.each([ ['a branch selector', 'branch:qa'], ['a relative path', 'path:qa-repo'], - // A UNC path is already the runtime's spelling; a backslash inside a Linux path has no UNC form. + // A Linux filename containing a backslash cannot be represented as a UNC path. ['a UNC path selector', `path:${uncPath(UBUNTU, LINUX_PATH)}`], ['a Linux path containing a backslash', 'path:/home/neil/qa\\repo'] ])('leaves %s untranslated', async (_label, selector) => {