From 1a46ef4fa2fbfd4c81bfe9892b0c8b9440f7e130 Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Tue, 15 Sep 2026 22:59:54 -0400 Subject: [PATCH] feat(mobile): validate the source-control domain's RPC replies at arrival Replaces all 17 unchecked readers in mobile/src/source-control/ with `rpcResultVariant(variant, schema)`, so a malformed reply is an `RpcIncompatibleReplyError` naming the operation instead of a TypeError three frames downstream. The inventory drops 201 -> 184 and the five source-control operations files leave it entirely. This is a behaviour change, scoped to malformed replies. Six reply-matrix goldens move; every named-scenario golden and every `normal` partition is byte-identical, which is the parity claim. Schemas live one module per reply domain, beside the operations that read them: git-status, git-compare, git-history, hosted-review and worktree-metadata. A member is required only where a consumer reads it unguarded, and each schema records the consumer line that justifies it. Nothing is `.strict()`; every reply a consumer publishes verbatim keeps `z.looseObject` so an undeclared host member still passes through. Six replies have no reader anywhere in mobile and get `z.unknown()`, which is the honest schema for them, not a holdout. Three readers stay total by construction, because their contract is that an unreadable reply is a value rather than an error: the `git.status` projection (a null status three screens route on), the `session.tabs.list` reveal (a null list means poll again) and the generated commit message (a screen's copy, never a decode error in a text field). They gain the salvage report, not a verdict. Consumers take the schema's output type, so `MobileGitStatusResult` and the branch-compare aliases now name what mobile reads rather than the desktop aggregate, and seven call-site casts are gone. Three requirements came from the goldens, not from the host types: `git.history` sends `timestamp: null`, `hostedReview.getCreationEligibility` sends a `reviewLookupOutcome` the shared union does not list, and the `git.status` projection writes an absent member as a present `undefined`. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb --- .../src/session/mobile-diff-review-loaders.ts | 2 +- .../use-mobile-diff-review-controller.ts | 2 +- .../source-control/MobileGitHistoryList.tsx | 7 +- .../git-compare-reply-schema.ts | 104 +++++++++++ .../git-history-reply-schema.ts | 42 +++++ .../source-control/git-status-reply-schema.ts | 168 ++++++++++++++++++ .../hosted-review-reply-schema.ts | 67 +++++++ .../source-control/mobile-branch-compare.ts | 15 +- .../source-control/mobile-create-pr-action.ts | 6 +- .../src/source-control/mobile-git-history.ts | 16 +- .../mobile-git-mutation-operations.ts | 127 ++++++++----- .../mobile-git-read-operations.ts | 46 ++--- .../src/source-control/mobile-git-status.ts | 21 +-- .../mobile-hosted-review-operations.ts | 27 ++- .../mobile-hosted-review-service.ts | 33 ++-- .../mobile-source-control-screen-state.ts | 14 +- .../mobile-source-file-open-operations.ts | 75 ++++---- .../mobile-worktree-metadata-operations.ts | 44 ++--- .../use-mobile-hosted-review-eligibility.ts | 4 +- .../use-mobile-source-control-loaders.ts | 16 +- .../use-mobile-source-control-openers.ts | 8 +- .../worktree-metadata-reply-schema.ts | 24 +++ .../unchecked-rpc-reader-inventory.ts | 8 +- 23 files changed, 660 insertions(+), 216 deletions(-) create mode 100644 mobile/src/source-control/git-compare-reply-schema.ts create mode 100644 mobile/src/source-control/git-history-reply-schema.ts create mode 100644 mobile/src/source-control/git-status-reply-schema.ts create mode 100644 mobile/src/source-control/hosted-review-reply-schema.ts create mode 100644 mobile/src/source-control/worktree-metadata-reply-schema.ts diff --git a/mobile/src/session/mobile-diff-review-loaders.ts b/mobile/src/session/mobile-diff-review-loaders.ts index 636275af68a..e213dbac6a5 100644 --- a/mobile/src/session/mobile-diff-review-loaders.ts +++ b/mobile/src/session/mobile-diff-review-loaders.ts @@ -122,7 +122,7 @@ export async function loadMobileDiffReviewSnapshot( const normalizedReviewState = normalizeMobileDiffReviewState(metadata.mobileDiffReview) const branchEntries = branch.result && canOpenMobileBranchCompareDiff(branch.result.summary) - ? branch.result.entries + ? (branch.result.entries ?? []) : [] const queue = buildMobileDiffReviewQueue({ worktreeId, diff --git a/mobile/src/session/use-mobile-diff-review-controller.ts b/mobile/src/session/use-mobile-diff-review-controller.ts index 5d74bccf156..3b048ba3b52 100644 --- a/mobile/src/session/use-mobile-diff-review-controller.ts +++ b/mobile/src/session/use-mobile-diff-review-controller.ts @@ -119,7 +119,7 @@ export function useMobileDiffReviewController(input: ControllerInput) { } const branchEntries = screenState.branchCompare && canOpenMobileBranchCompareDiff(screenState.branchCompare.summary) - ? screenState.branchCompare.entries + ? (screenState.branchCompare.entries ?? []) : [] return buildMobileDiffReviewQueue({ worktreeId, diff --git a/mobile/src/source-control/MobileGitHistoryList.tsx b/mobile/src/source-control/MobileGitHistoryList.tsx index babc4401b36..0daf598b281 100644 --- a/mobile/src/source-control/MobileGitHistoryList.tsx +++ b/mobile/src/source-control/MobileGitHistoryList.tsx @@ -6,13 +6,13 @@ import type { ConnectionState } from '../transport/types' import type { RpcClient } from '../transport/rpc-client' import { useForceReconnect } from '../transport/client-context' import { gitCommitCompareRead } from './mobile-git-read-operations' +import type { MobileGitChangedFile } from './git-compare-reply-schema' import { fetchMobileGitHistory, mapMobileCommitRows, type MobileCommitRow } from './mobile-git-history' import { resolveMobileHistoryScreenView } from './mobile-history-screen-state' -import type { GitBranchChangeEntry } from '../../../src/shared/git-diff-compare-types' type Props = { client: RpcClient | null @@ -42,7 +42,7 @@ export const MobileGitHistoryList = memo(function MobileGitHistoryList({ const [error, setError] = useState(null) const [reloadNonce, setReloadNonce] = useState(0) const [expanded, setExpanded] = useState(null) - const [filesById, setFilesById] = useState>({}) + const [filesById, setFilesById] = useState>({}) // Host or worktree identity change must wipe history immediately — even while // disconnected — so a kept-mounted hub segment never shows another tree's commits. @@ -110,8 +110,7 @@ export const MobileGitHistoryList = memo(function MobileGitHistoryList({ .request(client, { worktree: `id:${worktreeId}`, commitId }) .then((reply) => { const compared = gitCommitCompareRead.interpret(reply) - // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: Preserve the established response shape at this boundary. - const entries = compared.accepted ? (compared.value as GitBranchChangeEntry[]) : [] + const entries = compared.accepted ? compared.value.entries : [] if (!stale) { setFilesById((prev) => ({ ...prev, [commitId]: entries })) } diff --git a/mobile/src/source-control/git-compare-reply-schema.ts b/mobile/src/source-control/git-compare-reply-schema.ts new file mode 100644 index 00000000000..51dc854be8a --- /dev/null +++ b/mobile/src/source-control/git-compare-reply-schema.ts @@ -0,0 +1,104 @@ +import { z } from 'zod' +import { salvagedOptional, salvagingArray } from '../../../src/shared/zod-salvage' + +// The three compare replies the Changes screen and the history list read: `git.branchCompare`, +// `git.commitCompare` and `git.branchDiff`. Checked against GitBranchCompareResult, +// GitCommitCompareResult and GitDiffResult in src/shared/git-diff-compare-types.ts, which +// src/main/runtime/rpc/methods/git.ts returns from the runtime compare calls verbatim. + +const GIT_BRANCH_CHANGE_STATUS = ['modified', 'added', 'deleted', 'renamed', 'copied'] as const +const GIT_BRANCH_COMPARE_STATUS = [ + 'ready', + 'invalid-base', + 'unborn-head', + 'no-merge-base', + 'loading', + 'error' +] as const + +/** + * One changed file as the commit list reads it: `path` only. + * + * MobileGitHistoryList.tsx:171 keys the row by `path` and :176-177 renders `added`/`removed` + * behind a truthy check. It never reads `status`, so this list does not require it — the two + * compare replies share a host type but not a set of readers. + */ +export const gitChangedFileSchema = z.looseObject({ + path: z.string(), + status: z.enum(GIT_BRANCH_CHANGE_STATUS).optional(), + oldPath: z.string().optional(), + added: z.number().optional(), + removed: z.number().optional() +}) + +/** + * The same file as the branch list reads it, which additionally colours and labels the row by + * `status` with no guard (MobileSourceControlFileRows.tsx:219-220) and sorts on `path` + * (mobile-branch-compare.ts:27). + */ +export const gitBranchChangeEntrySchema = gitChangedFileSchema.extend({ + status: z.enum(GIT_BRANCH_CHANGE_STATUS) +}) + +/** + * The compare summary. `baseRef`, `changedFiles` and `status` are required: formatMobileBranch- + * CompareSummary reads all three unguarded (mobile-branch-compare.ts:38-45), and + * use-mobile-source-control-state.ts:149 reads `summary.status` again. `headOid` and `mergeBase` + * gate the branch-diff open (:50) and are nullable in the host type, so both stay nullable here. + */ +export const gitBranchCompareSummarySchema = z.looseObject({ + baseRef: z.string(), + changedFiles: z.number(), + status: z.enum(GIT_BRANCH_COMPARE_STATUS), + baseOid: z.string().nullable().optional(), + compareRef: z.string().optional(), + headOid: z.string().nullable().optional(), + mergeBase: z.string().nullable().optional(), + commitsAhead: salvagedOptional('commitsAhead', z.number()), + errorMessage: salvagedOptional('errorMessage', z.string()) +}) + +/** + * `summary` is required: use-mobile-source-control-state.ts:131/134/149 and + * MobileSourceControlFileRows.tsx:190 reach it with only the result null-checked. `entries` is + * optional because :127 reads `branchCompareResult?.entries ?? []` — a reply without a list is an + * empty section today, and making it fatal would turn that into a full-screen error. + */ +export const gitBranchCompareResultSchema = z.looseObject({ + summary: gitBranchCompareSummarySchema, + entries: salvagingArray(gitBranchChangeEntrySchema).optional() +}) + +/** The commit-compare list. Only `entries` has a reader: MobileGitHistoryList.tsx:114-115. */ +export const gitCommitCompareResultSchema = z.looseObject({ + entries: salvagingArray(gitChangedFileSchema) +}) + +/** + * A single file's diff. + * + * Two declared shapes rather than one: mobile renders only `kind: 'text'` and throws its own copy + * on anything else (use-mobile-source-control-openers.ts:264-267). So the text shape requires the + * two contents it renders, and every other kind needs nothing but a `kind` to route on — including + * a kind a newer host adds, which must reach that same throw rather than an incompatible reply. + */ +const gitDiffTextSchema = z.looseObject({ + kind: z.literal('text'), + originalContent: z.string(), + modifiedContent: z.string() +}) + +// Renamed to one discriminant so the consumer's `kind !== 'text'` still narrows; the host's own +// kind rides along for a diagnostic rather than being thrown away. +const gitDiffOtherKindSchema = z + .object({ kind: z.string() }) + .transform((value) => ({ kind: 'not-text' as const, hostKind: value.kind })) + +export const gitDiffResultSchema = z.union([gitDiffTextSchema, gitDiffOtherKindSchema]) + +export type MobileGitChangedFile = z.output +export type MobileGitBranchChangeEntry = z.output +export type MobileGitBranchCompareSummary = z.output +export type MobileGitBranchCompareReply = z.output +export type MobileGitCommitCompareReply = z.output +export type MobileGitDiffReply = z.output diff --git a/mobile/src/source-control/git-history-reply-schema.ts b/mobile/src/source-control/git-history-reply-schema.ts new file mode 100644 index 00000000000..5ea7e649fd7 --- /dev/null +++ b/mobile/src/source-control/git-history-reply-schema.ts @@ -0,0 +1,42 @@ +import { z } from 'zod' +import { salvagingArray } from '../../../src/shared/zod-salvage' + +// `git.history` reply. Checked against GitHistoryResult in src/shared/git-history-types.ts, which +// src/main/runtime/rpc/methods/git.ts returns from getRuntimeGitHistory verbatim. +// +// The host publishes nine members; mobile reads one. `currentRef`, `remoteRef`, `baseRef`, +// `mergeBase`, `hasIncomingChanges`, `hasOutgoingChanges`, `hasMore` and `limit` have no reader in +// mobile/, so they are undeclared and pass through rather than becoming eight ways for an older +// host to fail. + +/** + * One commit row. + * + * `id` and `parentIds` are required: mobile-git-history.ts:48 calls `item.id.slice(0, 7)` and :51 + * indexes `item.parentIds[0]`, both of which throw on absence. `subject`, `author`, `displayId` and + * `timestamp` are read through `||`, `??` or a nullish check on the same lines, so they are + * optional here even where the host type declares them required — an older host that omits one + * must still render a history list. + */ +export const gitHistoryItemSchema = z.looseObject({ + id: z.string(), + parentIds: z.array(z.string()), + subject: z.string().optional(), + displayId: z.string().optional(), + author: z.string().optional(), + // Nullish, not optional: the host sends `timestamp: null` for a commit with no date, and + // formatCommitTime's guard is `== null`. A number-or-absent schema would drop the whole row. + timestamp: z.number().nullish() +}) + +/** + * `items` is required and fatal on absence: mapMobileCommitRows maps it with no guard, which is the + * `Cannot read properties of null (reading 'items')` main records for every malformed partition. + * A salvaging array so one unreadable commit drops out of the list instead of emptying the screen. + */ +export const gitHistoryResultSchema = z.looseObject({ + items: salvagingArray(gitHistoryItemSchema) +}) + +export type MobileGitHistoryResult = z.output +export type MobileGitHistoryItem = z.output diff --git a/mobile/src/source-control/git-status-reply-schema.ts b/mobile/src/source-control/git-status-reply-schema.ts new file mode 100644 index 00000000000..301ac7e114c --- /dev/null +++ b/mobile/src/source-control/git-status-reply-schema.ts @@ -0,0 +1,168 @@ +import { z } from 'zod' +import { salvagedOptional, salvagingArray } from '../../../src/shared/zod-salvage' + +// Schemas for the two `git.status` replies mobile reads. Checked against the host's published +// shape in src/shared/git-status-types.ts (GitStatusResult / GitUncommittedEntry), which is what +// src/main/runtime/rpc/methods/git.ts returns from getRuntimeGitStatus verbatim. +// +// A member is required only where a consumer reads it without a guard. Everything the host may +// send and mobile does not read is undeclared and passes through: a schema that required a field +// an older host omits would turn every reply from that host into an incompatible error. + +const GIT_FILE_STATUS = ['modified', 'added', 'deleted', 'renamed', 'untracked', 'copied'] as const +const GIT_STAGING_AREA = ['staged', 'unstaged', 'untracked'] as const +const GIT_CONFLICT_STATUS = ['unresolved', 'resolved_locally'] as const +const GIT_CONFLICT_SOURCE = ['git', 'session'] as const +const GIT_CONFLICT_KIND = [ + 'both_modified', + 'both_added', + 'both_deleted', + 'added_by_us', + 'added_by_them', + 'deleted_by_us', + 'deleted_by_them' +] as const +const GIT_CONFLICT_OPERATION = ['merge', 'rebase', 'cherry-pick', 'unknown'] as const + +/** + * One working-tree entry. + * + * `path`, `status` and `area` are required because every list read reaches them unguarded: + * use-mobile-source-control-state.ts:122 sections by `entry.area`, mobile-git-status.ts:63 sorts on + * `entry.path`, and MOBILE_GIT_STATUS_LABELS is indexed by `entry.status`. The rest are optional in + * the host type and read through a guard or a default, so they stay optional here. + */ +export const gitStatusEntrySchema = z.looseObject({ + path: z.string(), + status: z.enum(GIT_FILE_STATUS), + area: z.enum(GIT_STAGING_AREA), + oldPath: z.string().optional(), + conflictKind: z.enum(GIT_CONFLICT_KIND).optional(), + conflictStatus: z.enum(GIT_CONFLICT_STATUS).optional(), + conflictStatusSource: z.enum(GIT_CONFLICT_SOURCE).optional(), + added: z.number().optional(), + removed: z.number().optional() +}) + +export const gitUpstreamStatusSchema = z.looseObject({ + hasUpstream: z.boolean(), + ahead: z.number(), + behind: z.number(), + upstreamName: z.string().optional(), + hasConfiguredPushTarget: z.boolean().optional(), + behindCommitsArePatchEquivalent: z.boolean().optional() +}) + +/** + * The verbatim host payload the Changes screen publishes into its ready state. + * + * Only `entries` is required: mobile-hosted-review-create-intent.ts:70 reads + * `status?.entries.some(...)`, which throws on a status object without it. Every other consumer + * optional-chains — MobileSourceControlPanel.tsx:126/274, use-mobile-source-control-state.ts:163. + * A salvaging array so one unreadable row drops instead of failing the whole screen's reply, and + * salvagedOptional so a malformed optional reads as absent rather than as an incompatible reply. + */ +export const gitStatusHostPayloadSchema = z.looseObject({ + entries: salvagingArray(gitStatusEntrySchema), + conflictOperation: salvagedOptional('conflictOperation', z.enum(GIT_CONFLICT_OPERATION)), + branch: salvagedOptional('branch', z.string()), + head: salvagedOptional('head', z.string()), + upstreamStatus: salvagedOptional('upstreamStatus', gitUpstreamStatusSchema) +}) + +export type MobileGitStatusEntry = z.output +export type MobileGitStatusHostPayload = z.output +export type MobileGitUpstreamStatus = z.output +type MobileProjectedUpstreamStatus = z.output +type GitConflictOperation = (typeof GIT_CONFLICT_OPERATION)[number] + +/** What readMobileGitStatusResult publishes: the host payload narrowed to five members. */ +export type MobileGitStatusProjection = { + entries: MobileGitStatusEntry[] + conflictOperation: GitConflictOperation + branch: string | undefined + head: string | undefined + upstreamStatus: MobileProjectedUpstreamStatus | undefined +} + +// The projection's own shapes: the fields readMobileGitStatusResult kept, nothing else, and every +// one of them written out even when absent. Not loose, and not zod's own optional-key omission — +// main built these objects by hand, so an absent member is a present `undefined`, which is what +// the recorded projections carry. +const projectedUpstreamStatusSchema = z + .object({ + hasUpstream: z.boolean(), + ahead: z.number().finite(), + behind: z.number().finite(), + upstreamName: salvagedOptional('upstreamName', z.string()), + hasConfiguredPushTarget: salvagedOptional('hasConfiguredPushTarget', z.boolean()), + behindCommitsArePatchEquivalent: salvagedOptional( + 'behindCommitsArePatchEquivalent', + z.boolean() + ) + }) + .transform((value) => ({ + hasUpstream: value.hasUpstream, + upstreamName: value.upstreamName, + ahead: value.ahead, + behind: value.behind, + hasConfiguredPushTarget: value.hasConfiguredPushTarget, + behindCommitsArePatchEquivalent: value.behindCommitsArePatchEquivalent + })) + +const projectedEntrySchema = z.object({ + // `.min(1)` because main's `!path` drop is falsy, not nullish: an empty path was never a row. + path: z.string().min(1), + status: z.enum(GIT_FILE_STATUS), + area: z.enum(GIT_STAGING_AREA), + oldPath: salvagedOptional('oldPath', z.string()), + conflictStatus: salvagedOptional('conflictStatus', z.enum(GIT_CONFLICT_STATUS)), + conflictStatusSource: salvagedOptional('conflictStatusSource', z.enum(GIT_CONFLICT_SOURCE)), + added: salvagedOptional('added', z.number().finite()), + removed: salvagedOptional('removed', z.number().finite()) +}) + +/** + * The normalized projection hosted-review preparation and the diff-review loaders read. + * + * `.catch(null)` keeps this reader's verdict exactly where main put it: a payload that is not a + * record, or whose `entries` is not an array, is a decoded `null`, not an incompatible reply. + * Three call sites route on that null — a refused status must leave their screens alone — so + * tightening it is a product decision with its own expectation, not part of this step. + * The gain here is the salvage report: a dropped row now names its index instead of vanishing. + */ +export const gitStatusProjectionSchema: z.ZodType = z + .object({ + entries: salvagingArray(projectedEntrySchema), + conflictOperation: z.unknown().optional(), + branch: salvagedOptional('branch', z.string()), + head: salvagedOptional('head', z.string()), + upstreamStatus: salvagedOptional('upstreamStatus', projectedUpstreamStatusSchema) + }) + .transform((value): MobileGitStatusProjection => ({ + entries: value.entries.map((entry) => ({ + path: entry.path, + status: entry.status, + area: entry.area, + oldPath: entry.oldPath, + // Never projected: main dropped the host's conflictKind and stamped undefined instead. + conflictKind: undefined, + conflictStatus: entry.conflictStatus, + conflictStatusSource: entry.conflictStatusSource, + added: entry.added, + removed: entry.removed + })), + conflictOperation: readProjectedConflictOperation(value.conflictOperation), + branch: value.branch, + head: value.head, + upstreamStatus: value.upstreamStatus + })) + .nullable() + .catch(null) + +// Main coerced an unreadable operation to 'unknown' rather than dropping the reply; four screens +// render off that value, so the coercion is the behaviour, not a defect. +function readProjectedConflictOperation(value: unknown): GitConflictOperation { + const parsed = z.enum(GIT_CONFLICT_OPERATION).safeParse(value) + return parsed.success ? parsed.data : 'unknown' +} diff --git a/mobile/src/source-control/hosted-review-reply-schema.ts b/mobile/src/source-control/hosted-review-reply-schema.ts new file mode 100644 index 00000000000..3b4c84bb8d9 --- /dev/null +++ b/mobile/src/source-control/hosted-review-reply-schema.ts @@ -0,0 +1,67 @@ +import { z } from 'zod' +import { salvagedOptional } from '../../../src/shared/zod-salvage' + +// `hostedReview.getCreationEligibility` and `hostedReview.create`. Checked against +// HostedReviewCreationEligibility and CreateHostedReviewResult in src/shared/hosted-review.ts, +// which src/main/runtime/rpc/methods/hosted-review.ts:31-57 returns from the runtime service +// verbatim. Both answer in-band, so an accepted reply can still say no. + +const HOSTED_REVIEW_PROVIDER = [ + 'github', + 'gitlab', + 'bitbucket', + 'azure-devops', + 'gitea', + 'unsupported' +] as const + +/** + * Creation eligibility. + * + * `provider` is the one required member: mobile-hosted-review-service.ts:120 puts it in the + * prefill's required `provider` slot, and hostedReviewCopy() routes the whole compose form's copy + * off it. Everything on :121-126 is read through `||` or lands in an optional prefill slot. + * + * The three routing tokens are strings, not the shared type's closed unions, because the host's + * vocabulary is already wider than the type: the recorded `sc-eligibility-fetched` reply carries + * `reviewLookupOutcome: 'none'`, which HostedReviewLookupOutcome does not list. Mobile only ever + * compares them to a handful of literals, so passing an unrecognised token through is both what + * main did and what keeps a newer host from blocking create on this screen. + */ +export const hostedReviewEligibilitySchema = z.looseObject({ + provider: z.enum(HOSTED_REVIEW_PROVIDER), + canCreate: salvagedOptional('canCreate', z.boolean()), + blockedReason: salvagedOptional('blockedReason', z.string().nullable()), + nextAction: salvagedOptional('nextAction', z.string().nullable()), + reviewLookupOutcome: salvagedOptional('reviewLookupOutcome', z.string()), + defaultBaseRef: salvagedOptional('defaultBaseRef', z.string().nullable()), + title: salvagedOptional('title', z.string().nullable()), + body: salvagedOptional('body', z.string().nullable()) +}) + +const hostedReviewSummarySchema = z.looseObject({ url: z.string(), number: z.number().optional() }) + +/** + * Creation outcome, declared as the host's own two arms. + * + * The success arm requires `number` and `url` because :262 hands both to the link step, which + * writes `number` into worktree metadata. The failure arm requires `error`: :205 returns it as the + * form's message and :208 calls `.replace` on it, which is where main threw a TypeError on a + * refusal that omitted it. `existingReview` is probed with `?.` on :264, so it stays optional. + */ +export const hostedReviewCreateOkSchema = z.looseObject({ + ok: z.literal(true), + number: z.number(), + url: z.string() +}) + +export const hostedReviewCreateFailedSchema = z.looseObject({ + ok: z.literal(false), + error: z.string(), + code: z.string().optional(), + existingReview: salvagedOptional('existingReview', hostedReviewSummarySchema) +}) + +export type MobileHostedReviewEligibilityReply = z.output +export type MobileHostedReviewCreateOk = z.output +export type MobileHostedReviewCreateFailed = z.output diff --git a/mobile/src/source-control/mobile-branch-compare.ts b/mobile/src/source-control/mobile-branch-compare.ts index 0cbd7bdab79..441962e267e 100644 --- a/mobile/src/source-control/mobile-branch-compare.ts +++ b/mobile/src/source-control/mobile-branch-compare.ts @@ -1,12 +1,13 @@ import type { - GitBranchChangeEntry, - GitBranchCompareResult, - GitBranchCompareSummary -} from '../../../src/shared/git-diff-compare-types' + MobileGitBranchChangeEntry, + MobileGitBranchCompareReply, + MobileGitBranchCompareSummary +} from './git-compare-reply-schema' -export type MobileGitBranchChangeEntry = GitBranchChangeEntry -export type MobileGitBranchCompareSummary = GitBranchCompareSummary -export type MobileGitBranchCompareResult = GitBranchCompareResult +// The shapes mobile reads off the compare replies are the reply schemas' outputs, not the desktop +// aggregates: a member with no reader in mobile/ is stripped rather than re-declared here. +export type { MobileGitBranchChangeEntry, MobileGitBranchCompareSummary } +export type MobileGitBranchCompareResult = MobileGitBranchCompareReply export type MobileBranchCompareSection< TEntry extends MobileGitBranchChangeEntry = MobileGitBranchChangeEntry diff --git a/mobile/src/source-control/mobile-create-pr-action.ts b/mobile/src/source-control/mobile-create-pr-action.ts index 3268a1796c3..3edd3b94d35 100644 --- a/mobile/src/source-control/mobile-create-pr-action.ts +++ b/mobile/src/source-control/mobile-create-pr-action.ts @@ -1,12 +1,12 @@ -import type { HostedReviewCreationEligibility } from '../../../src/shared/hosted-review' +import type { MobileHostedReviewEligibilityReply } from './hosted-review-reply-schema' import { supportsHostedReviewCreation } from '../../../src/shared/hosted-review-creation-providers' import { hostedReviewCopy } from './hosted-review-copy' import { getMobilePrCreateBlockMessage } from './mobile-pr-create' export type MobileCreatePrEligibilityState = | { kind: 'idle' } - | { kind: 'loading'; eligibility: HostedReviewCreationEligibility | null } - | { kind: 'ready'; eligibility: HostedReviewCreationEligibility } + | { kind: 'loading'; eligibility: MobileHostedReviewEligibilityReply | null } + | { kind: 'ready'; eligibility: MobileHostedReviewEligibilityReply } | { kind: 'error' } export type MobileCreatePrAction = { diff --git a/mobile/src/source-control/mobile-git-history.ts b/mobile/src/source-control/mobile-git-history.ts index 53795b63b44..0b555dd7926 100644 --- a/mobile/src/source-control/mobile-git-history.ts +++ b/mobile/src/source-control/mobile-git-history.ts @@ -1,4 +1,4 @@ -import type { GitHistoryItem, GitHistoryResult } from '../../../src/shared/git-history-types' +import type { MobileGitHistoryItem, MobileGitHistoryResult } from './git-history-reply-schema' import { refusedRpcMessageOrFallback } from '../transport/rpc-refusal-message' import { gitHistoryRead } from './mobile-git-read-operations' import type { RpcOperationSender } from '../transport/rpc-operation-sender' @@ -14,7 +14,7 @@ export type MobileCommitRow = { // Short relative time for a commit list (just now / Xm / Xh / Xd / Xmo / Xy). // `timestampMs` is epoch ms, the unit GitHistoryItem.timestamp already carries. -export function formatCommitTime(timestampMs: number | undefined, nowMs: number): string { +export function formatCommitTime(timestampMs: number | null | undefined, nowMs: number): string { // Nullish — not falsy — so a real epoch-0 timestamp still formats. if (timestampMs == null) { return '' @@ -42,7 +42,7 @@ export function formatCommitTime(timestampMs: number | undefined, nowMs: number) return `${Math.floor(months / 12)}y` } -export function toMobileCommitRow(item: GitHistoryItem, nowMs: number): MobileCommitRow { +export function toMobileCommitRow(item: MobileGitHistoryItem, nowMs: number): MobileCommitRow { return { id: item.id, shortId: item.displayId ?? item.id.slice(0, 7), @@ -53,7 +53,10 @@ export function toMobileCommitRow(item: GitHistoryItem, nowMs: number): MobileCo } } -export function mapMobileCommitRows(result: GitHistoryResult, nowMs: number): MobileCommitRow[] { +export function mapMobileCommitRows( + result: MobileGitHistoryResult, + nowMs: number +): MobileCommitRow[] { return result.items.map((item) => toMobileCommitRow(item, nowMs)) } @@ -61,12 +64,11 @@ export async function fetchMobileGitHistory( client: RpcOperationSender, worktreeId: string, limit = 50 -): Promise { +): Promise { // Not inside the try: a transport rejection must reach the caller as the original error object. const reply = await gitHistoryRead.request(client, { worktree: `id:${worktreeId}`, limit }) try { - // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: Preserve the established response shape at this boundary. - return gitHistoryRead.interpret(reply) as GitHistoryResult + return gitHistoryRead.interpret(reply) } catch (error) { throw new Error(refusedRpcMessageOrFallback(error, 'Failed to load commit history')) } diff --git a/mobile/src/source-control/mobile-git-mutation-operations.ts b/mobile/src/source-control/mobile-git-mutation-operations.ts index be5a11720f8..1962c7eee61 100644 --- a/mobile/src/source-control/mobile-git-mutation-operations.ts +++ b/mobile/src/source-control/mobile-git-mutation-operations.ts @@ -1,6 +1,6 @@ +import { z } from 'zod' import { bindDeferredRpcOperation, defineRpcOperation } from '../transport/rpc-operation' -import type { RpcCompatibleReader } from '../transport/rpc-operation-contract' -import { rpcReadUnchecked, rpcUncheckedPayloadReader } from '../transport/rpc-reader-payload' +import { rpcResultVariant } from '../transport/rpc-operation-result-reader' // Mirrors the host GenerateCommitMessageResult (src/main/text-generation/ // commit-message-text-generation.ts) — a single resolved result, not a stream. @@ -11,32 +11,40 @@ export type MobileGenerateCommitMessageResult = // Host-state changes. A lost reply here is unknown, never failed: none of these operations // interprets a transport rejection, so the delivery-unknown marker reaches the caller intact. -/** Exactly `result?.key`, so a null or absent commit payload reads as absent, not as a throw. */ -function optionalPayloadMember(raw: unknown, key: string): unknown { - return raw == null ? undefined : Object(raw)[key] -} - -const gitCommitOutcomeReader: RpcCompatibleReader< - unknown, - 'commit-outcome', - { success: unknown; error: unknown } -> = (raw) => - rpcReadUnchecked('commit-outcome', { - success: optionalPayloadMember(raw, 'success'), - error: optionalPayloadMember(raw, 'error') +/** + * git.commit answers in-band: an accepted reply can still carry `success: false`. + * + * Nullish and every member optional because that is exactly what the consumer tolerates — + * mobile-hosted-review-git-preparation.ts:107 compares `outcome.success === true` and :109 passes + * `outcome.error` through hostReplyErrorTextOrFallback, which already reads a non-string as absent. + * An absent or null payload stays a failed commit carrying the screen's copy, which is main's + * documented contract for this reply ("reads as absent, not as a throw"), not an accident. + * A present but non-boolean `success` is a malformed reply and now says so. + */ +const gitCommitOutcomeSchema = z + .object({ + success: z.boolean().optional(), + error: z.unknown().optional() }) -/** git.commit answers in-band: an accepted reply can still carry `success: false`. */ + .nullish() + .transform((value) => ({ success: value?.success, error: value?.error })) + export const gitCommitRun = bindDeferredRpcOperation( defineRpcOperation({ name: 'git.commit-staged', method: 'git.commit', acceptance: 'require-result-or-throw-message', barrier: 'after-caller-barrier', - read: gitCommitOutcomeReader + read: rpcResultVariant('commit-outcome', gitCommitOutcomeSchema) }) ) +// Three replies with no reader anywhere in mobile: the caller needs acceptance and nothing else. +// `z.unknown()` is the honest schema for that, not a holdout — there is no member to require, and +// requiring a shape mobile never looks at would reject hosts for no gain. +const unreadPayload = z.unknown() + /** Publish, push and force-with-lease are one operation; only the params differ. */ export const gitPushRun = bindDeferredRpcOperation( defineRpcOperation({ @@ -44,7 +52,7 @@ export const gitPushRun = bindDeferredRpcOperation( method: 'git.push', acceptance: 'require-result-or-throw-message', barrier: 'after-caller-barrier', - read: rpcUncheckedPayloadReader('push-accepted') + read: rpcResultVariant('push-accepted', unreadPayload) }) ) @@ -54,37 +62,64 @@ export const gitBulkStageRun = bindDeferredRpcOperation( method: 'git.bulkStage', acceptance: 'require-result-or-throw-message', barrier: 'after-caller-barrier', - read: rpcUncheckedPayloadReader('stage-accepted') + read: rpcResultVariant('stage-accepted', unreadPayload) }) ) const GENERATE_FAILED = 'Failed to generate commit message' -// Normalizes the host GenerateCommitMessageResult into the discriminated result the UI switches -// on. A malformed `{ success:false }` could leave `error` undefined, which breaks that contract, -// so the message is always coerced to a non-empty string. -const generatedCommitMessageReader: RpcCompatibleReader< - unknown, - 'generated-commit-message', - MobileGenerateCommitMessageResult -> = (raw) => { - if (!raw || typeof raw !== 'object') { - return rpcReadUnchecked('generated-commit-message', { success: false, error: GENERATE_FAILED }) - } - const result: { success?: unknown; message?: unknown; error?: unknown; canceled?: unknown } = raw - if (result.success === true && typeof result.message === 'string' && result.message.length > 0) { - return rpcReadUnchecked('generated-commit-message', { success: true, message: result.message }) - } - const hostError = - result.success === false && typeof result.error === 'string' && result.error.length > 0 - ? result.error - : 'No commit message generated' - return rpcReadUnchecked('generated-commit-message', { - success: false, - error: hostError, - ...(result.success === false && result.canceled ? { canceled: true } : {}) - }) -} +/** + * Normalizes the host GenerateCommitMessageResult into the discriminated result the UI switches on. + * + * Always compatible by construction: every shape maps to a declared outcome, because a malformed + * reply here must show the screen's copy rather than a decode error in a commit-message field. + * The arms are main's four branches in main's order, including the one the recorded + * `sc-commit-message-canceled` scenario takes — `{ success: false, error: '', canceled: true }` + * keeps its cancel mark while its empty error falls back to the screen's copy. + */ +const NO_MESSAGE_GENERATED = 'No commit message generated' + +const generatedCommitMessageSchema: z.ZodType = z + .union([ + z + .object({ success: z.literal(true), message: z.string().min(1) }) + + .transform((value): MobileGenerateCommitMessageResult => ({ + success: true, + message: value.message + })), + z + .object({ + success: z.literal(false), + error: z.string().min(1), + canceled: z.unknown().optional() + }) + + .transform((value): MobileGenerateCommitMessageResult => ({ + success: false, + error: value.error, + ...(value.canceled ? { canceled: true } : {}) + })), + z + .object({ success: z.literal(false), canceled: z.unknown().optional() }) + + .transform((value): MobileGenerateCommitMessageResult => ({ + success: false, + error: NO_MESSAGE_GENERATED, + ...(value.canceled ? { canceled: true } : {}) + })), + // Main split its fallback in two: a non-object reply says the generation failed, while an + // object it could not read says none was generated. `typeof null === 'object'` is why the + // guard is falsy rather than nullish. + z + .unknown() + .refine((value) => !value || typeof value !== 'object') + .transform((): MobileGenerateCommitMessageResult => ({ + success: false, + error: GENERATE_FAILED + })) + ]) + .catch({ success: false, error: NO_MESSAGE_GENERATED }) export const gitGenerateCommitMessageRun = bindDeferredRpcOperation( defineRpcOperation({ @@ -92,7 +127,7 @@ export const gitGenerateCommitMessageRun = bindDeferredRpcOperation( method: 'git.generateCommitMessage', acceptance: 'require-result-or-throw-message', barrier: 'after-caller-barrier', - read: generatedCommitMessageReader + read: rpcResultVariant('generated-commit-message', generatedCommitMessageSchema) }) ) @@ -103,6 +138,6 @@ export const gitCancelGenerateCommitMessageRun = bindDeferredRpcOperation( method: 'git.cancelGenerateCommitMessage', acceptance: 'success-result-or-skip', barrier: 'after-caller-barrier', - read: rpcUncheckedPayloadReader('cancel-accepted') + read: rpcResultVariant('cancel-accepted', unreadPayload) }) ) diff --git a/mobile/src/source-control/mobile-git-read-operations.ts b/mobile/src/source-control/mobile-git-read-operations.ts index ff9af96f6f2..1a50d0380e4 100644 --- a/mobile/src/source-control/mobile-git-read-operations.ts +++ b/mobile/src/source-control/mobile-git-read-operations.ts @@ -1,14 +1,17 @@ import { bindDeferredRpcOperation, defineRpcOperation } from '../transport/rpc-operation' import type { RpcCompatibleReader } from '../transport/rpc-operation-contract' +import { rpcResultVariant } from '../transport/rpc-operation-result-reader' import { - rpcUncheckedMemberReader, - rpcUncheckedPayloadReader -} from '../transport/rpc-reader-payload' -import { readMobileGitStatusResult } from '../session/mobile-diff-review-rpc' + gitBranchCompareResultSchema, + gitCommitCompareResultSchema, + gitDiffResultSchema +} from './git-compare-reply-schema' +import { gitHistoryResultSchema } from './git-history-reply-schema' +import { gitStatusHostPayloadSchema, gitStatusProjectionSchema } from './git-status-reply-schema' import type { MobileGitStatusResult } from './mobile-git-status' -// Source-control reads. Every one of these replies used to be re-typed with a cast at the call -// site; the reader below is now the only place that says what the payload is. +// Source-control reads. Every reply below is validated against the members its consumer actually +// reads; the schema module beside each one records which consumer line justifies each requirement. /** * git.status, first of two readers. The Changes screen publishes the host payload verbatim. @@ -25,21 +28,22 @@ export const gitStatusHostPayloadRead = bindDeferredRpcOperation( method: 'git.status', acceptance: 'require-result-or-throw-message', barrier: 'after-caller-barrier', - read: rpcUncheckedPayloadReader('host-status-payload') + read: rpcResultVariant('host-status-payload', gitStatusHostPayloadSchema) }) ) -/** Shared with the session's branch-context read, which wants the same projection under a skip. */ +/** + * Shared with the session's branch-context read, which wants the same projection under a skip. + * + * Still always compatible: the projection's own contract is that an unreadable payload is a null + * status, which three screens route on. What the schema adds is the salvage report — a dropped + * entry now arrives as `salvage.droppedPaths` instead of silently thinning the list. + */ export const gitStatusProjectionReader: RpcCompatibleReader< unknown, 'normalized-status', MobileGitStatusResult | null -> = (raw) => ({ - compatible: true, - variant: 'normalized-status', - value: readMobileGitStatusResult(raw), - salvage: { droppedPaths: [], droppedCount: 0 } -}) +> = rpcResultVariant('normalized-status', gitStatusProjectionSchema) /** git.status, second reader: the normalized projection hosted-review preparation reads. */ export const gitStatusProjectionRead = bindDeferredRpcOperation( @@ -58,14 +62,14 @@ export const gitHistoryRead = bindDeferredRpcOperation( method: 'git.history', acceptance: 'require-result-or-throw-message', barrier: 'after-caller-barrier', - read: rpcUncheckedPayloadReader('history-page') + read: rpcResultVariant('history-page', gitHistoryResultSchema) }) ) /** - * A refused compare leaves the row's file list untouched, so refusal is a skip, not a throw. The - * member read keeps the property-read exception a null result throws, which is what leaves an - * already-loaded file list alone. + * A refused compare leaves the row's file list untouched, so refusal is a skip, not a throw. A + * reply that carries no readable `entries` is now an incompatible reply rather than an undefined + * list: the row's `.catch` resolves it to "No file changes" instead of spinning forever. */ export const gitCommitCompareRead = bindDeferredRpcOperation( defineRpcOperation({ @@ -73,7 +77,7 @@ export const gitCommitCompareRead = bindDeferredRpcOperation( method: 'git.commitCompare', acceptance: 'success-result-or-skip', barrier: 'after-caller-barrier', - read: rpcUncheckedMemberReader('commit-compare-entries', 'entries') + read: rpcResultVariant('commit-compare', gitCommitCompareResultSchema) }) ) @@ -83,7 +87,7 @@ export const gitBranchCompareRead = bindDeferredRpcOperation( method: 'git.branchCompare', acceptance: 'require-result-or-throw-message', barrier: 'after-caller-barrier', - read: rpcUncheckedPayloadReader('branch-compare') + read: rpcResultVariant('branch-compare', gitBranchCompareResultSchema) }) ) @@ -93,6 +97,6 @@ export const gitBranchDiffRead = bindDeferredRpcOperation( method: 'git.branchDiff', acceptance: 'require-result-or-throw-message', barrier: 'after-caller-barrier', - read: rpcUncheckedPayloadReader('branch-diff') + read: rpcResultVariant('branch-diff', gitDiffResultSchema) }) ) diff --git a/mobile/src/source-control/mobile-git-status.ts b/mobile/src/source-control/mobile-git-status.ts index 522d4873fc9..314043ceb26 100644 --- a/mobile/src/source-control/mobile-git-status.ts +++ b/mobile/src/source-control/mobile-git-status.ts @@ -1,17 +1,18 @@ -import type { - GitFileStatus, - GitStagingArea, - GitStatusEntry, - GitStatusResult, - GitUpstreamStatus -} from '../../../src/shared/git-status-types' +import type { GitFileStatus, GitStagingArea } from '../../../src/shared/git-status-types' import type { RpcResponse } from '../transport/types' +import type { + MobileGitStatusEntry, + MobileGitStatusHostPayload, + MobileGitUpstreamStatus +} from './git-status-reply-schema' export type MobileGitFileStatus = GitFileStatus export type MobileGitStagingArea = GitStagingArea -export type MobileGitStatusEntry = GitStatusEntry -export type MobileGitUpstreamStatus = GitUpstreamStatus -export type MobileGitStatusResult = GitStatusResult +export type { MobileGitStatusEntry, MobileGitUpstreamStatus } + +// The shape mobile reads off `git.status`, which is the reply schema's output, not the desktop +// aggregate: every member mobile does not read is stripped rather than re-declared here. +export type MobileGitStatusResult = MobileGitStatusHostPayload export type MobileSourceControlSection = { diff --git a/mobile/src/source-control/mobile-hosted-review-operations.ts b/mobile/src/source-control/mobile-hosted-review-operations.ts index 579b1be6a4d..05954bfe380 100644 --- a/mobile/src/source-control/mobile-hosted-review-operations.ts +++ b/mobile/src/source-control/mobile-hosted-review-operations.ts @@ -1,5 +1,16 @@ import { bindDeferredRpcOperation, defineRpcOperation } from '../transport/rpc-operation' -import { rpcUncheckedPayloadReader } from '../transport/rpc-reader-payload' +import { rpcResultVariant, rpcResultVariants } from '../transport/rpc-operation-result-reader' +import { + hostedReviewCreateFailedSchema, + hostedReviewCreateOkSchema, + hostedReviewEligibilitySchema, + type MobileHostedReviewCreateFailed, + type MobileHostedReviewCreateOk +} from './hosted-review-reply-schema' + +export type MobileHostedReviewCreateReply = + | MobileHostedReviewCreateOk + | MobileHostedReviewCreateFailed /** * Eligibility is advisory: when the host cannot answer, mobile fails closed on its own rather @@ -11,17 +22,25 @@ export const hostedReviewEligibilityRead = bindDeferredRpcOperation( method: 'hostedReview.getCreationEligibility', acceptance: 'success-result-or-skip', barrier: 'after-caller-barrier', - read: rpcUncheckedPayloadReader('creation-eligibility') + read: rpcResultVariant('creation-eligibility', hostedReviewEligibilitySchema) }) ) -/** Creation answers in-band too: an accepted reply can carry `ok: false` plus an existing review. */ +/** + * Creation answers in-band too: an accepted reply can carry `ok: false` plus an existing review. + * Two variants rather than one schema because the host's own result is a discriminated union and + * the arms require different members; the success arm is declared first so a reply carrying both + * `ok: true` and a stray `error` reads as the success it is. + */ export const hostedReviewCreateRun = bindDeferredRpcOperation( defineRpcOperation({ name: 'hostedReview.create', method: 'hostedReview.create', acceptance: 'require-result-or-throw-message', barrier: 'after-caller-barrier', - read: rpcUncheckedPayloadReader('create-result') + read: rpcResultVariants<'create-succeeded' | 'create-refused', MobileHostedReviewCreateReply>([ + rpcResultVariant('create-succeeded', hostedReviewCreateOkSchema), + rpcResultVariant('create-refused', hostedReviewCreateFailedSchema) + ]) }) ) diff --git a/mobile/src/source-control/mobile-hosted-review-service.ts b/mobile/src/source-control/mobile-hosted-review-service.ts index 80d76ac893a..5b4baf25529 100644 --- a/mobile/src/source-control/mobile-hosted-review-service.ts +++ b/mobile/src/source-control/mobile-hosted-review-service.ts @@ -1,17 +1,12 @@ -import type { - CreateHostedReviewResult, - HostedReviewCreationBlockedReason, - HostedReviewCreationEligibility, - HostedReviewCreationNextAction, - HostedReviewLookupOutcome, - HostedReviewProvider -} from '../../../src/shared/hosted-review' +import type { HostedReviewProvider } from '../../../src/shared/hosted-review' +import type { MobileHostedReviewEligibilityReply } from './hosted-review-reply-schema' import type { RpcSendParams } from '../transport/rpc-params-contract' import { refusedRpcMessageOrFallback } from '../transport/rpc-refusal-message' import { hostedReviewCopy } from './hosted-review-copy' import { hostedReviewCreateRun, - hostedReviewEligibilityRead + hostedReviewEligibilityRead, + type MobileHostedReviewCreateReply } from './mobile-hosted-review-operations' import { pushMobileHostedReviewBranch } from './mobile-hosted-review-git-preparation' import { linkMobileHostedReview } from './mobile-pr-link' @@ -40,7 +35,7 @@ export async function fetchMobileHostedReviewEligibility( client: RpcOperationSender, worktreeId: string, input: MobileHostedReviewEligibilityInput -): Promise { +): Promise { const reply = await hostedReviewEligibilityRead.request(client, { repo: mobileRepoSelectorFromWorktreeId(worktreeId), worktree: `id:${worktreeId}`, @@ -56,8 +51,7 @@ export async function fetchMobileHostedReviewEligibility( linkedGitLabMR: input.linkedGitLabMR ?? null }) const eligibility = hostedReviewEligibilityRead.interpret(reply) - // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: Preserve the established response shape at this boundary. - return eligibility.accepted ? (eligibility.value as HostedReviewCreationEligibility) : null + return eligibility.accepted ? eligibility.value : null } export type MobileHostedReviewPrefill = { @@ -66,12 +60,14 @@ export type MobileHostedReviewPrefill = { title: string body: string canCreate?: boolean - blockedReason?: HostedReviewCreationBlockedReason - nextAction?: HostedReviewCreationNextAction + // Strings, not the shared closed unions: the host publishes tokens those unions do not list, and + // mobile only compares them to the handful it acts on. See hosted-review-reply-schema.ts. + blockedReason?: string | null + nextAction?: string | null // Why: mobile lacks the desktop refresh/review-lookup signals, so it fails // closed on ambiguity. When the host could not prove the branch has no review // (`unavailable`), create — including the Push & Create path — stays blocked. - reviewLookupOutcome?: HostedReviewLookupOutcome + reviewLookupOutcome?: string } // Resolve the mobile compose prefill from the same hosted-review eligibility @@ -194,7 +190,7 @@ async function pushMobileBranchBeforeCreate( } function formatMobileHostedReviewCreateError( - result: CreateHostedReviewResult, + result: MobileHostedReviewCreateReply, pushed: boolean, shortLabel: string ): string { @@ -248,10 +244,9 @@ export async function createMobileHostedReview( client, buildMobileHostedReviewCreateParams(worktreeId, input) ) - let result: CreateHostedReviewResult + let result: MobileHostedReviewCreateReply try { - // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: Preserve the established response shape at this boundary. - result = hostedReviewCreateRun.interpret(reply) as CreateHostedReviewResult + result = hostedReviewCreateRun.interpret(reply) } catch (error) { return { ok: false, diff --git a/mobile/src/source-control/mobile-source-control-screen-state.ts b/mobile/src/source-control/mobile-source-control-screen-state.ts index 78a588d19ff..b6cf39c11fb 100644 --- a/mobile/src/source-control/mobile-source-control-screen-state.ts +++ b/mobile/src/source-control/mobile-source-control-screen-state.ts @@ -16,21 +16,21 @@ import type { MobileDiffLine } from '../session/mobile-diff-lines' import type { MobileHighlightedDiffLine } from '../session/mobile-file-syntax' import type { MobileGitBranchChangeEntry, - MobileGitBranchCompareResult, - MobileGitBranchCompareSummary -} from './mobile-branch-compare' + MobileGitBranchCompareReply +} from './git-compare-reply-schema' +import type { MobileGitBranchCompareSummary } from './mobile-branch-compare' +import type { MobileGitStatusHostPayload } from './git-status-reply-schema' import { canOpenMobileGitStatusEntry, isMobileGitDiscardableEntry, isMobileGitStageableEntry, type MobileGitFileStatus, - type MobileGitStatusEntry, - type MobileGitStatusResult + type MobileGitStatusEntry } from './mobile-git-status' export type ScreenState = | { kind: 'loading' } - | { kind: 'ready'; status: MobileGitStatusResult } + | { kind: 'ready'; status: MobileGitStatusHostPayload } | { kind: 'unavailable'; message: string } | { kind: 'error'; message: string } @@ -77,7 +77,7 @@ export function buildMobileGitStatusEntryViews( export type MobileBranchCompareState = | { kind: 'idle' } | { kind: 'loading' } - | { kind: 'ready'; result: MobileGitBranchCompareResult } + | { kind: 'ready'; result: MobileGitBranchCompareReply } | { kind: 'error'; message: string } export type MobileBranchEntryView = MobileGitBranchChangeEntry & { diff --git a/mobile/src/source-control/mobile-source-file-open-operations.ts b/mobile/src/source-control/mobile-source-file-open-operations.ts index f9090dfaf65..5b84368011b 100644 --- a/mobile/src/source-control/mobile-source-file-open-operations.ts +++ b/mobile/src/source-control/mobile-source-file-open-operations.ts @@ -1,9 +1,11 @@ +import { z } from 'zod' import { bindDeferredRpcOperation, defineRpcOperation } from '../transport/rpc-operation' -import type { RpcCompatibleReader } from '../transport/rpc-operation-contract' -import { rpcUncheckedPayloadReader } from '../transport/rpc-reader-payload' +import { rpcResultVariant } from '../transport/rpc-operation-result-reader' -// Opening a file from the Changes list. Neither reply's payload is read: the tab arrives over the -// session stream, and the caller only needs to know the host accepted. +// Opening a file from the Changes list. Neither open reply's payload is read: the tab arrives over +// the session stream, and the caller only needs to know the host accepted. `z.unknown()` is the +// honest schema for a payload with no reader, not a holdout. +const unreadPayload = z.unknown() export const sourceFileDiffOpenRun = bindDeferredRpcOperation( defineRpcOperation({ @@ -11,7 +13,7 @@ export const sourceFileDiffOpenRun = bindDeferredRpcOperation( method: 'files.openDiff', acceptance: 'require-result-or-throw-message', barrier: 'after-caller-barrier', - read: rpcUncheckedPayloadReader('diff-tab-opened') + read: rpcResultVariant('diff-tab-opened', unreadPayload) }) ) @@ -22,40 +24,43 @@ export const sourceFileOpenRun = bindDeferredRpcOperation( method: 'files.open', acceptance: 'require-result-or-throw-message', barrier: 'after-caller-barrier', - read: rpcUncheckedPayloadReader('edit-tab-opened') + read: rpcResultVariant('edit-tab-opened', unreadPayload) }) ) -export type MobileSessionFileTabCandidate = { - readonly id: string - readonly type: string - readonly mode?: unknown - readonly relativePath?: unknown - readonly diffSource?: unknown -} - -function isRecord(value: unknown): value is Record { - return typeof value === 'object' && value !== null -} - -function isSessionFileTabCandidate(value: unknown): value is MobileSessionFileTabCandidate { - return isRecord(value) && typeof value.id === 'string' && typeof value.type === 'string' -} - -const sessionFileTabsReader: RpcCompatibleReader< - unknown, - 'session-file-tabs', - { tabs: MobileSessionFileTabCandidate[] } | null -> = (raw) => ({ - compatible: true, - variant: 'session-file-tabs', - value: - isRecord(raw) && Array.isArray(raw.tabs) && raw.tabs.every(isSessionFileTabCandidate) - ? { tabs: raw.tabs } - : null, - salvage: { droppedPaths: [], droppedCount: 0 } +/** + * One candidate tab from `session.tabs.list`. + * + * `id` and `type` are required because the reveal filters on `tab.type` and returns the candidate + * by identity (reveal-mobile-source-control-session-diff.ts:70). `mode`, `relativePath` and + * `diffSource` stay `unknown`: :72-81 compares each to a literal or to null, so a host that sends + * a shape mobile does not recognise must simply not match, never fail the list. + */ +const sessionFileTabSchema = z.looseObject({ + id: z.string(), + type: z.string(), + mode: z.unknown().optional(), + relativePath: z.unknown().optional(), + diffSource: z.unknown().optional() }) +export type MobileSessionFileTabCandidate = z.output + +/** + * The reveal polls this list, so an unreadable reply must read as "not yet", not as a failure: + * :64 treats a null value exactly like a refusal and polls again. `.catch(null)` keeps that, + * and it is also what main did — one bad tab in the array made the whole list null. + */ +const sessionFileTabListSchema: z.ZodType< + { tabs: MobileSessionFileTabCandidate[] } | null, + unknown +> = z + .object({ tabs: z.array(sessionFileTabSchema) }) + + .transform((value) => ({ tabs: value.tabs })) + .nullable() + .catch(null) + /** A refused list means poll again, so refusal is a skip rather than the end of the reveal. */ export const sessionFileTabListRead = bindDeferredRpcOperation( defineRpcOperation({ @@ -63,6 +68,6 @@ export const sessionFileTabListRead = bindDeferredRpcOperation( method: 'session.tabs.list', acceptance: 'success-result-or-skip', barrier: 'after-caller-barrier', - read: sessionFileTabsReader + read: rpcResultVariant('session-file-tabs', sessionFileTabListSchema) }) ) diff --git a/mobile/src/source-control/mobile-worktree-metadata-operations.ts b/mobile/src/source-control/mobile-worktree-metadata-operations.ts index 6619b356a8e..56603bf447a 100644 --- a/mobile/src/source-control/mobile-worktree-metadata-operations.ts +++ b/mobile/src/source-control/mobile-worktree-metadata-operations.ts @@ -1,39 +1,31 @@ +import { z } from 'zod' import { bindDeferredRpcOperation, defineRpcOperation } from '../transport/rpc-operation' -import type { RpcCompatibleReader } from '../transport/rpc-operation-contract' -import { rpcUncheckedPayloadReader } from '../transport/rpc-reader-payload' +import { rpcResultVariant } from '../transport/rpc-operation-result-reader' +import { worktreeSummaryReplySchema } from './worktree-metadata-reply-schema' export type MobileWorktreeSummary = { readonly baseRef: string | null readonly linkedPR: number | null } -function isRecord(value: unknown): value is Record { - return typeof value === 'object' && value !== null -} - /** * One reader for both worktree.show consumers. Branch compare read `worktree.baseRef` behind an * `isRecord` guard and the PR sidebar read `worktree.linkedPR` through optional chaining; both * yield null on the same inputs, so the fields merge without changing either answer. + * + * Total on purpose: both callers treat a missing summary as "no hint" and fall back to another + * source, so a reply this cannot read is a null summary rather than an incompatible reply. The + * schema is what turns a `baseRef` of the wrong type into that null instead of into a string the + * branch-compare request would then send to the host. */ -const worktreeSummaryReader: RpcCompatibleReader< - unknown, - 'worktree-summary', - MobileWorktreeSummary | null -> = (raw) => { - const worktree = isRecord(raw) ? raw.worktree : undefined - return { - compatible: true, - variant: 'worktree-summary', - value: isRecord(worktree) - ? { - baseRef: typeof worktree.baseRef === 'string' ? worktree.baseRef : null, - linkedPR: typeof worktree.linkedPR === 'number' ? worktree.linkedPR : null - } - : null, - salvage: { droppedPaths: [], droppedCount: 0 } - } -} +const worktreeSummarySchema: z.ZodType = + worktreeSummaryReplySchema + .transform((value): MobileWorktreeSummary | null => + value.worktree + ? { baseRef: value.worktree.baseRef ?? null, linkedPR: value.worktree.linkedPR ?? null } + : null + ) + .catch(null) /** A refused show is a missing hint, not a failure: both callers fall back to another source. */ export const worktreeSummaryRead = bindDeferredRpcOperation( @@ -42,7 +34,7 @@ export const worktreeSummaryRead = bindDeferredRpcOperation( method: 'worktree.show', acceptance: 'success-result-or-skip', barrier: 'after-caller-barrier', - read: worktreeSummaryReader + read: rpcResultVariant('worktree-summary', worktreeSummarySchema) }) ) @@ -53,6 +45,6 @@ export const worktreeLinkSet = bindDeferredRpcOperation( method: 'worktree.set', acceptance: 'require-result-or-throw-message', barrier: 'after-caller-barrier', - read: rpcUncheckedPayloadReader('link-accepted') + read: rpcResultVariant('link-accepted', z.unknown()) }) ) diff --git a/mobile/src/source-control/use-mobile-hosted-review-eligibility.ts b/mobile/src/source-control/use-mobile-hosted-review-eligibility.ts index e229d495311..5b0c3f04241 100644 --- a/mobile/src/source-control/use-mobile-hosted-review-eligibility.ts +++ b/mobile/src/source-control/use-mobile-hosted-review-eligibility.ts @@ -1,5 +1,5 @@ import { useEffect, useState } from 'react' -import type { HostedReviewCreationEligibility } from '../../../src/shared/hosted-review' +import type { MobileHostedReviewEligibilityReply } from './hosted-review-reply-schema' import type { RpcClient } from '../transport/rpc-client' import type { ConnectionState } from '../transport/types' import { @@ -144,7 +144,7 @@ export function useMobileHostedReviewEligibility( behind } void fetchMobileHostedReviewEligibility(client, worktreeId, requestInput) - .then((eligibility: HostedReviewCreationEligibility | null) => { + .then((eligibility: MobileHostedReviewEligibilityReply | null) => { if (!active) { return } diff --git a/mobile/src/source-control/use-mobile-source-control-loaders.ts b/mobile/src/source-control/use-mobile-source-control-loaders.ts index da03e8639b5..8ab4cbc0e2a 100644 --- a/mobile/src/source-control/use-mobile-source-control-loaders.ts +++ b/mobile/src/source-control/use-mobile-source-control-loaders.ts @@ -8,10 +8,9 @@ import { gitBranchCompareRead, gitStatusHostPayloadRead } from './mobile-git-rea import { isMobileGitTransientRefreshError, isMobileGitUnavailableReply, - readMobileGitRefusal, - type MobileGitStatusResult + readMobileGitRefusal } from './mobile-git-status' -import type { MobileGitBranchCompareResult } from './mobile-branch-compare' +import type { MobileGitBranchCompareReply } from './git-compare-reply-schema' import { SELECTOR_RETRY_COUNT, SELECTOR_RETRY_DELAY_MS, @@ -138,17 +137,13 @@ export function useMobileSourceControlLoaders(params: Params): MobileSourceContr }) return false } - let compared: unknown + let compared: MobileGitBranchCompareReply try { compared = gitBranchCompareRead.interpret(reply) } catch (error) { throw new Error(refusedRpcMessageOrFallback(error, 'Unable to load committed changes')) } - setBranchCompareState({ - kind: 'ready', - // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: Preserve the established response shape at this boundary. - result: compared as MobileGitBranchCompareResult - }) + setBranchCompareState({ kind: 'ready', result: compared }) return true } catch (err) { if (!isCurrentLoad()) { @@ -214,8 +209,7 @@ export function useMobileSourceControlLoaders(params: Params): MobileSourceContr // refusal's code, which no acceptance policy carries through. const refusal = readMobileGitRefusal(reply) if (!refusal) { - // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: Preserve the established response shape at this boundary. - const result = gitStatusHostPayloadRead.interpret(reply) as MobileGitStatusResult + const result = gitStatusHostPayloadRead.interpret(reply) setScreenState({ kind: 'ready', status: result }) void loadBranchCompare({ preserveReadyOnFailure: true }) if (options?.clearActionErrorOnSuccess !== false) { diff --git a/mobile/src/source-control/use-mobile-source-control-openers.ts b/mobile/src/source-control/use-mobile-source-control-openers.ts index 4d92964c803..840a5fdab5a 100644 --- a/mobile/src/source-control/use-mobile-source-control-openers.ts +++ b/mobile/src/source-control/use-mobile-source-control-openers.ts @@ -23,10 +23,10 @@ import { sourceFileDiffOpenRun, sourceFileOpenRun } from './mobile-source-file-o import { buildMobileReviewFileRoute } from './mobile-review-route' import { revealMobileSourceControlSessionDiff } from './reveal-mobile-source-control-session-diff' import type { - GitDiffTextResult, MobileBranchCompareState, MobileBranchDiffPreviewState } from './mobile-source-control-screen-state' +import type { MobileGitDiffReply } from './git-compare-reply-schema' type Params = { client: RpcClient | null @@ -253,14 +253,12 @@ export function useMobileSourceControlOpeners(params: Params) { mergeBase: summary.mergeBase } }) - let interpreted: unknown + let result: MobileGitDiffReply try { - interpreted = gitBranchDiffRead.interpret(reply) + result = gitBranchDiffRead.interpret(reply) } catch (error) { throw new Error(refusedRpcMessageOrFallback(error, 'Unable to load committed diff')) } - // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: Preserve the established response shape at this boundary. - const result = interpreted as GitDiffTextResult | { kind: 'binary' } if (result.kind !== 'text') { throw new Error('Binary branch diff preview unavailable on mobile') } diff --git a/mobile/src/source-control/worktree-metadata-reply-schema.ts b/mobile/src/source-control/worktree-metadata-reply-schema.ts new file mode 100644 index 00000000000..92ee23b73fb --- /dev/null +++ b/mobile/src/source-control/worktree-metadata-reply-schema.ts @@ -0,0 +1,24 @@ +import { z } from 'zod' +import { salvagedOptional } from '../../../src/shared/zod-salvage' + +/** + * The `worktree.show` members the two source-control readers take: branch compare wants + * `worktree.baseRef` and the PR sidebar wants `worktree.linkedPR`. + * + * Both are salvaged optionals, and the wrapper is nullable, because both callers fall back to + * another source when the hint is missing — mobile-branch-base-ref.ts:25 and mobile-pr-link.ts:119. + * The host's worktree record carries dozens of members mobile never reads; declaring only these two + * keeps a host that renames an unrelated field from breaking the base-ref chain. + */ +export const worktreeSummaryReplySchema = z.object({ + worktree: z + .object({ + baseRef: salvagedOptional('baseRef', z.string()), + linkedPR: salvagedOptional('linkedPR', z.number()) + }) + + .nullable() + .optional() +}) + +export type MobileWorktreeSummaryReply = z.output diff --git a/mobile/src/transport/unchecked-rpc-reader-inventory.ts b/mobile/src/transport/unchecked-rpc-reader-inventory.ts index 926a3a1a84b..6bdccaae50d 100644 --- a/mobile/src/transport/unchecked-rpc-reader-inventory.ts +++ b/mobile/src/transport/unchecked-rpc-reader-inventory.ts @@ -28,7 +28,7 @@ export type UncheckedRpcReaderEntry = { /** * Files holding at least one unchecked reader, grouped by the feature area that owns them. * - * The reason is shared by every line and is stated once here instead of 42 times: the reply has no + * The reason is shared by every line and is stated once here instead of 37 times: the reply has no * schema, so the operation declares what the payload is by assertion. Writing one schema per * consumed member — required exactly where the consumer reads it unguarded, optional everywhere * else, never `.strict()` — turns the assertion into a check and deletes the line. @@ -67,12 +67,6 @@ export const UNCHECKED_RPC_READERS: readonly UncheckedRpcReaderEntry[] = [ { file: 'src/session/mobile-session-launch-operations.ts', readers: 7 }, { file: 'src/session/mobile-session-read-operations.ts', readers: 10 }, { file: 'src/session/mobile-session-write-operations.ts', readers: 8 }, - // source-control - { file: 'src/source-control/mobile-git-mutation-operations.ts', readers: 7 }, - { file: 'src/source-control/mobile-git-read-operations.ts', readers: 5 }, - { file: 'src/source-control/mobile-hosted-review-operations.ts', readers: 2 }, - { file: 'src/source-control/mobile-source-file-open-operations.ts', readers: 2 }, - { file: 'src/source-control/mobile-worktree-metadata-operations.ts', readers: 1 }, // tasks { file: 'src/tasks/mobile-task-item-comment-operations.ts', readers: 7 }, { file: 'src/tasks/mobile-task-item-detail-operations.ts', readers: 8 },