diff --git a/src/cli/args.ts b/src/cli/args.ts index 2f6bcdaeb0f..68ae1cae913 100644 --- a/src/cli/args.ts +++ b/src/cli/args.ts @@ -37,6 +37,7 @@ export const BOOLEAN_FLAGS = new Set([ 'mobile', 'mobile-pairing', 'no-pairing', + 'screen', 'parent-current', 'provision', 'ready', diff --git a/src/cli/handlers/terminal.ts b/src/cli/handlers/terminal.ts index 04ac703cd53..00c0a5cd807 100644 --- a/src/cli/handlers/terminal.ts +++ b/src/cli/handlers/terminal.ts @@ -76,11 +76,30 @@ export const TERMINAL_HANDLERS: Record = { if (cursorFlag !== undefined && cursor === undefined) { throw new RuntimeClientError('invalid_argument', '--cursor must be a non-negative integer') } + const screen = flags.get('screen') === true + // Why: a cursor pages through accumulated output. A screen read is the current frame and has + // nothing behind it to page, so accepting both would imply history that is not there. + if (screen && cursorFlag !== undefined) { + throw new RuntimeClientError( + 'invalid_argument', + '--screen reads the current rendered screen, which has no cursor to page from. Use --cursor without --screen to page through accumulated output.' + ) + } const result = await client.call<{ terminal: RuntimeTerminalRead }>('terminal.read', { terminal: await getTerminalHandle(flags, cwd, client), ...(cursor !== undefined ? { cursor } : {}), + ...(screen ? { screen: true } : {}), limit: getOptionalPositiveIntegerFlag(flags, 'limit') }) + // Why: an older host drops the unknown `screen` param and answers with its ordinary stream + // read, which carries no source. Returning that silently is the exact failure this flag + // exists to prevent, so refuse rather than hand back the other question's answer. + if (screen && result.result.terminal.source === undefined) { + throw new RuntimeClientError( + 'incompatible_runtime', + 'This Orca host does not support --screen reads, so it answered with accumulated output instead of the rendered screen. Update Orca on the host, or drop --screen to read accumulated output deliberately.' + ) + } printResult(result, json, formatTerminalRead) }, 'terminal send': async ({ flags, client, cwd, json }) => { diff --git a/src/cli/specs/core.ts b/src/cli/specs/core.ts index 714115680f6..7aa503fad36 100644 --- a/src/cli/specs/core.ts +++ b/src/cli/specs/core.ts @@ -197,17 +197,23 @@ export const CORE_COMMAND_SPECS: CommandSpec[] = [ { path: ['terminal', 'read'], summary: 'Read bounded terminal output', - usage: 'orca terminal read [--terminal ] [--cursor ] [--limit ] [--json]', - allowedFlags: [...GLOBAL_FLAGS, 'terminal', 'cursor', 'limit'], + usage: + 'orca terminal read [--terminal ] [--cursor ] [--limit ] [--screen] [--json]', + allowedFlags: [...GLOBAL_FLAGS, 'terminal', 'cursor', 'limit', 'screen'], notes: [ 'Omit --terminal to target the active terminal in the current worktree.', + 'By default this returns accumulated terminal output with escape sequences stripped, not the rendered screen. Any program that repaints a line — shells, progress bars, TUIs — comes back as stacked fragments, so one `clear` keystroke by keystroke reads as `cclclecleaclear`, and spaces a prompt draws by moving the cursor are absent.', + 'Use --screen to read what the terminal actually renders. Prefer it whenever the answer depends on how output looks rather than what was emitted over time; the default is unsuitable for verifying rendered output.', + 'The result reports source: stream or screen, so a caller can tell which question was answered. A --screen read falls back to source: stream when no rendered state exists rather than passing the stream off as a screen.', + '--screen and --cursor are mutually exclusive: a screen read is the current frame and has no history to page.', 'Use --cursor with the nextCursor value from a previous read to get only new output since that read.', 'Use --limit to request more retained lines for long agent responses; output reports oldestCursor when older lines were dropped.', 'Useful for capturing the response to a command: read before sending, then read --cursor after waiting.' ], examples: [ 'orca terminal read --json', - 'orca terminal read --terminal term_abc123 --cursor 42 --limit 1000 --json' + 'orca terminal read --terminal term_abc123 --cursor 42 --limit 1000 --json', + 'orca terminal read --terminal term_abc123 --screen --json' ] }, { diff --git a/src/cli/terminal-format.ts b/src/cli/terminal-format.ts index 97da0e890c6..806a39cb588 100644 --- a/src/cli/terminal-format.ts +++ b/src/cli/terminal-format.ts @@ -140,11 +140,19 @@ export function formatTerminalRead(result: { terminal: RuntimeTerminalRead }): s const header = [ `handle: ${terminal.handle}`, `status: ${terminal.status}`, + ...(terminal.source ? [`source: ${terminal.source}`] : []), ...(terminal.nextCursor !== null ? [`cursor: ${terminal.nextCursor}`] : []), ...oldestCursor, ...latestCursor, ...(terminal.truncated ? ['warning: older output is no longer retained'] : []), - ...(limitedWarning ? [limitedWarning] : []) + ...(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' + ] + : []) ] return [...header, '', ...terminal.tail].join('\n') } diff --git a/src/cli/terminal-read-screen.test.ts b/src/cli/terminal-read-screen.test.ts new file mode 100644 index 00000000000..b3aba57c6bf --- /dev/null +++ b/src/cli/terminal-read-screen.test.ts @@ -0,0 +1,155 @@ +import { describe, expect, it, vi } from 'vitest' + +const { + callMock, + runtimeClientConstructorMock, + serveOrcaAppMock, + getDefaultUserDataPathMock, + addEnvironmentFromPairingCodeMock, + listEnvironmentsMock, + spawnMock +} = vi.hoisted(() => ({ + callMock: vi.fn(), + runtimeClientConstructorMock: vi.fn(), + serveOrcaAppMock: vi.fn(), + getDefaultUserDataPathMock: vi.fn(() => '/tmp/orca-user-data'), + addEnvironmentFromPairingCodeMock: vi.fn(), + listEnvironmentsMock: vi.fn(), + spawnMock: vi.fn() +})) + +vi.mock('./runtime-client', async () => { + const { createRuntimeClientModuleMock } = await import('./index-test-harness.js') + return createRuntimeClientModuleMock({ + callMock, + runtimeClientConstructorMock, + serveOrcaAppMock, + getDefaultUserDataPathMock + }) +}) + +vi.mock('./runtime/environments', () => ({ + addEnvironmentFromPairingCode: addEnvironmentFromPairingCodeMock, + listEnvironments: listEnvironmentsMock, + removeEnvironment: vi.fn(), + resolveEnvironment: vi.fn() +})) + +vi.mock('child_process', async () => { + const { createChildProcessModuleMock } = await import('./index-test-harness.js') + return createChildProcessModuleMock(spawnMock) +}) + +import { main } from './index' +import { okFixture, queueFixtures } from './test-fixtures' +import { useWorktreeAwarenessEnvironment } from './index-test-harness' + +function readFixture(overrides: Record = {}) { + return okFixture('req_terminal_read', { + terminal: { + handle: 'term_abc', + status: 'running', + tail: ['clear'], + truncated: false, + nextCursor: null, + ...overrides + } + }) +} + +describe('orca terminal read --screen', () => { + useWorktreeAwarenessEnvironment({ + callMock, + serveOrcaAppMock, + getDefaultUserDataPathMock, + addEnvironmentFromPairingCodeMock, + listEnvironmentsMock, + spawnMock + }) + + it('does not ask for a screen unless requested', async () => { + queueFixtures(callMock, readFixture({ source: 'stream' })) + vi.spyOn(console, 'log').mockImplementation(() => {}) + + await main(['terminal', 'read', '--terminal', 'term_abc', '--json'], '/tmp/repo') + + expect(callMock).toHaveBeenCalledWith( + 'terminal.read', + expect.not.objectContaining({ screen: true }) + ) + }) + + it('requests the rendered screen with --screen', async () => { + queueFixtures(callMock, readFixture({ source: 'screen' })) + vi.spyOn(console, 'log').mockImplementation(() => {}) + + await main(['terminal', 'read', '--terminal', 'term_abc', '--screen', '--json'], '/tmp/repo') + + expect(callMock).toHaveBeenCalledWith( + 'terminal.read', + expect.objectContaining({ terminal: 'term_abc', screen: true }) + ) + }) + + it('reports which question was answered in human output', async () => { + queueFixtures(callMock, readFixture({ source: 'screen' })) + const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}) + + await main(['terminal', 'read', '--terminal', 'term_abc', '--screen'], '/tmp/repo') + + 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, + readFixture({ source: 'screen-unavailable', tail: ['cclclecleaclear'] }) + ) + const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}) + + await main(['terminal', 'read', '--terminal', 'term_abc', '--screen'], '/tmp/repo') + + const printed = String(logSpy.mock.calls[0]?.[0]) + expect(printed).toContain('no rendered screen was available') + 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. + 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(() => {}) + const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) + const priorExitCode = process.exitCode + + await main(['terminal', 'read', '--terminal', 'term_abc', '--screen', '--json'], '/tmp/repo') + + expect([...logSpy.mock.calls, ...errSpy.mock.calls].flat().join('\n')).toContain( + 'does not support --screen reads' + ) + expect(process.exitCode).toBe(1) + + process.exitCode = priorExitCode + }) + + it('rejects --screen with --cursor instead of implying history that is not there', async () => { + const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}) + const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) + const priorExitCode = process.exitCode + + await main( + ['terminal', 'read', '--terminal', 'term_abc', '--screen', '--cursor', '42', '--json'], + '/tmp/repo' + ) + + expect(callMock).not.toHaveBeenCalled() + expect([...logSpy.mock.calls, ...errSpy.mock.calls].flat().join('\n')).toContain( + 'has no cursor to page from' + ) + expect(process.exitCode).toBe(1) + + process.exitCode = priorExitCode + }) +}) diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index a964dc4c361..bbb51353427 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -18332,14 +18332,16 @@ export class OrcaRuntimeService { async readTerminal( handle: string, - opts: { cursor?: number; limit?: number } = {} + opts: { cursor?: number; limit?: number; screen?: boolean } = {} ): Promise { const pty = this.getLivePtyForHandle(handle) if (pty) { const read = this.readPtyTerminal(handle, pty.pty, opts) - const visibleRead = await this.withVisibleSnapshotFallback(pty.pty.ptyId, read, opts) + const visibleRead = opts.screen + ? await this.readRenderedScreen(pty.pty.ptyId, read, opts) + : await this.withVisibleSnapshotFallback(pty.pty.ptyId, read, opts) this.assertLiveTerminalHandleTargetsPty(handle, pty.pty.ptyId) - return visibleRead + return labelTerminalReadSource(visibleRead) } const { leaf } = this.getLiveLeafForHandle(handle) @@ -18355,11 +18357,33 @@ export class OrcaRuntimeService { limit: opts.limit }) if (!leaf.ptyId) { - return read + return { ...read, source: opts.screen ? 'screen-unavailable' : 'stream' } } - const visibleRead = await this.withVisibleSnapshotFallback(leaf.ptyId, read, opts) + const visibleRead = opts.screen + ? await this.readRenderedScreen(leaf.ptyId, read, opts) + : await this.withVisibleSnapshotFallback(leaf.ptyId, read, opts) this.assertLiveTerminalHandleTargetsPty(handle, leaf.ptyId) - return visibleRead + return labelTerminalReadSource(visibleRead) + } + + // Why: the default read is the accumulated pty stream, which stacks every repaint of a line + // ("cclclecleaclear" for one `clear`) and drops spaces a prompt draws with cursor-forward. + // That is the right answer for "what happened over time" and the wrong one for "what is on + // screen", so an explicit screen read goes to the emulator state instead. When no rendered + // state exists the stream is still returned, but labelled `stream` rather than passed off as + // a screen — silently answering the other question is the defect this exists to stop. + private async readRenderedScreen( + ptyId: string, + read: RuntimeTerminalRead, + opts: { limit?: number } = {} + ): Promise { + const visibleState = await this.readVisibleTerminalState(ptyId) + const lines = + visibleState?.lines ?? (await this.readProviderTerminalTailLines(ptyId, opts.limit)) + if (lines.length === 0) { + return { ...read, source: 'screen-unavailable' } + } + return buildVisibleSnapshotReadFallback(read, lines, opts.limit) } // Why a cache: leaf-branch sends may arrive per keystroke; one proven-absent @@ -39560,6 +39584,15 @@ function visibleNonBlankTerminalLines(lines: string[]): string[] { return lines.map((line) => line.trimEnd()).filter((line) => line.trim().length > 0) } +// Why: every read carries its source, so a caller that asked for a screen and got a response +// with no source at all knows it reached a host that predates screen reads — rather than +// mistaking the stream for the screen. Rendered lines only ever enter a read through +// buildVisibleSnapshotReadFallback, which stamps `screen` itself, so anything still unlabelled +// here is the accumulated stream. +function labelTerminalReadSource(resolved: RuntimeTerminalRead): RuntimeTerminalRead { + return resolved.source ? resolved : { ...resolved, source: 'stream' } +} + function buildVisibleSnapshotReadFallback( read: RuntimeTerminalRead, visibleLines: string[], @@ -39576,7 +39609,8 @@ function buildVisibleSnapshotReadFallback( tail: charBoundedTail.tail, limited: read.limited || lineBoundedTail.length < visibleLines.length || charBoundedTail.limited, - returnedLineCount: charBoundedTail.tail.length + returnedLineCount: charBoundedTail.tail.length, + source: 'screen' } } diff --git a/src/main/runtime/rpc/methods/terminal.ts b/src/main/runtime/rpc/methods/terminal.ts index 9ef1426dbcb..f27c8a43006 100644 --- a/src/main/runtime/rpc/methods/terminal.ts +++ b/src/main/runtime/rpc/methods/terminal.ts @@ -882,7 +882,10 @@ const TerminalRead = TerminalHandle.extend({ }) ) .optional(), - limit: OptionalFiniteNumber + limit: OptionalFiniteNumber, + // Why: optional so an older host that does not understand it simply drops the key and answers + // with its usual stream read; the response's `source` is what tells the caller which it got. + screen: z.literal(true).optional() }) // Why: preserve the legacy contract — `title: string | null` only, `undefined` rejected, so the CLI's "reset" signal stays distinct. @@ -1184,7 +1187,8 @@ export const TERMINAL_METHODS: RpcAnyMethod[] = [ handler: async (params, { runtime }) => ({ terminal: await runtime.readTerminal(params.terminal, { cursor: params.cursor, - limit: params.limit + limit: params.limit, + screen: params.screen }) }) }), diff --git a/src/shared/runtime-types.ts b/src/shared/runtime-types.ts index 5d03b9523c0..8a00c11c540 100644 --- a/src/shared/runtime-types.ts +++ b/src/shared/runtime-types.ts @@ -646,6 +646,13 @@ export type RuntimeTerminalRead = { nextCursor: string | null latestCursor?: string returnedLineCount?: number + // Why: these are two different questions and they disagree whenever a program repaints. + // `stream` is the accumulated pty output with escapes stripped, so a redrawn line arrives as + // stacked fragments; `screen` is what the terminal actually renders. Naming the source keeps + // a caller that asked for one and got the other from reading the answer as the wrong thing. + // `screen-unavailable` means a screen was asked for, none could be rendered, and this is the + // stream instead — distinct from `stream`, which is the caller getting what they asked for. + source?: 'stream' | 'screen' | 'screen-unavailable' } export type RuntimeTerminalRename = {