From e62cf76bd50f30cc4af1ebfc780d902d99e89a81 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 5 Jul 2026 09:23:45 -0700 Subject: [PATCH] fix(ci): catch native-only mobile protocol drift (#100278) * fix(ci): cover native-only protocol event drift * fix(ci): ignore quoted Swift event constants --- .github/workflows/ci.yml | 5 +- scripts/check-protocol-event-coverage.mjs | 95 ++++++++++++++++--- .../protocol-event-coverage.allowlist.json | 2 - .../check-protocol-event-coverage.test.ts | 44 ++++++++- test/scripts/ci-workflow-guards.test.ts | 16 ++++ 5 files changed, 143 insertions(+), 19 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7e6f2527b20d..a580358d1f8b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -424,6 +424,10 @@ jobs: } EOF + - name: Check mobile protocol event coverage + if: ${{ steps.manifest.outputs.run_node == 'true' || steps.manifest.outputs.run_ios_build == 'true' || steps.manifest.outputs.run_android_job == 'true' }} + run: node scripts/check-protocol-event-coverage.mjs + # Run dependency-free security checks in parallel with scope detection so the # main Node jobs do not have to wait for Python/pre-commit setup. security-fast: @@ -1206,7 +1210,6 @@ jobs: case "$TASK" in guards) pnpm check:no-conflict-markers - pnpm check:protocol-coverage pnpm tool-display:check pnpm check:host-env-policy:swift pnpm dup:check:coverage diff --git a/scripts/check-protocol-event-coverage.mjs b/scripts/check-protocol-event-coverage.mjs index 4e8f16793158..892d81b2eaa6 100644 --- a/scripts/check-protocol-event-coverage.mjs +++ b/scripts/check-protocol-event-coverage.mjs @@ -11,11 +11,11 @@ // Client "handled" sets are extracted with deliberately simple parsing over // the mobile app sources: Swift `switch .event { case "..." }` blocks plus // `.event == "..."` comparisons, and Kotlin `when (event) { "..." -> }` blocks -// plus `event == "..."` comparisons scoped to `fun handle*Event(...)` bodies -// so predicate helpers outside the dispatch path do not count as coverage. Events a client intentionally does not -// consume live in scripts/protocol-event-coverage.allowlist.json with a -// one-line reason. New gateway events that no client handles (and are not -// allowlisted) fail the check. +// plus `event == "..."` comparisons scoped to `fun handle*Event(...)` bodies. +// Swift case labels may use qualified static string constants; those are +// resolved across the scanned source tree so deleting the real handler cannot +// hide behind an allowlist entry. Events a client intentionally does not +// consume live in scripts/protocol-event-coverage.allowlist.json. import fs from "node:fs"; import path from "node:path"; import { fileURLToPath } from "node:url"; @@ -42,6 +42,10 @@ const MIN_EXPECTED_GATEWAY_EVENTS = 10; const GATEWAY_EVENTS_BLOCK_RE = /export const GATEWAY_EVENTS = \[([\s\S]*?)\];/u; const SWIFT_EVENT_SWITCH_RE = /\bswitch\s+\w+(?:\.\w+)*\.event\s*\{/u; const SWIFT_CASE_LABEL_RE = /^\s*case\s+(.+?):/u; +const SWIFT_TYPE_DECLARATION_RE = + /^\s*(?:(?:private|fileprivate|internal|public)\s+)?(?:enum|struct|class|actor|extension)\s+([A-Za-z_]\w*)[^{]*\x7b/u; +const SWIFT_STATIC_STRING_CONSTANT_RE = /^\s*static\s+let\s+([A-Za-z_]\w*)\s*=\s*"([^"]+)"/u; +const SWIFT_QUALIFIED_CONSTANT_RE = /\b([A-Za-z_]\w*\.[A-Za-z_]\w*)\b/gu; const KOTLIN_EVENT_WHEN_RE = /\bwhen\s*\(\s*event\s*\)\s*\{/u; // Kotlin gateway handlers follow the `handle*Event` naming convention // (handleEvent, handleGatewayEvent, handleExecApprovalGatewayEvent, ...). @@ -148,16 +152,57 @@ function pushStringLiterals(segment, names) { } /** - * Extracts event names a Swift source handles: string-literal case labels of - * `switch .event` blocks plus `.event == "..."` comparisons. Case labels - * built from constants are invisible to this extractor and need an allowlist - * entry explaining that. + * Extracts qualified static string constants declared at Swift type scope. + * Type qualification avoids resolving unrelated constants that share a short + * member name elsewhere in the app. */ -export function extractSwiftHandledEvents(source) { +export function extractSwiftStaticStringConstants(source) { + const constants = new Map(); + const lines = source.split("\n"); + for (let i = 0; i < lines.length; i += 1) { + const declaration = SWIFT_TYPE_DECLARATION_RE.exec(lines[i]); + if (!declaration) { + continue; + } + const typeName = declaration[1]; + let depth = 0; + for (let j = i; j < lines.length; j += 1) { + const line = lines[j]; + if (depth === 1) { + const constant = SWIFT_STATIC_STRING_CONSTANT_RE.exec(line); + if (constant) { + constants.set(`${typeName}.${constant[1]}`, constant[2]); + } + } + const braceSource = sanitizeLineForBraces(line); + for (const char of braceSource) { + if (char === "{") { + depth += 1; + } else if (char === "}") { + depth -= 1; + } + } + if (j > i && depth <= 0) { + break; + } + } + } + return constants; +} + +/** Extracts Swift gateway-event case labels, including qualified constants. */ +export function extractSwiftHandledEvents(source, constants = new Map()) { const names = collectBlockCaseLabels(source, SWIFT_EVENT_SWITCH_RE, (line, sink) => { const label = SWIFT_CASE_LABEL_RE.exec(line); if (label) { pushStringLiterals(label[1], sink); + const constantReferences = sanitizeLineForBraces(label[1]); + for (const reference of constantReferences.matchAll(SWIFT_QUALIFIED_CONSTANT_RE)) { + const value = constants.get(reference[1]); + if (value) { + sink.push(value); + } + } } }); for (const comparison of source.matchAll(SWIFT_EVENT_COMPARISON_RE)) { @@ -325,8 +370,9 @@ function loadAllowlist(rootDir, fsImpl) { } function collectClientHandledEvents(params) { - const { rootDir, roots, extension, extract, sentinels, fsImpl } = params; + const { rootDir, roots, extension, extract, buildExtractContext, sentinels, fsImpl } = params; const handled = new Set(); + const sources = new Map(); for (const root of roots) { const rootPath = path.resolve(rootDir, root); if (!fsImpl.existsSync(rootPath)) { @@ -335,14 +381,18 @@ function collectClientHandledEvents(params) { ); } for (const filePath of listFilesRecursive(rootPath, extension, fsImpl)) { - for (const event of extract(fsImpl.readFileSync(filePath, "utf8"))) { - handled.add(event); - } + sources.set(filePath, fsImpl.readFileSync(filePath, "utf8")); + } + } + const extractContext = buildExtractContext?.(sources.values()); + for (const source of sources.values()) { + for (const event of extract(source, extractContext)) { + handled.add(event); } } for (const sentinel of sentinels) { const source = readRequiredFile(rootDir, sentinel, fsImpl); - if (extract(source).size === 0) { + if (extract(source, extractContext).size === 0) { throw new Error( `Sentinel dispatch file ${sentinel} no longer matches any event names; ` + "its event handling likely moved or changed shape. Update scripts/check-protocol-event-coverage.mjs.", @@ -352,6 +402,20 @@ function collectClientHandledEvents(params) { return handled; } +function collectSwiftStaticStringConstants(sources) { + const constants = new Map(); + for (const source of sources) { + for (const [name, value] of extractSwiftStaticStringConstants(source)) { + const existing = constants.get(name); + if (existing !== undefined && existing !== value) { + throw new Error(`Conflicting Swift string constant values for ${name}.`); + } + constants.set(name, value); + } + } + return constants; +} + /** * Runs the full coverage check against a repo checkout and returns error * strings plus a summary for logging. @@ -373,6 +437,7 @@ export function collectProtocolEventCoverageErrors(params = {}) { roots: IOS_SCAN_ROOTS, extension: ".swift", extract: extractSwiftHandledEvents, + buildExtractContext: collectSwiftStaticStringConstants, sentinels: [IOS_SENTINEL_FILE], fsImpl, }), diff --git a/scripts/protocol-event-coverage.allowlist.json b/scripts/protocol-event-coverage.allowlist.json index 40c7fa432b4d..2e2dbda85f7d 100644 --- a/scripts/protocol-event-coverage.allowlist.json +++ b/scripts/protocol-event-coverage.allowlist.json @@ -13,8 +13,6 @@ "device.pair.requested": "Device pairing flows poll via device.pair.* methods on iOS.", "device.pair.resolved": "Device pairing flows poll via device.pair.* methods on iOS.", "voicewake.routing.changed": "iOS only consumes voicewake.changed trigger updates; routing changes are not surfaced.", - "exec.approval.requested": "Handled in NodeAppModel via ExecApprovalNotificationBridge constants; the literal-only extractor cannot see constant case labels.", - "exec.approval.resolved": "Handled in NodeAppModel via ExecApprovalNotificationBridge constants; the literal-only extractor cannot see constant case labels.", "plugin.approval.requested": "Plugin approval prompts are not implemented on iOS.", "plugin.approval.resolved": "Plugin approval prompts are not implemented on iOS.", "terminal.data": "Embedded terminal is a web/desktop surface; iOS has no terminal client.", diff --git a/test/scripts/check-protocol-event-coverage.test.ts b/test/scripts/check-protocol-event-coverage.test.ts index 39d5e13b252d..4aadc9a44ea4 100644 --- a/test/scripts/check-protocol-event-coverage.test.ts +++ b/test/scripts/check-protocol-event-coverage.test.ts @@ -5,6 +5,7 @@ import { extractGatewayEventNames, extractKotlinHandledEvents, extractSwiftHandledEvents, + extractSwiftStaticStringConstants, } from "../../scripts/check-protocol-event-coverage.mjs"; const GATEWAY_LIST_FIXTURE = ` @@ -51,6 +52,11 @@ describe("extractGatewayEventNames", () => { describe("extractSwiftHandledEvents", () => { it("collects switch case literals and comparisons, skipping nested and non-event code", () => { + const constants = extractSwiftStaticStringConstants(` + enum SomeBridge { + static let requestedKind = "exec.approval.requested" + } + `); const source = ` static func mapEventFrame(_ evt: EventFrame) -> Event? { switch evt.event { @@ -80,14 +86,50 @@ describe("extractSwiftHandledEvents", () => { } if evt.event == "connect.challenge" { return } `; - const handled = extractSwiftHandledEvents(source); + const handled = extractSwiftHandledEvents(source, constants); expect([...handled].toSorted()).toEqual([ "chat", "connect.challenge", + "exec.approval.requested", "session.message", "tick", ]); }); + + it("extracts only type-scoped static string constants", () => { + const constants = extractSwiftStaticStringConstants(` + enum ApprovalBridge { + static let requestedKind = "exec.approval.requested" + private static let nested = makeValue { + "not.an.event" + } + } + let requestedKind = "wrong.global.value" + `); + + expect([...constants]).toEqual([["ApprovalBridge.requestedKind", "exec.approval.requested"]]); + }); + + it("does not resolve qualified constants inside quoted case labels", () => { + const constants = extractSwiftStaticStringConstants(` + enum ApprovalBridge { + static let requestedKind = "exec.approval.requested" + } + `); + const handled = extractSwiftHandledEvents( + ` + switch evt.event { + case "ApprovalBridge.requestedKind": + return .approval + default: + return nil + } + `, + constants, + ); + + expect(handled).toEqual(new Set(["ApprovalBridge.requestedKind"])); + }); }); describe("extractKotlinHandledEvents", () => { diff --git a/test/scripts/ci-workflow-guards.test.ts b/test/scripts/ci-workflow-guards.test.ts index 710dda3bac76..b4bc7a370807 100644 --- a/test/scripts/ci-workflow-guards.test.ts +++ b/test/scripts/ci-workflow-guards.test.ts @@ -623,6 +623,22 @@ describe("ci workflow guards", () => { expect(preflightGuards).toContain("pnpm deps:patches:check"); }); + it("runs mobile protocol coverage for Node and native-only changes", () => { + const workflow = readCiWorkflow(); + const coverageStep = workflow.jobs.preflight.steps.find( + (step) => step.name === "Check mobile protocol event coverage", + ); + const checkShardRun = workflow.jobs["check-shard"].steps.find( + (step) => step.name === "Run check shard", + ).run; + + expect(coverageStep.run).toBe("node scripts/check-protocol-event-coverage.mjs"); + expect(coverageStep.if).toContain("steps.manifest.outputs.run_node == 'true'"); + expect(coverageStep.if).toContain("steps.manifest.outputs.run_ios_build == 'true'"); + expect(coverageStep.if).toContain("steps.manifest.outputs.run_android_job == 'true'"); + expect(checkShardRun).not.toContain("check:protocol-coverage"); + }); + it("does not rebuild Control UI after build:ci-artifacts", () => { const workflow = readCiWorkflow(); const buildArtifactSteps = workflow.jobs["build-artifacts"].steps;