From 3f7199923732acb00010cd4e9e0317ff336c6a0b Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Mon, 14 Sep 2026 01:31:40 -0400 Subject: [PATCH] fix(mobile): two known main bugs the RPC migration preserved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A malformed host `error` and a null settings result both reach a property read that throws. Both are deliberate behaviour changes; the goldens move in the follow-up commit. `hostReplyErrorTextOrFallback` passed a truthy non-string through under a `string` annotation. Its one caller is the in-band `git.commit` failure, and every consumer of that text is display or prompt copy: `use-mobile-create-pr-runner` and `PrSidebarCreateEmptyState` record it as a commit failure, `use-mobile-commit-failure-recovery` hands it to `summarizeCommitFailure`, which starts with `raw.slice(...).replace(...)`. So no consumer needs the value, and the decision is the fallback rather than `String(value)` — the relay handler declares `commit(): Promise<{ success: boolean; error?: string }>`, so a non-string is a malformed reply, and `generatedCommitMessageReader` in the same domain already reads a non-string host error as absent. The parameter stays `unknown`, which it honestly is, and the `SAFETY` cast is gone. `useNewWorkspaceRuntimeContext` read settings through `settingsRead`, whose reader preserves main's `boxed!.settings` throw, so a `null` or absent result threw a TypeError out of the effect — losing the trusted-hooks publish and the available-provider computation that follow it, not just the settings. It now uses `optionalSettingsRead`, the operation that already reads a null or absent result as absent settings, so the reply degrades exactly the way a reply with no `settings` member does. Reply-side only: same method, same params, same barrier, no wire change. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb --- .../use-new-workspace-runtime-context.test.ts | 99 +++++++++++++++++++ .../use-new-workspace-runtime-context.ts | 8 +- .../src/transport/rpc-refusal-message.test.ts | 43 ++++++++ mobile/src/transport/rpc-refusal-message.ts | 19 ++-- .../src/transport/settings-read-operations.ts | 2 +- 5 files changed, 154 insertions(+), 17 deletions(-) create mode 100644 mobile/src/components/use-new-workspace-runtime-context.test.ts create mode 100644 mobile/src/transport/rpc-refusal-message.test.ts diff --git a/mobile/src/components/use-new-workspace-runtime-context.test.ts b/mobile/src/components/use-new-workspace-runtime-context.test.ts new file mode 100644 index 00000000000..377e76a5083 --- /dev/null +++ b/mobile/src/components/use-new-workspace-runtime-context.test.ts @@ -0,0 +1,99 @@ +import { createElement } from 'react' +import { act, create, type ReactTestRenderer } from 'react-test-renderer' +import { afterEach, describe, expect, it, vi } from 'vitest' +import type { RpcClient } from '../transport/rpc-client' +import type { RpcResponse } from '../transport/types' +import { useNewWorkspaceRuntimeContext } from './use-new-workspace-runtime-context' + +type RuntimeContext = ReturnType + +const TRUSTED_HOOKS = { '/repo/orca.yaml': 'sha-1' } + +function reply(result: unknown): RpcResponse { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the test scripts raw host replies, not validated payloads. + return { id: 'r', ok: true, result, _meta: { runtimeId: 'runtime-1' } } as RpcResponse +} + +/** Every prerequisite answers normally; only the settings result varies. */ +function clientAnsweringSettingsWith(settingsResult: unknown): RpcClient { + const sendRequest = vi.fn(async (method: string) => { + switch (method) { + case 'settings.get': + return reply(settingsResult) + case 'ui.get': + return reply({ ui: { trustedOrcaHooks: TRUSTED_HOOKS } }) + case 'preflight.check': + return reply({ glab: { installed: false } }) + default: + return reply({ connected: false }) + } + }) + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the hook reaches only sendRequest on the client. + return { sendRequest } as unknown as RpcClient +} + +describe('useNewWorkspaceRuntimeContext', () => { + let renderer: ReactTestRenderer | null = null + + afterEach(() => { + act(() => renderer?.unmount()) + renderer = null + }) + + async function mount(settingsResult: unknown): Promise { + // One client for the whole mount: the hook keys its effect on client identity. + const client = clientAnsweringSettingsWith(settingsResult) + let context!: RuntimeContext + function Harness(): null { + context = useNewWorkspaceRuntimeContext(client, true) + return null + } + await act(async () => { + renderer = create(createElement(Harness)) + }) + await act(async () => {}) + return context + } + + it('degrades a null result the way a reply without a settings member degrades', async () => { + const absent = await mount({}) + const absentState = { + runtimeSettings: absent.runtimeSettings, + trustedOrcaHooks: absent.trustedOrcaHooks, + availableProviders: absent.availableProviders + } + act(() => renderer?.unmount()) + renderer = null + + const nullResult = await mount(null) + expect({ + runtimeSettings: nullResult.runtimeSettings, + trustedOrcaHooks: nullResult.trustedOrcaHooks, + availableProviders: nullResult.availableProviders + }).toEqual(absentState) + // The state the property-read TypeError used to skip on its way out of the effect. + expect(absentState).toEqual({ + runtimeSettings: null, + trustedOrcaHooks: TRUSTED_HOOKS, + availableProviders: ['github'] + }) + }) + + it('degrades an absent result the same way', async () => { + const context = await mount(undefined) + expect(context.runtimeSettings).toBeNull() + expect(context.trustedOrcaHooks).toEqual(TRUSTED_HOOKS) + expect(context.availableProviders).toEqual(['github']) + }) + + it('publishes the settings a host does send', async () => { + const context = await mount({ + settings: { defaultTuiAgent: 'codex', visibleTaskProviders: ['github', 'linear'] } + }) + expect(context.runtimeSettings).toEqual({ + defaultTuiAgent: 'codex', + visibleTaskProviders: ['github', 'linear'] + }) + expect(context.availableProviders).toEqual(['github']) + }) +}) diff --git a/mobile/src/components/use-new-workspace-runtime-context.ts b/mobile/src/components/use-new-workspace-runtime-context.ts index e178d714ce8..c5cc21224a7 100644 --- a/mobile/src/components/use-new-workspace-runtime-context.ts +++ b/mobile/src/components/use-new-workspace-runtime-context.ts @@ -1,4 +1,4 @@ -import { settingsRead } from '../transport/settings-read-operations' +import { optionalSettingsRead } from '../transport/settings-read-operations' import { useEffect, useState } from 'react' import type { PersistedTrustedOrcaHooks } from '../../../src/shared/orca-yaml-hook-types' import type { RpcClient } from '../transport/rpc-client' @@ -40,7 +40,7 @@ export function useNewWorkspaceRuntimeContext( client.sendRequest('linear.status') ]) const [settingsRes, uiRes] = await Promise.allSettled([ - settingsRead.request(client), + optionalSettingsRead.request(client), client.sendRequest('ui.get') ]) if (stale) { @@ -48,7 +48,9 @@ export function useNewWorkspaceRuntimeContext( } const settingsResult = - settingsRes.status === 'fulfilled' ? settingsRead.interpret(settingsRes.value) : null + settingsRes.status === 'fulfilled' + ? optionalSettingsRead.interpret(settingsRes.value) + : null const settingsValue = settingsResult?.accepted ? // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: Preserve the established response shape at this boundary. (settingsResult.value as NewWorktreeRuntimeSettings & { visibleTaskProviders?: unknown }) diff --git a/mobile/src/transport/rpc-refusal-message.test.ts b/mobile/src/transport/rpc-refusal-message.test.ts new file mode 100644 index 00000000000..2003c155b59 --- /dev/null +++ b/mobile/src/transport/rpc-refusal-message.test.ts @@ -0,0 +1,43 @@ +import { describe, expect, it } from 'vitest' +import { summarizeCommitFailure } from '../../../src/shared/source-control-commit-failure' +import { hostReplyErrorTextOrFallback, refusedRpcMessageOrFallback } from './rpc-refusal-message' + +describe('refusedRpcMessageOrFallback', () => { + it('falls back for a message-less refusal and for a non-Error throw', () => { + const messageless = new Error('cleared below') + messageless.message = '' + expect(refusedRpcMessageOrFallback(new Error('refused'), 'Commit failed')).toBe('refused') + expect(refusedRpcMessageOrFallback(messageless, 'Commit failed')).toBe('Commit failed') + expect(refusedRpcMessageOrFallback('refused', 'Commit failed')).toBe('Commit failed') + }) +}) + +describe('hostReplyErrorTextOrFallback', () => { + it('keeps a non-empty host string', () => { + expect(hostReplyErrorTextOrFallback('nothing staged', 'Commit failed')).toBe('nothing staged') + }) + + it('falls back for an absent, null or empty host error', () => { + expect(hostReplyErrorTextOrFallback(undefined, 'Commit failed')).toBe('Commit failed') + expect(hostReplyErrorTextOrFallback(null, 'Commit failed')).toBe('Commit failed') + expect(hostReplyErrorTextOrFallback('', 'Commit failed')).toBe('Commit failed') + }) + + it('falls back for a truthy non-string, which the host contract does not allow', () => { + expect(hostReplyErrorTextOrFallback({ message: 'inner refused' }, 'Commit failed')).toBe( + 'Commit failed' + ) + expect(hostReplyErrorTextOrFallback(['a'], 'Commit failed')).toBe('Commit failed') + expect(hostReplyErrorTextOrFallback(7, 'Commit failed')).toBe('Commit failed') + expect(hostReplyErrorTextOrFallback(true, 'Commit failed')).toBe('Commit failed') + }) + + // The consumer that main's pass-through broke: `.slice` on an object, `.replace` on an array. + it('yields text the commit-failure summarizer can read', () => { + for (const malformed of [{ message: 'inner refused' }, ['inner refused'], 7]) { + expect(summarizeCommitFailure(hostReplyErrorTextOrFallback(malformed, 'Commit failed'))).toBe( + 'Commit failed' + ) + } + }) +}) diff --git a/mobile/src/transport/rpc-refusal-message.ts b/mobile/src/transport/rpc-refusal-message.ts index ceb17792746..1cc1f6fc592 100644 --- a/mobile/src/transport/rpc-refusal-message.ts +++ b/mobile/src/transport/rpc-refusal-message.ts @@ -1,11 +1,6 @@ /** - * A refused operation's message, or the screen's own copy when the host sent none. - * - * Call sites spelled this as `response.error?.message || fallback`. Once the refusal arrives as - * the acceptance policy's thrown Error, the `||` has to live somewhere — and it must not also - * cover a transport rejection, whose message main surfaced verbatim, empty string included. So - * a migrated call site keeps two catches where it had two paths, and only the refusal one calls - * this. + * A refused operation's message, or the screen's own copy when the host sent none. Only the + * refusal catch may call this: a transport rejection's message is surfaced verbatim, empty included. */ export function refusedRpcMessageOrFallback(error: unknown, fallback: string): string { return (error instanceof Error ? error.message : '') || fallback @@ -14,12 +9,10 @@ export function refusedRpcMessageOrFallback(error: unknown, fallback: string): s /** * An error a host reported inside an accepted reply, or the screen's copy when it sent none. * - * Exactly `result?.error || fallback`, including for a truthy non-string: main passed that value - * through under a `string` annotation, and a downstream `.replace` then threw. Stringifying it - * here would be an improvement, but an unannounced one inside a migration whose contract is that - * no behaviour changes — so the pass-through stays and the latent throw is ticketed separately. + * The host contract declares `error` as a string, so a non-string is a malformed reply and reads + * as absent — every consumer is display or prompt text, and main's pass-through made + * `summarizeCommitFailure` throw on `.slice`. */ export function hostReplyErrorTextOrFallback(value: unknown, fallback: string): string { - // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: reproduces main's own annotation of an unvalidated host field. - return ((value as string | undefined) || fallback) as string + return (typeof value === 'string' ? value : '') || fallback } diff --git a/mobile/src/transport/settings-read-operations.ts b/mobile/src/transport/settings-read-operations.ts index f33471dcaf2..8a17f0a6c2f 100644 --- a/mobile/src/transport/settings-read-operations.ts +++ b/mobile/src/transport/settings-read-operations.ts @@ -49,7 +49,7 @@ export const settingsRead = bindDeferredRpcOperation( }) ) -/** History resume and repo labels historically tolerate an absent or null result. */ +/** A null or absent result reads as absent settings instead of throwing the property read. */ export const optionalSettingsRead = bindDeferredRpcOperation( defineRpcOperation({ name: 'settings.optional-member-or-skip',