diff --git a/mobile/src/components/codex-reset-credit-capability.ts b/mobile/src/components/codex-reset-credit-capability.ts index 1a32ef37873..8e88f74f2f9 100644 --- a/mobile/src/components/codex-reset-credit-capability.ts +++ b/mobile/src/components/codex-reset-credit-capability.ts @@ -2,6 +2,7 @@ import { useEffect, useState } from 'react' import { CODEX_RESET_CREDIT_RUNTIME_CAPABILITY } from '../../../src/shared/protocol-version' import type { RpcClient } from '../transport/rpc-client' import { startRuntimeCapabilityProbe } from '../transport/runtime-capability-probe' +import { rpcObjectResultOrNull } from '../transport/rpc-acceptance-policies' // Why: source the capability string from the shared contract so a host bump can never // silently drift from the mobile probe. @@ -12,10 +13,7 @@ export async function readCodexResetCreditCapability( ): Promise { try { const response = await client.sendRequest('status.get') - if (!response.ok || !response.result || typeof response.result !== 'object') { - return false - } - const capabilities = (response.result as { capabilities?: unknown }).capabilities + const capabilities = rpcObjectResultOrNull(response)?.capabilities return ( Array.isArray(capabilities) && capabilities.includes(MOBILE_CODEX_RESET_CREDIT_CAPABILITY) ) diff --git a/mobile/src/host-screen/use-host-repo-metadata.ts b/mobile/src/host-screen/use-host-repo-metadata.ts index 198efbaf3ea..6840184c3a0 100644 --- a/mobile/src/host-screen/use-host-repo-metadata.ts +++ b/mobile/src/host-screen/use-host-repo-metadata.ts @@ -18,7 +18,7 @@ type SshTargetSummaryRow = { id: string; label: string } async function requestResult(client: RpcClient, method: string): Promise { try { const response = await client.sendRequest(method) - return response.ok ? (response as RpcSuccess).result : null + return response.ok ? response.result : null } catch { // Best-effort: hosts that predate a method still list repos; labels degrade to host ids. return null diff --git a/mobile/src/session/mobile-structured-agent-session-launch.test.ts b/mobile/src/session/mobile-structured-agent-session-launch.test.ts index 8a020d2eea9..2deb3042ca2 100644 --- a/mobile/src/session/mobile-structured-agent-session-launch.test.ts +++ b/mobile/src/session/mobile-structured-agent-session-launch.test.ts @@ -228,6 +228,34 @@ describe('mobile structured agent-session launch', () => { }) }) + describe.each(['top-level', 'nested'])('%s refusal messages', (location) => { + function refusalClient(message: unknown) { + const refusal = { code: 'method_not_found', ...(message === undefined ? {} : { message }) } + return clientReturning( + { ok: true, result: { supported: true } }, + location === 'top-level' + ? { ok: false, error: refusal } + : { ok: true, result: { ok: false, refusal } } + ) + } + + it.each( + [undefined, null, 42, false, { text: 'unavailable' }, ['unavailable']].map((message) => ({ + message + })) + )('keeps a malformed message $message unknown', async ({ message }) => { + await expect( + createMobileStructuredAgentSession(refusalClient(message), 'workspace-1', 'codex') + ).resolves.toMatchObject({ kind: 'unknown' }) + }) + + it('preserves the fallback for an empty string message', async () => { + await expect( + createMobileStructuredAgentSession(refusalClient(''), 'workspace-1', 'codex') + ).resolves.toEqual({ kind: 'failed', message: 'Could not open Codex chat.' }) + }) + }) + it.each(['structured_agent_session_unsupported', 'method_not_found'])( 'treats a top-level %s as a definitive refusal', async (code) => { diff --git a/mobile/src/session/mobile-structured-agent-session-launch.ts b/mobile/src/session/mobile-structured-agent-session-launch.ts index 9e26eaab91e..bd15e595736 100644 --- a/mobile/src/session/mobile-structured-agent-session-launch.ts +++ b/mobile/src/session/mobile-structured-agent-session-launch.ts @@ -137,18 +137,22 @@ export async function createMobileStructuredAgentSession( } } + // Why: this path distrusts the declared RpcResponse type — a malformed reply must read as + // unconfirmed, not as a refusal we can classify. if (!response || typeof response !== 'object' || typeof response.ok !== 'boolean') { return unknownCreateResult(agent, new Error(unconfirmedMessage(agent))) } if (!response.ok) { + const error = response.error as { code?: unknown; message?: unknown } | null | undefined if ( - !response.error || - typeof response.error !== 'object' || - typeof response.error.code !== 'string' + !error || + typeof error !== 'object' || + typeof error.code !== 'string' || + typeof error.message !== 'string' ) { return unknownCreateResult(agent, new Error(unconfirmedMessage(agent))) } - return classifyCreateRefusal(agent, response.error.code, response.error.message) + return classifyCreateRefusal(agent, error.code, error.message) } const result = response.result as AgentSessionMutationResult if (!result || typeof result !== 'object' || typeof result.ok !== 'boolean') { @@ -158,7 +162,8 @@ export async function createMobileStructuredAgentSession( if ( !result.refusal || typeof result.refusal !== 'object' || - typeof result.refusal.code !== 'string' + typeof result.refusal.code !== 'string' || + typeof result.refusal.message !== 'string' ) { return unknownCreateResult(agent, new Error(unconfirmedMessage(agent))) } diff --git a/mobile/src/terminal/terminal-viewport-refit-state.ts b/mobile/src/terminal/terminal-viewport-refit-state.ts index dfa4daadf4c..766345d8c73 100644 --- a/mobile/src/terminal/terminal-viewport-refit-state.ts +++ b/mobile/src/terminal/terminal-viewport-refit-state.ts @@ -1,4 +1,8 @@ import type { RpcResponse } from '../transport/types' +import { + isMethodNotFoundRefusal, + rpcObjectResultOrNull +} from '../transport/rpc-acceptance-policies' export type TerminalUpdateViewportCapability = 'unknown' | 'supported' | 'unsupported' @@ -14,17 +18,11 @@ export type TerminalViewportRefitTargetState = { } export function isTerminalUpdateViewportUpdated(response: RpcResponse): boolean { - if (!response.ok || typeof response.result !== 'object' || response.result == null) { - return false - } - return (response.result as { updated?: unknown }).updated === true + return rpcObjectResultOrNull(response)?.updated === true } export function isTerminalUpdateViewportApplied(response: RpcResponse): boolean { - if (!response.ok || typeof response.result !== 'object' || response.result == null) { - return false - } - return (response.result as { applied?: unknown }).applied === true + return rpcObjectResultOrNull(response)?.applied === true } export function resolveTerminalUpdateViewportCapability( @@ -33,7 +31,7 @@ export function resolveTerminalUpdateViewportCapability( if (response.ok) { return 'supported' } - return response.error.code === 'method_not_found' ? 'unsupported' : 'unknown' + return isMethodNotFoundRefusal(response) ? 'unsupported' : 'unknown' } // Why: defer height refits while typing, then coalesce every skipped layout diff --git a/mobile/src/transport/mobile-relay-direct-upgrade.ts b/mobile/src/transport/mobile-relay-direct-upgrade.ts index b804d6fb610..ed019262bca 100644 --- a/mobile/src/transport/mobile-relay-direct-upgrade.ts +++ b/mobile/src/transport/mobile-relay-direct-upgrade.ts @@ -21,7 +21,11 @@ import { type MobileRelayDirectUpgradeJournal } from './mobile-relay-direct-upgrade-journal' import type { RpcClient } from './rpc-client' -import type { HostProfile, RpcResponse } from './types' +import type { HostProfile } from './types' +import { + isMethodNotFoundRefusal, + requireRpcResultOrThrowCodedError +} from './rpc-acceptance-policies' export type MobileRelayDirectUpgradeResult = { host: HostProfile @@ -79,11 +83,13 @@ export async function upgradeDirectMobileRelay(args: { reqId: journal.reqId, newResumeTokenHash: journal.pendingResumeTokenHash }) - if (isMethodNotFound(provisionResponse)) { + if (isMethodNotFoundRefusal(provisionResponse)) { await dependencies.clearJournal(args.host.id) return null } - const installed = DeviceCredentialInstalledSchema.parse(requireSuccess(provisionResponse)) + const installed = DeviceCredentialInstalledSchema.parse( + requireRpcResultOrThrowCodedError(provisionResponse) + ) assertDirectInstall(journal, installed) const reconciled = await getEndpoints(args.client, journal.reqId) if (reconciled === 'method-not-found') { @@ -136,10 +142,10 @@ async function getEndpoints( installReqId: string ): Promise { const response = await client.sendRequest('pairing.getEndpoints', { installReqId }) - if (isMethodNotFound(response)) { + if (isMethodNotFoundRefusal(response)) { return 'method-not-found' } - return PairingGetEndpointsResultSchema.parse(requireSuccess(response)) + return PairingGetEndpointsResultSchema.parse(requireRpcResultOrThrowCodedError(response)) } function assertDirectInstall( @@ -162,14 +168,3 @@ function assertCommitted( throw new Error('relay credential install was not authoritatively reconciled') } } - -function requireSuccess(response: RpcResponse): unknown { - if (!response.ok) { - throw new Error(`${response.error.code}: ${response.error.message}`) - } - return response.result -} - -function isMethodNotFound(response: RpcResponse): boolean { - return !response.ok && response.error.code === 'method_not_found' -} diff --git a/mobile/src/transport/mobile-relay-pairing-recovery.ts b/mobile/src/transport/mobile-relay-pairing-recovery.ts index 26dc08620de..b37a18a06c2 100644 --- a/mobile/src/transport/mobile-relay-pairing-recovery.ts +++ b/mobile/src/transport/mobile-relay-pairing-recovery.ts @@ -25,7 +25,8 @@ import { type PairingCandidateClient } from './mobile-relay-physical-client' import { createRecoveringPairingRelayCandidate } from './pairing-relay-candidate' -import type { HostProfile, RpcResponse } from './types' +import type { HostProfile } from './types' +import { requireRpcResultOrThrowCodedError } from './rpc-acceptance-policies' export type MobileRelayPairingRecoveryResult = 'none' | 'recovered' | 'deferred' | 'abandoned' @@ -128,7 +129,7 @@ async function runRecovery( if (credential.kind === 'invite' && endpoints.installStatus?.state === 'not-found') { journal = await transitionToInviteAuthorization(journal, dependencies) const installed = DeviceCredentialInstalledSchema.parse( - requireSuccess( + requireRpcResultOrThrowCodedError( await client.sendRequest('pairing.provisionRelay', { reqId: journal.metadata.installReqId, newResumeTokenHash: journal.metadata.pendingResumeTokenHash @@ -220,7 +221,7 @@ async function getRecoveryStatus( kind: 'resume' | 'invite' ) { return PairingGetEndpointsResultSchema.parse( - requireSuccess( + requireRpcResultOrThrowCodedError( await client.sendRequest('pairing.getEndpoints', { installReqId: journal.metadata.installReqId, ...(kind === 'resume' ? { resumeConfirmReqId: journal.metadata.resumeConfirmReqId } : {}) @@ -294,13 +295,6 @@ function pairingRelay(journal: MobileRelayPairingJournal): PairingRelay { return { ...journal.metadata.relay, inviteToken: journal.secrets.inviteToken } } -function requireSuccess(response: RpcResponse): unknown { - if (!response.ok) { - throw new Error(`${response.error.code}: ${response.error.message}`) - } - return response.result -} - function assertCommitted( endpoints: ReturnType, installed: DeviceCredentialInstalled diff --git a/mobile/src/transport/mobile-relay-rpc-streams.ts b/mobile/src/transport/mobile-relay-rpc-streams.ts index abb2c12564b..3af5b2b3f5e 100644 --- a/mobile/src/transport/mobile-relay-rpc-streams.ts +++ b/mobile/src/transport/mobile-relay-rpc-streams.ts @@ -9,6 +9,7 @@ import { updateTerminalSubscriptionViewport } from './rpc-client-terminal-subscription' import { buildReadyStreamUnsubscribe } from './rpc-client-server-subscription' +import { isStreamingOpenerReply } from './rpc-acceptance-policies' import type { RpcClient } from './rpc-client' import type { RpcResponse, RpcSuccess } from './types' @@ -119,7 +120,7 @@ export class MobileRelayRpcStreams { } } } - if (response.ok && response.streaming !== true) { + if (response.ok && !isStreamingOpenerReply(response)) { this.cancelledSubscriptions.delete(response.id) } return true diff --git a/mobile/src/transport/pre-profile-pairing-coordinator.ts b/mobile/src/transport/pre-profile-pairing-coordinator.ts index a9b04320471..eb1f524334e 100644 --- a/mobile/src/transport/pre-profile-pairing-coordinator.ts +++ b/mobile/src/transport/pre-profile-pairing-coordinator.ts @@ -7,7 +7,11 @@ import { } from '../../../src/shared/mobile-relay-credential-contract' import { connect, type ConnectOptions } from './rpc-client' import { resolvePairingHostIdentity, saveHost } from './host-store' -import type { HostProfile, PairingOffer, RpcResponse } from './types' +import type { HostProfile, PairingOffer } from './types' +import { + isMethodNotFoundRefusal, + requireRpcResultOrThrowCodedError +} from './rpc-acceptance-policies' import { createMobileRelayPairingJournal, type MobileRelayPairingJournal @@ -219,7 +223,7 @@ async function runPairing( reqId: journal.metadata.installReqId, newResumeTokenHash: journal.metadata.pendingResumeTokenHash }) - if (isMethodNotFound(provision)) { + if (isMethodNotFoundRefusal(provision)) { if (winner.path !== 'direct') { throw new Error('relay pairing RPC unavailable after relay path authentication') } @@ -227,9 +231,11 @@ async function runPairing( await dependencies.clearJournal(journal.metadata.journalId) return { hostId } } - const installed = DeviceCredentialInstalledSchema.parse(requireSuccess(provision)) + const installed = DeviceCredentialInstalledSchema.parse( + requireRpcResultOrThrowCodedError(provision) + ) const endpoints = PairingGetEndpointsResultSchema.parse( - requireSuccess( + requireRpcResultOrThrowCodedError( await winner.client.sendRequest('pairing.getEndpoints', { installReqId: journal.metadata.installReqId }) @@ -283,17 +289,6 @@ function relayWebSocketUrl(relay: MobileRelayEndpoint): string { return url.toString() } -function requireSuccess(response: RpcResponse): unknown { - if (!response.ok) { - throw new Error(`${response.error.code}: ${response.error.message}`) - } - return response.result -} - -function isMethodNotFound(response: RpcResponse): boolean { - return !response.ok && response.error.code === 'method_not_found' -} - function assertCommittedInstall( status: | { state: 'not-found' } diff --git a/mobile/src/transport/rpc-acceptance-policies.test.ts b/mobile/src/transport/rpc-acceptance-policies.test.ts new file mode 100644 index 00000000000..2f2abc90e16 --- /dev/null +++ b/mobile/src/transport/rpc-acceptance-policies.test.ts @@ -0,0 +1,168 @@ +import { describe, expect, it } from 'vitest' +import type { RpcResponse } from './types' +import { + isMethodNotFoundRefusal, + isStreamingOpenerReply, + requireRpcResultOrThrowCodedError, + rpcObjectResultOrNull +} from './rpc-acceptance-policies' + +const meta = { runtimeId: 'runtime-1' } + +function success(result: unknown, streaming?: true): RpcResponse { + return { id: 'rpc-1', ok: true, result, _meta: meta, ...(streaming ? { streaming } : {}) } +} + +function refusal(code: string, message = 'Nope'): RpcResponse { + return { id: 'rpc-1', ok: false, error: { code, message }, _meta: meta } +} + +/** Every result partition a policy has to survive. */ +const resultPartitions: [string, unknown][] = [ + ['object result', { value: 1 }], + ['undefined result', undefined], + ['null result', null], + ['empty object result', {}], + ['numeric result', 7], + ['zero result', 0], + ['string result', 'done'], + ['empty string result', ''], + ['boolean result', false], + ['array result', [1, 2]], + ['empty array result', []] +] + +describe('requireRpcResultOrThrowCodedError', () => { + it.each(resultPartitions)('returns the %s untouched', (_label, result) => { + expect(requireRpcResultOrThrowCodedError(success(result))).toEqual(result) + }) + + it('returns an absent result field as undefined', () => { + const response = { id: 'rpc-1', ok: true, _meta: meta } as unknown as RpcResponse + expect(requireRpcResultOrThrowCodedError(response)).toBeUndefined() + }) + + it('throws a code-prefixed message on refusal', () => { + expect(() => requireRpcResultOrThrowCodedError(refusal('method_not_found', 'no such'))).toThrow( + 'method_not_found: no such' + ) + }) + + it('throws even when the refusal carries an empty message', () => { + expect(() => requireRpcResultOrThrowCodedError(refusal('runtime_error', ''))).toThrow( + 'runtime_error: ' + ) + }) +}) + +describe('rpcObjectResultOrNull', () => { + it('accepts a plain object result', () => { + expect(rpcObjectResultOrNull(success({ value: 1 }))).toEqual({ value: 1 }) + }) + + it('accepts an empty object result', () => { + expect(rpcObjectResultOrNull(success({}))).toEqual({}) + }) + + it('accepts an array result, because arrays are objects', () => { + expect(rpcObjectResultOrNull(success([1, 2]))).toEqual([1, 2]) + }) + + it.each([ + ['null', null], + ['undefined', undefined], + ['numeric', 7], + ['zero', 0], + ['string', 'done'], + ['empty string', ''], + ['boolean', false], + ['true', true] + ])('refuses a %s result', (_label, result) => { + expect(rpcObjectResultOrNull(success(result))).toBeNull() + }) + + it('refuses a refusal regardless of its code', () => { + expect(rpcObjectResultOrNull(refusal('method_not_found'))).toBeNull() + expect(rpcObjectResultOrNull(refusal('runtime_error'))).toBeNull() + }) +}) + +// A refusal that illegally carries success-shaped fields: without the `ok` check each of these +// would read the stray field and answer as if the call had succeeded. +describe('a refusal carrying stray success fields', () => { + const strayRefusal = { + id: 'rpc-1', + ok: false, + error: { code: 'method_not_found', message: 'Nope' }, + result: { value: 1 }, + streaming: true, + _meta: meta + } as unknown as RpcResponse + + it('yields null rather than the stray result', () => { + expect(rpcObjectResultOrNull(strayRefusal)).toBeNull() + }) + + it('is still recognised as method-not-found', () => { + expect(isMethodNotFoundRefusal(strayRefusal)).toBe(true) + }) + + // The mirror case: a success carrying a stray error must not read as a refusal. + it('does not read a success carrying a stray error as a refusal', () => { + const straySuccess = { + id: 'rpc-1', + ok: true, + result: { value: 1 }, + error: { code: 'method_not_found', message: 'Nope' }, + _meta: meta + } as unknown as RpcResponse + expect(isMethodNotFoundRefusal(straySuccess)).toBe(false) + }) +}) + +describe('isMethodNotFoundRefusal', () => { + it('matches only the method_not_found code', () => { + expect(isMethodNotFoundRefusal(refusal('method_not_found'))).toBe(true) + expect(isMethodNotFoundRefusal(refusal('runtime_error'))).toBe(false) + expect(isMethodNotFoundRefusal(refusal('METHOD_NOT_FOUND'))).toBe(false) + }) + + it('never matches a success, including one with a null result', () => { + expect(isMethodNotFoundRefusal(success(null))).toBe(false) + expect(isMethodNotFoundRefusal(success({ code: 'method_not_found' }))).toBe(false) + }) +}) + +describe('isStreamingOpenerReply', () => { + it('accepts a success flagged streaming', () => { + expect(isStreamingOpenerReply(success({ subscriptionId: 's1' }, true))).toBe(true) + }) + + it('refuses a success with no streaming flag', () => { + expect(isStreamingOpenerReply(success({ subscriptionId: 's1' }))).toBe(false) + }) + + // A truthy non-boolean off the wire must not open a stream: the registry would route it to + // handleStreamingResponse and wait for frames that never come. + it.each([['yes'], [1], [{}]])('refuses a truthy non-boolean streaming flag %j', (flag) => { + const response = { + id: 'rpc-1', + ok: true, + result: { subscriptionId: 's1' }, + streaming: flag, + _meta: meta + } as unknown as RpcResponse + expect(isStreamingOpenerReply(response)).toBe(false) + }) + + it('refuses a refusal even when it carries a streaming flag', () => { + const response = { + id: 'rpc-1', + ok: false, + error: { code: 'runtime_error', message: 'Nope' }, + streaming: true, + _meta: meta + } as unknown as RpcResponse + expect(isStreamingOpenerReply(response)).toBe(false) + }) +}) diff --git a/mobile/src/transport/rpc-acceptance-policies.ts b/mobile/src/transport/rpc-acceptance-policies.ts new file mode 100644 index 00000000000..742d94aafa7 --- /dev/null +++ b/mobile/src/transport/rpc-acceptance-policies.ts @@ -0,0 +1,33 @@ +import type { RpcResponse, RpcSuccess } from './types' + +// Named acceptance policies for RPC replies. Call sites used to hand-roll these +// predicates and did not agree with each other; each policy here preserves one +// call site's existing acceptance exactly. Do not merge two policies without +// proving every caller of both tolerates the wider or narrower set. + +/** Throws `code: message` on refusal. Diagnostic text; not user-facing. */ +export function requireRpcResultOrThrowCodedError(response: RpcResponse): unknown { + if (!response.ok) { + throw new Error(`${response.error.code}: ${response.error.message}`) + } + return response.result +} + +/** Accepts only a success whose result is a non-null object. Arrays qualify. */ +export function rpcObjectResultOrNull(response: RpcResponse): Record | null { + if (!response.ok || typeof response.result !== 'object' || response.result === null) { + return null + } + return response.result as Record +} + +export function isMethodNotFoundRefusal(response: RpcResponse): boolean { + return !response.ok && response.error.code === 'method_not_found' +} + +/** A success that opened a stream rather than delivering a terminal result. */ +export function isStreamingOpenerReply( + response: RpcResponse +): response is RpcSuccess & { streaming: true } { + return response.ok && response.streaming === true +} diff --git a/mobile/src/transport/rpc-client-stream-registry.ts b/mobile/src/transport/rpc-client-stream-registry.ts index ac20acdecc0..68e50a6e6dd 100644 --- a/mobile/src/transport/rpc-client-stream-registry.ts +++ b/mobile/src/transport/rpc-client-stream-registry.ts @@ -8,6 +8,7 @@ import { updateTerminalSubscriptionViewport } from './rpc-client-terminal-subscription' import { buildReadyStreamUnsubscribe } from './rpc-client-server-subscription' +import { isStreamingOpenerReply } from './rpc-acceptance-policies' import { isStreamingSubscriptionReadyResult, isTerminalSubscribedResult @@ -112,7 +113,7 @@ export class RpcClientStreamRegistry { } handleResponse(response: RpcResponse): boolean { - if (response.ok && response.streaming === true) { + if (isStreamingOpenerReply(response)) { this.handleStreamingResponse(response) return true }