diff --git a/packages/platform-apple/src/runner/__tests__/runner-adoption.test.ts b/packages/platform-apple/src/runner/__tests__/runner-adoption.test.ts index b9bb459d9f..9e34383806 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-adoption.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-adoption.test.ts @@ -289,6 +289,27 @@ test('adoption is skipped when the probe fails', async () => { expect(await tryAdoptRunnerSessionFromLease(simulator, {})).toBeNull(); }); +// #2662: the probe decodes through the command path's decoder, so a body that +// path would refuse cannot be read as an answer here either. Adoption must not +// re-stamp the lease for a runner it cannot read. +test('adoption is skipped when the uptime probe answers a truncated body', async () => { + const lease = writeStaleLease(); + mockIsProcessAlive.mockReturnValue(true); + mockSendRunnerCommandOnce.mockResolvedValue(new Response('{"ok":true,"data":{"uptime":')); + + expect(await tryAdoptRunnerSessionFromLease(simulator, {})).toBeNull(); + expect(readStaleRunnerLease(simulator.id)?.ownerToken).toBe(lease.ownerToken); +}); + +test('adoption is skipped when the uptime probe answers an ok that is not the boolean true', async () => { + const lease = writeStaleLease(); + mockIsProcessAlive.mockReturnValue(true); + mockSendRunnerCommandOnce.mockResolvedValue(new Response('{"ok":"true"}')); + + expect(await tryAdoptRunnerSessionFromLease(simulator, {})).toBeNull(); + expect(readStaleRunnerLease(simulator.id)?.ownerToken).toBe(lease.ownerToken); +}); + test('adoption is disabled by the kill switch', async () => { writeStaleLease(); mockIsProcessAlive.mockReturnValue(true); diff --git a/packages/platform-apple/src/runner/__tests__/runner-command-recovery.test.ts b/packages/platform-apple/src/runner/__tests__/runner-command-recovery.test.ts index 7deacf9811..fbbf235cb0 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-command-recovery.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-command-recovery.test.ts @@ -204,3 +204,55 @@ test('a journaled RUNNER_WEDGED keeps its fatal code and stays unretriable', asy return true; }); }); + +/** + * #2662: the `status` read has no decoder of its own, so it accepts what the one + * decoder accepts. A stringly-typed `ok` used to be the seam: a private truthiness + * rule read this retained body as the command's own result. + */ +test('a retained response whose ok is not the boolean true is not recovered', async () => { + const { result, invalidate } = await runRecovery({ + script: [ + { + kind: 'ok', + data: { + lifecycleState: 'completed', + lifecycleResponseJson: '{"ok":"true","data":{"tapped":true}}', + }, + }, + ], + }); + + await assert.rejects(result, (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.details?.recovery, 'completed_without_retained_response'); + return true; + }); + assert.equal(invalidate.mock.calls.length, 0); +}); + +/** + * A retained body cut off mid-write answers nothing. The session is kept — the + * runner is reachable, it proved that by serving `status` — but the truncated + * command result is not handed back. + */ +test('a truncated retained response is not recovered', async () => { + const { result, invalidate } = await runRecovery({ + script: [ + { + kind: 'ok', + data: { + lifecycleState: 'completed', + lifecycleResponseJson: '{"ok":true,"data":{"nodes":[{"label":"Sign In"', + }, + }, + ], + }); + + await assert.rejects(result, (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.details?.recovery, 'completed_without_retained_response'); + return true; + }); + assert.equal(invalidate.mock.calls.length, 0); +}); diff --git a/packages/platform-apple/src/runner/__tests__/runner-response.test.ts b/packages/platform-apple/src/runner/__tests__/runner-response.test.ts new file mode 100644 index 0000000000..b0e4358a31 --- /dev/null +++ b/packages/platform-apple/src/runner/__tests__/runner-response.test.ts @@ -0,0 +1,139 @@ +import assert from 'node:assert/strict'; +import { describe, test } from 'vitest'; +import { AppError } from '@agent-device/kernel/errors'; +import { + buildRunnerResponseError, + decodeRunnerResponseBody, + isRunnerResponseOk, + readRunnerResponseData, +} from '../runner-contract.ts'; +import { parseRunnerResponse } from '../runner-session.ts'; + +// A body cut off mid-write: the shape a runner that died while answering leaves behind. +const TRUNCATED_BODY = '{"ok":true,"data":{"nodes":[{"label":"Sign In"'; +// `ok` is a Swift `Bool`, so this is what a private truthiness rule used to accept as an answer. +const STRINGLY_TYPED_BODY = '{"ok":"true","data":{"tapped":true}}'; + +describe('decodeRunnerResponseBody', () => { + test('decodes the runner envelope', () => { + assert.deepEqual(decodeRunnerResponseBody('{"ok":true,"data":{"tapped":true}}'), { + ok: true, + data: { tapped: true }, + }); + }); + + test('rejects a truncated body instead of reading a reply out of it', () => { + assert.throws( + () => decodeRunnerResponseBody(TRUNCATED_BODY), + (error: unknown) => + error instanceof AppError && + error.code === 'COMMAND_FAILED' && + error.message === 'Invalid runner response' && + error.details?.text === TRUNCATED_BODY, + ); + }); + + test('never reads a JSON body that is not an envelope as an answer', () => { + for (const body of ['null', '42', '"ok"']) { + assert.equal(isRunnerResponseOk(decodeRunnerResponseBody(body)), false, body); + } + }); + + test('carries a bare-array body through unread rather than inventing an envelope', () => { + const payload = decodeRunnerResponseBody('[]'); + assert.equal(isRunnerResponseOk(payload), false); + assert.deepEqual(readRunnerResponseData(payload), {}); + }); +}); + +describe('isRunnerResponseOk', () => { + test('accepts only the boolean true the runner sends', () => { + assert.equal(isRunnerResponseOk({ ok: true }), true); + assert.equal(isRunnerResponseOk({ ok: false }), false); + assert.equal(isRunnerResponseOk({}), false); + // Stringly-typed truthiness would let a proxy or a wedged socket answer for the runner. + assert.equal(isRunnerResponseOk({ ok: 'true' }), false); + assert.equal(isRunnerResponseOk({ ok: 1 }), false); + }); +}); + +describe('readRunnerResponseData', () => { + test('reads only an object data payload', () => { + assert.deepEqual(readRunnerResponseData({ ok: true, data: { tapped: true } }), { + tapped: true, + }); + assert.deepEqual(readRunnerResponseData({ ok: true }), {}); + assert.deepEqual(readRunnerResponseData({ ok: true, data: null }), {}); + assert.deepEqual(readRunnerResponseData({ ok: true, data: [] }), {}); + assert.deepEqual(readRunnerResponseData({ ok: true, data: 'tapped' }), {}); + }); +}); + +describe('buildRunnerResponseError', () => { + test('carries the payload, the runner hint, and the classified code', () => { + const error = buildRunnerResponseError( + { + ok: false, + error: { + code: 'RUNNER_BUSY', + message: 'Main thread is busy', + hint: 'Retry after it drains', + }, + }, + '/tmp/runner.log', + ); + + assert.ok(error instanceof AppError); + assert.equal(error.code, 'COMMAND_FAILED'); + assert.equal(error.message, 'Main thread is busy'); + assert.equal(error.details?.hint, 'Retry after it drains'); + assert.equal(error.details?.logPath, '/tmp/runner.log'); + assert.equal(error.details?.retriable, true); + assert.deepEqual((error.details as { runner?: unknown }).runner, { + ok: false, + error: { code: 'RUNNER_BUSY', message: 'Main thread is busy', hint: 'Retry after it drains' }, + }); + }); + + test('keeps a missing runner message as a generic runner error', () => { + const error = buildRunnerResponseError({ ok: false, error: {} }); + assert.equal(error.message, 'Runner error'); + assert.equal(error.details?.hint, undefined); + }); +}); + +describe('parseRunnerResponse', () => { + test('refuses a body whose ok is not the boolean true', async () => { + const session = { ready: false }; + + await assert.rejects( + () => parseRunnerResponse(new Response(STRINGLY_TYPED_BODY), session), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.code, 'COMMAND_FAILED'); + assert.deepEqual(error.details?.runner, { ok: 'true', data: { tapped: true } }); + return true; + }, + ); + + assert.equal(session.ready, false); + }); + + test('refuses a body that is not readable JSON', async () => { + const session = { ready: false }; + + await assert.rejects( + () => parseRunnerResponse(new Response(TRUNCATED_BODY), session), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.code, 'COMMAND_FAILED'); + assert.equal(error.details?.text, TRUNCATED_BODY); + // Transport-shaped: no `runner` detail, so the session keeps its recency bets (#2552). + assert.equal(error.details?.runner, undefined); + return true; + }, + ); + + assert.equal(session.ready, false); + }); +}); diff --git a/packages/platform-apple/src/runner/runner-adoption.ts b/packages/platform-apple/src/runner/runner-adoption.ts index 3c2819def1..0b1cf03a83 100644 --- a/packages/platform-apple/src/runner/runner-adoption.ts +++ b/packages/platform-apple/src/runner/runner-adoption.ts @@ -9,7 +9,11 @@ import { import type { DeviceInfo } from '@agent-device/kernel/device'; import { isRequestCanceledError } from '@agent-device/kernel/errors'; import { sendRunnerCommandOnce } from './runner-transport.ts'; -import { withRunnerCommandId } from './runner-contract.ts'; +import { + decodeRunnerResponseBody, + isRunnerResponseOk, + withRunnerCommandId, +} from './runner-contract.ts'; import { buildRunnerLease, readStaleRunnerLease, @@ -137,8 +141,7 @@ async function probeRunnerAnswersUptime(device: DeviceInfo, port: number): Promi withRunnerCommandId({ command: 'uptime' }), RUNNER_ADOPTION_PROBE_TIMEOUT_MS, ); - const payload = JSON.parse(await response.text()) as { ok?: unknown }; - return payload?.ok === true; + return isRunnerResponseOk(decodeRunnerResponseBody(await response.text())); } catch { return false; } diff --git a/packages/platform-apple/src/runner/runner-command-recovery.ts b/packages/platform-apple/src/runner/runner-command-recovery.ts index dd25b720ba..6345e0fd68 100644 --- a/packages/platform-apple/src/runner/runner-command-recovery.ts +++ b/packages/platform-apple/src/runner/runner-command-recovery.ts @@ -1,16 +1,18 @@ import { AppError } from '@agent-device/kernel/errors'; import type { DeviceInfo } from '@agent-device/kernel/device'; import { emitDiagnostic } from './host.ts'; -import { classifyRunnerReportedError, type RunnerCommand } from './runner-contract.ts'; +import { + classifyRunnerReportedError, + decodeRunnerResponseBody, + isRunnerResponseOk, + readRunnerResponseData, + type RunnerCommand, + type RunnerResponsePayload, +} from './runner-contract.ts'; import { isReadOnlyRunnerCommand } from './runner-command-traits.ts'; import type { AppleRunnerCommandOptions } from './runner-provider.ts'; import { executeRunnerCommandWithSession, type RunnerSession } from './runner-session.ts'; -type LifecycleResponsePayload = { - ok?: unknown; - data?: unknown; -}; - type RunnerTransportRecovery = | { type: 'recovered'; data: Record; reason: string; lifecycleState?: string } | { type: 'skipInvalidation'; error: AppError; reason: string; lifecycleState?: string } @@ -340,20 +342,16 @@ function runnerStatusInFlightError( function parseLifecycleResponseJson(value: unknown): Record | undefined { if (typeof value !== 'string' || value.trim().length === 0) return undefined; - const parsed = parseLifecycleResponsePayload(value); - if (!parsed.ok) return undefined; - if (parsed.data && typeof parsed.data === 'object' && !Array.isArray(parsed.data)) { - return parsed.data as Record; - } - return {}; -} - -function parseLifecycleResponsePayload(value: string): LifecycleResponsePayload { + let payload: RunnerResponsePayload; try { - const raw: unknown = JSON.parse(value); - if (raw && typeof raw === 'object') return raw as LifecycleResponsePayload; - } catch {} - return {}; + payload = decodeRunnerResponseBody(value); + } catch { + // A retained body the one decoder refuses is not a recoverable result; the + // caller keeps the session and reports the retained response as unreadable + // instead of returning a truncated command result (#2662). + return undefined; + } + return isRunnerResponseOk(payload) ? readRunnerResponseData(payload) : undefined; } function completedWithoutRetainedResponseHint( diff --git a/packages/platform-apple/src/runner/runner-contract.ts b/packages/platform-apple/src/runner/runner-contract.ts index d670b9854f..7835c38662 100644 --- a/packages/platform-apple/src/runner/runner-contract.ts +++ b/packages/platform-apple/src/runner/runner-contract.ts @@ -468,6 +468,66 @@ export function classifyRunnerReportedError( }); } +export type RunnerResponsePayload = { + ok?: unknown; + error?: { code?: unknown; message?: unknown; hint?: unknown }; + data?: unknown; +}; + +/** + * The one decoding of a runner response body (#2662). The envelope arrives at three readers — a + * command's own response, the lifecycle journal a status probe reads back after the transport + * response was lost, and the adoption `uptime` probe — and all three must agree on what is + * readable, or a body one of them refuses becomes an answer for another. A body that is not JSON + * at all is transport-shaped failure: a runner that died mid-write must not be read as having + * answered. + */ +export function decodeRunnerResponseBody(text: string): RunnerResponsePayload { + let parsed: unknown; + try { + parsed = JSON.parse(text); + } catch { + throw new AppError('COMMAND_FAILED', 'Invalid runner response', { text }); + } + return parsed && typeof parsed === 'object' ? (parsed as RunnerResponsePayload) : {}; +} + +/** The runner's `ok` is a Swift `Bool`, so only the literal `true` is an answer. */ +export function isRunnerResponseOk(payload: RunnerResponsePayload): boolean { + return payload.ok === true; +} + +export function readRunnerResponseData(payload: RunnerResponsePayload): Record { + if (!payload.data || typeof payload.data !== 'object' || Array.isArray(payload.data)) return {}; + return payload.data as Record; +} + +export function buildRunnerResponseError( + payload: RunnerResponsePayload, + logPath?: string, +): AppError { + const runnerErrorCode = readRunnerErrorCode(payload.error?.code); + const errorMessage = + typeof payload.error?.message === 'string' ? payload.error.message : undefined; + const hint = typeof payload.error?.hint === 'string' ? payload.error.hint : undefined; + const classification = classifyRunnerReportedError(runnerErrorCode); + return new AppError(classification.code, errorMessage ?? 'Runner error', { + runner: payload, + ...classification.details, + xcodebuild: { + exitCode: 1, + stdout: '', + stderr: '', + }, + hint, + logPath, + }); +} + +function readRunnerErrorCode(rawCode: unknown): string | undefined { + return typeof rawCode === 'string' && rawCode.trim().length > 0 ? rawCode.trim() : undefined; +} + export function resolveRunnerEarlyExitHint( message: string, stdout: string, diff --git a/packages/platform-apple/src/runner/runner-session.ts b/packages/platform-apple/src/runner/runner-session.ts index 6c77ca4a8a..8b47a5bfb4 100644 --- a/packages/platform-apple/src/runner/runner-session.ts +++ b/packages/platform-apple/src/runner/runner-session.ts @@ -28,7 +28,10 @@ import { type RunnerPhaseBudget, } from './runner-xctestrun.ts'; import { - classifyRunnerReportedError, + buildRunnerResponseError, + decodeRunnerResponseBody, + isRunnerResponseOk, + readRunnerResponseData, resolveRunnerRequestSignal, withRunnerCommandId, type RunnerCommand, @@ -946,62 +949,20 @@ function emitRunnerReadinessPreflightSkipped( }); } -type RunnerResponsePayload = { - ok?: unknown; - error?: { code?: unknown; message?: unknown; hint?: unknown }; - data?: unknown; -}; - export async function parseRunnerResponse( response: Response, session: Pick, logPath?: string, ): Promise> { - const json = parseRunnerResponsePayload(await response.text()); - if (!json.ok) { + const payload = decodeRunnerResponseBody(await response.text()); + if (!isRunnerResponseOk(payload)) { throw await enrichRunnerFailureFromLog({ - error: buildRunnerResponseError(json, logPath), + error: buildRunnerResponseError(payload, logPath), logPath, }); } session.ready = true; - return readRunnerResponseData(json); -} - -function parseRunnerResponsePayload(text: string): RunnerResponsePayload { - try { - const parsed: unknown = JSON.parse(text); - return parsed && typeof parsed === 'object' ? (parsed as RunnerResponsePayload) : {}; - } catch { - throw new AppError('COMMAND_FAILED', 'Invalid runner response', { text }); - } -} - -function buildRunnerResponseError(json: RunnerResponsePayload, logPath?: string): AppError { - const runnerErrorCode = readRunnerErrorCode(json.error?.code); - const errorMessage = typeof json.error?.message === 'string' ? json.error.message : undefined; - const hint = typeof json.error?.hint === 'string' ? json.error.hint : undefined; - const classification = classifyRunnerReportedError(runnerErrorCode); - return new AppError(classification.code, errorMessage ?? 'Runner error', { - runner: json, - ...classification.details, - xcodebuild: { - exitCode: 1, - stdout: '', - stderr: '', - }, - hint, - logPath, - }); -} - -function readRunnerErrorCode(rawCode: unknown): string | undefined { - return typeof rawCode === 'string' && rawCode.trim().length > 0 ? rawCode.trim() : undefined; -} - -function readRunnerResponseData(json: RunnerResponsePayload): Record { - if (!json.data || typeof json.data !== 'object' || Array.isArray(json.data)) return {}; - const data = json.data as Record; + const data = readRunnerResponseData(payload); emitRunnerResponseDiagnostics(data); return data; }