From 4b0009d41433893dd40d2a93e565f77172e2ca6c Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Tue, 15 Sep 2026 22:30:15 -0400 Subject: [PATCH] test(mobile): ratchet the 201 unchecked RPC reply readers Step 4 moved every call-site cast into an RpcOperation's `read`, but 201 of those readers still answer `compatible: true` for any payload: `rpcUncheckedPayloadReader` (163), `rpcReadUnchecked` (26 outside its own module) and `rpcUncheckedMemberReader` (12), across 42 files. The cast moved; it did not become true. Held as data with an AST boundary test, shaped on the raw-request-port ratchet: a file that is not listed fails, a listed file that no longer has one fails, and a count that rises fails. Only a call counts, so an import is not a reader and prose never is. No behaviour change: this commit adds a list and a test. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb --- .../unchecked-rpc-reader-boundary.test.ts | 180 ++++++++++++++++++ .../unchecked-rpc-reader-inventory.ts | 93 +++++++++ 2 files changed, 273 insertions(+) create mode 100644 mobile/src/transport/unchecked-rpc-reader-boundary.test.ts create mode 100644 mobile/src/transport/unchecked-rpc-reader-inventory.ts diff --git a/mobile/src/transport/unchecked-rpc-reader-boundary.test.ts b/mobile/src/transport/unchecked-rpc-reader-boundary.test.ts new file mode 100644 index 00000000000..790008acef6 --- /dev/null +++ b/mobile/src/transport/unchecked-rpc-reader-boundary.test.ts @@ -0,0 +1,180 @@ +import { readFileSync, readdirSync } from 'node:fs' +import { fileURLToPath } from 'node:url' +import { extname, join, relative } from 'node:path' +import ts from 'typescript' +import { describe, expect, it } from 'vitest' +import { + UNCHECKED_RPC_READERS, + type UncheckedRpcReaderEntry +} from './unchecked-rpc-reader-inventory' + +/** + * Ratchet for unchecked reply readers. + * + * `rpcUncheckedPayloadReader('x')` says nothing about the payload: it answers `compatible: true` + * for a string, a null, an error envelope and the shape the consumer expects alike. The operation + * then hands that value to a consumer typed as if it had been checked. This list is what the + * count-down to zero is measured against, and this test is what makes it bind. + * + * Three failures, all of which mean "edit the list": + * - a file holds an unchecked reader and is on the list of none, + * - a listed file no longer holds one (stale entry — how allow-lists rot), + * - a listed file's reader count went up. + * + * What this does NOT catch, all accepted: + * - A hand-written `{ compatible: true, value: raw as T }` reader. Same hole, different bytes; + * `unchecked-rpc-reader-inventory.ts` says so in prose because no AST rule separates a + * projecting reader that validated its input from one that asserted. + * - Whether a *checked* schema is any good. A `z.unknown()` reader counts as checked here and is + * the right answer for a payload the consumer forwards opaquely; it is the wrong answer for one + * it destructures, and only the consumer trace tells the two apart. + * - Test files. `*.test.ts(x)` is not scanned: a suite asserting on an unchecked reader is + * testing the helper, and a test does not ship. + */ + +const mobileRoot = fileURLToPath(new URL('../..', import.meta.url)) +const scannedRoots = ['app', 'src'].map((directory) => join(mobileRoot, directory)) +const sourceExtensions = new Set(['.js', '.jsx', '.ts', '.tsx']) + +/** The three helpers, and the module that defines them — it is not an offender. */ +const UNCHECKED_READER_NAMES = new Set([ + 'rpcUncheckedPayloadReader', + 'rpcUncheckedMemberReader', + 'rpcReadUnchecked' +]) +const SELF_FILES = new Set([ + 'src/transport/rpc-reader-payload.ts', + 'src/transport/unchecked-rpc-reader-inventory.ts' +]) + +function sourceFiles(directory: string): string[] { + return readdirSync(directory, { withFileTypes: true }).flatMap((entry) => { + const path = join(directory, entry.name) + if (entry.isDirectory()) { + return entry.name === 'node_modules' ? [] : sourceFiles(path) + } + return [path] + }) +} + +function parse(path: string, source: string): ts.SourceFile { + const extension = extname(path) + return ts.createSourceFile( + path, + source, + ts.ScriptTarget.Latest, + true, + extension === '.tsx' || extension === '.jsx' ? ts.ScriptKind.TSX : ts.ScriptKind.TS + ) +} + +/** How many unchecked readers this file builds. Only a call counts: an import is not a reader. */ +export function uncheckedReaderCount(path: string, source: string): number { + let readers = 0 + const visit = (node: ts.Node): void => { + if ( + ts.isCallExpression(node) && + ts.isIdentifier(node.expression) && + UNCHECKED_READER_NAMES.has(node.expression.text) + ) { + readers += 1 + } + ts.forEachChild(node, visit) + } + visit(parse(path, source)) + return readers +} + +const scanned = scannedRoots + .flatMap(sourceFiles) + .filter((path) => sourceExtensions.has(extname(path))) + .filter((path) => !/\.test\.tsx?$/.test(path)) + .map((path) => relative(mobileRoot, path).split(/[/\\]/).join('/')) + .filter((file) => !SELF_FILES.has(file)) + +const observed = new Map( + scanned + .map( + (file) => + [ + file, + uncheckedReaderCount(join(mobileRoot, file), readFileSync(join(mobileRoot, file), 'utf8')) + ] as const + ) + .filter(([, readers]) => readers > 0) +) + +const inventory: readonly UncheckedRpcReaderEntry[] = UNCHECKED_RPC_READERS + +describe('unchecked RPC reader boundary', () => { + const probe = join(mobileRoot, 'src', 'transport', 'probe.ts') + + it('counts every shape that builds an unchecked reader', () => { + expect(uncheckedReaderCount(probe, "read: rpcUncheckedPayloadReader('x')")).toBe(1) + expect(uncheckedReaderCount(probe, "read: rpcUncheckedMemberReader('x', 'entries')")).toBe(1) + expect(uncheckedReaderCount(probe, "return rpcReadUnchecked('x', raw)")).toBe(1) + expect(uncheckedReaderCount(probe, "a(rpcReadUnchecked('x', rpcReadUnchecked('y', 1)))")).toBe( + 2 + ) + }) + + it('does not count prose, an import or a checked reader', () => { + expect(uncheckedReaderCount(probe, '// rpcUncheckedPayloadReader is the old shape')).toBe(0) + expect(uncheckedReaderCount(probe, '/* rpcReadUnchecked */ export const x = 1')).toBe(0) + expect( + uncheckedReaderCount( + probe, + "import { rpcUncheckedPayloadReader } from './rpc-reader-payload'" + ) + ).toBe(0) + expect(uncheckedReaderCount(probe, "read: rpcResultVariant('x', schema)")).toBe(0) + }) + + it('scans a plausible number of files', () => { + // A broken root or extension filter would make every check below vacuously pass. The floor on + // the offender count comes down with the list, so a successful migration step does not fail it. + expect(scanned.length).toBeGreaterThan(400) + expect(observed.size).toBeGreaterThan(20) + }) + + it('lists each file once', () => { + const seen = inventory.map((entry) => entry.file) + expect(seen.filter((file, index) => seen.indexOf(file) !== index)).toEqual([]) + }) + + it('has no unlisted file holding an unchecked reader', () => { + const listed = new Set(inventory.map((entry) => entry.file)) + const unlisted = [...observed.keys()].filter((file) => !listed.has(file)) + expect( + unlisted, + 'A new operation validates its reply with rpcResultVariant. Nothing may be added to unchecked-rpc-reader-inventory.ts.' + ).toEqual([]) + }) + + it('has no stale inventory entry', () => { + const stale = inventory.filter((entry) => !observed.has(entry.file)) + expect( + stale.map((entry) => entry.file), + 'File holds no unchecked reader — delete its line from unchecked-rpc-reader-inventory.ts.' + ).toEqual([]) + }) + + it('has no inventory entry whose file gained readers', () => { + const grown = inventory + .filter((entry) => (observed.get(entry.file) ?? 0) > entry.readers) + .map((entry) => `${entry.file}: listed ${entry.readers}, found ${observed.get(entry.file)}`) + expect(grown, 'The counts are a ceiling. Validate the new reply with a schema.').toEqual([]) + }) + + it('reports a count that has fallen so the entry can be lowered', () => { + const overstated = inventory + .filter( + (entry) => observed.has(entry.file) && (observed.get(entry.file) ?? 0) < entry.readers + ) + .map((entry) => `${entry.file}: listed ${entry.readers}, found ${observed.get(entry.file)}`) + expect( + overstated, + 'Fewer unchecked readers than listed — lower the count so the ratchet holds.' + ).toEqual([]) + }) +}) diff --git a/mobile/src/transport/unchecked-rpc-reader-inventory.ts b/mobile/src/transport/unchecked-rpc-reader-inventory.ts new file mode 100644 index 00000000000..926a3a1a84b --- /dev/null +++ b/mobile/src/transport/unchecked-rpc-reader-inventory.ts @@ -0,0 +1,93 @@ +/** + * Every RpcOperation reader that re-types its reply instead of validating it, held as data. + * + * A reader is unchecked when it answers `compatible: true` for every payload a byte can carry: + * a call to `rpcUncheckedPayloadReader`, `rpcUncheckedMemberReader` or `rpcReadUnchecked` in + * rpc-reader-payload.ts. Step 4 moved the call-site cast into the operation's `read`; it did not + * make the cast true. A malformed reply still reaches the consumer as the declared type and fails + * somewhere downstream — a property read on null, a `.map` on a string, a rendered `undefined` — + * with nothing naming the reply as the cause. + * + * The count is per file and is a ceiling, not a target: unchecked-rpc-reader-boundary.test.ts fails + * on a file that is not listed, on a listed file that no longer has one, and on a listed file whose + * count went up. Replacing a reader with `rpcResultVariant(variant, schema)` lowers its line; the + * list only shrinks. + * + * Two holes this list does not close, both deliberate: + * - A hand-written reader that returns `{ compatible: true, ... }` without going through those + * three helpers is not counted. It is the same hole with different bytes; the AST cannot tell + * a projecting reader that validated its input from one that did not. + * - `rpcPayloadMember` at a call site outside a reader. That is an unchecked member read, not a + * reader, and it is fenced by the raw-port inventory instead. + */ +export type UncheckedRpcReaderEntry = { + readonly file: string + readonly readers: number +} + +/** + * 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 + * 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. + */ +export const UNCHECKED_RPC_READERS: readonly UncheckedRpcReaderEntry[] = [ + // agent-history + { file: 'src/agent-history/mobile-agent-history-operations.ts', readers: 6 }, + // browser + { file: 'src/browser/mobile-browser-command-operations.ts', readers: 1 }, + // components + { file: 'src/components/codex-reset-credit-capability-operations.ts', readers: 1 }, + { file: 'src/components/codex-reset-credit-consume-operations.ts', readers: 1 }, + { file: 'src/components/new-workspace-operations.ts', readers: 2 }, + // dictation + { file: 'src/dictation/mobile-dictation-operations.ts', readers: 8 }, + // files + { file: 'src/files/mobile-file-explorer-operations.ts', readers: 2 }, + { file: 'src/files/mobile-file-ownership-operations.ts', readers: 2 }, + { file: 'src/files/mobile-file-preview-operations.ts', readers: 6 }, + { file: 'src/files/mobile-file-tab-doc-operations.ts', readers: 3 }, + // home + { file: 'src/home/mobile-home-host-operations.ts', readers: 2 }, + // host-screen + { file: 'src/host-screen/host-screen-operations.ts', readers: 8 }, + // notifications + { file: 'src/notifications/mobile-push-delivery-test-operations.ts', readers: 1 }, + { file: 'src/notifications/mobile-push-registration-operations.ts', readers: 2 }, + { file: 'src/notifications/push-dismissal-operations.ts', readers: 1 }, + // session + { file: 'src/session/github-pr-mutation-operations.ts', readers: 4 }, + { file: 'src/session/github-pr-read-operations.ts', readers: 8 }, + { file: 'src/session/mobile-clipboard-image-operations.ts', readers: 5 }, + { file: 'src/session/mobile-diff-review-git-operations.ts', readers: 2 }, + { file: 'src/session/mobile-diff-review-operations.ts', readers: 3 }, + { file: 'src/session/mobile-review-terminal-operations.ts', readers: 3 }, + { 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 }, + { file: 'src/tasks/mobile-task-item-state-operations.ts', readers: 17 }, + { file: 'src/tasks/mobile-task-list-operations.ts', readers: 6 }, + { file: 'src/tasks/mobile-task-project-board-operations.ts', readers: 17 }, + { file: 'src/tasks/mobile-task-runtime-operations.ts', readers: 7 }, + { file: 'src/tasks/mobile-task-source-search-operations.ts', readers: 7 }, + { file: 'src/tasks/mobile-workspace-create-operations.ts', readers: 4 }, + { file: 'src/tasks/mobile-workspace-source-operations.ts', readers: 7 }, + // terminal + { file: 'src/terminal/mobile-terminal-operations.ts', readers: 3 }, + // transport + { file: 'src/transport/host-status-probe-operations.ts', readers: 1 }, + { file: 'src/transport/mobile-relay-pairing-operations.ts', readers: 2 }, + // worktree + { file: 'src/worktree/worktree-catalog-operations.ts', readers: 2 } +]