Architecture review 2026-10-07: findings index and decision audit #109

This review covers main at 18f1b46: docs, ADRs, every source and test file, CI, all 279 commits, and PRs/issues #1–#108. Protocol claims were checked against teslamotors/vehicle-command at the pinned TESLA_COMMIT (a4b43c1). Each finding has its own sub-issue; this issue is the index and the decision audit.

Summary

The protocol core is small, readable, and well tested at the level of primitives: framing, anti-replay, metadata and session crypto. Several product decisions are sound:

• CHARGING_MANAGER as the default role;
• one key per car, freshly generated for every pairing;
• no INTERNET permission;
• a transport seam that allows a simulated car.

The risks sit elsewhere:

1. The BLE/protocol runtime is one large, untested Android class. It has threading bugs, drops car faults silently, and uses a polling policy that may keep the car awake (BLE: GATT callbacks mutate shared state from Binder threads, and GATT operations aren't serialized #110, Protocol: replies are routed by trial-decoding; car faults and session resyncs are silently dropped #111, Polling: Infotainment is polled every 10 s whenever the car is awake, which may keep it from sleeping #112, Background: the reconnect and discovery loop drains the phone, and unfiltered scans stop with the screen off #113). It also stands in the way of the library goal (Architecture: split the 1,500-line TeslaBleController and move the per-vehicle protocol state machine into core #118).
2. What ships is not what is tested. Published APKs are debuggable debug builds, the release variant is never built, and version numbers don't follow the tags (Release: published APKs are debuggable debug builds, the release variant is never built, and versionName doesn't follow tags #115). Key storage deletes keys on transient errors (Keys: transient Keystore errors delete pairing keys, and private keys are decrypted on the main thread just to show IDs #116).
3. History is short-lived. Retention is far shorter than battery-health trends need (Data policy: decide when "no migration" ends, and plan history retention for health trends #123), and capacity is mis-estimated for charge sessions the app only partly observed (Health: capacity and full range are over-estimated when a charge session is first seen mid-way #114).
4. Effort goes to the wrong places. Nearly a quarter of all commits (63 of 279) went into the emulator screenshot pipeline (CI: replace the emulator screenshot pipeline with JVM screenshot tests #125), while the controller has no tests (Tests: Go vectors aren't wired to expected.txt, the request path has no oracle, and several tests are tautological #121).

Decision audit

Findings by area

• Architecture: Architecture: split the 1,500-line TeslaBleController and move the per-vehicle protocol state machine into core #118 (controller → core VehicleClient), Core: the crypto seam shouldn't require exportable private keys (StrongBox / Secure Enclave), input for ADR-0007 #119 (key-handle crypto seam), UI: no state holders; derivations recomputed in composition, two navigation stacks, storage called from composables #124 (UI state holders)
• Correctness: BLE: GATT callbacks mutate shared state from Binder threads, and GATT operations aren't serialized #110 (threading, GATT queue), Protocol: replies are routed by trial-decoding; car faults and session resyncs are silently dropped #111 (fault handling and resync), Polling: Infotainment is polled every 10 s whenever the car is awake, which may keep it from sleeping #112 (car wakefulness), Background: the reconnect and discovery loop drains the phone, and unfiltered scans stop with the screen off #113 (phone battery), Health: capacity and full range are over-estimated when a charge session is first seen mid-way #114 (health capacity), Keys: transient Keystore errors delete pairing keys, and private keys are decrypted on the main thread just to show IDs #116 (key storage)
• Release and CI: Release: published APKs are debuggable debug builds, the release variant is never built, and versionName doesn't follow tags #115 (debug builds shipped), CI: replace the emulator screenshot pipeline with JVM screenshot tests #125 (screenshot pipeline), CI: preview-health fails on every run since #105, so #107 is a false positive #126 (preview-health false positive)
• Tests: Tests: Go vectors aren't wired to expected.txt, the request path has no oracle, and several tests are tautological #121 (Go oracle gaps, tautological tests)
• Slop and consistency: Health/range: estimators merged ahead of use never run in the app, and "full range" has three inconsistent definitions #122 (unwired estimators), Core API: stringly-typed state, null as the only error, no explicit API mode; not yet library-grade #120 (stringly-typed API, null-as-error), Docs: contradictions between docs, ADRs and code #128 (docs drift)
• Portability: Core: the crypto seam shouldn't require exportable private keys (StrongBox / Secure Enclave), input for ADR-0007 #119, Core API: stringly-typed state, null as the only error, no explicit API mode; not yet library-grade #120, Tests: Go vectors aren't wired to expected.txt, the request path has no oracle, and several tests are tautological #121 (the 4-byte-nonce test), together with Record the core KMP path and crypto-library choice (ADR-0007) #108
• Libraries and tooling: Tooling: the static-analysis "zero baseline" comes from raised thresholds; dependency and build hygiene #127
• Privacy: Privacy: full VIN on the cars list, a real VIN in tests, and debug tools in the shipped UI #117
• Data and decisions: Data policy: decide when "no migration" ends, and plan history retention for health trends #123

Process notes

• Decisions were reversed quickly and the docs fell behind. There were 279 commits and 770 workflow runs in four days, and several decisions flipped within hours: backup excluded → included → opt-in; VIN hidden → shown; tabs → list; PreciseReading added → removed. When a decision is reversed, update or supersede its ADR in the same PR (Docs: contradictions between docs, ADRs and code #128).
• PR descriptions don't always match the diff. Record the core KMP path and crypto-library choice (ADR-0007) #108's title and description described a crypto swap, but the diff only adds ADR-0007 and an AGENTS.md rule (since corrected). Scanning list: RSSI for all cars, stable order, spinner, auto-stop #100's description mentioned a CHANGELOG that Generate release notes from commits and drop the hand-edited changelog #101 removed (since corrected). A PR-template line such as "description matches the diff" helps reviewers.
• Required checks are still deferred. Branch protection with required checks on main is postponed "while agents push directly". Agents already go through PRs, so requiring Android CI costs little, and it would catch problems like the versionName/tag mismatch.

Suggested order: #111, #110, #112 (correctness on the car), then #115, #116, #114; then #118 (it makes the testing in #121 possible) and #123 alongside #94; the rest after that.