chore(ci): fail CI when gateway events go unhandled by the mobile apps#100206
Conversation
openclaw-barnacle
Bot
added
scripts
|
Codex review: needs changes before merge. Reviewed July 5, 2026, 5:48 AM ET / 09:48 UTC. Summary PR surface: Tests +189, Config +2, Other +466. Total +657 across 7 files. Reproducibility: yes. The scheduling gap is source-reproducible by calling detectChangedScope for scanned native roots and observing runNode:false while the PR runs the checker only under run_check; the iOS approval gap is source-reproducible from the allowlist plus current constant-backed switch cases. Review metrics: 1 noteworthy metric.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Copy recommended automerge instructionNext step before merge
Security Review findings
Review detailsBest possible solution: Wire the protocol coverage check into the CI paths that run for the scanned native app roots, and make constant-backed iOS exec approval events count as handled instead of allowlisted. Do we have a high-confidence way to reproduce the issue? Yes. The scheduling gap is source-reproducible by calling detectChangedScope for scanned native roots and observing runNode:false while the PR runs the checker only under run_check; the iOS approval gap is source-reproducible from the allowlist plus current constant-backed switch cases. Is this the best way to solve the issue? No. The checker is the right general layer, but the current PR is not the best fix until it covers native-only dispatch changes and treats existing iOS approval handlers as handled events instead of exemptions. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 78685aec4686. Label changesLabel justifications:
Evidence reviewedPR surface: Tests +189, Config +2, Other +466. Total +657 across 7 files. View PR surface stats
Acceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (6 earlier review cycles)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f9ac14b7f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const KOTLIN_EVENT_WHEN_RE = /\bwhen\s*\(\s*event\s*\)\s*\{/u; | ||
| const KOTLIN_CASE_LABEL_RE = /^\s*((?:"[^"]+"\s*,\s*)*"[^"]+")\s*->/u; | ||
| const SWIFT_EVENT_COMPARISON_RE = /\.event\s*==\s*"([^"]+)"/gu; | ||
| const KOTLIN_EVENT_COMPARISON_RE = /\bevent\s*==\s*"([^"]+)"/gu; |
There was a problem hiding this comment.
When any Kotlin source has a parameter/local named event and compares it to a gateway literal, this regex counts that event as handled even if the code is not a gateway dispatch handler. In the current tree, apps/android/app/src/main/java/ai/openclaw/app/NodeRuntime.kt:3717 compares node.pair.requested/node.pair.resolved inside an unused helper (repo-wide rg shows no production callers), so this new check reports Android coverage for those events and omits them from the allowlist even though the runtime never routes them; a real unhandled event can now pass CI merely by appearing in a predicate. Please restrict comparison extraction to known handler scopes or require these entries to stay allowlisted.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
proof: sufficient
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b21f1b279
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| case "$TASK" in | ||
| guards) | ||
| pnpm check:no-conflict-markers | ||
| pnpm check:protocol-coverage |
There was a problem hiding this comment.
When the only changed files are mobile dispatch code, this new guard never runs: .github/workflows/ci.yml gates this check-shard on needs.preflight.outputs.run_check, while scripts/ci-changed-scope.mjs classifies paths like apps/android/.../ChatController.kt as runNode: false, runAndroid: true and apps/ios/.../IOSGatewayChatTransport.swift as runNode: false with native lanes only. That means a native-only PR can remove handling for a gateway event, or add handling for an allowlisted event, and CI will not run the new missing/stale coverage check; please invoke it from the native lanes too or make preflight set run_check for the app roots this script scans.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a7d537cf6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "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.", |
There was a problem hiding this comment.
Because allowlist entries suppress missing-coverage errors, putting the currently handled iOS exec approval events here means the new gate will still pass if the NodeAppModel cases using ExecApprovalNotificationBridge.requestedKind/resolvedKind are removed or drift from the gateway event names (apps/ios/Sources/Model/NodeAppModel.swift:969 and :974). For these user approval notifications, please resolve the Swift constants or add an explicit handled-constant map instead of exempting them as allowlisted gaps.
Useful? React with 👍 / 👎.
|
Land-ready review complete.
|
openclaw-barnacle
Bot
added
docs
openclaw-barnacle
Bot
added
size: L
and removed
cli
This comment has been minimized.
This comment has been minimized.
clawsweeper
Bot
added
proof: sufficient
8877e3f to
2d525c3
Compare
…00198) Adds scripts/check-protocol-event-coverage.mjs, which derives the server->client event catalog from GATEWAY_EVENTS in src/gateway/server-methods-list.ts, extracts the events each mobile app handles from Swift/Kotlin dispatch sites, and fails on gateway events no client handles unless allowlisted with a reason in scripts/protocol-event-coverage.allowlist.json. Wired as pnpm check:protocol-coverage in the CI guards shard.
…ions (#100198) Bare event == "..." literals in predicate helpers outside the dispatch path (gatewayEventInvalidatesNodesDevices in NodeRuntime.kt, which has no production caller) counted as Android coverage, silently masking that node.pair.requested/resolved have no live handler. Kotlin extraction now only reads when(event) labels and event comparisons inside fun handle*Event(...) bodies; node.pair.* moved to the Android allowlist with a truthful reason. Swift extraction stays tree-wide because consumption there always reads .event off a received EventFrame.
2d525c3 to
5110ab5
Compare
|
Merged via squash.
|
openclaw#100206) * ci: guard gateway protocol event coverage for iOS/Android clients (openclaw#100198) Adds scripts/check-protocol-event-coverage.mjs, which derives the server->client event catalog from GATEWAY_EVENTS in src/gateway/server-methods-list.ts, extracts the events each mobile app handles from Swift/Kotlin dispatch sites, and fails on gateway events no client handles unless allowlisted with a reason in scripts/protocol-event-coverage.allowlist.json. Wired as pnpm check:protocol-coverage in the CI guards shard. * fix(ci): scope Kotlin event extraction to handle*Event dispatch functions (openclaw#100198) Bare event == "..." literals in predicate helpers outside the dispatch path (gatewayEventInvalidatesNodesDevices in NodeRuntime.kt, which has no production caller) counted as Android coverage, silently masking that node.pair.requested/resolved have no live handler. Kotlin extraction now only reads when(event) labels and event comparisons inside fun handle*Event(...) bodies; node.pair.* moved to the Android allowlist with a truthful reason. Swift extraction stays tree-wide because consumption there always reads .event off a received EventFrame. * fix(android): remove dead node pairing event helper * fix(ci): preserve protocol allowlist parse errors * test(ci): align tooling import plan
1 participant
What Problem This Solves
Gateway server→client events (
GATEWAY_EVENTSinsrc/gateway/server-methods-list.ts) and the event handling in the iOS and Android apps can silently drift apart: a new gateway event ships and no mobile client ever handles it, with no signal anywhere.Closes #100198
Why This Change Was Made
There was no machine-checked link between the advertised event catalog and mobile event dispatch. The protocol package only types the event envelope (
event: NonEmptyString), so nothing fails when a new event name is added server-side. This addspnpm check:protocol-coverage, which derives the event inventory from the gateway catalog, extracts each client's handled events from Swift/Kotlin dispatch sites (switch/when case literals plus.event == "…"comparisons), and fails CI for any gateway event a client neither handles nor allowlists.scripts/protocol-event-coverage.allowlist.jsondocuments currently-unhandled events per client with a one-line reason each; stale entries (event removed, or now handled) also fail, so the list self-maintains. Sentinel dispatch files fail the check loudly if they move.User Impact
No runtime behavior change. Contributors adding a gateway event now get an explicit CI failure until each mobile client either handles it or records an intentional exemption, preventing silently-dropped events in the apps.
Evidence
node scripts/check-protocol-event-coverage.mjs→Protocol event coverage OK: 29 gateway events; ios handles 10, allowlists 19; android handles 12, allowlists 17.GATEWAY_EVENTS) → exit 1 listing the unhandled event for both clients.node scripts/run-vitest.mjs test/scripts/check-protocol-event-coverage.test.ts— 8 passed.guardscheck shard in.github/workflows/ci.yml.Known limitation (documented in the allowlist): the extractor reads string-literal dispatch only, so iOS
exec.approval.*handling routed through constants is allowlisted with a reason saying exactly that.Follow-up commit
0b21f1b279d(from review): Kotlin extraction is now scoped tofun handle*Event(...)dispatch bodies soevent == "…"literals in non-dispatch predicate helpers can't count as coverage — previouslynode.pair.requested/node.pair.resolvedwere marked handled via an uncalled helper (gatewayEventInvalidatesNodesDevices, no production caller). Those two events are now allowlisted with that reason. Regression test added (predicate-helper literals must not count); 9/9 vitest green; Codex autoreview on the follow-up commit clean.Follow-up commit
2a7d537cf6e: The dead Android predicate and its unit test are removed. The node-pair events remain allowlisted with their actual delivery contract: the gateway gates them behindoperator.pairing, while Android onboarding intentionally receives a bounded operator token without that scope and refreshes approval state through explicitnode.listrequests. Focused Android unit proof passed (GatewayNodeApprovalStateTest), protocol coverage reports 29 events / Android 12 handled + 17 allowlisted, and the final Codex autoreview is clean.