fix: make RPC send tests independent of host bridge availability - #277
Conversation
Three RPCServerTests cases build an RPCServer without passing isBridgeReady,
which defaults to `{ true }`. The send handler therefore takes the bridge branch
in `transport == .bridge || isBridgeReady()`, and the injected sendMessage stub
is never called.
In CI that goes unnoticed: with no injected helper the bridge invocation throws
and the handler falls back to AppleScript, which is what the tests assert. On a
developer machine with the helper actually injected the bridge call succeeds, so
the assertions see `bridge_v2` instead of `applescript` and nil captured options.
Pin the transport the tests intend to exercise by passing `isBridgeReady:
{ false }`, matching the existing explicit `isBridgeReady: { true }` at the top
of the file. Test-only change; no production behavior is affected.
`make test` on macOS 26 with a live injected bridge: 744 tests, all passing.
Previously 3 failed on the same host.
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
clawsweeper
Bot
added
P2
|
Codex review: needs maintainer review before merge. Reviewed September 6, 2026, 9:19 PM ET / September 7, 2026, 01:19 UTC. ClawSweeper reviewWhat this changesPins three RPC send tests to their injected AppleScript sender so an available Messages bridge cannot bypass their fixtures. Merge readiness✅ Ready for maintainer review The focused fix remains necessary on current main, preserves the existing assertions, and has relevant verification from the affected host. No blocking defects found. Likely related people: Peter Steinberger (low-confidence routing candidate from recent RPC history). Priority: P2 Review scores
Verification
How this fits togetherimsg’s JSON-RPC server turns client send requests into bridge or AppleScript operations and returns message identifiers or delivery errors. These tests use an in-memory database and injected senders to verify that response behavior. flowchart LR
A[Test send request] --> B[RPC server]
C[In-memory chat database] --> B
B --> D{Bridge readiness}
D -->|Available| E[Messages bridge]
D -->|Pinned false| F[Injected sender]
F --> G[Response assertions]
Before mergeNone. Agent review detailsSecurityNone. Review metricsNone. Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Technical reviewBest possible solution: Keep response-shaping tests deterministic through the existing readiness injection while retaining separate bridge-routing coverage. Do we have a high-confidence way to reproduce the issue? Yes: current-main source explains how these tests reach the host bridge instead of their sender stubs, and the paired report records failures with the bridge injected. This review did not execute the tests. Is this the best way to solve the issue? Yes: using the existing injectable readiness predicate is a narrow repair that preserves test assertions and production defaults. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 646ea7af9616. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
|
Verified head
The affected-host bridge-on before/after evidence in this PR remains the transport-isolation proof. This verification Mac has SIP enabled. The full local suite exposed a separate output-capture deadlock that also reproduces serially: the subprocess helper waits before draining its pipes. That root cause is repaired independently in #279, with a failing-before/passing-after 128 KiB regression and an unchanged three-second timeout. Land #279 first so the remaining local validation is reproducible on constrained hosts. |
2 participants
Fixes #275.
Problem
Three
RPCServerTestscases fail on any macOS host where the bridge helper is actually injected into Messages.app, and pass in CI. That makesmake testred for anyone developing on a real, working setup, which hides genuine regressions behind expected noise.rpcSendResolvesChatID()rpcSendReportsMisroutedChatGhost()rpcSendReturnsSentMessageIdentifiersWhenResolved()Cause
All three construct
RPCServerwithout passingisBridgeReady, which defaults to{ true }. The send handler then takes the bridge branch:So the injected
sendMessagestub is never called.In CI this is invisible: with no injected helper the bridge invocation throws and the handler falls back to AppleScript, which is exactly what the tests assert. On a machine where the helper is injected the bridge call succeeds, and the assertions see
bridge_v2instead ofapplescript, withcapturedleft nil:The tests are about RPC result shaping, not about which transport happens to be reachable — so the transport should be pinned rather than inherited from host state.
Change
Pass
isBridgeReady: { false }at the three call sites. This matches the existing explicitisBridgeReady: { true }already used elsewhere in the same file, so the pattern is established.Test-only. No production behavior changes; the parameter was already injectable.
Verify
Host: macOS 26, Apple Silicon, SIP disabled, Messages.app running with
imsg-bridge-helper.dylibinjected,imsg statusreportingv2_ready: true.Before, on this host:
After:
make lint:Found 15 violations, 0 serious in 249 files, exit 0 — identical to base646ea7a; I diffed the violation sets and this change introduces none.Disclosure: AI assisted in writing and verifying this change.